| Message ID | 20210723040036.32346-9-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2021-07-23 07:00:27 +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@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); Going forward would it make sens to move the streams (and other standard but camera specific) property inside the Private class? I'm thinking it could be useful in the core code to have standard properties available by the Camera id_. This is unrelated to this patch however and if thought to be a good idea should be done on-top. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > ~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 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 >
Hi Niklas, On Sat, Jul 24, 2021 at 09:09:22AM +0200, Niklas Söderlund wrote: > On 2021-07-23 07:00:27 +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@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); > > Going forward would it make sens to move the streams (and other standard > but camera specific) property inside the Private class? I'm thinking it > could be useful in the core code to have standard properties available > by the Camera id_. This is unrelated to this patch however and if > thought to be a good idea should be done on-top. It's an idea I'm experimenting at the moment :-) > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > ~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 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;
Hi Laurent, 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() ? It will expose streams and id to the internal API but that shouldn't be an issue > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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; o> + d->streams_ = streams; That's a consequence of the friend Extensible; declaration in Extensible::Private ? 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 ? Thanks j > } > > 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 >
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@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;
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; } 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;
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@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(-)