[libcamera-devel] [PATCH v2 01/11] libcamera: camera: Pass Private pointer to Camera constructor
Jacopo Mondi
jacopo at jmondi.org
Tue Aug 10 14:53:44 CEST 2021
Hi Laurent,
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 ?
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.
This probably does not only affect Camera but it might apply to
the Extensible in full.
Thanks
j
> 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