[libcamera-devel] [RFC PATCH 08/17] libcamera: camera: Pass Private pointer to Camera constructor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 30 04:49:20 CEST 2021
Hi Jacopo,
On Thu, Jul 29, 2021 at 11:04:23PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 23, 2021 at 07:00:27AM +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.
>
> As suggested by Niklas, shouldn't stream and id be part of the Private
> class and the public API rely on d_->streams() ?
The id is already stored in the Camera::Private class. The pointers to
the streams are stored there too, but not the stream instances
themselves. This is what I believe Niklas was referring to, moving the
actual stream storage from the CameraData subclasses (turned into
Camera::Private subclasses in this series) into the Camera::Private
class itself. I've considered this, but it would prevent pipeline
handlers from subclassing the Stream class, which isn't nice. I've also
toyed with the idea of storing a std::vector<std::unique_ptr<Stream>> in
Camera::Private, which would allow pipeline handlers to subclass Stream,
but would make Camera::Private manage the lifetime of stream objects. It
ended up requiring pipeline handlers to always cast from Stream to their
Stream subclass, and didn't really bring much added value.
> It will expose streams and id to the internal API but that shouldn't
> be an issue
>
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 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,
> > 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;
>
> That's a consequence of the
> friend Extensible;
> declaration in Extensible::Private ?
Correct.
> In this case, it really makes the interface between the two classes
> permeable, I mean, as long as it's the public one that can access the
> private it's ok, but isn't it a too much of a shortcut that allow to
> circumvent the proper definition of a Public/Private interface ?
>From the point of view of the Camera, the Camera and Camera::Private
split is internal, and both classes form a whole. They can access each
other, in the same way that functions of the Camera class could access
all members, public, protected or private, if the Extensible mechanism
wasn't used. The Camera and Camera::Private classes are tightly coupled,
developed and maintained together, are not used separately. I thus don't
see a need to artificially prevent them from accessing some members of
their counterpart class.
The access specifiers to the Camera class are used to control what
members can be accessed by users of the public API, and to some extent
by internal users too. Similarly, the access specifiers to the
Camera::Private class are used to control what members can be accessed
by internal users.
> > }
> >
> > 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 f821d8fe1b6c..2411f73f48e0 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