[{"id":21071,"web_url":"https://patchwork.libcamera.org/comment/21071/","msgid":"","date":"2021-11-21T16:31:35","subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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 Sat, Nov 20, 2021 at 12:13:02PM +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\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 | 51 ++++\n> .../libcamera/internal/tracepoints/request.tp | 9 +-\n> include/libcamera/request.h | 19 +-\n> src/libcamera/pipeline_handler.cpp | 15 +-\n> src/libcamera/request.cpp | 256 ++++++++++++------\n> 6 files changed, 245 insertions(+), 106 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..59bddde3a090\n> --- /dev/null\n> +++ b/include/libcamera/internal/request.h\n> @@ -0,0 +1,51 @@\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> +\tuint64_t cookie() const;\n\nWhy do we need a cookie() function here, which wraps Request::cookie(),\nwhen cookie_ is stored in the Request class ?\n\n> +\tRequest::Status status() const;\n\nSame here.\n\n> +\n> +\tbool completeBuffer(FrameBuffer *buffer);\n> +\tvoid complete();\n> +\tvoid cancel();\n> +\tvoid reuse();\n> +\n> +\tuint32_t sequence_ = 0;\n\nThe reason why you can drop the friend statement is because you make\nthis member variable public. That's not very nice :-S I'd keep it in\nRequest for now, along with cookie_ and status_, until we decide how to\nhandle those members.\n\n> +\n> +private:\n> +\tvoid _cancel();\n> +\n> +\tCamera *camera_;\n> +\tbool cancelled_;\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..37cd2f8864ce 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,7 +80,7 @@ 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> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index d16904e6b679..f0c5163d987e 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..90c436648405 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,165 @@ 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> +\t_cancel();\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> + * \\copydoc Request::cookie()\n> + *\n> + * Used by the tracing framework\n> + */\n> +uint64_t Request::Private::cookie() const\n> +{\n> +\treturn _o()->cookie();\n> +}\n> +\n> +/**\n> + * \\copydoc Request::status()\n> + *\n> + * Used by the tracing framework\n> + */\n> +Request::Status Request::Private::status() const\n> +{\n> +\treturn _o()->status();\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::_cancel()\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> +\t_cancel();\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> + * \\var Request::Private::sequence_\n> + * \\brief The request sequence number\n> + *\n> + * \\copydoc Request::sequence()\n> + */\n> +\n> /**\n> * \\enum Request::Status\n> * Request completion status\n> @@ -75,8 +234,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 +272,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 +346,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 +385,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 +399,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 +424,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 +450,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 12F12BF415\n\tfor ;\n\tSun, 21 Nov 2021 16:32:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7699A60371;\n\tSun, 21 Nov 2021 17:32:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB92B60233\n\tfor ;\n\tSun, 21 Nov 2021 17:31:58 +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 619BC9DD;\n\tSun, 21 Nov 2021 17:31:58 +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=\"M+qC0fvl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637512318;\n\tbh=HP4Mf6YvirK2KiCeCQmLvm+3Eeu4IRXYClJEVZNxYFA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M+qC0fvlOxMxYrFY0R1Wg+YEiSGFa2j+Lo881tnrAx7XsR0C9lf0uaTWwWY4RMRCB\n\t83mFmYl5J1n95X3q8kUZZ4BoXXYgg5EcMhSbzrry2Eq0cRz5l0RYYcD5mU+07QUXpl\n\tyEwzvIYvvOluNtAY3fWzAWzxtnLyf7Yh4xak5NEM=","Date":"Sun, 21 Nov 2021 18:31:35 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211120111313.106621-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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\" "}},{"id":21080,"web_url":"https://patchwork.libcamera.org/comment/21080/","msgid":"","date":"2021-11-21T21:35:08","subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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\nAnother comment.\n\nOn Sun, Nov 21, 2021 at 06:31:37PM +0200, Laurent Pinchart wrote:\n> On Sat, Nov 20, 2021 at 12:13:02PM +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> \n> s/allow/allows/\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 | 51 ++++\n> > .../libcamera/internal/tracepoints/request.tp | 9 +-\n> > include/libcamera/request.h | 19 +-\n> > src/libcamera/pipeline_handler.cpp | 15 +-\n> > src/libcamera/request.cpp | 256 ++++++++++++------\n> > 6 files changed, 245 insertions(+), 106 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..59bddde3a090\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -0,0 +1,51 @@\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> > +\tuint64_t cookie() const;\n> \n> Why do we need a cookie() function here, which wraps Request::cookie(),\n> when cookie_ is stored in the Request class ?\n> \n> > +\tRequest::Status status() const;\n> \n> Same here.\n> \n> > +\n> > +\tbool completeBuffer(FrameBuffer *buffer);\n> > +\tvoid complete();\n> > +\tvoid cancel();\n> > +\tvoid reuse();\n> > +\n> > +\tuint32_t sequence_ = 0;\n> \n> The reason why you can drop the friend statement is because you make\n> this member variable public. That's not very nice :-S I'd keep it in\n> Request for now, along with cookie_ and status_, until we decide how to\n> handle those members.\n> \n> > +\n> > +private:\n> > +\tvoid _cancel();\n\nI'm not fond of the underscore prefix. A name that describes the purpose\nof the function would be better.\n\n> > +\n> > +\tCamera *camera_;\n> > +\tbool cancelled_;\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..37cd2f8864ce 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,7 +80,7 @@ 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> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index d16904e6b679..f0c5163d987e 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..90c436648405 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,165 @@ 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> > +\t_cancel();\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> > + * \\copydoc Request::cookie()\n> > + *\n> > + * Used by the tracing framework\n> > + */\n> > +uint64_t Request::Private::cookie() const\n> > +{\n> > +\treturn _o()->cookie();\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc Request::status()\n> > + *\n> > + * Used by the tracing framework\n> > + */\n> > +Request::Status Request::Private::status() const\n> > +{\n> > +\treturn _o()->status();\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::_cancel()\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> > +\t_cancel();\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> > + * \\var Request::Private::sequence_\n> > + * \\brief The request sequence number\n> > + *\n> > + * \\copydoc Request::sequence()\n> > + */\n> > +\n> > /**\n> > * \\enum Request::Status\n> > * Request completion status\n> > @@ -75,8 +234,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 +272,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 +346,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 +385,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 +399,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 +424,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 +450,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 94834BF415\n\tfor ;\n\tSun, 21 Nov 2021 21:35:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB96C60378;\n\tSun, 21 Nov 2021 22:35:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9044760233\n\tfor ;\n\tSun, 21 Nov 2021 22:35:31 +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 033FA9DD;\n\tSun, 21 Nov 2021 22:35:30 +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=\"eXwmfEgz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637530531;\n\tbh=EqGYvNRT1mLN6sCiTW0EcI7OHsa9DjwuqJsyA+jOHJg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eXwmfEgzY2dh/8YA3whcP4lvQK9FHVFF77IQ6GsTjiJCDOaMP/7F4U9IuAyN1Wmu1\n\t0pcM5qDGC8qYVMfflcL0i8xEPkTX8JmT0+usJjgPymqUQ8rLUztTzdVpYLDGMHuaBH\n\t6gtmWFnqhANY41v2u0kYcV0MVDr5PRW2NBUcBkhc=","Date":"Sun, 21 Nov 2021 23:35:08 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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\" "}},{"id":21281,"web_url":"https://patchwork.libcamera.org/comment/21281/","msgid":"<20211126110506.24uofj5wiocwgj6o@uno.localdomain>","date":"2021-11-26T11:05:06","subject":"Re: [libcamera-devel] [PATCH v2 01/12] libcamera: request: Make\n\tRequest class Extensible","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Nov 21, 2021 at 06:31:35PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Sat, Nov 20, 2021 at 12:13:02PM +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>\n> s/allow/allows/\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 | 51 ++++\n> > .../libcamera/internal/tracepoints/request.tp | 9 +-\n> > include/libcamera/request.h | 19 +-\n> > src/libcamera/pipeline_handler.cpp | 15 +-\n> > src/libcamera/request.cpp | 256 ++++++++++++------\n> > 6 files changed, 245 insertions(+), 106 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..59bddde3a090\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -0,0 +1,51 @@\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> > +\tuint64_t cookie() const;\n>\n> Why do we need a cookie() function here, which wraps Request::cookie(),\n> when cookie_ is stored in the Request class ?\n>\n\nAs the comment on the function implementation reports, it is required\nby the tracing framework.\n\nTRACEPOINT_EVENT(\n\tlibcamera,\n\trequest_complete_buffer,\n\tTP_ARGS(\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_hex(uintptr_t, buffer, reinterpret_cast(buf))\n\t\tctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status)\n\t)\n)\n\n*req is now a Request::Private and we have\n\n\t\tctf_integer(uint64_t, cookie, req->cookie())\n\t\tctf_integer(int, status, req->status())\n\nWhich I can workaround with:\n\n\t\tctf_integer(uint64_t, cookie, req->_o()->cookie())\n\t\tctf_integer(int, status, req->_o()->status())\n\n\n> > +\tRequest::Status status() const;\n>\n> Same here.\n>\n> > +\n> > +\tbool completeBuffer(FrameBuffer *buffer);\n> > +\tvoid complete();\n> > +\tvoid cancel();\n> > +\tvoid reuse();\n> > +\n> > +\tuint32_t sequence_ = 0;\n>\n> The reason why you can drop the friend statement is because you make\n> this member variable public. That's not very nice :-S I'd keep it in\n> Request for now, along with cookie_ and status_, until we decide how to\n> handle those members.\n>\n\nack, I was unsure. Actually removing a friend to then give access to\nall other to one field is not a huge win, I concur :)\n\nBut at least I can move the friend statement to the Private class if\nthe field is moved there too!\n\n> > +\n> > +private:\n> > +\tvoid _cancel();\n> > +\n> > +\tCamera *camera_;\n> > +\tbool cancelled_;\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..37cd2f8864ce 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,7 +80,7 @@ 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> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index d16904e6b679..f0c5163d987e 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..90c436648405 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,165 @@ 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> > +\t_cancel();\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> > + * \\copydoc Request::cookie()\n> > + *\n> > + * Used by the tracing framework\n> > + */\n> > +uint64_t Request::Private::cookie() const\n> > +{\n> > +\treturn _o()->cookie();\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc Request::status()\n> > + *\n> > + * Used by the tracing framework\n> > + */\n> > +Request::Status Request::Private::status() const\n> > +{\n> > +\treturn _o()->status();\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::_cancel()\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> > +\t_cancel();\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> > + * \\var Request::Private::sequence_\n> > + * \\brief The request sequence number\n> > + *\n> > + * \\copydoc Request::sequence()\n> > + */\n> > +\n> > /**\n> > * \\enum Request::Status\n> > * Request completion status\n> > @@ -75,8 +234,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 +272,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 +346,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 +385,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 +399,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 +424,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 +450,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();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 0EFF4BDB13\n\tfor ;\n\tFri, 26 Nov 2021 11:04:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26B17604FD;\n\tFri, 26 Nov 2021 12:04:16 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE7696011E\n\tfor ;\n\tFri, 26 Nov 2021 12:04:14 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 20AEC240003;\n\tFri, 26 Nov 2021 11:04:13 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 12:05:06 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211126110506.24uofj5wiocwgj6o@uno.localdomain>","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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\" "}},{"id":21404,"web_url":"https://patchwork.libcamera.org/comment/21404/","msgid":"","date":"2021-11-30T04:54:47","subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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\nOn Fri, Nov 26, 2021 at 12:05:06PM +0100, Jacopo Mondi wrote:\n> On Sun, Nov 21, 2021 at 06:31:35PM +0200, Laurent Pinchart wrote:\n> > On Sat, Nov 20, 2021 at 12:13:02PM +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> >\n> > s/allow/allows/\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 | 51 ++++\n> > > .../libcamera/internal/tracepoints/request.tp | 9 +-\n> > > include/libcamera/request.h | 19 +-\n> > > src/libcamera/pipeline_handler.cpp | 15 +-\n> > > src/libcamera/request.cpp | 256 ++++++++++++------\n> > > 6 files changed, 245 insertions(+), 106 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..59bddde3a090\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/request.h\n> > > @@ -0,0 +1,51 @@\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> > > +\tuint64_t cookie() const;\n> >\n> > Why do we need a cookie() function here, which wraps Request::cookie(),\n> > when cookie_ is stored in the Request class ?\n> \n> As the comment on the function implementation reports, it is required\n> by the tracing framework.\n\nAh indeed sorry.\n\n> TRACEPOINT_EVENT(\n> \tlibcamera,\n> \trequest_complete_buffer,\n> \tTP_ARGS(\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_hex(uintptr_t, buffer, reinterpret_cast(buf))\n> \t\tctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status)\n> \t)\n> )\n> \n> *req is now a Request::Private and we have\n> \n> \t\tctf_integer(uint64_t, cookie, req->cookie())\n> \t\tctf_integer(int, status, req->status())\n> \n> Which I can workaround with:\n> \n> \t\tctf_integer(uint64_t, cookie, req->_o()->cookie())\n> \t\tctf_integer(int, status, req->_o()->status())\n\nYou can pick either option. If you prefer keeping the Private::cookie()\nfunction, you can also make it inline if desired.\n\n> > > +\tRequest::Status status() const;\n> >\n> > Same here.\n> >\n> > > +\n> > > +\tbool completeBuffer(FrameBuffer *buffer);\n> > > +\tvoid complete();\n> > > +\tvoid cancel();\n> > > +\tvoid reuse();\n> > > +\n> > > +\tuint32_t sequence_ = 0;\n> >\n> > The reason why you can drop the friend statement is because you make\n> > this member variable public. That's not very nice :-S I'd keep it in\n> > Request for now, along with cookie_ and status_, until we decide how to\n> > handle those members.\n> \n> ack, I was unsure. Actually removing a friend to then give access to\n> all other to one field is not a huge win, I concur :)\n> \n> But at least I can move the friend statement to the Private class if\n> the field is moved there too!\n> \n> > > +\n> > > +private:\n> > > +\tvoid _cancel();\n> > > +\n> > > +\tCamera *camera_;\n> > > +\tbool cancelled_;\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..37cd2f8864ce 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,7 +80,7 @@ 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> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index d16904e6b679..f0c5163d987e 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..90c436648405 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,165 @@ 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> > > +\t_cancel();\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> > > + * \\copydoc Request::cookie()\n> > > + *\n> > > + * Used by the tracing framework\n> > > + */\n> > > +uint64_t Request::Private::cookie() const\n> > > +{\n> > > +\treturn _o()->cookie();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc Request::status()\n> > > + *\n> > > + * Used by the tracing framework\n> > > + */\n> > > +Request::Status Request::Private::status() const\n> > > +{\n> > > +\treturn _o()->status();\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::_cancel()\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> > > +\t_cancel();\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> > > + * \\var Request::Private::sequence_\n> > > + * \\brief The request sequence number\n> > > + *\n> > > + * \\copydoc Request::sequence()\n> > > + */\n> > > +\n> > > /**\n> > > * \\enum Request::Status\n> > > * Request completion status\n> > > @@ -75,8 +234,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 +272,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 +346,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 +385,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 +399,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 +424,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 +450,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 357EABF415\n\tfor ;\n\tTue, 30 Nov 2021 04:55:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AC1B605B7;\n\tTue, 30 Nov 2021 05:55:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8078F604FC\n\tfor ;\n\tTue, 30 Nov 2021 05:55:12 +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 E32508F0;\n\tTue, 30 Nov 2021 05:55:11 +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=\"uffmCz3c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638248112;\n\tbh=D+Le1+dT0ox9dvF9Jw1YUB4ltwOGMPR+JYdAqPzdTx0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uffmCz3cYm6nkHTgNt/TBF0M5nw7sesGCoqr6KLa1xna28/PBItP0qoH1xXODfYCv\n\tSMixP7TYibzp+HlIXpGem9x5a6JIjH+IO+t33RRcMuNqiWUVHON+U/f1TEVvs64X60\n\tOVyHnA1xhvN/TNHLBhMxc+othOBcQJ+0fWWs2a+Q=","Date":"Tue, 30 Nov 2021 06:54:47 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-2-jacopo@jmondi.org>\n\t\n\t<20211126110506.24uofj5wiocwgj6o@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211126110506.24uofj5wiocwgj6o@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 01/12] 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\" "}}]