[libcamera-devel] [PATCH 1/3] libcamera: camera: create a association with the responsible pipeline handler
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 23 19:03:59 CET 2019
Hi Niklas,
Thank you for the patch.
On Wed, Jan 23, 2019 at 10:10:44AM +0100, Niklas Söderlund wrote:
> On 2019-01-23 09:56:35 +0100, Jacopo Mondi wrote:
> > On Wed, Jan 23, 2019 at 12:29:53AM +0100, Niklas Söderlund wrote:
> >> The PipelineHandler which creates a Camera is responsible for serving
> >> any operation requested by the user. In order to do so the Camera needs
> >> to store a reference to its creator.
> >>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >> ---
> >> include/libcamera/camera.h | 8 ++++++--
> >> src/libcamera/camera.cpp | 15 +++++++++------
> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-
> >> src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> >> src/libcamera/pipeline/vimc.cpp | 10 ++--------
> >> 5 files changed, 20 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> >> index 2ea1a6883311cf9f..d3bae4cbee1e0cea 100644
> >> --- a/include/libcamera/camera.h
> >> +++ b/include/libcamera/camera.h
> >> @@ -12,10 +12,13 @@
> >>
> >> namespace libcamera {
> >>
> >> +class PipelineHandler;
> >> +
> >> class Camera final
> >> {
> >> public:
> >> - static std::shared_ptr<Camera> create(const std::string &name);
> >> + static std::shared_ptr<Camera> create(const std::string &name,
> >> + class PipelineHandler *pipe);
> >>
> >> Camera(const Camera &) = delete;
> >> void operator=(const Camera &) = delete;
> >> @@ -23,10 +26,11 @@ public:
> >> const std::string &name() const;
> >>
> >> private:
> >> - explicit Camera(const std::string &name);
> >> + explicit Camera(const std::string &name, class PipelineHandler *pipe);
> >
> > You can drop the 'explicit' here, as it avoid copy-construction and
> > conversion-contruction, which can't happen with 2 parameters (if I got
> > this right :)
>
> Thanks, did not know that will fix.
>
> >
> > I'm fine with rest, with the minor thing that I feel more natural to
> > have 'this' passed as first parameter of the Camera constructor.
>
> I agree, I will swap the arguments around!
I would have said the other way around, I think that the camera is first
qualified by its name, then by its pipeline handler, but it's not a big
deal :-)
> >> ~Camera();
> >>
> >> std::string name_;
> >> + class PipelineHandler *pipe_;
This screams of lifetime management issues. How about storing it as a
std::shared_ptr<>, given that it will potentially be shared by multiple
cameras, and should not be destroyed as long as the camera object keeps
a reference to the pipeline handler (otherwise the pipe->*() call
fowarding will bring painful surprises) ?
> >> };
> >>
> >> } /* namespace libcamera */
> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> >> index acf912bee95cbec4..f198eb4978b12239 100644
> >> --- a/src/libcamera/camera.cpp
> >> +++ b/src/libcamera/camera.cpp
> >> @@ -52,17 +52,20 @@ namespace libcamera {
> >> /**
> >> * \brief Create a camera instance
> >> * \param[in] name The name of the camera device
> >> + * \param[in] pipe The pipeline handler responsible for the camera device
> >> *
> >> * The caller is responsible for guaranteeing unicity of the camera name.
> >> *
> >> * \return A shared pointer to the newly created camera object
> >> */
> >> -std::shared_ptr<Camera> Camera::create(const std::string &name)
> >> +std::shared_ptr<Camera> Camera::create(const std::string &name,
> >> + class PipelineHandler *pipe)
> >> {
> >> struct Allocator : std::allocator<Camera> {
> >> - void construct(void *p, const std::string &name)
> >> + void construct(void *p, const std::string &name,
> >> + class PipelineHandler *pipe)
> >> {
> >> - ::new(p) Camera(name);
> >> + ::new(p) Camera(name, pipe);
> >> }
> >> void destroy(Camera *p)
> >> {
> >> @@ -70,7 +73,7 @@ std::shared_ptr<Camera> Camera::create(const std::string &name)
> >> }
> >> };
> >>
> >> - return std::allocate_shared<Camera>(Allocator(), name);
> >> + return std::allocate_shared<Camera>(Allocator(), name, pipe);
> >> }
> >>
> >> /**
> >> @@ -83,8 +86,8 @@ const std::string &Camera::name() const
> >> return name_;
> >> }
> >>
> >> -Camera::Camera(const std::string &name)
> >> - : name_(name)
> >> +Camera::Camera(const std::string &name, class PipelineHandler *pipe)
> >> + : name_(name), pipe_(pipe)
> >> {
> >> }
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 8cbc10acfbb571fd..48d028f7e6cd9b4d 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -171,7 +171,8 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
> >> continue;
> >>
> >> std::string cameraName = sensor->name() + " " + std::to_string(id);
> >> - std::shared_ptr<Camera> camera = Camera::create(cameraName);
> >> + std::shared_ptr<Camera> camera = Camera::create(cameraName,
> >> + this);
> >> manager->addCamera(std::move(camera));
> >>
> >> LOG(IPU3, Info)
> >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >> index 3ba69da8b77586e3..3651250b683e7810 100644
> >> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >> @@ -48,7 +48,7 @@ bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumera
> >>
> >> dev_->acquire();
> >>
> >> - std::shared_ptr<Camera> camera = Camera::create(dev_->model());
> >> + std::shared_ptr<Camera> camera = Camera::create(dev_->model(), this);
> >> manager->addCamera(std::move(camera));
> >>
> >> return true;
> >> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> >> index 82b9237a3d7d93e5..81d8319eb88e06d2 100644
> >> --- a/src/libcamera/pipeline/vimc.cpp
> >> +++ b/src/libcamera/pipeline/vimc.cpp
> >> @@ -57,14 +57,8 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
> >>
> >> dev_->acquire();
> >>
> >> - /*
> >> - * NOTE: A more complete Camera implementation could
> >> - * be passed the MediaDevice(s) it controls here or
> >> - * a reference to the PipelineHandler. Which method
> >> - * will be chosen depends on how the Camera
> >> - * object is modeled.
> >> - */
> >> - std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
> >> + std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera",
> >> + this);
> >> manager->addCamera(std::move(camera));
> >>
> >> return true;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list