[libcamera-devel] [PATCH 2/3] libcamera: pipelines: keep reference to cameras created

[Jacopo Mondi]

Hi Niklas,

On Wed, Jan 23, 2019 at 12:29:54AM +0100, Niklas Söderlund wrote:
> The pipeline handlers needs to keep a reference to the cameras it
> creates in order to notify it when it's being disconnected. When
> hot-unplug support is added the pipeline handler would be deleted when
> the hardware is removed from under it.

I might have missed a piece here, I understand cameras are subject to
hot-unplug, and might go away at run time. what is the use case you
considered for removing a pipeline handler at run time?

>
> After the deletion of pipeline handler all the Camera objects created by
> the pipeline handler might still be around as they might be in use by a
> application. At this point the pipeline handler should inform the camera
> that it's disconnected.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  7 ++++++-
>  src/libcamera/pipeline/uvcvideo.cpp  | 10 +++++++---
>  src/libcamera/pipeline/vimc.cpp      | 11 +++++++----
>  3 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 48d028f7e6cd9b4d..19f73011f8b75438 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -32,6 +32,8 @@ private:
>  	MediaDevice *cio2_;
>  	MediaDevice *imgu_;
>
> +	std::vector<std::shared_ptr<Camera>> cameras_;
> +

Instead of doing this in each pipeline hander, can't this be moved to
the base class?

>  	void registerCameras(CameraManager *manager);
>  };
>
> @@ -42,6 +44,8 @@ PipelineHandlerIPU3::PipelineHandlerIPU3()
>
>  PipelineHandlerIPU3::~PipelineHandlerIPU3()
>  {
> +	cameras_.clear();
> +
>  	if (cio2_)
>  		cio2_->release();
>
> @@ -173,7 +177,8 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
>  		std::string cameraName = sensor->name() + " " + std::to_string(id);
>  		std::shared_ptr<Camera> camera = Camera::create(cameraName,
>  								this);
> -		manager->addCamera(std::move(camera));
> +		cameras_.push_back(camera);

Be careful, this copies the shared reference, increasing the reference
count if I got this right. What's your opinion?


> +		manager->addCamera(camera);
>
>  		LOG(IPU3, Info)
>  			<< "Registered Camera[" << numCameras << "] \""
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 3651250b683e7810..2162824eb571fba7 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -24,15 +24,19 @@ public:
>
>  private:
>  	MediaDevice *dev_;
> +	std::shared_ptr<Camera> camera_;

Shouldn't this, and a method to register cameras to the manager should be
moved to the base class?

The base class can provide a "registerCamera" method,
that takes the camera name and the camera manager. It creates the
camera as shared_ptr, move it to the  'camera_' vector, and register
it with the camera manager (which receives it by value, and move it in
its own vector too).

What do you think? Have you considered it and went for this
implementation for some reason I am missing?

Thanks
   j


>  };
>
>  PipelineHandlerUVC::PipelineHandlerUVC()
> -	: dev_(nullptr)
> +	: dev_(nullptr), camera_(nullptr)
>  {
>  }
>
>  PipelineHandlerUVC::~PipelineHandlerUVC()
>  {
> +	if (camera_)
> +		camera_ = nullptr;
> +
>  	if (dev_)
>  		dev_->release();
>  }
> @@ -48,8 +52,8 @@ bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumera
>
>  	dev_->acquire();
>
> -	std::shared_ptr<Camera> camera = Camera::create(dev_->model(), this);
> -	manager->addCamera(std::move(camera));
> +	camera_ = Camera::create(dev_->model(), this);
> +	manager->addCamera(camera_);
>
>  	return true;
>  }
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 81d8319eb88e06d2..373fc0ee5339f9ae 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -24,15 +24,19 @@ public:
>
>  private:
>  	MediaDevice *dev_;
> +	std::shared_ptr<Camera> camera_;
>  };
>
>  PipeHandlerVimc::PipeHandlerVimc()
> -	: dev_(nullptr)
> +	: dev_(nullptr), camera_(nullptr)
>  {
>  }
>
>  PipeHandlerVimc::~PipeHandlerVimc()
>  {
> +	if (camera_)
> +		camera_ = nullptr;
> +
>  	if (dev_)
>  		dev_->release();
>  }
> @@ -57,9 +61,8 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
>
>  	dev_->acquire();
>
> -	std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera",
> -							this);
> -	manager->addCamera(std::move(camera));
> +	camera_ = Camera::create("Dummy VIMC Camera", this);
> +	manager->addCamera(camera_);
>
>  	return true;
>  }
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190123/fc542e2e/attachment.sig>