[{"id":18103,"web_url":"https://patchwork.libcamera.org/comment/18103/","msgid":"<1f37fe69-cf83-fcf7-5141-89e710c236b8@ideasonboard.com>","date":"2021-07-12T08:34:51","subject":"Re: [libcamera-devel] [PATCH v2 3/3] libcamera: framebuffer: Make\n\tFrameBuffer class Extensible","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 11/07/2021 18:03, Laurent Pinchart wrote:\n> Implement the D-Pointer design pattern in the FrameBuffer class to allow\n> changing internal data without affecting the public ABI.\n> \n> Move the request_ field and the setRequest() function to the\n> FrameBuffer::Private class. This allows hiding the setRequest() function\n> from the public API, removing one todo item. More fields may be moved\n> later.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Umang Jain \n\nReviewed-by: Kieran Bingham \n\n> ---\n> Changes since v1:\n> \n> - Drop extraneous semicolon\n> - Use _d() instead of LIBCAMERA_D_PTR()\n> ---\n> include/libcamera/framebuffer.h | 8 +++---\n> include/libcamera/internal/framebuffer.h | 13 +++++++++\n> src/libcamera/framebuffer.cpp | 34 ++++++++++++++----------\n> src/libcamera/pipeline/ipu3/cio2.cpp | 3 ++-\n> src/libcamera/pipeline/ipu3/frames.cpp | 5 ++--\n> src/libcamera/request.cpp | 7 ++---\n> 6 files changed, 46 insertions(+), 24 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index baf22a466907..28307890eac9 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -35,8 +35,10 @@ struct FrameMetadata {\n> \tstd::vector planes;\n> };\n> \n> -class FrameBuffer final\n> +class FrameBuffer final : public Extensible\n> {\n> +\tLIBCAMERA_DECLARE_PRIVATE()\n> +\n> public:\n> \tstruct Plane {\n> \t\tFileDescriptor fd;\n> @@ -47,8 +49,7 @@ public:\n> \n> \tconst std::vector &planes() const { return planes_; }\n> \n> -\tRequest *request() const { return request_; }\n> -\tvoid setRequest(Request *request) { request_ = request; }\n> +\tRequest *request() const;\n> \tconst FrameMetadata &metadata() const { return metadata_; }\n> \n> \tunsigned int cookie() const { return cookie_; }\n> @@ -63,7 +64,6 @@ private:\n> \n> \tstd::vector planes_;\n> \n> -\tRequest *request_;\n> \tFrameMetadata metadata_;\n> \n> \tunsigned int cookie_;\n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index 0c76fc62af1d..a11e895d9b88 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -47,6 +47,19 @@ public:\n> \tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> };\n> \n> +class FrameBuffer::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> +\n> +public:\n> +\tPrivate(FrameBuffer *buffer);\n> +\n> +\tvoid setRequest(Request *request) { request_ = request; }\n> +\n> +private:\n> +\tRequest *request_;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__ */\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 4bde556c4013..40bf64b0a4fe 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -100,6 +100,21 @@ LOG_DEFINE_CATEGORY(Buffer)\n> * \\brief Array of per-plane metadata\n> */\n> \n> +FrameBuffer::Private::Private(FrameBuffer *buffer)\n> +\t: Extensible::Private(buffer), request_(nullptr)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn FrameBuffer::Private::setRequest()\n> + * \\brief Set the request this buffer belongs to\n> + * \\param[in] request Request to set\n> + *\n> + * For buffers added to requests by applications, this method is called by\n> + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> + * handlers, it is called by the pipeline handlers themselves.\n> + */\n> +\n> /**\n> * \\class FrameBuffer\n> * \\brief Frame buffer data and its associated dynamic metadata\n> @@ -161,7 +176,7 @@ LOG_DEFINE_CATEGORY(Buffer)\n> * \\param[in] cookie Cookie\n> */\n> FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> -\t: planes_(planes), request_(nullptr), cookie_(cookie)\n> +\t: Extensible(new Private(this)), planes_(planes), cookie_(cookie)\n> {\n> }\n> \n> @@ -172,7 +187,6 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> */\n> \n> /**\n> - * \\fn FrameBuffer::request()\n> * \\brief Retrieve the request this buffer belongs to\n> *\n> * The intended callers of this method are buffer completion handlers that\n> @@ -185,18 +199,10 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> * \\return The Request the FrameBuffer belongs to, or nullptr if the buffer is\n> * not associated with a request\n> */\n> -\n> -/**\n> - * \\fn FrameBuffer::setRequest()\n> - * \\brief Set the request this buffer belongs to\n> - * \\param[in] request Request to set\n> - *\n> - * For buffers added to requests by applications, this method is called by\n> - * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> - * handlers, it is called by the pipeline handlers themselves.\n> - *\n> - * \\todo Shall be hidden from applications with a d-pointer design.\n> - */\n> +Request *FrameBuffer::request() const\n> +{\n> +\treturn _d()->request_;\n> +}\n> \n> /**\n> * \\fn FrameBuffer::metadata()\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 1be2cbcd5cac..1bcd580e251c 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> \n> #include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> \n> @@ -278,7 +279,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)\n> \n> \t\tbuffer = availableBuffers_.front();\n> \t\tavailableBuffers_.pop();\n> -\t\tbuffer->setRequest(request);\n> +\t\tbuffer->_d()->setRequest(request);\n> \t}\n> \n> \tint ret = output_->queueBuffer(buffer);\n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index ce5ccbf18e41..a4c3477cd9ef 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> @@ -56,8 +57,8 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> \tFrameBuffer *paramBuffer = availableParamBuffers_.front();\n> \tFrameBuffer *statBuffer = availableStatBuffers_.front();\n> \n> -\tparamBuffer->setRequest(request);\n> -\tstatBuffer->setRequest(request);\n> +\tparamBuffer->_d()->setRequest(request);\n> +\tstatBuffer->_d()->setRequest(request);\n> \n> \tavailableParamBuffers_.pop();\n> \tavailableStatBuffers_.pop();\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 5faf3c71ff02..c095c9f45b09 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -18,6 +18,7 @@\n> #include \n> \n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/tracepoints.h\"\n> \n> /**\n> @@ -121,7 +122,7 @@ void Request::reuse(ReuseFlag flags)\n> \tif (flags & ReuseBuffers) {\n> \t\tfor (auto pair : bufferMap_) {\n> \t\t\tFrameBuffer *buffer = pair.second;\n> -\t\t\tbuffer->setRequest(this);\n> +\t\t\tbuffer->_d()->setRequest(this);\n> \t\t\tpending_.insert(buffer);\n> \t\t}\n> \t} else {\n> @@ -191,7 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> \t\treturn -EEXIST;\n> \t}\n> \n> -\tbuffer->setRequest(this);\n> +\tbuffer->_d()->setRequest(this);\n> \tpending_.insert(buffer);\n> \tbufferMap_[stream] = buffer;\n> \n> @@ -336,7 +337,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> \tint ret = pending_.erase(buffer);\n> \tASSERT(ret == 1);\n> \n> -\tbuffer->setRequest(nullptr);\n> +\tbuffer->_d()->setRequest(nullptr);\n> \n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> \t\tcancelled_ = true;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 169B6BD794\n\tfor ;\n\tMon, 12 Jul 2021 08:34:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76A4F68524;\n\tMon, 12 Jul 2021 10:34:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2311968506\n\tfor ;\n\tMon, 12 Jul 2021 10:34:54 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6C3ACC;\n\tMon, 12 Jul 2021 10:34:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UeqPcmJe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626078893;\n\tbh=UjePn+4kfkPNzd06gihVy4ZH/iQv0D3ss7xcDGAVWs4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=UeqPcmJeUVP/8imm5QwgwjrvkWomljUU8L6FD7zJazD0fVFDKzg5o47oiA0yI6O91\n\ttm8oKeI2QgW/SdKTmftKm0BWW58qbQweu/eKnpFwWp5mOAx5gqpvaP3SupywXmkjFX\n\tmjPTxNKlTx6d6OEqG9O4ooP8P30IMEM1W2fvIGdY=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210711170359.300-1-laurent.pinchart@ideasonboard.com>\n\t<20210711170359.300-4-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<1f37fe69-cf83-fcf7-5141-89e710c236b8@ideasonboard.com>","Date":"Mon, 12 Jul 2021 09:34:51 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210711170359.300-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] libcamera: framebuffer: Make\n\tFrameBuffer 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]