[libcamera-devel] [PATCH v2 01/11] libcamera: camera: Pass Private pointer to Camera constructor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 11 18:52:25 CEST 2021
Hi Jacopo,
On Tue, Aug 10, 2021 at 02:53:44PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 05, 2021 at 08:58:38PM +0300, Laurent Pinchart wrote:
> > In order to allow subclassing Camera::Private in pipeline handlers, pass
> > the pointer to the private data to the Camera constructor, and to the
> > Camera::createCamera() function.
> >
> > The Camera::Private id_ and streams_ members now need to be initialized
> > by the Camera constructor instead of the Camera::Private constructor, to
> > allow storage of the streams in a pipeline handler-specific subclass of
> > Camera::Private.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 5 ++--
> > include/libcamera/internal/camera.h | 3 +--
> > src/libcamera/camera.cpp | 26 ++++++++++++-------
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-
> > .../pipeline/raspberrypi/raspberrypi.cpp | 4 ++-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++-
> > src/libcamera/pipeline/simple/simple.cpp | 4 ++-
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++-
> > src/libcamera/pipeline/vimc/vimc.cpp | 4 ++-
> > 9 files changed, 37 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index b081907e0cb1..17ddddc2722a 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -78,8 +78,7 @@ class Camera final : public Object, public std::enable_shared_from_this<Camera>,
> > LIBCAMERA_DECLARE_PRIVATE()
> >
> > public:
> > - static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> > - const std::string &id,
> > + static std::shared_ptr<Camera> create(Private *d, const std::string &id,
> > const std::set<Stream *> &streams);
> >
> > const std::string &id() const;
> > @@ -107,7 +106,7 @@ public:
> > private:
> > LIBCAMERA_DISABLE_COPY(Camera)
> >
> > - Camera(PipelineHandler *pipe, const std::string &id,
> > + Camera(Private *d, const std::string &id,
>
> As discussed offline, is passing a raw pointer the best solution here ?
Absolutely not :-) I'll add a new patch at the bottom of this series to
replace the Private * passed to the Extensible constructor with a
std::unique_ptr<Private>. The rest of the series will be adapted
accordingly.
> If the creation of the Private instance is now disjointed from the
> construction of the Extensible derived class, shouldn't its lifetime
> be managed by the class that constructed it, or rather moved here.
Yes, it should be moved. Managing it in the caller would be cumbersome
and error prone.
> This probably does not only affect Camera but it might apply to
> the Extensible in full.
>
> > const std::set<Stream *> &streams);
> > ~Camera();
> >
> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > index b60ed140356a..9ec8321a9a21 100644
> > --- a/include/libcamera/internal/camera.h
> > +++ b/include/libcamera/internal/camera.h
> > @@ -26,8 +26,7 @@ class Camera::Private : public Extensible::Private
> > LIBCAMERA_DECLARE_PUBLIC(Camera)
> >
> > public:
> > - Private(PipelineHandler *pipe, const std::string &id,
> > - const std::set<Stream *> &streams);
> > + Private(PipelineHandler *pipe);
> > ~Private();
> >
> > private:
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 4b5bc891fc37..a5bb60c698bc 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -332,11 +332,13 @@ std::size_t CameraConfiguration::size() const
> > * \brief The vector of stream configurations
> > */
> >
> > -Camera::Private::Private(PipelineHandler *pipe,
> > - const std::string &id,
> > - const std::set<Stream *> &streams)
> > - : pipe_(pipe->shared_from_this()), id_(id), streams_(streams),
> > - disconnected_(false), state_(CameraAvailable)
> > +/**
> > + * \brief Construct a Camera::Private instance
> > + * \param[in] pipe The pipeline handler responsible for the camera device
> > + */
> > +Camera::Private::Private(PipelineHandler *pipe)
> > + : pipe_(pipe->shared_from_this()), disconnected_(false),
> > + state_(CameraAvailable)
> > {
> > }
> >
> > @@ -513,7 +515,7 @@ void Camera::Private::setState(State state)
> >
> > /**
> > * \brief Create a camera instance
> > - * \param[in] pipe The pipeline handler responsible for the camera device
> > + * \param[in] d Camera private data
> > * \param[in] id The ID of the camera device
> > * \param[in] streams Array of streams the camera provides
> > *
> > @@ -527,10 +529,12 @@ void Camera::Private::setState(State state)
> > *
> > * \return A shared pointer to the newly created camera object
> > */
> > -std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> > +std::shared_ptr<Camera> Camera::create(Private *d,
> > const std::string &id,
> > const std::set<Stream *> &streams)
> > {
> > + ASSERT(d);
> > +
> > struct Deleter : std::default_delete<Camera> {
> > void operator()(Camera *camera)
> > {
> > @@ -541,7 +545,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> > }
> > };
> >
> > - Camera *camera = new Camera(pipe, id, streams);
> > + Camera *camera = new Camera(d, id, streams);
> >
> > return std::shared_ptr<Camera>(camera, Deleter());
> > }
> > @@ -594,10 +598,12 @@ const std::string &Camera::id() const
> > * application API calls by returning errors immediately.
> > */
> >
> > -Camera::Camera(PipelineHandler *pipe, const std::string &id,
> > +Camera::Camera(Private *d, const std::string &id,
> > const std::set<Stream *> &streams)
> > - : Extensible(new Private(pipe, id, streams))
> > + : Extensible(d)
> > {
> > + d->id_ = id;
> > + d->streams_ = streams;
> > }
> >
> > Camera::~Camera()
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 76c3bb3d8aa9..15d6cca609ff 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -23,6 +23,7 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera.h"
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/delayed_controls.h"
> > #include "libcamera/internal/device_enumerator.h"
> > @@ -1185,7 +1186,8 @@ int PipelineHandlerIPU3::registerCameras()
> > /* Create and register the Camera instance. */
> > std::string cameraId = cio2->sensor()->id();
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, cameraId, streams);
> > + Camera::create(new Camera::Private(this), cameraId,
> > + streams);
> >
> > registerCamera(std::move(camera), std::move(data));
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0bab3bedd402..33cd5765b52e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -30,6 +30,7 @@
> > #include <linux/videodev2.h>
> >
> > #include "libcamera/internal/bayer_format.h"
> > +#include "libcamera/internal/camera.h"
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/delayed_controls.h"
> > #include "libcamera/internal/device_enumerator.h"
> > @@ -1106,7 +1107,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >
> > /* Create and register the camera. */
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, data->sensor_->id(), streams);
> > + Camera::create(new Camera::Private(this), data->sensor_->id(),
> > + streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > return true;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 42911a8fdfdb..4a8ac97d5ef0 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -27,6 +27,7 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera.h"
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/delayed_controls.h"
> > #include "libcamera/internal/device_enumerator.h"
> > @@ -970,7 +971,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > &data->selfPathStream_,
> > };
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, data->sensor_->id(), streams);
> > + Camera::create(new Camera::Private(this), data->sensor_->id(),
> > + streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > return 0;
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index b29fff9820e5..43af3fafa475 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -28,6 +28,7 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera.h"
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/device_enumerator.h"
> > #include "libcamera/internal/media_device.h"
> > @@ -1046,7 +1047,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > [](Stream &stream) { return &stream; });
> >
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, data->sensor_->id(), streams);
> > + Camera::create(new Camera::Private(this),
> > + data->sensor_->id(), streams);
> > registerCamera(std::move(camera), std::move(data));
> > registered = true;
> > }
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 0f634b8da609..63cb1daaae22 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -22,6 +22,7 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera.h"
> > #include "libcamera/internal/device_enumerator.h"
> > #include "libcamera/internal/media_device.h"
> > #include "libcamera/internal/pipeline_handler.h"
> > @@ -470,7 +471,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > }
> >
> > std::set<Stream *> streams{ &data->stream_ };
> > - std::shared_ptr<Camera> camera = Camera::create(this, id, streams);
> > + std::shared_ptr<Camera> camera =
> > + Camera::create(new Camera::Private(this), id, streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > /* Enable hot-unplug notifications. */
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 12f7517fd0ae..d63562b1ee54 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -29,6 +29,7 @@
> > #include <libcamera/ipa/vimc_ipa_interface.h>
> > #include <libcamera/ipa/vimc_ipa_proxy.h>
> >
> > +#include "libcamera/internal/camera.h"
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/device_enumerator.h"
> > #include "libcamera/internal/ipa_manager.h"
> > @@ -438,7 +439,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> > /* Create and register the camera. */
> > std::set<Stream *> streams{ &data->stream_ };
> > std::shared_ptr<Camera> camera =
> > - Camera::create(this, data->sensor_->id(), streams);
> > + Camera::create(new Camera::Private(this), data->sensor_->id(),
> > + streams);
> > registerCamera(std::move(camera), std::move(data));
> >
> > return true;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list