[{"id":21480,"web_url":"https://patchwork.libcamera.org/comment/21480/","msgid":"","date":"2021-12-01T00:37:14","subject":"Re: [libcamera-devel] [PATCH v3 02/11] libcamera: request: Make\n\tRequest class Extensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Dec 01, 2021 at 12:36:25AM +0100, Jacopo Mondi wrote:\n> From: Laurent Pinchart \n> \n> Implement the D-Pointer design pattern in the Request class to allow\n> changing internal data without affecting the public ABI.\n> \n> Move the internal fields that are not needed to implement the public\n> API to the Request::Private class already. This allow to remove\n\ns/allow/allows/\n\nReviewed-by: Laurent Pinchart \n\n> the friend class declaration for the PipelineHandler class, which can\n> now use the Request::Private API.\n> \n> Signed-off-by: Laurent Pinchart \n> [Move all internal fields to Request::Private and remove friend declaration]\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/request.h | 49 ++++\n> .../libcamera/internal/tracepoints/request.tp | 13 +-\n> include/libcamera/request.h | 19 +-\n> src/libcamera/pipeline_handler.cpp | 15 +-\n> src/libcamera/request.cpp | 230 ++++++++++++------\n> 6 files changed, 219 insertions(+), 108 deletions(-)\n> create mode 100644 include/libcamera/internal/request.h\n> \n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 665fd6de4ed3..cae65b0604ff 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -34,6 +34,7 @@ libcamera_internal_headers = files([\n> 'pipeline_handler.h',\n> 'process.h',\n> 'pub_key.h',\n> + 'request.h',\n> 'source_paths.h',\n> 'sysfs.h',\n> 'v4l2_device.h',\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> new file mode 100644\n> index 000000000000..1340ffa2a683\n> --- /dev/null\n> +++ b/include/libcamera/internal/request.h\n> @@ -0,0 +1,49 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * request.h - Request class private data\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> +#define __LIBCAMERA_INTERNAL_REQUEST_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Camera;\n> +class FrameBuffer;\n> +\n> +class Request::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(Request)\n> +\n> +public:\n> +\tPrivate(Camera *camera);\n> +\t~Private();\n> +\n> +\tCamera *camera() const { return camera_; }\n> +\tbool hasPendingBuffers() const;\n> +\n> +\tbool completeBuffer(FrameBuffer *buffer);\n> +\tvoid complete();\n> +\tvoid cancel();\n> +\tvoid reuse();\n> +\n> +private:\n> +\tfriend class PipelineHandler;\n> +\n> +\tvoid doCancelRequest();\n> +\n> +\tCamera *camera_;\n> +\tbool cancelled_;\n> +\tuint32_t sequence_ = 0;\n> +\n> +\tstd::unordered_set pending_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */\n> diff --git a/include/libcamera/internal/tracepoints/request.tp b/include/libcamera/internal/tracepoints/request.tp\n> index 37d8c46f4d96..f039ffd4f88d 100644\n> --- a/include/libcamera/internal/tracepoints/request.tp\n> +++ b/include/libcamera/internal/tracepoints/request.tp\n> @@ -5,8 +5,9 @@\n> * request.tp - Tracepoints for the request object\n> */\n> \n> +#include \n> +\n> #include \n> -#include \n> \n> TRACEPOINT_EVENT_CLASS(\n> \tlibcamera,\n> @@ -62,7 +63,7 @@ TRACEPOINT_EVENT_INSTANCE(\n> \trequest,\n> \trequest_complete,\n> \tTP_ARGS(\n> -\t\tlibcamera::Request *, req\n> +\t\tlibcamera::Request::Private *, req\n> \t)\n> )\n> \n> @@ -71,7 +72,7 @@ TRACEPOINT_EVENT_INSTANCE(\n> \trequest,\n> \trequest_cancel,\n> \tTP_ARGS(\n> -\t\tlibcamera::Request *, req\n> +\t\tlibcamera::Request::Private *, req\n> \t)\n> )\n> \n> @@ -79,13 +80,13 @@ TRACEPOINT_EVENT(\n> \tlibcamera,\n> \trequest_complete_buffer,\n> \tTP_ARGS(\n> -\t\tlibcamera::Request *, req,\n> +\t\tlibcamera::Request::Private *, req,\n> \t\tlibcamera::FrameBuffer *, buf\n> \t),\n> \tTP_FIELDS(\n> \t\tctf_integer_hex(uintptr_t, request, reinterpret_cast(req))\n> -\t\tctf_integer(uint64_t, cookie, req->cookie())\n> -\t\tctf_integer(int, status, req->status())\n> +\t\tctf_integer(uint64_t, cookie, req->_o()->cookie())\n> +\t\tctf_integer(int, status, req->_o()->status())\n> \t\tctf_integer_hex(uintptr_t, buffer, reinterpret_cast(buf))\n> \t\tctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status)\n> \t)\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index f434335b243a..8c78970d88ab 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -25,8 +25,10 @@ class CameraControlValidator;\n> class FrameBuffer;\n> class Stream;\n> \n> -class Request\n> +class Request : public Extensible\n> {\n> +\tLIBCAMERA_DECLARE_PRIVATE()\n> +\n> public:\n> \tenum Status {\n> \t\tRequestPending,\n> @@ -52,34 +54,23 @@ public:\n> \tint addBuffer(const Stream *stream, FrameBuffer *buffer);\n> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> \n> -\tuint32_t sequence() const { return sequence_; }\n> +\tuint32_t sequence() const;\n> \tuint64_t cookie() const { return cookie_; }\n> \tStatus status() const { return status_; }\n> \n> -\tbool hasPendingBuffers() const { return !pending_.empty(); }\n> +\tbool hasPendingBuffers() const;\n> \n> \tstd::string toString() const;\n> \n> private:\n> \tLIBCAMERA_DISABLE_COPY(Request)\n> \n> -\tfriend class PipelineHandler;\n> -\n> -\tvoid complete();\n> -\tvoid cancel();\n> -\n> -\tbool completeBuffer(FrameBuffer *buffer);\n> -\n> -\tCamera *camera_;\n> \tControlList *controls_;\n> \tControlList *metadata_;\n> \tBufferMap bufferMap_;\n> -\tstd::unordered_set pending_;\n> \n> -\tuint32_t sequence_;\n> \tconst uint64_t cookie_;\n> \tStatus status_;\n> -\tbool cancelled_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index f69c4f03b80f..67fdf1d8db01 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -19,6 +19,7 @@\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/request.h\"\n> #include \"libcamera/internal/tracepoints.h\"\n> \n> /**\n> @@ -311,15 +312,15 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> -\tCamera *camera = request->camera_;\n> +\tCamera *camera = request->_d()->camera();\n> \tCamera::Private *data = camera->_d();\n> \tdata->queuedRequests_.push_back(request);\n> \n> -\trequest->sequence_ = data->requestSequence_++;\n> +\trequest->_d()->sequence_ = data->requestSequence_++;\n> \n> \tint ret = queueRequestDevice(camera, request);\n> \tif (ret) {\n> -\t\trequest->cancel();\n> +\t\trequest->_d()->cancel();\n> \t\tcompleteRequest(request);\n> \t}\n> }\n> @@ -360,9 +361,9 @@ void PipelineHandler::queueRequest(Request *request)\n> */\n> bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> {\n> -\tCamera *camera = request->camera_;\n> +\tCamera *camera = request->_d()->camera();\n> \tcamera->bufferCompleted.emit(request, buffer);\n> -\treturn request->completeBuffer(buffer);\n> +\treturn request->_d()->completeBuffer(buffer);\n> }\n> \n> /**\n> @@ -381,9 +382,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> */\n> void PipelineHandler::completeRequest(Request *request)\n> {\n> -\tCamera *camera = request->camera_;\n> +\tCamera *camera = request->_d()->camera();\n> \n> -\trequest->complete();\n> +\trequest->_d()->complete();\n> \n> \tCamera::Private *data = camera->_d();\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 17fefab7ad0e..692202473360 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -5,7 +5,7 @@\n> * request.cpp - Capture request handling\n> */\n> \n> -#include \n> +#include \"libcamera/internal/request.h\"\n> \n> #include \n> #include \n> @@ -23,7 +23,7 @@\n> #include \"libcamera/internal/tracepoints.h\"\n> \n> /**\n> - * \\file request.h\n> + * \\file libcamera/request.h\n> * \\brief Describes a frame capture request to be processed by a camera\n> */\n> \n> @@ -31,6 +31,139 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Request)\n> \n> +/**\n> + * \\class Request::Private\n> + * \\brief Request private data\n> + *\n> + * The Request::Private class stores all private data associated with a\n> + * request. It implements the d-pointer design pattern to hide core\n> + * Request data from the public API, and exposes utility functions to\n> + * internal users of the request (namely the PipelineHandler class and its\n> + * subclasses).\n> + */\n> +\n> +/**\n> + * \\brief Create a Request::Private\n> + * \\param camera The Camera that creates the request\n> + */\n> +Request::Private::Private(Camera *camera)\n> +\t: camera_(camera), cancelled_(false)\n> +{\n> +}\n> +\n> +Request::Private::~Private()\n> +{\n> +\tdoCancelRequest();\n> +}\n> +\n> +/**\n> + * \\fn Request::Private::camera()\n> + * \\brief Retrieve the camera this request has been queued to\n> + * \\return The Camera this request has been queued to, or nullptr if the\n> + * request hasn't been queued\n> + */\n> +\n> +/**\n> + * \\brief Check if a request has buffers yet to be completed\n> + *\n> + * \\return True if the request has buffers pending for completion, false\n> + * otherwise\n> + */\n> +bool Request::Private::hasPendingBuffers() const\n> +{\n> +\treturn !pending_.empty();\n> +}\n> +\n> +/**\n> + * \\brief Complete a buffer for the request\n> + * \\param[in] buffer The buffer that has completed\n> + *\n> + * A request tracks the status of all buffers it contains through a set of\n> + * pending buffers. This function removes the \\a buffer from the set to mark it\n> + * as complete. All buffers associate with the request shall be marked as\n> + * complete by calling this function once and once only before reporting the\n> + * request as complete with the complete() function.\n> + *\n> + * \\return True if all buffers contained in the request have completed, false\n> + * otherwise\n> + */\n> +bool Request::Private::completeBuffer(FrameBuffer *buffer)\n> +{\n> +\tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> +\n> +\tint ret = pending_.erase(buffer);\n> +\tASSERT(ret == 1);\n> +\n> +\tbuffer->_d()->setRequest(nullptr);\n> +\n> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> +\t\tcancelled_ = true;\n> +\n> +\treturn !hasPendingBuffers();\n> +}\n> +\n> +/**\n> + * \\brief Complete a queued request\n> + *\n> + * Mark the request as complete by updating its status to RequestComplete,\n> + * unless buffers have been cancelled in which case the status is set to\n> + * RequestCancelled.\n> + */\n> +void Request::Private::complete()\n> +{\n> +\tRequest *request = _o();\n> +\n> +\tASSERT(request->status() == RequestPending);\n> +\tASSERT(!hasPendingBuffers());\n> +\n> +\trequest->status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> +\n> +\tLOG(Request, Debug) << request->toString();\n> +\n> +\tLIBCAMERA_TRACEPOINT(request_complete, this);\n> +}\n> +\n> +void Request::Private::doCancelRequest()\n> +{\n> +\tRequest *request = _o();\n> +\n> +\tfor (FrameBuffer *buffer : pending_) {\n> +\t\tbuffer->cancel();\n> +\t\tcamera_->bufferCompleted.emit(request, buffer);\n> +\t}\n> +\n> +\tcancelled_ = true;\n> +\tpending_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Cancel a queued request\n> + *\n> + * Mark the request and its associated buffers as cancelled and complete it.\n> + *\n> + * Set each pending buffer in error state and emit the buffer completion signal\n> + * before completing the Request.\n> + */\n> +void Request::Private::cancel()\n> +{\n> +\tLIBCAMERA_TRACEPOINT(request_cancel, this);\n> +\n> +\tRequest *request = _o();\n> +\tASSERT(request->status() == RequestPending);\n> +\n> +\tdoCancelRequest();\n> +}\n> +\n> +/**\n> + * \\copydoc Request::reuse()\n> + */\n> +void Request::Private::reuse()\n> +{\n> +\tsequence_ = 0;\n> +\tcancelled_ = false;\n> +\tpending_.clear();\n> +}\n> +\n> /**\n> * \\enum Request::Status\n> * Request completion status\n> @@ -75,8 +208,8 @@ LOG_DEFINE_CATEGORY(Request)\n> * completely opaque to libcamera.\n> */\n> Request::Request(Camera *camera, uint64_t cookie)\n> -\t: camera_(camera), sequence_(0), cookie_(cookie),\n> -\t status_(RequestPending), cancelled_(false)\n> +\t: Extensible(std::make_unique(camera)),\n> +\t cookie_(cookie), status_(RequestPending)\n> {\n> \tcontrols_ = new ControlList(controls::controls,\n> \t\t\t\t camera->_d()->validator());\n> @@ -113,20 +246,19 @@ void Request::reuse(ReuseFlag flags)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n> \n> -\tpending_.clear();\n> +\t_d()->reuse();\n> +\n> \tif (flags & ReuseBuffers) {\n> \t\tfor (auto pair : bufferMap_) {\n> \t\t\tFrameBuffer *buffer = pair.second;\n> \t\t\tbuffer->_d()->setRequest(this);\n> -\t\t\tpending_.insert(buffer);\n> +\t\t\t_d()->pending_.insert(buffer);\n> \t\t}\n> \t} else {\n> \t\tbufferMap_.clear();\n> \t}\n> \n> -\tsequence_ = 0;\n> \tstatus_ = RequestPending;\n> -\tcancelled_ = false;\n> \n> \tcontrols_->clear();\n> \tmetadata_->clear();\n> @@ -188,7 +320,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> \t}\n> \n> \tbuffer->_d()->setRequest(this);\n> -\tpending_.insert(buffer);\n> +\t_d()->pending_.insert(buffer);\n> \tbufferMap_[stream] = buffer;\n> \n> \treturn 0;\n> @@ -227,7 +359,6 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> */\n> \n> /**\n> - * \\fn Request::sequence()\n> * \\brief Retrieve the sequence number for the request\n> *\n> * When requests are queued, they are given a sequential number to track the\n> @@ -242,6 +373,10 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> *\n> * \\return The request sequence number\n> */\n> +uint32_t Request::sequence() const\n> +{\n> +\treturn _d()->sequence_;\n> +}\n> \n> /**\n> * \\fn Request::cookie()\n> @@ -263,81 +398,14 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> */\n> \n> /**\n> - * \\fn Request::hasPendingBuffers()\n> * \\brief Check if a request has buffers yet to be completed\n> *\n> * \\return True if the request has buffers pending for completion, false\n> * otherwise\n> */\n> -\n> -/**\n> - * \\brief Complete a queued request\n> - *\n> - * Mark the request as complete by updating its status to RequestComplete,\n> - * unless buffers have been cancelled in which case the status is set to\n> - * RequestCancelled.\n> - */\n> -void Request::complete()\n> -{\n> -\tASSERT(status_ == RequestPending);\n> -\tASSERT(!hasPendingBuffers());\n> -\n> -\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n> -\n> -\tLOG(Request, Debug) << toString();\n> -\n> -\tLIBCAMERA_TRACEPOINT(request_complete, this);\n> -}\n> -\n> -/**\n> - * \\brief Cancel a queued request\n> - *\n> - * Mark the request and its associated buffers as cancelled and complete it.\n> - *\n> - * Set each pending buffer in error state and emit the buffer completion signal\n> - * before completing the Request.\n> - */\n> -void Request::cancel()\n> -{\n> -\tLIBCAMERA_TRACEPOINT(request_cancel, this);\n> -\n> -\tASSERT(status_ == RequestPending);\n> -\n> -\tfor (FrameBuffer *buffer : pending_) {\n> -\t\tbuffer->cancel();\n> -\t\tcamera_->bufferCompleted.emit(this, buffer);\n> -\t}\n> -\n> -\tpending_.clear();\n> -\tcancelled_ = true;\n> -}\n> -\n> -/**\n> - * \\brief Complete a buffer for the request\n> - * \\param[in] buffer The buffer that has completed\n> - *\n> - * A request tracks the status of all buffers it contains through a set of\n> - * pending buffers. This function removes the \\a buffer from the set to mark it\n> - * as complete. All buffers associate with the request shall be marked as\n> - * complete by calling this function once and once only before reporting the\n> - * request as complete with the complete() function.\n> - *\n> - * \\return True if all buffers contained in the request have completed, false\n> - * otherwise\n> - */\n> -bool Request::completeBuffer(FrameBuffer *buffer)\n> +bool Request::hasPendingBuffers() const\n> {\n> -\tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> -\n> -\tint ret = pending_.erase(buffer);\n> -\tASSERT(ret == 1);\n> -\n> -\tbuffer->_d()->setRequest(nullptr);\n> -\n> -\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> -\t\tcancelled_ = true;\n> -\n> -\treturn !hasPendingBuffers();\n> +\treturn !_d()->pending_.empty();\n> }\n> \n> /**\n> @@ -356,8 +424,8 @@ std::string Request::toString() const\n> \tstatic const char *statuses = \"PCX\";\n> \n> \t/* Example Output: Request(55:P:1/2:6523524) */\n> -\tss << \"Request(\" << sequence_ << \":\" << statuses[status_] << \":\"\n> -\t << pending_.size() << \"/\" << bufferMap_.size() << \":\"\n> +\tss << \"Request(\" << sequence() << \":\" << statuses[status_] << \":\"\n> +\t << _d()->pending_.size() << \"/\" << bufferMap_.size() << \":\"\n> \t << cookie_ << \")\";\n> \n> \treturn ss.str();","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 9704FBF415\n\tfor ;\n\tWed, 1 Dec 2021 00:37:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBA3660723;\n\tWed, 1 Dec 2021 01:37:40 +0100 (CET)","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 EDB5E60592\n\tfor ;\n\tWed, 1 Dec 2021 01:37:39 +0100 (CET)","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 CC81F8AE;\n\tWed, 1 Dec 2021 01:37:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AWp5MM5i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638319059;\n\tbh=EGdwbRmPUfeQv1NaB4a8uQ/tkW/WL5bwMZ79XEbcMPo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AWp5MM5ibDEO7CbPe/5a2auyPbgLUYZnxAWa8UtWBH0ZaY/0vx7zMjH+ZNCjeNYQ5\n\tCCrxK1PEFfrvIataGrDDUuNJZ6ShS56RfHJZ92Ix2+On8u3Pfg0TKojCO4CNO9qoXA\n\tPpZsPJKD+JXCUOXvRWIvxkP+JlemwbOMP4yGNA44=","Date":"Wed, 1 Dec 2021 02:37:14 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211130233634.34173-1-jacopo@jmondi.org>\n\t<20211130233634.34173-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211130233634.34173-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 02/11] libcamera: request: Make\n\tRequest class Extensible","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\" "}}]