[libcamera-devel] [RFC PATCH 08/17] libcamera: camera: Pass Private pointer to Camera constructor
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jul 24 09:09:22 CEST 2021
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 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);
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 at 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
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list