[{"id":18725,"web_url":"https://patchwork.libcamera.org/comment/18725/","msgid":"<20210812075411.5no2a5zvvcjq5feg@uno.localdomain>","date":"2021-08-12T07:54:11","subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: pipeline_handler:\n\tMove CameraData members to Camera::Private","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:14AM +0300, Laurent Pinchart wrote:\n> With pipeline handlers now being able to subclass Camera::Private, start\n> the migration from CameraData to Camera::Private by moving the members\n> of the base CameraData class. The controlInfo_, properties_ and pipe_\n> members are duplicated for now, to allow migrating pipeline handlers one\n> by one.\n>\n> The Camera::Private class is now properly documented, don't exclude it\n> from documentation generation.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> Changes since v2:\n>\n> - Include where appropriate\n> - Minor documentation update\n>\n> Changes since v1:\n>\n> - Add \\todo comment for controlInfo_\n> ---\n> Documentation/Doxyfile.in | 1 -\n> include/libcamera/internal/camera.h | 9 +++\n> include/libcamera/internal/pipeline_handler.h | 6 +-\n> src/libcamera/camera.cpp | 65 ++++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 45 +++++--------\n> 6 files changed, 89 insertions(+), 39 deletions(-)\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index fb321ad25f6f..e3b77428cd4f 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -882,7 +882,6 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> libcamera::BoundMethodPack \\\n> libcamera::BoundMethodPackBase \\\n> libcamera::BoundMethodStatic \\\n> - libcamera::Camera::Private \\\n> libcamera::CameraManager::Private \\\n> libcamera::SignalBase \\\n> *::details \\\n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 9ec8321a9a21..1a08da0cedc4 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -8,6 +8,7 @@\n> #define __LIBCAMERA_INTERNAL_CAMERA_H__\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -29,6 +30,14 @@ public:\n> \tPrivate(PipelineHandler *pipe);\n> \t~Private();\n>\n> +\tPipelineHandler *pipe() { return pipe_.get(); }\n> +\n> +\tstd::list queuedRequests_;\n> +\tControlInfoMap controlInfo_;\n> +\tControlList properties_;\n> +\n> +\tuint32_t requestSequence_;\n> +\n> private:\n> \tenum State {\n> \t\tCameraAvailable,\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 9e2d65d6f2c5..24b0c5ca081c 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -7,7 +7,6 @@\n> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n>\n> -#include \n> #include \n> #include \n> #include \n> @@ -39,18 +38,15 @@ class CameraData\n> {\n> public:\n> \texplicit CameraData(PipelineHandler *pipe)\n> -\t\t: pipe_(pipe), requestSequence_(0)\n> +\t\t: pipe_(pipe)\n> \t{\n> \t}\n> \tvirtual ~CameraData() = default;\n>\n> \tPipelineHandler *pipe_;\n> -\tstd::list queuedRequests_;\n> \tControlInfoMap controlInfo_;\n> \tControlList properties_;\n>\n> -\tuint32_t requestSequence_;\n> -\n> private:\n> \tLIBCAMERA_DISABLE_COPY(CameraData)\n> };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index d9f6b784e0dc..4080da151af1 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -332,13 +332,25 @@ std::size_t CameraConfiguration::size() const\n> * \\brief The vector of stream configurations\n> */\n>\n> +/**\n> + * \\class Camera::Private\n> + * \\brief Base class for camera private data\n> + *\n> + * The Camera::Private class stores all private data associated with a camera.\n> + * In addition to hiding core Camera data from the public API, it is expected to\n> + * be subclassed by pipeline handlers to store pipeline-specific data.\n> + *\n> + * Pipeline handlers can obtain the Camera::Private instance associated with a\n> + * camera by calling Camera::_d().\n> + */\n> +\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> +\t: requestSequence_(0), pipe_(pipe->shared_from_this()),\n> +\t disconnected_(false), state_(CameraAvailable)\n> {\n> }\n>\n> @@ -348,6 +360,55 @@ Camera::Private::~Private()\n> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> }\n>\n> +/**\n> + * \\fn Camera::Private::pipe()\n> + * \\brief Retrieve the pipeline handler related to this camera\n> + * \\return The pipeline handler that created this camera\n> + */\n> +\n> +/**\n> + * \\var Camera::Private::queuedRequests_\n> + * \\brief The list of queued and not yet completed request\n> + *\n> + * The list of queued request is used to track requests queued in order to\n> + * ensure completion of all requests when the pipeline handler is stopped.\n> + *\n> + * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> + * PipelineHandler::completeRequest()\n> + */\n> +\n> +/**\n> + * \\var Camera::Private::controlInfo_\n> + * \\brief The set of controls supported by the camera\n> + *\n> + * The control information shall be initialised by the pipeline handler when\n> + * creating the camera.\n> + *\n> + * \\todo This member was initially meant to stay constant after the camera is\n> + * created. Several pipeline handlers are already updating it when the camera\n> + * is configured. Update the documentation accordingly, and possibly the API as\n> + * well, when implementing official support for control info updates.\n> + */\n> +\n> +/**\n> + * \\var Camera::Private::properties_\n> + * \\brief The list of properties supported by the camera\n> + *\n> + * The list of camera properties shall be initialised by the pipeline handler\n> + * when creating the camera, and shall not be modified afterwards.\n> + */\n> +\n> +/**\n> + * \\var Camera::Private::requestSequence_\n> + * \\brief The queuing sequence of the request\n> + *\n> + * When requests are queued, they are given a per-camera sequence number to\n> + * facilitate debugging of internal request usage.\n> + *\n> + * The requestSequence_ tracks the number of requests queued to a camera\n> + * over its lifetime.\n> + */\n> +\n> static const char *const camera_state_names[] = {\n> \t\"Available\",\n> \t\"Acquired\",\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c2872289337b..cebd94b2c18b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -847,7 +847,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> \t\tLOG(RkISP1, Warning)\n> \t\t\t<< \"Failed to stop parameters for \" << camera->id();\n>\n> -\tASSERT(data->queuedRequests_.empty());\n> +\tASSERT(camera->_d()->queuedRequests_.empty());\n> \tdata->frameInfo_.clear();\n>\n> \tfreeBuffers(camera);\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 1ab237c8052e..0e571d8981df 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \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/tracepoints.h\"\n> @@ -71,17 +72,6 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * and remains valid until the instance is destroyed.\n> */\n>\n> -/**\n> - * \\var CameraData::queuedRequests_\n> - * \\brief The list of queued and not yet completed request\n> - *\n> - * The list of queued request is used to track requests queued in order to\n> - * ensure completion of all requests when the pipeline handler is stopped.\n> - *\n> - * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> - * PipelineHandler::completeRequest()\n> - */\n> -\n> /**\n> * \\var CameraData::controlInfo_\n> * \\brief The set of controls supported by the camera\n> @@ -98,17 +88,6 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * when creating the camera, and shall not be modified afterwards.\n> */\n>\n> -/**\n> - * \\var CameraData::requestSequence_\n> - * \\brief The queuing sequence of the request\n> - *\n> - * When requests are queued, they are given a per-camera sequence number to\n> - * facilitate debugging of internal request usage.\n> - *\n> - * The requestSequence_ tracks the number of requests queued to a camera\n> - * over its lifetime.\n> - */\n> -\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> @@ -254,8 +233,7 @@ void PipelineHandler::unlock()\n> */\n> const ControlInfoMap &PipelineHandler::controls(const Camera *camera) const\n> {\n> -\tconst CameraData *data = cameraData(camera);\n> -\treturn data->controlInfo_;\n> +\treturn camera->_d()->controlInfo_;\n> }\n>\n> /**\n> @@ -265,8 +243,7 @@ const ControlInfoMap &PipelineHandler::controls(const Camera *camera) const\n> */\n> const ControlList &PipelineHandler::properties(const Camera *camera) const\n> {\n> -\tconst CameraData *data = cameraData(camera);\n> -\treturn data->properties_;\n> +\treturn camera->_d()->properties_;\n> }\n>\n> /**\n> @@ -380,8 +357,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> */\n> bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> {\n> -\tconst CameraData *data = cameraData(camera);\n> -\treturn !data->queuedRequests_.empty();\n> +\treturn !camera->_d()->queuedRequests_.empty();\n> }\n>\n> /**\n> @@ -406,7 +382,7 @@ void PipelineHandler::queueRequest(Request *request)\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n>\n> \tCamera *camera = request->camera_;\n> -\tCameraData *data = cameraData(camera);\n> +\tCamera::Private *data = camera->_d();\n> \tdata->queuedRequests_.push_back(request);\n>\n> \trequest->sequence_ = data->requestSequence_++;\n> @@ -479,7 +455,7 @@ void PipelineHandler::completeRequest(Request *request)\n>\n> \trequest->complete();\n>\n> -\tCameraData *data = cameraData(camera);\n> +\tCamera::Private *data = camera->_d();\n>\n> \twhile (!data->queuedRequests_.empty()) {\n> \t\tRequest *req = data->queuedRequests_.front();\n> @@ -507,6 +483,15 @@ void PipelineHandler::completeRequest(Request *request)\n> void PipelineHandler::registerCamera(std::shared_ptr camera,\n> \t\t\t\t std::unique_ptr data)\n> {\n> +\t/*\n> +\t * To be removed once all pipeline handlers will have migrated from\n> +\t * CameraData to Camera::Private.\n> +\t */\n> +\tif (data) {\n> +\t\tcamera->_d()->controlInfo_ = std::move(data->controlInfo_);\n> +\t\tcamera->_d()->properties_ = std::move(data->properties_);\n\nWith the introduction of std::move() you would need \n\nAs this goes away in the next patches I think it's anyway ok\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\t}\n> +\n> \tcameraData_[camera.get()] = std::move(data);\n> \tcameras_.push_back(camera);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","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 A7343C3240\n\tfor ;\n\tThu, 12 Aug 2021 07:53:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 728A468864;\n\tThu, 12 Aug 2021 09:53:25 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2426760264\n\tfor ;\n\tThu, 12 Aug 2021 09:53:24 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 75B71C000A;\n\tThu, 12 Aug 2021 07:53:23 +0000 (UTC)"],"Date":"Thu, 12 Aug 2021 09:54:11 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210812075411.5no2a5zvvcjq5feg@uno.localdomain>","References":"<20210811232625.17280-1-laurent.pinchart@ideasonboard.com>\n\t<20210811232625.17280-4-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-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: pipeline_handler:\n\tMove CameraData members to Camera::Private","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18735,"web_url":"https://patchwork.libcamera.org/comment/18735/","msgid":"","date":"2021-08-12T12:54:09","subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: pipeline_handler:\n\tMove CameraData members to Camera::Private","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Aug 12, 2021 at 09:54:11AM +0200, Jacopo Mondi wrote:\n> On Thu, Aug 12, 2021 at 02:26:14AM +0300, Laurent Pinchart wrote:\n> > With pipeline handlers now being able to subclass Camera::Private, start\n> > the migration from CameraData to Camera::Private by moving the members\n> > of the base CameraData class. The controlInfo_, properties_ and pipe_\n> > members are duplicated for now, to allow migrating pipeline handlers one\n> > by one.\n> >\n> > The Camera::Private class is now properly documented, don't exclude it\n> > from documentation generation.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > Changes since v2:\n> >\n> > - Include where appropriate\n> > - Minor documentation update\n> >\n> > Changes since v1:\n> >\n> > - Add \\todo comment for controlInfo_\n> > ---\n> > Documentation/Doxyfile.in | 1 -\n> > include/libcamera/internal/camera.h | 9 +++\n> > include/libcamera/internal/pipeline_handler.h | 6 +-\n> > src/libcamera/camera.cpp | 65 ++++++++++++++++++-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 45 +++++--------\n> > 6 files changed, 89 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index fb321ad25f6f..e3b77428cd4f 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -882,7 +882,6 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> > libcamera::BoundMethodPack \\\n> > libcamera::BoundMethodPackBase \\\n> > libcamera::BoundMethodStatic \\\n> > - libcamera::Camera::Private \\\n> > libcamera::CameraManager::Private \\\n> > libcamera::SignalBase \\\n> > *::details \\\n> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > index 9ec8321a9a21..1a08da0cedc4 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -8,6 +8,7 @@\n> > #define __LIBCAMERA_INTERNAL_CAMERA_H__\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -29,6 +30,14 @@ public:\n> > \tPrivate(PipelineHandler *pipe);\n> > \t~Private();\n> >\n> > +\tPipelineHandler *pipe() { return pipe_.get(); }\n> > +\n> > +\tstd::list queuedRequests_;\n> > +\tControlInfoMap controlInfo_;\n> > +\tControlList properties_;\n> > +\n> > +\tuint32_t requestSequence_;\n> > +\n> > private:\n> > \tenum State {\n> > \t\tCameraAvailable,\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 9e2d65d6f2c5..24b0c5ca081c 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -7,7 +7,6 @@\n> > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> >\n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -39,18 +38,15 @@ class CameraData\n> > {\n> > public:\n> > \texplicit CameraData(PipelineHandler *pipe)\n> > -\t\t: pipe_(pipe), requestSequence_(0)\n> > +\t\t: pipe_(pipe)\n> > \t{\n> > \t}\n> > \tvirtual ~CameraData() = default;\n> >\n> > \tPipelineHandler *pipe_;\n> > -\tstd::list queuedRequests_;\n> > \tControlInfoMap controlInfo_;\n> > \tControlList properties_;\n> >\n> > -\tuint32_t requestSequence_;\n> > -\n> > private:\n> > \tLIBCAMERA_DISABLE_COPY(CameraData)\n> > };\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index d9f6b784e0dc..4080da151af1 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -332,13 +332,25 @@ std::size_t CameraConfiguration::size() const\n> > * \\brief The vector of stream configurations\n> > */\n> >\n> > +/**\n> > + * \\class Camera::Private\n> > + * \\brief Base class for camera private data\n> > + *\n> > + * The Camera::Private class stores all private data associated with a camera.\n> > + * In addition to hiding core Camera data from the public API, it is expected to\n> > + * be subclassed by pipeline handlers to store pipeline-specific data.\n> > + *\n> > + * Pipeline handlers can obtain the Camera::Private instance associated with a\n> > + * camera by calling Camera::_d().\n> > + */\n> > +\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> > +\t: requestSequence_(0), pipe_(pipe->shared_from_this()),\n> > +\t disconnected_(false), state_(CameraAvailable)\n> > {\n> > }\n> >\n> > @@ -348,6 +360,55 @@ Camera::Private::~Private()\n> > \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > }\n> >\n> > +/**\n> > + * \\fn Camera::Private::pipe()\n> > + * \\brief Retrieve the pipeline handler related to this camera\n> > + * \\return The pipeline handler that created this camera\n> > + */\n> > +\n> > +/**\n> > + * \\var Camera::Private::queuedRequests_\n> > + * \\brief The list of queued and not yet completed request\n> > + *\n> > + * The list of queued request is used to track requests queued in order to\n> > + * ensure completion of all requests when the pipeline handler is stopped.\n> > + *\n> > + * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> > + * PipelineHandler::completeRequest()\n> > + */\n> > +\n> > +/**\n> > + * \\var Camera::Private::controlInfo_\n> > + * \\brief The set of controls supported by the camera\n> > + *\n> > + * The control information shall be initialised by the pipeline handler when\n> > + * creating the camera.\n> > + *\n> > + * \\todo This member was initially meant to stay constant after the camera is\n> > + * created. Several pipeline handlers are already updating it when the camera\n> > + * is configured. Update the documentation accordingly, and possibly the API as\n> > + * well, when implementing official support for control info updates.\n> > + */\n> > +\n> > +/**\n> > + * \\var Camera::Private::properties_\n> > + * \\brief The list of properties supported by the camera\n> > + *\n> > + * The list of camera properties shall be initialised by the pipeline handler\n> > + * when creating the camera, and shall not be modified afterwards.\n> > + */\n> > +\n> > +/**\n> > + * \\var Camera::Private::requestSequence_\n> > + * \\brief The queuing sequence of the request\n> > + *\n> > + * When requests are queued, they are given a per-camera sequence number to\n> > + * facilitate debugging of internal request usage.\n> > + *\n> > + * The requestSequence_ tracks the number of requests queued to a camera\n> > + * over its lifetime.\n> > + */\n> > +\n> > static const char *const camera_state_names[] = {\n> > \t\"Available\",\n> > \t\"Acquired\",\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index c2872289337b..cebd94b2c18b 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -847,7 +847,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > \t\tLOG(RkISP1, Warning)\n> > \t\t\t<< \"Failed to stop parameters for \" << camera->id();\n> >\n> > -\tASSERT(data->queuedRequests_.empty());\n> > +\tASSERT(camera->_d()->queuedRequests_.empty());\n> > \tdata->frameInfo_.clear();\n> >\n> > \tfreeBuffers(camera);\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 1ab237c8052e..0e571d8981df 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \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/tracepoints.h\"\n> > @@ -71,17 +72,6 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * and remains valid until the instance is destroyed.\n> > */\n> >\n> > -/**\n> > - * \\var CameraData::queuedRequests_\n> > - * \\brief The list of queued and not yet completed request\n> > - *\n> > - * The list of queued request is used to track requests queued in order to\n> > - * ensure completion of all requests when the pipeline handler is stopped.\n> > - *\n> > - * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> > - * PipelineHandler::completeRequest()\n> > - */\n> > -\n> > /**\n> > * \\var CameraData::controlInfo_\n> > * \\brief The set of controls supported by the camera\n> > @@ -98,17 +88,6 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * when creating the camera, and shall not be modified afterwards.\n> > */\n> >\n> > -/**\n> > - * \\var CameraData::requestSequence_\n> > - * \\brief The queuing sequence of the request\n> > - *\n> > - * When requests are queued, they are given a per-camera sequence number to\n> > - * facilitate debugging of internal request usage.\n> > - *\n> > - * The requestSequence_ tracks the number of requests queued to a camera\n> > - * over its lifetime.\n> > - */\n> > -\n> > /**\n> > * \\class PipelineHandler\n> > * \\brief Create and manage cameras based on a set of media devices\n> > @@ -254,8 +233,7 @@ void PipelineHandler::unlock()\n> > */\n> > const ControlInfoMap &PipelineHandler::controls(const Camera *camera) const\n> > {\n> > -\tconst CameraData *data = cameraData(camera);\n> > -\treturn data->controlInfo_;\n> > +\treturn camera->_d()->controlInfo_;\n> > }\n> >\n> > /**\n> > @@ -265,8 +243,7 @@ const ControlInfoMap &PipelineHandler::controls(const Camera *camera) const\n> > */\n> > const ControlList &PipelineHandler::properties(const Camera *camera) const\n> > {\n> > -\tconst CameraData *data = cameraData(camera);\n> > -\treturn data->properties_;\n> > +\treturn camera->_d()->properties_;\n> > }\n> >\n> > /**\n> > @@ -380,8 +357,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> > */\n> > bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> > {\n> > -\tconst CameraData *data = cameraData(camera);\n> > -\treturn !data->queuedRequests_.empty();\n> > +\treturn !camera->_d()->queuedRequests_.empty();\n> > }\n> >\n> > /**\n> > @@ -406,7 +382,7 @@ void PipelineHandler::queueRequest(Request *request)\n> > \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> >\n> > \tCamera *camera = request->camera_;\n> > -\tCameraData *data = cameraData(camera);\n> > +\tCamera::Private *data = camera->_d();\n> > \tdata->queuedRequests_.push_back(request);\n> >\n> > \trequest->sequence_ = data->requestSequence_++;\n> > @@ -479,7 +455,7 @@ void PipelineHandler::completeRequest(Request *request)\n> >\n> > \trequest->complete();\n> >\n> > -\tCameraData *data = cameraData(camera);\n> > +\tCamera::Private *data = camera->_d();\n> >\n> > \twhile (!data->queuedRequests_.empty()) {\n> > \t\tRequest *req = data->queuedRequests_.front();\n> > @@ -507,6 +483,15 @@ void PipelineHandler::completeRequest(Request *request)\n> > void PipelineHandler::registerCamera(std::shared_ptr camera,\n> > \t\t\t\t std::unique_ptr data)\n> > {\n> > +\t/*\n> > +\t * To be removed once all pipeline handlers will have migrated from\n> > +\t * CameraData to Camera::Private.\n> > +\t */\n> > +\tif (data) {\n> > +\t\tcamera->_d()->controlInfo_ = std::move(data->controlInfo_);\n> > +\t\tcamera->_d()->properties_ = std::move(data->properties_);\n> \n> With the introduction of std::move() you would need \n\nThis is related to a discussion we had on Tuesday I think. How far\nshould we go when it comes to inclusion of headers ? One particular case\nthat I'm not sure how to handle is if a .cpp file should include headers\nfor classes that are used in the corresponding .h. For instance,\n\nfoo.h\n\n#include \n\nclass Foo\n{\npublic:\n\tBar bar_;\n};\n\n\nThere's a guarantee that bar.h is included by foo.h, so we don't\nnecessarily have to include it in foo.cpp. This could also be extended\nto std::move(): if a class definition uses std::unique_ptr<>, there's a\nguarantee it includes , and we don't necessarily have to\ninclude it in the .cpp file. I wonder what the best practice would be in\nthis case.\n\nThis being said, here we use the move assignment operators of\nControlInfoMap and ControlList, not std::unique_ptr<>, so we should\nstill include if the PipelineHandler class didn't have other\nunique pointers.\n\n> As this goes away in the next patches I think it's anyway ok\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > +\t}\n> > +\n> > \tcameraData_[camera.get()] = std::move(data);\n> > \tcameras_.push_back(camera);\n> >","headers":{"Return-Path":"","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 9D6F6C3240\n\tfor ;\n\tThu, 12 Aug 2021 12:54:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14D0368888;\n\tThu, 12 Aug 2021 14:54:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 962C160264\n\tfor ;\n\tThu, 12 Aug 2021 14:54:13 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F300DEE;\n\tThu, 12 Aug 2021 14:54:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mhSdzNoX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628772853;\n\tbh=c0FTEkJQzZW8/zfJ8K9UUJu5yFhGwpvVCxw+MQ4J53A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mhSdzNoX9YhMRifZqEMQ5lcGSx5rCLIFzAL1HKv8D97PZndGXkElr212KP+pUW3YS\n\t7mAPvVHShchmLcpPSGvAnJEzzECSc4KSh+ajbrrz7CRBlrTrZJfuxELNK8yDReg8tA\n\tDRemKcx80GSLdUpRUXmERS2uBXtEGASDMLSmdUC4=","Date":"Thu, 12 Aug 2021 15:54:09 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210811232625.17280-1-laurent.pinchart@ideasonboard.com>\n\t<20210811232625.17280-4-laurent.pinchart@ideasonboard.com>\n\t<20210812075411.5no2a5zvvcjq5feg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210812075411.5no2a5zvvcjq5feg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: pipeline_handler:\n\tMove CameraData members to Camera::Private","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]