| Message ID | 20190122232955.31783-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Jan 23, 2019 at 10:37:41AM +0100, Niklas Söderlund wrote: > 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. I see. Thinking about pipeline handler managing ISPs embedded in the SoC the only "removal" use case is driver unbinding then, while you expect, say, the UVC pipeline handler to be removed once the USB camera is disconnected. As I understood this, the DeviceEnumerator should catch those events, notify the pipeline manager that it has to invalidate all cameras (as they might outlive the pipeline handler) and then "unmatch" it, which involves deleting the pipeline handler instance. It seems we're on the same page, right? > > 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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2019-01-23 10:51:02 +0100, Jacopo Mondi wrote: > Hi Niklas, > > On Wed, Jan 23, 2019 at 10:37:41AM +0100, Niklas Söderlund wrote: > > 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. > > I see. Thinking about pipeline handler managing ISPs embedded in the > SoC the only "removal" use case is driver unbinding then, while you > expect, say, the UVC pipeline handler to be removed once the USB > camera is disconnected. > > As I understood this, the DeviceEnumerator should catch those events, > notify the pipeline manager that it has to invalidate all cameras (as > they might outlive the pipeline handler) and then "unmatch" it, which > involves deleting the pipeline handler instance. It seems we're on the > same page, right? Yes it seems so :-) In the current design the DeviceEnumerator would simply delete the pipeline handler when it detects that the hardware is being removed. How it will know which pipeline to delete, and how to react to one entity in a media graph and related issue is still not known. > > > > > 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@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > -- > > Regards, > > Niklas Söderlund
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_; + 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); + 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_; }; 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; }
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. 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@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(-)