[{"id":18676,"web_url":"https://patchwork.libcamera.org/comment/18676/","msgid":"<20210810133137.7azjiitlodoxf5jr@uno.localdomain>","date":"2021-08-10T13:31:37","subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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 05, 2021 at 08:58:39PM +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 v1:\n>\n> - Add \\todo comment for controlInfo_\n> ---\n> Documentation/Doxyfile.in | 1 -\n> include/libcamera/internal/camera.h | 8 +++\n> include/libcamera/internal/pipeline_handler.h | 5 +-\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, 88 insertions(+), 38 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..44242332a52f 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -29,6 +29,14 @@ public:\n> \tPrivate(PipelineHandler *pipe);\n> \t~Private();\n>\n> +\tPipelineHandler *pipe() { return pipe_.get(); }\n> +\n> +\tstd::list queuedRequests_;\n\nDo you need to include ?\n\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..86727f90bb65 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -39,18 +39,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\nand you can drop from this file\n\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 a5bb60c698bc..f0893f89a1b3 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\nit's maybe not necessary, as it's a rather C++ standard thing (or\nrather OOP in general) but mentioning that the pointer returned\nthrough _d() should be subclassed ?\n\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\nI admit if I were totally new to the thing I could wonder why a\npipeline handler has to call pipe() to get a pointer to itself :)\n\n> + * \\return The pipeline handler for this camera\n\nI would rather say \"The pipeline that created the camera\" as it might\nsuggest each Camera could potentially have a different ph associated\n(which is true in general, but not in the ph developer perspective)\n\n> + */\n> +\n> +/**\n> + * \\var Camera::Private::queuedRequests_\n> + * \\brief The list of queued and not yet completed request\n\nrequests\n\n> + *\n> + * The list of queued request is used to track requests queued in order to\n\n'queued' is repeated twice\n\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\nThanks :)\n\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\nThe queueing sequence -number- ?\n\nAh well, the documentation was already there...\n\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 4a8ac97d5ef0..cc279ce733ef 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 c9928d444b04..1f1de19d81c5 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\nI'm sorry if we discussed this already, I have a vague remembering of\nthat... but do Camera need to go through the PipelineHander base class\nto do so as it has direct access to _d() ?\n\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> +\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 2C14CBD87D\n\tfor ;\n\tTue, 10 Aug 2021 13:30:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 851D56884F;\n\tTue, 10 Aug 2021 15:30:50 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDD49687F0\n\tfor ;\n\tTue, 10 Aug 2021 15:30:49 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 5B095100005;\n\tTue, 10 Aug 2021 13:30:49 +0000 (UTC)"],"Date":"Tue, 10 Aug 2021 15:31:37 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210810133137.7azjiitlodoxf5jr@uno.localdomain>","References":"<20210805175848.24188-1-laurent.pinchart@ideasonboard.com>\n\t<20210805175848.24188-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":"<20210805175848.24188-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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":18712,"web_url":"https://patchwork.libcamera.org/comment/18712/","msgid":"","date":"2021-08-11T17:24:11","subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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 Tue, Aug 10, 2021 at 03:31:37PM +0200, Jacopo Mondi wrote:\n> On Thu, Aug 05, 2021 at 08:58:39PM +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 v1:\n> >\n> > - Add \\todo comment for controlInfo_\n> > ---\n> > Documentation/Doxyfile.in | 1 -\n> > include/libcamera/internal/camera.h | 8 +++\n> > include/libcamera/internal/pipeline_handler.h | 5 +-\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, 88 insertions(+), 38 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..44242332a52f 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -29,6 +29,14 @@ public:\n> > \tPrivate(PipelineHandler *pipe);\n> > \t~Private();\n> >\n> > +\tPipelineHandler *pipe() { return pipe_.get(); }\n> > +\n> > +\tstd::list queuedRequests_;\n> \n> Do you need to include ?\n\nIndeed.\n\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..86727f90bb65 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -39,18 +39,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> \n> and you can drop from this file\n\nIndeed too :-)\n\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 a5bb60c698bc..f0893f89a1b3 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> it's maybe not necessary, as it's a rather C++ standard thing (or\n> rather OOP in general) but mentioning that the pointer returned\n> through _d() should be subclassed ?\n> \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> \n> I admit if I were totally new to the thing I could wonder why a\n> pipeline handler has to call pipe() to get a pointer to itself :)\n> \n> > + * \\return The pipeline handler for this camera\n> \n> I would rather say \"The pipeline that created the camera\" as it might\n> suggest each Camera could potentially have a different ph associated\n> (which is true in general, but not in the ph developer perspective)\n\nGood point. Will fix.\n\n> > + */\n> > +\n> > +/**\n> > + * \\var Camera::Private::queuedRequests_\n> > + * \\brief The list of queued and not yet completed request\n> \n> requests\n\nI'll fix this and the other issues below in a separate patch, to avoid\nhiding the changes in the move.\n\n> > + *\n> > + * The list of queued request is used to track requests queued in order to\n> \n> 'queued' is repeated twice\n> \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> Thanks :)\n> \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> The queueing sequence -number- ?\n> \n> Ah well, the documentation was already there...\n> \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 4a8ac97d5ef0..cc279ce733ef 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 c9928d444b04..1f1de19d81c5 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> I'm sorry if we discussed this already, I have a vague remembering of\n> that... but do Camera need to go through the PipelineHander base class\n> to do so as it has direct access to _d() ?\n\n11/11 in this series, as you've noticed :-)\n\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> > +\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 8FB5BBD87D\n\tfor ;\n\tWed, 11 Aug 2021 17:24:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BD206884E;\n\tWed, 11 Aug 2021 19:24:17 +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 45E1A68826\n\tfor ;\n\tWed, 11 Aug 2021 19:24:15 +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 AD9B44A1;\n\tWed, 11 Aug 2021 19:24:14 +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=\"plOIQOoI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628702654;\n\tbh=aaRdd0EVCBAq+PvGk1Ggi2KqCS4Y/mDq4HA6ZZBMnKQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=plOIQOoIsVwf3OAi8zrWUYOYE1Sfp13GJdDGZ0/c6i2USdN3KdcPvHiwdt8fdn+/a\n\t/ijmn8M4csOXBu3+aJHqRMTyNX5KrBWUCNmKUyPCDYOGwYkFe9YIUncNNvJ+rMX2/v\n\tmw4d9R9Wstzjh1c+KxD368EJZ71szET7vJYqu26U=","Date":"Wed, 11 Aug 2021 20:24:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210805175848.24188-1-laurent.pinchart@ideasonboard.com>\n\t<20210805175848.24188-3-laurent.pinchart@ideasonboard.com>\n\t<20210810133137.7azjiitlodoxf5jr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210810133137.7azjiitlodoxf5jr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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":18713,"web_url":"https://patchwork.libcamera.org/comment/18713/","msgid":"","date":"2021-08-11T17:31:46","subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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 Wed, Aug 11, 2021 at 08:24:12PM +0300, Laurent Pinchart wrote:\n> On Tue, Aug 10, 2021 at 03:31:37PM +0200, Jacopo Mondi wrote:\n> > On Thu, Aug 05, 2021 at 08:58:39PM +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 v1:\n> > >\n> > > - Add \\todo comment for controlInfo_\n> > > ---\n> > > Documentation/Doxyfile.in | 1 -\n> > > include/libcamera/internal/camera.h | 8 +++\n> > > include/libcamera/internal/pipeline_handler.h | 5 +-\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, 88 insertions(+), 38 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..44242332a52f 100644\n> > > --- a/include/libcamera/internal/camera.h\n> > > +++ b/include/libcamera/internal/camera.h\n> > > @@ -29,6 +29,14 @@ public:\n> > > \tPrivate(PipelineHandler *pipe);\n> > > \t~Private();\n> > >\n> > > +\tPipelineHandler *pipe() { return pipe_.get(); }\n> > > +\n> > > +\tstd::list queuedRequests_;\n> > \n> > Do you need to include ?\n> \n> Indeed.\n> \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..86727f90bb65 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -39,18 +39,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> > \n> > and you can drop from this file\n> \n> Indeed too :-)\n> \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 a5bb60c698bc..f0893f89a1b3 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> > it's maybe not necessary, as it's a rather C++ standard thing (or\n> > rather OOP in general) but mentioning that the pointer returned\n> > through _d() should be subclassed ?\n\nThe documentation already states the the pipeline handler is expected to\nsubclass Camera::Private in the previous paragraph, what do you think is\nmissing here ?\n\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> > \n> > I admit if I were totally new to the thing I could wonder why a\n> > pipeline handler has to call pipe() to get a pointer to itself :)\n> > \n> > > + * \\return The pipeline handler for this camera\n> > \n> > I would rather say \"The pipeline that created the camera\" as it might\n> > suggest each Camera could potentially have a different ph associated\n> > (which is true in general, but not in the ph developer perspective)\n> \n> Good point. Will fix.\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var Camera::Private::queuedRequests_\n> > > + * \\brief The list of queued and not yet completed request\n> > \n> > requests\n> \n> I'll fix this and the other issues below in a separate patch, to avoid\n> hiding the changes in the move.\n> \n> > > + *\n> > > + * The list of queued request is used to track requests queued in order to\n> > \n> > 'queued' is repeated twice\n> > \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> > Thanks :)\n> > \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> > The queueing sequence -number- ?\n> > \n> > Ah well, the documentation was already there...\n> > \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 4a8ac97d5ef0..cc279ce733ef 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 c9928d444b04..1f1de19d81c5 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> > I'm sorry if we discussed this already, I have a vague remembering of\n> > that... but do Camera need to go through the PipelineHander base class\n> > to do so as it has direct access to _d() ?\n> \n> 11/11 in this series, as you've noticed :-)\n> \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> > > +\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 C7406C3240\n\tfor ;\n\tWed, 11 Aug 2021 17:31:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B8E96884F;\n\tWed, 11 Aug 2021 19:31:51 +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 D1A3368826\n\tfor ;\n\tWed, 11 Aug 2021 19:31:49 +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 42F48EE;\n\tWed, 11 Aug 2021 19:31:49 +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=\"X24tSB1f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628703109;\n\tbh=8nANBdi9TD88GLwMLSpJuXTtAt4diOWzQQol5csTrRU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X24tSB1f6AV47dDzW03JwYkxw3Qy0b7nQvPL25ukXuxKMohdVcs7aq/PE4b/mJk3p\n\tWkUEj14EUykqhsu5731nvwc/m0OHEoxCkMCP1jip9lbdBn6aSi+ro2+SBsCodo2TfO\n\tKgYUi81twHRgtyqNM+k9IVX3r4z88SaJ9YA+tgn4=","Date":"Wed, 11 Aug 2021 20:31:46 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210805175848.24188-1-laurent.pinchart@ideasonboard.com>\n\t<20210805175848.24188-3-laurent.pinchart@ideasonboard.com>\n\t<20210810133137.7azjiitlodoxf5jr@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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":18721,"web_url":"https://patchwork.libcamera.org/comment/18721/","msgid":"<20210812072647.wy644dpgsg6z7f52@uno.localdomain>","date":"2021-08-12T07:26:47","subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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":"On Wed, Aug 11, 2021 at 08:31:46PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Aug 11, 2021 at 08:24:12PM +0300, Laurent Pinchart wrote:\n> > On Tue, Aug 10, 2021 at 03:31:37PM +0200, Jacopo Mondi wrote:\n> > > On Thu, Aug 05, 2021 at 08:58:39PM +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 v1:\n> > > >\n> > > > - Add \\todo comment for controlInfo_\n> > > > ---\n> > > > Documentation/Doxyfile.in | 1 -\n> > > > include/libcamera/internal/camera.h | 8 +++\n> > > > include/libcamera/internal/pipeline_handler.h | 5 +-\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, 88 insertions(+), 38 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..44242332a52f 100644\n> > > > --- a/include/libcamera/internal/camera.h\n> > > > +++ b/include/libcamera/internal/camera.h\n> > > > @@ -29,6 +29,14 @@ public:\n> > > > \tPrivate(PipelineHandler *pipe);\n> > > > \t~Private();\n> > > >\n> > > > +\tPipelineHandler *pipe() { return pipe_.get(); }\n> > > > +\n> > > > +\tstd::list queuedRequests_;\n> > >\n> > > Do you need to include ?\n> >\n> > Indeed.\n> >\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..86727f90bb65 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -39,18 +39,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> > >\n> > > and you can drop from this file\n> >\n> > Indeed too :-)\n> >\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 a5bb60c698bc..f0893f89a1b3 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> > > it's maybe not necessary, as it's a rather C++ standard thing (or\n> > > rather OOP in general) but mentioning that the pointer returned\n> > > through _d() should be subclassed ?\n>\n> The documentation already states the the pipeline handler is expected to\n> subclass Camera::Private in the previous paragraph, what do you think is\n> missing here ?\n>\n\nI meant 'downcasted' :)","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 F2D34BD87D\n\tfor ;\n\tThu, 12 Aug 2021 07:26:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5BA586884F;\n\tThu, 12 Aug 2021 09:26:01 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB2E860264\n\tfor ;\n\tThu, 12 Aug 2021 09:25:59 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 388D61C000A;\n\tThu, 12 Aug 2021 07:25:58 +0000 (UTC)"],"Date":"Thu, 12 Aug 2021 09:26:47 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210812072647.wy644dpgsg6z7f52@uno.localdomain>","References":"<20210805175848.24188-1-laurent.pinchart@ideasonboard.com>\n\t<20210805175848.24188-3-laurent.pinchart@ideasonboard.com>\n\t<20210810133137.7azjiitlodoxf5jr@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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":18728,"web_url":"https://patchwork.libcamera.org/comment/18728/","msgid":"","date":"2021-08-12T08:41:49","subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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:26:47AM +0200, Jacopo Mondi wrote:\n> On Wed, Aug 11, 2021 at 08:31:46PM +0300, Laurent Pinchart wrote:\n> > On Wed, Aug 11, 2021 at 08:24:12PM +0300, Laurent Pinchart wrote:\n> > > On Tue, Aug 10, 2021 at 03:31:37PM +0200, Jacopo Mondi wrote:\n> > > > On Thu, Aug 05, 2021 at 08:58:39PM +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 v1:\n> > > > >\n> > > > > - Add \\todo comment for controlInfo_\n> > > > > ---\n> > > > > Documentation/Doxyfile.in | 1 -\n> > > > > include/libcamera/internal/camera.h | 8 +++\n> > > > > include/libcamera/internal/pipeline_handler.h | 5 +-\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, 88 insertions(+), 38 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..44242332a52f 100644\n> > > > > --- a/include/libcamera/internal/camera.h\n> > > > > +++ b/include/libcamera/internal/camera.h\n> > > > > @@ -29,6 +29,14 @@ public:\n> > > > > \tPrivate(PipelineHandler *pipe);\n> > > > > \t~Private();\n> > > > >\n> > > > > +\tPipelineHandler *pipe() { return pipe_.get(); }\n> > > > > +\n> > > > > +\tstd::list queuedRequests_;\n> > > >\n> > > > Do you need to include ?\n> > >\n> > > Indeed.\n> > >\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..86727f90bb65 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -39,18 +39,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> > > >\n> > > > and you can drop from this file\n> > >\n> > > Indeed too :-)\n> > >\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 a5bb60c698bc..f0893f89a1b3 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> > > > it's maybe not necessary, as it's a rather C++ standard thing (or\n> > > > rather OOP in general) but mentioning that the pointer returned\n> > > > through _d() should be subclassed ?\n> >\n> > The documentation already states the the pipeline handler is expected to\n> > subclass Camera::Private in the previous paragraph, what do you think is\n> > missing here ?\n> \n> I meant 'downcasted' :)\n\nAh yes of course :-) I think that's standard C++ indeed, and the\ncompiler will certainly remind anyone who doesn't do so, so I don't\nthink it needs to be explicitly specified. We have enough to document\nalready without teaching C++ to the reader :-)","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 9446CBD87D\n\tfor ;\n\tThu, 12 Aug 2021 08:41:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D66D568886;\n\tThu, 12 Aug 2021 10:41:54 +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 BB14960261\n\tfor ;\n\tThu, 12 Aug 2021 10:41:53 +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 33DC0EE;\n\tThu, 12 Aug 2021 10:41:53 +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=\"W4Z0wP3r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628757713;\n\tbh=bnkhICEWWGbc+6QVTru2D20Han/JYlBSoeXO9tqxDnQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W4Z0wP3rMoKdMvhJDwjW/yLJUdz5FzUkOSZnKUbZC/Sf786wJic5Yqp89wGlEeJxO\n\t/STfeJjqALzy3EjXNUgZRiFk7m9i1daji4akIaeCPPOEAVKGKc9n3wbM1O96V/cDLr\n\t5YWKC4DfZXteRAVKc5kb+Kto/0EC68YZSH2kQMkY=","Date":"Thu, 12 Aug 2021 11:41:49 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210805175848.24188-1-laurent.pinchart@ideasonboard.com>\n\t<20210805175848.24188-3-laurent.pinchart@ideasonboard.com>\n\t<20210810133137.7azjiitlodoxf5jr@uno.localdomain>\n\t\n\t\n\t<20210812072647.wy644dpgsg6z7f52@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210812072647.wy644dpgsg6z7f52@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] 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\" "}}]