[libcamera-devel] [PATCH 2/3] libcamera: pipelines: keep reference to cameras created
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 23 10:37:41 CET 2019
Hi Jacopo,
Thanks for your feedback.
On 2019-01-23 10:04:14 +0100, Jacopo Mondi wrote:
> 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?
My idea here is that the object of the specific PipelineHandler
implementation (e.g. PipelineHandlerIPU3) that is i instantiated to
support a specific hardware is deleted when the hardware it represents
is deleted due to hot-plug. In this event the pipeline handler that is
deleted needs to inform all Camera objects it created that the hardware
is gone and it will serve no more requests.
So I'm not taking about removing the ability to create new instances of
a specific PipelineHandler, but rather that a instantiation of one that
is 'bound' to a hardware block would be deleted if the hardware is
removed.
As we both know a Camera object might live after the hardware is removed
and needs to be 'disconnected'. That is what happens in the last patch
in this series and that completes the life-time management of the
association between the Camera and PipelineHandler classes.
>
> >
> > 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?
I have considered it, and I should have outlined this in the cover
letter. Instead I added it to the cover letter of the series which
depends on this series. Sorry about that.
In essence I think it's a good idea that we should move as much as
possible to the base classes to remove boiler plate for the specific
implementations. I had on my todo list for today to try and improve this
in this and my other series. Your idea of a registerCamera() in the base
class sounds like the way to go so I will start with that, thanks for
the suggestion!
>
> 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list