[{"id":18305,"web_url":"https://patchwork.libcamera.org/comment/18305/","msgid":"<YPu9upP4a5j6Y2rJ@oden.dyn.berto.se>","date":"2021-07-24T07:14:02","subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move CameraData members to Camera::Private","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2021-07-23 07:00:28 +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 <laurent.pinchart@ideasonboard.com>\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                      | 60 ++++++++++++++++++-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +-\n>  src/libcamera/pipeline_handler.cpp            | 45 +++++---------\n>  6 files changed, 83 insertions(+), 38 deletions(-)\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index aee5c9fd7e59..b8777b0c4522 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> -\t\t\t libcamera::Camera::Private \\\n>  \t\t\t 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<Request *> 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..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<Request *> 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 a5bb60c698bc..72a42dbeb3d3 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,50 @@ 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 for 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, and shall not be modified afterwards.\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 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\nThis looks like something that either should be moved to the core code \nor removed. This is outside the scope of this patch however,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\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 <libcamera/camera_manager.h>\n>  #include <libcamera/framebuffer.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/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> camera,\n>  \t\t\t\t     std::unique_ptr<CameraData> 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":"<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 1D4D2C322C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 24 Jul 2021 07:14:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 87EF4687A9;\n\tSat, 24 Jul 2021 09:14:04 +0200 (CEST)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0913668540\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 24 Jul 2021 09:14:04 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id u3so5796502lff.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 24 Jul 2021 00:14:03 -0700 (PDT)","from localhost (h-46-59-88-219.A463.priv.bahnhof.se.\n\t[46.59.88.219]) by smtp.gmail.com with ESMTPSA id\n\ts12sm2543021ljg.60.2021.07.24.00.14.02\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 24 Jul 2021 00:14:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Neischbc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=xXLfn/62Y2A26xqj1xRZL/T2CVOVW9x7KmG67ud/RjA=;\n\tb=NeischbcJHOz772G+rSyam2hpmY7rF06DIJQ5h3YovTVmT+Ib96jzVYQABaZqFytWh\n\txEfCupmnQc9uYCxpGVv2Hk3ZwXgk+IIAlzrYvINLepf/CpHqWkxKHYrL6+28VfrxDyYE\n\tHJVTmYimXhVjjD/SXCHlHNYiPbO4ylnwcbYhAjaKQ9+zULJcq2FEpZND9P+rujBJdBCP\n\tqkDw8q5/ryUbfaRgovhJeQOFo2zox2mDwu4c8UzBgsrews++ee/SIgqMPrt5BoDSgra/\n\tpYRBFd4SOk0NbUKEx6Tc4IO+wEejsDUgOM9xhUx9qDduWUSDtEPJT4RxKDS6wehOc/eu\n\tU2Tw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=xXLfn/62Y2A26xqj1xRZL/T2CVOVW9x7KmG67ud/RjA=;\n\tb=QBqesRVRNQgZUECIi7G1PH9seCy4kh94VcnfNqsjloseR7z2KOLUgRGR6Zt+h4v1J8\n\tMsZwa/4qf7Pbaxctdn03ihsLDYl8Md1JFZABlDLOd4pxsxa3WjEI+2ojQMugvTTt7BET\n\tfqIQUDhatW7+RWzT2kUBaJweM41GMMYLavZg+FWnT8eroSEYbABdrVU6zShY4RGFu6et\n\tZsRtX7tNDahlAHlwdb9OUVkyotXq3sxOVmGiFtZZKc43DZvq+u4chdyGxcjJwCDJ29ys\n\tHeq0lqxd8F0NWSqlweOYpxKwu1HyS/qchXw/1dMfejYKxZP87oxfWCJ/1/Cy+nre8jHj\n\thGbQ==","X-Gm-Message-State":"AOAM532qqZ+qGPyXXgz7uuDDasK53v9IkrtwK/ou3YqxGRhGUop8fwwZ\n\tkGVsZpQzO/8syjS1f0r1eep7QAoC3aYZZg==","X-Google-Smtp-Source":"ABdhPJx6VyE4ESqcok7uVV5doYbIfDcF0jftUUzl3OfaASFtKFnqgg4MiCrjeqsrCgI+ca0JAI/bAQ==","X-Received":"by 2002:a05:6512:3395:: with SMTP id\n\th21mr5563901lfg.489.1627110843309; \n\tSat, 24 Jul 2021 00:14:03 -0700 (PDT)","Date":"Sat, 24 Jul 2021 09:14:02 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<YPu9upP4a5j6Y2rJ@oden.dyn.berto.se>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move CameraData members to Camera::Private","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>"}},{"id":18430,"web_url":"https://patchwork.libcamera.org/comment/18430/","msgid":"<20210729211107.dr3rgzc2yts5u66m@uno.localdomain>","date":"2021-07-29T21:11:07","subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move 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 Fri, Jul 23, 2021 at 07:00:28AM +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 <laurent.pinchart@ideasonboard.com>\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                      | 60 ++++++++++++++++++-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +-\n>  src/libcamera/pipeline_handler.cpp            | 45 +++++---------\n>  6 files changed, 83 insertions(+), 38 deletions(-)\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index aee5c9fd7e59..b8777b0c4522 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> -\t\t\t libcamera::Camera::Private \\\n>  \t\t\t 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<Request *> 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..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<Request *> 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 a5bb60c698bc..72a42dbeb3d3 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,50 @@ 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 for 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, and shall not be modified afterwards.\n\nWell, that's not true, but I understand this is a copy of the existing\ndocumentation...\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> + * 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 <libcamera/camera_manager.h>\n>  #include <libcamera/framebuffer.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/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\nIt might happen later in the series, but the Camera is now calling the\npipeline handler while it could actually just call its private class.\n\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> camera,\n>  \t\t\t\t     std::unique_ptr<CameraData> 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":"<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 CD15EC3230\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 29 Jul 2021 21:10:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27E25687BF;\n\tThu, 29 Jul 2021 23:10:23 +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 AD832687BA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Jul 2021 23:10:21 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 9838C1C0002;\n\tThu, 29 Jul 2021 21:10:20 +0000 (UTC)"],"Date":"Thu, 29 Jul 2021 23:11:07 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210729211107.dr3rgzc2yts5u66m@uno.localdomain>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move CameraData members to Camera::Private","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>"}},{"id":18438,"web_url":"https://patchwork.libcamera.org/comment/18438/","msgid":"<YQNrFYbJfRTyj6eh@pendragon.ideasonboard.com>","date":"2021-07-30T02:59:33","subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move 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, Jul 29, 2021 at 11:11:07PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 23, 2021 at 07:00:28AM +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 <laurent.pinchart@ideasonboard.com>\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                      | 60 ++++++++++++++++++-\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +-\n> >  src/libcamera/pipeline_handler.cpp            | 45 +++++---------\n> >  6 files changed, 83 insertions(+), 38 deletions(-)\n> >\n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index aee5c9fd7e59..b8777b0c4522 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> > -\t\t\t libcamera::Camera::Private \\\n> >  \t\t\t 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<Request *> 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..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<Request *> 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 a5bb60c698bc..72a42dbeb3d3 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,50 @@ 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 for 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, and shall not be modified afterwards.\n> \n> Well, that's not true, but I understand this is a copy of the existing\n> documentation...\n\nI'll update this. Should I just s/when/before/ ?\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> > + * 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 <libcamera/camera_manager.h>\n> >  #include <libcamera/framebuffer.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/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> It might happen later in the series, but the Camera is now calling the\n> pipeline handler while it could actually just call its private class.\n\nIt doesn't, but it's a very good point. I'll do so (or at least try to,\nwho knows what will happen ? :-)) in a patch on top.\n\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> camera,\n> >  \t\t\t\t     std::unique_ptr<CameraData> 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":"<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 8FC76C3230\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 30 Jul 2021 02:59:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11E30687C4;\n\tFri, 30 Jul 2021 04:59:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9568E687BD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 30 Jul 2021 04:59:41 +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 1B8CA89B;\n\tFri, 30 Jul 2021 04:59:41 +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=\"LQD5rU36\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627613981;\n\tbh=XUK0Sep6CFmRopXINc88RERLtcW4rdBtHxDg+2u5X54=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LQD5rU36BN4VaQf6TKpkktbwAQGfuwyhem9u/2QcHYbOGuaOproXAhF2Kkd9qjBHk\n\t8bjrLvFUUsG9x/N3s2HPdqKbMERQM21Z0XB6F8lCwlbzxicpCIy+PsEsM+M4wIREhx\n\tLr/ZXDURGyoSpLkhH/ag+KmsmRk6Ir98w6yue0G8=","Date":"Fri, 30 Jul 2021 05:59:33 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YQNrFYbJfRTyj6eh@pendragon.ideasonboard.com>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>\n\t<20210729211107.dr3rgzc2yts5u66m@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210729211107.dr3rgzc2yts5u66m@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move CameraData members to Camera::Private","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>"}},{"id":18440,"web_url":"https://patchwork.libcamera.org/comment/18440/","msgid":"<20210730072707.hubh34ufdyqd44lg@uno.localdomain>","date":"2021-07-30T07:27:07","subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move 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 Fri, Jul 30, 2021 at 05:59:33AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Jul 29, 2021 at 11:11:07PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 23, 2021 at 07:00:28AM +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 <laurent.pinchart@ideasonboard.com>\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                      | 60 ++++++++++++++++++-\n> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +-\n> > >  src/libcamera/pipeline_handler.cpp            | 45 +++++---------\n> > >  6 files changed, 83 insertions(+), 38 deletions(-)\n> > >\n> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > index aee5c9fd7e59..b8777b0c4522 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> > > -\t\t\t libcamera::Camera::Private \\\n> > >  \t\t\t 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<Request *> 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..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<Request *> 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 a5bb60c698bc..72a42dbeb3d3 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,50 @@ 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 for 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, and shall not be modified afterwards.\n> >\n> > Well, that's not true, but I understand this is a copy of the existing\n> > documentation...\n>\n> I'll update this. Should I just s/when/before/ ?\n\nI would rather\ns/and shall not be modified afterwards//\n\nas we're working on updating controlInfo_ in response to a Camera\nconfiguration ;)\n\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> > > + * 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 <libcamera/camera_manager.h>\n> > >  #include <libcamera/framebuffer.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/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> > It might happen later in the series, but the Camera is now calling the\n> > pipeline handler while it could actually just call its private class.\n>\n> It doesn't, but it's a very good point. I'll do so (or at least try to,\n> who knows what will happen ? :-)) in a patch on top.\n>\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> camera,\n> > >  \t\t\t\t     std::unique_ptr<CameraData> 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> --\n> Regards,\n>\n> Laurent Pinchart","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 440E3C3230\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 30 Jul 2021 07:26:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6EA6687C5;\n\tFri, 30 Jul 2021 09:26:22 +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 3584E687B1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 30 Jul 2021 09:26:21 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5D2611C0004;\n\tFri, 30 Jul 2021 07:26:19 +0000 (UTC)"],"Date":"Fri, 30 Jul 2021 09:27:07 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210730072707.hubh34ufdyqd44lg@uno.localdomain>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>\n\t<20210729211107.dr3rgzc2yts5u66m@uno.localdomain>\n\t<YQNrFYbJfRTyj6eh@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<YQNrFYbJfRTyj6eh@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move CameraData members to Camera::Private","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>"}},{"id":18446,"web_url":"https://patchwork.libcamera.org/comment/18446/","msgid":"<YQSVwYx7KLlO7CaC@pendragon.ideasonboard.com>","date":"2021-07-31T00:13:53","subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move 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 Fri, Jul 30, 2021 at 09:27:07AM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 30, 2021 at 05:59:33AM +0300, Laurent Pinchart wrote:\n> > On Thu, Jul 29, 2021 at 11:11:07PM +0200, Jacopo Mondi wrote:\n> > > On Fri, Jul 23, 2021 at 07:00:28AM +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 <laurent.pinchart@ideasonboard.com>\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                      | 60 ++++++++++++++++++-\n> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +-\n> > > >  src/libcamera/pipeline_handler.cpp            | 45 +++++---------\n> > > >  6 files changed, 83 insertions(+), 38 deletions(-)\n> > > >\n> > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > > index aee5c9fd7e59..b8777b0c4522 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> > > > -\t\t\t libcamera::Camera::Private \\\n> > > >  \t\t\t 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<Request *> 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..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<Request *> 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 a5bb60c698bc..72a42dbeb3d3 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,50 @@ 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 for 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, and shall not be modified afterwards.\n> > >\n> > > Well, that's not true, but I understand this is a copy of the existing\n> > > documentation...\n> >\n> > I'll update this. Should I just s/when/before/ ?\n> \n> I would rather\n> s/and shall not be modified afterwards//\n> \n> as we're working on updating controlInfo_ in response to a Camera\n> configuration ;)\n\nWe'll have to revisit this documentation when implementing proper\nsupport for updating control information. I was thinking about adding a\n\\todo to mention this and keeping the documentation aligned with what is\nofficially supported, but the existing implementation departs for the\nofficially supported API already...\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> > > > + * 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 <libcamera/camera_manager.h>\n> > > >  #include <libcamera/framebuffer.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/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> > > It might happen later in the series, but the Camera is now calling the\n> > > pipeline handler while it could actually just call its private class.\n> >\n> > It doesn't, but it's a very good point. I'll do so (or at least try to,\n> > who knows what will happen ? :-)) in a patch on top.\n> >\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> camera,\n> > > >  \t\t\t\t     std::unique_ptr<CameraData> 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":"<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 C6254C3232\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 31 Jul 2021 00:14:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24C88687C3;\n\tSat, 31 Jul 2021 02:14:05 +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 119BC687BA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 31 Jul 2021 02:14:03 +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 834852A3;\n\tSat, 31 Jul 2021 02:14:02 +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=\"XPh0u2XX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627690442;\n\tbh=ElA4N61PfxGdmo1PaQOy54UG5A/KsVdwNQhJdfFnyNI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XPh0u2XX2SWlp4XmE2nDBCXQ8ZeqSmqIX1WmKkWlp84zKvDuqAYGXRFIRDImBi7Df\n\tiJAOo5ubfgeL1w2Qxm+HG6tumM2uP3bpPD6fCM7dRRFTOCG3Fz/E/4r6SUcBb9Jx0c\n\tIAQBBZVpwNd7WVxoJEt1TbFIagOcTOm3gQ6sCeKc=","Date":"Sat, 31 Jul 2021 03:13:53 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YQSVwYx7KLlO7CaC@pendragon.ideasonboard.com>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-10-laurent.pinchart@ideasonboard.com>\n\t<20210729211107.dr3rgzc2yts5u66m@uno.localdomain>\n\t<YQNrFYbJfRTyj6eh@pendragon.ideasonboard.com>\n\t<20210730072707.hubh34ufdyqd44lg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210730072707.hubh34ufdyqd44lg@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 09/17] libcamera:\n\tpipeline_handler: Move CameraData members to Camera::Private","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>"}}]