[{"id":18727,"web_url":"https://patchwork.libcamera.org/comment/18727/","msgid":"<20210812081256.6j74bwxuou54jml2@uno.localdomain>","date":"2021-08-12T08:12:56","subject":"Re: [libcamera-devel] [PATCH v3 02/14] libcamera: camera: Pass\n\tPrivate pointer to Camera constructor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Aug 12, 2021 at 02:26:13AM +0300, Laurent Pinchart wrote:\n> In order to allow subclassing Camera::Private in pipeline handlers, pass\n> the pointer to the private data to the Camera constructor, and to the\n> Camera::createCamera() function.\n>\n> The Camera::Private id_ and streams_ members now need to be initialized\n> by the Camera constructor instead of the Camera::Private constructor, to\n> allow storage of the streams in a pipeline handler-specific subclass of\n> Camera::Private.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n   j\n\n> ---\n>  include/libcamera/camera.h                    |  4 +--\n>  include/libcamera/internal/camera.h           |  3 +--\n>  src/libcamera/camera.cpp                      | 26 ++++++++++++-------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  4 ++-\n>  .../pipeline/raspberrypi/raspberrypi.cpp      |  4 ++-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 ++-\n>  src/libcamera/pipeline/simple/simple.cpp      |  4 ++-\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  5 +++-\n>  src/libcamera/pipeline/vimc/vimc.cpp          |  4 ++-\n>  9 files changed, 38 insertions(+), 20 deletions(-)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index b081907e0cb1..05cdab724b10 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -78,7 +78,7 @@ class Camera final : public Object, public std::enable_shared_from_this<Camera>,\n>  \tLIBCAMERA_DECLARE_PRIVATE()\n>\n>  public:\n> -\tstatic std::shared_ptr<Camera> create(PipelineHandler *pipe,\n> +\tstatic std::shared_ptr<Camera> create(std::unique_ptr<Private> d,\n>  \t\t\t\t\t      const std::string &id,\n>  \t\t\t\t\t      const std::set<Stream *> &streams);\n>\n> @@ -107,7 +107,7 @@ public:\n>  private:\n>  \tLIBCAMERA_DISABLE_COPY(Camera)\n>\n> -\tCamera(PipelineHandler *pipe, const std::string &id,\n> +\tCamera(std::unique_ptr<Private> d, const std::string &id,\n>  \t       const std::set<Stream *> &streams);\n>  \t~Camera();\n>\n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index b60ed140356a..9ec8321a9a21 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -26,8 +26,7 @@ class Camera::Private : public Extensible::Private\n>  \tLIBCAMERA_DECLARE_PUBLIC(Camera)\n>\n>  public:\n> -\tPrivate(PipelineHandler *pipe, const std::string &id,\n> -\t\tconst std::set<Stream *> &streams);\n> +\tPrivate(PipelineHandler *pipe);\n>  \t~Private();\n>\n>  private:\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 8e99e2a96eaf..d9f6b784e0dc 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -332,11 +332,13 @@ std::size_t CameraConfiguration::size() const\n>   * \\brief The vector of stream configurations\n>   */\n>\n> -Camera::Private::Private(PipelineHandler *pipe,\n> -\t\t\t const std::string &id,\n> -\t\t\t const std::set<Stream *> &streams)\n> -\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> -\t  disconnected_(false), state_(CameraAvailable)\n> +/**\n> + * \\brief Construct a Camera::Private instance\n> + * \\param[in] pipe The pipeline handler responsible for the camera device\n> + */\n> +Camera::Private::Private(PipelineHandler *pipe)\n> +\t: pipe_(pipe->shared_from_this()), disconnected_(false),\n> +\t  state_(CameraAvailable)\n>  {\n>  }\n>\n> @@ -513,7 +515,7 @@ void Camera::Private::setState(State state)\n>\n>  /**\n>   * \\brief Create a camera instance\n> - * \\param[in] pipe The pipeline handler responsible for the camera device\n> + * \\param[in] d Camera private data\n>   * \\param[in] id The ID of the camera device\n>   * \\param[in] streams Array of streams the camera provides\n>   *\n> @@ -527,10 +529,12 @@ void Camera::Private::setState(State state)\n>   *\n>   * \\return A shared pointer to the newly created camera object\n>   */\n> -std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n> +std::shared_ptr<Camera> Camera::create(std::unique_ptr<Private> d,\n>  \t\t\t\t       const std::string &id,\n>  \t\t\t\t       const std::set<Stream *> &streams)\n>  {\n> +\tASSERT(d);\n> +\n>  \tstruct Deleter : std::default_delete<Camera> {\n>  \t\tvoid operator()(Camera *camera)\n>  \t\t{\n> @@ -541,7 +545,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n>  \t\t}\n>  \t};\n>\n> -\tCamera *camera = new Camera(pipe, id, streams);\n> +\tCamera *camera = new Camera(std::move(d), id, streams);\n>\n>  \treturn std::shared_ptr<Camera>(camera, Deleter());\n>  }\n> @@ -594,10 +598,12 @@ const std::string &Camera::id() const\n>   * application API calls by returning errors immediately.\n>   */\n>\n> -Camera::Camera(PipelineHandler *pipe, const std::string &id,\n> +Camera::Camera(std::unique_ptr<Private> d, const std::string &id,\n>  \t       const std::set<Stream *> &streams)\n> -\t: Extensible(std::make_unique<Private>(pipe, id, streams))\n> +\t: Extensible(std::move(d))\n>  {\n> +\t_d()->id_ = id;\n> +\t_d()->streams_ = streams;\n>  }\n>\n>  Camera::~Camera()\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 19cb5c4ec9c3..b9c0941c5ea8 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -23,6 +23,7 @@\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>\n> +#include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/camera_sensor.h\"\n>  #include \"libcamera/internal/delayed_controls.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n> @@ -1199,7 +1200,8 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\t/* Create and register the Camera instance. */\n>  \t\tstd::string cameraId = cio2->sensor()->id();\n>  \t\tstd::shared_ptr<Camera> camera =\n> -\t\t\tCamera::create(this, cameraId, streams);\n> +\t\t\tCamera::create(std::make_unique<Camera::Private>(this),\n> +\t\t\t\t       cameraId, streams);\n>\n>  \t\tregisterCamera(std::move(camera), std::move(data));\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 2e8774ae360d..91e906ef14ce 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -29,6 +29,7 @@\n>  #include <linux/videodev2.h>\n>\n>  #include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/camera_sensor.h\"\n>  #include \"libcamera/internal/delayed_controls.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n> @@ -1105,7 +1106,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n>\n>  \t/* Create and register the camera. */\n>  \tstd::shared_ptr<Camera> camera =\n> -\t\tCamera::create(this, data->sensor_->id(), streams);\n> +\t\tCamera::create(std::make_unique<Camera::Private>(this),\n> +\t\t\t       data->sensor_->id(), streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n>\n>  \treturn true;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42911a8fdfdb..c2872289337b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -27,6 +27,7 @@\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>\n> +#include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/camera_sensor.h\"\n>  #include \"libcamera/internal/delayed_controls.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n> @@ -970,7 +971,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \t\t&data->selfPathStream_,\n>  \t};\n>  \tstd::shared_ptr<Camera> camera =\n> -\t\tCamera::create(this, data->sensor_->id(), streams);\n> +\t\tCamera::create(std::make_unique<Camera::Private>(this),\n> +\t\t\t       data->sensor_->id(), streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n>\n>  \treturn 0;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index b29fff9820e5..ad581969fca1 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -28,6 +28,7 @@\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>\n> +#include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/camera_sensor.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n>  #include \"libcamera/internal/media_device.h\"\n> @@ -1046,7 +1047,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>  \t\t\t       [](Stream &stream) { return &stream; });\n>\n>  \t\tstd::shared_ptr<Camera> camera =\n> -\t\t\tCamera::create(this, data->sensor_->id(), streams);\n> +\t\t\tCamera::create(std::make_unique<Camera::Private>(this),\n> +\t\t\t\t       data->sensor_->id(), streams);\n>  \t\tregisterCamera(std::move(camera), std::move(data));\n>  \t\tregistered = true;\n>  \t}\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 0f634b8da609..a9b6af4b5bdf 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -22,6 +22,7 @@\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>\n> +#include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n>  #include \"libcamera/internal/media_device.h\"\n>  #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -470,7 +471,9 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \t}\n>\n>  \tstd::set<Stream *> streams{ &data->stream_ };\n> -\tstd::shared_ptr<Camera> camera = Camera::create(this, id, streams);\n> +\tstd::shared_ptr<Camera> camera =\n> +\t\tCamera::create(std::make_unique<Camera::Private>(this), id,\n> +\t\t\t       streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n>\n>  \t/* Enable hot-unplug notifications. */\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 12f7517fd0ae..999d975716fa 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -29,6 +29,7 @@\n>  #include <libcamera/ipa/vimc_ipa_interface.h>\n>  #include <libcamera/ipa/vimc_ipa_proxy.h>\n>\n> +#include \"libcamera/internal/camera.h\"\n>  #include \"libcamera/internal/camera_sensor.h\"\n>  #include \"libcamera/internal/device_enumerator.h\"\n>  #include \"libcamera/internal/ipa_manager.h\"\n> @@ -438,7 +439,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \t/* Create and register the camera. */\n>  \tstd::set<Stream *> streams{ &data->stream_ };\n>  \tstd::shared_ptr<Camera> camera =\n> -\t\tCamera::create(this, data->sensor_->id(), streams);\n> +\t\tCamera::create(std::make_unique<Camera::Private>(this),\n> +\t\t\t       data->sensor_->id(), streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n>\n>  \treturn true;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C6DADC3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 12 Aug 2021 08:12:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A2A56884D;\n\tThu, 12 Aug 2021 10:12:10 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1D0060264\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 12 Aug 2021 10:12:08 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 3A2E1FF806;\n\tThu, 12 Aug 2021 08:12:07 +0000 (UTC)"],"Date":"Thu, 12 Aug 2021 10:12:56 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210812081256.6j74bwxuou54jml2@uno.localdomain>","References":"<20210811232625.17280-1-laurent.pinchart@ideasonboard.com>\n\t<20210811232625.17280-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210811232625.17280-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 02/14] libcamera: camera: Pass\n\tPrivate pointer to Camera constructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]