[libcamera-devel] [PATCH 5/5] libcamera: pipeline: Refuse to substitute camera data

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Jan 25, 2019 at 06:04:00PM +0100, Jacopo Mondi wrote:
> Once a pipeline-specific data has been associated with a camera, refuse
> to update it in order to avoid invalidating the previously set
> reference, which might still be owned by the caller.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/pipeline_handler.h |  2 +-
>  src/libcamera/pipeline_handler.cpp       | 23 ++++++++++++++---------
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index ca77a40..01a0f0b 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -47,7 +47,7 @@ protected:
>  	void hotplugMediaDevice(MediaDevice *media);
>  
>  	CameraData *cameraData(const Camera *camera);
> -	void setCameraData(const Camera *camera, std::unique_ptr<CameraData> data);
> +	int setCameraData(const Camera *camera, std::unique_ptr<CameraData> data);
>  
>  private:
>  	virtual void disconnect();
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index dc55f8f..7e927a7 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -212,21 +212,26 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)
>   * information with \a camera. Ownership of \a data is transferred to
>   * the PipelineHandler.
>   *
> - * If pipeline-specific data has already been associated with the camera by a
> - * previous call to this method, is it replaced by \a data and the previous data
> - * are deleted, rendering all references to them invalid.
> + * Once pipeline-specific data has already been associated with the camera
> + * by a previous call to this method it cannot be changed later on.

Maybe "Pipeline-specific data can only be set once. Any attempt to call
this method after the first one with the same camera will return an
error and won't affect the pipeline-specific data." ? Up to you.

>   *
>   * The data can be retrieved by pipeline handlers using the cameraData() method.
> + *
> + * \return 0 on success, or a negative error code if camera specific data have
> + * already been set
>   */
> -void PipelineHandler::setCameraData(const Camera *camera,
> -				    std::unique_ptr<CameraData> data)
> +int PipelineHandler::setCameraData(const Camera *camera,
> +				   std::unique_ptr<CameraData> data)
>  {
> -	if (cameraData_.count(camera))
> -		LOG(Pipeline, Debug)
> -			<< "Replacing data associated with "
> -			<< camera->name();
> +	if (cameraData_.find(camera) != cameraData_.end()) {
> +		LOG(Pipeline, Error)
> +			<< "Replacing data associated with a camera is forbidden ";

s/ ";/";/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		return -EINVAL;
> +	}
>  
>  	cameraData_[camera] = std::move(data);
> +
> +	return 0;
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart