[{"id":17973,"web_url":"https://patchwork.libcamera.org/comment/17973/","msgid":"<20210705103739.qqzxgtkr6bbitkz6@uno.localdomain>","date":"2021-07-05T10:37:39","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make\n\tFrameBuffer 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 Mon, Jul 05, 2021 at 02:28:17AM +0300, 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> ---\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, 47 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index baf22a466907..7265e816b036 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\nOh, I should have read the series in full before asking\n\nThe \"Private\" is available to other internal classes as it gets\ndefined in an internal header.\n\nLooks good\nReviewed-by: Jacopo Mondi for 1/3 too\n\nThanks\n j\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..5e13e281fb8c 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,12 @@ 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> +Request *FrameBuffer::request() const\n> +{\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\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> +\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> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1E5CEC0100\n\tfor ;\n\tMon, 5 Jul 2021 10:36:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B640684E4;\n\tMon, 5 Jul 2021 12:36:52 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2FC66050A\n\tfor ;\n\tMon, 5 Jul 2021 12:36:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 291FF60003;\n\tMon, 5 Jul 2021 10:36:49 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 12:37:39 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210705103739.qqzxgtkr6bbitkz6@uno.localdomain>","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17993,"web_url":"https://patchwork.libcamera.org/comment/17993/","msgid":"<148c087d-a73b-2bbe-3d26-f78f516a3279@ideasonboard.com>","date":"2021-07-06T04:47:16","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make\n\tFrameBuffer class Extensible","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 7/5/21 4:58 AM, 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\n\nReviewed-by: Umang Jain \n\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, 47 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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> +Request *FrameBuffer::request() const\n> +{\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\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> +\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;","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 91B33BD794\n\tfor ;\n\tTue, 6 Jul 2021 04:47:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE137684E6;\n\tTue, 6 Jul 2021 06:47:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77F0060284\n\tfor ;\n\tTue, 6 Jul 2021 06:47:24 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.59])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A6623F5;\n\tTue, 6 Jul 2021 06:47:22 +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=\"aamRUbPR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625546844;\n\tbh=IKgHZtz61/lZ6dyuSCjUp3JLtUDtPPA+LZTZZZHb6nI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=aamRUbPRSQDrPNj4R9Wa/ac5F/oFv5bHw7tmTN2hfhDywtiVvyC7uX9B0iPyYqNvb\n\t+HtIR12TqaGHqLHnowIp8w0bsYF7KUSdY06r7d+WU8Gobfh9hRcp6uTvBk7czghwBa\n\tyYUzPue85kFKvX6KlAzIrfHxvgzJJM8h7IPDKFd4=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<148c087d-a73b-2bbe-3d26-f78f516a3279@ideasonboard.com>","Date":"Tue, 6 Jul 2021 10:17:16 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":18003,"web_url":"https://patchwork.libcamera.org/comment/18003/","msgid":"<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>","date":"2021-07-07T08:41:40","subject":"Re: [libcamera-devel] [PATCH 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":"Hi Laurent,\n\nOn 05/07/2021 00:28, 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> ---\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, 47 insertions(+), 23 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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> +Request *FrameBuffer::request() const\n> +{\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\n\nso internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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> +\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\nBut anywhere else, is through the _d()?\n\nIs LIBCAMERA_D_PTR now redundant, that even internally the access could\nbe done through _d()?\n\nThough I find\n\n\tbuffer->_d()->setRequest(request);\n\nA bit more ugly to read... and now the read has to know which parts of\nthe object are accessible through buffer-> and which ones have to be\nindirected - but I guess that's not really an issue. Just an adjustment\nrequired on the developers.\n\nDoes _d convey the right meaning to everyone for that? Or would the\naccessor be better named as\n\n\tbuffer->internal()->setRequest(request) ?\n\n\nI see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\nthere to maintain the existing functionality anyway, but my question is\nmore on the \"Do we want two ways to access the same data\"\n\n\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 54235C3224\n\tfor ;\n\tWed, 7 Jul 2021 08:41:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C065768502;\n\tWed, 7 Jul 2021 10:41:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F7E7684E6\n\tfor ;\n\tWed, 7 Jul 2021 10:41:44 +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 C32572E4;\n\tWed, 7 Jul 2021 10:41:43 +0200 (CEST)"],"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=\"sAqXvvqM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625647303;\n\tbh=ATyhrl4SHfCtAJ2OdoimdQBLeq/cPpUjoontv0khXx4=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=sAqXvvqMFyNjFA6DyowGw0QftSCMUKba843jWl7dPZJ6+YLyEp5z199HoxCjgVvWL\n\t4oHV6J8M/gEzEoStkGpazlXs9pLbeiWSf8LzPLaJwRoistzRE3Rqa2BcQeqOU/DZ3n\n\tQkHwEM2WksH6rP33XtH4/2LECoeWqMduPxDGAXic=","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>","Message-ID":"<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>","Date":"Wed, 7 Jul 2021 09:41:40 +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":"<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":18012,"web_url":"https://patchwork.libcamera.org/comment/18012/","msgid":"","date":"2021-07-07T11:52:22","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make\n\tFrameBuffer class Extensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:\n> On 05/07/2021 00:28, 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> > ---\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, 47 insertions(+), 23 deletions(-)\n> > \n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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> > +Request *FrameBuffer::request() const\n> > +{\n> > +\tconst Private *const d = LIBCAMERA_D_PTR();\n> \n> so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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> > +\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> \n> But anywhere else, is through the _d()?\n> \n> Is LIBCAMERA_D_PTR now redundant, that even internally the access could\n> be done through _d()?\n> \n> Though I find\n> \n> \tbuffer->_d()->setRequest(request);\n> \n> A bit more ugly to read... and now the read has to know which parts of\n> the object are accessible through buffer-> and which ones have to be\n> indirected - but I guess that's not really an issue. Just an adjustment\n> required on the developers.\n\nAnd that's also the whole point of the D-Pointer pattern, if we added\nFrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it\nwouldn't make much sense :-)\n\n> Does _d convey the right meaning to everyone for that? Or would the\n> accessor be better named as\n> \n> \tbuffer->internal()->setRequest(request) ?\n\nI'd like to keep the name as short as possible.\n\n> I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\n> there to maintain the existing functionality anyway, but my question is\n> more on the \"Do we want two ways to access the same data\"\n\nWe could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there\noriginally to hide the template argument, but now that it's\ngone, I think it makes sense to drop the macro.\n\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 96315C3224\n\tfor ;\n\tWed, 7 Jul 2021 11:53:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FAEC6028E;\n\tWed, 7 Jul 2021 13:53:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA3E560284\n\tfor ;\n\tWed, 7 Jul 2021 13:53:06 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 556912E4;\n\tWed, 7 Jul 2021 13:53:06 +0200 (CEST)"],"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=\"juijlKsf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625658786;\n\tbh=wB2k3JG/Il5Othmb0iTUuCLid53dWCzC+rOKXC/7wQ8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=juijlKsfIVqHSFFZZ9pIIKxTJW+IQyehFCa1UN2MmsZYVnR1kSNdfNg5IDTjNa34A\n\txeIjLT9JlOsUjX88LXbzwpH0zwgPmoNx/5rTr/8yOvF5Dw6b9hqOeDNkQdq/lR0VZs\n\tce9yRhTsmyS319tZkmjLik6qQtysOxAze7MDYusw=","Date":"Wed, 7 Jul 2021 14:52:22 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>\n\t<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18015,"web_url":"https://patchwork.libcamera.org/comment/18015/","msgid":"","date":"2021-07-07T12:35:33","subject":"Re: [libcamera-devel] [PATCH 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":"Hi Laurent,\n\nOn 07/07/2021 12:52, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:\n>> On 05/07/2021 00:28, 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>>> ---\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, 47 insertions(+), 23 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n>>> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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>>> +Request *FrameBuffer::request() const\n>>> +{\n>>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n>>\n>> so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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>>> +\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>>\n>> But anywhere else, is through the _d()?\n>>\n>> Is LIBCAMERA_D_PTR now redundant, that even internally the access could\n>> be done through _d()?\n>>\n>> Though I find\n>>\n>> \tbuffer->_d()->setRequest(request);\n>>\n>> A bit more ugly to read... and now the read has to know which parts of\n>> the object are accessible through buffer-> and which ones have to be\n>> indirected - but I guess that's not really an issue. Just an adjustment\n>> required on the developers.\n> \n> And that's also the whole point of the D-Pointer pattern, if we added\n> FrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it\n> wouldn't make much sense :-)\n\nI get that, I'm not suggesting wrapping it to be available in the\n'public' methods of the class...\n\n> \n>> Does _d convey the right meaning to everyone for that? Or would the\n>> accessor be better named as\n>>\n>> \tbuffer->internal()->setRequest(request) ?\n> \n> I'd like to keep the name as short as possible.\n\nWhat does _d actually mean?\n\n->internal() I could understand.\n->private() I could understand (but it's a reserved keyword).\n\nIf it was _p I could understand it was shorthand for either pointer or\nprivate.\n\nBut 'd' sounds like a random letter to me currently. it doesn't convey\nthe message \"Access the internal object\" in any way other than a magic\nshort letter.\n\nI.e. is there a distinction as to why it is a 'd' rather than a 'z'...\n\nhttps://en.wikipedia.org/wiki/Opaque_pointer tells me that it's simply\nthe pattern name ... so is _d data? or something else?\n\n\n\n\n>> I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\n>> there to maintain the existing functionality anyway, but my question is\n>> more on the \"Do we want two ways to access the same data\"\n> \n> We could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there\n> originally to hide the template argument, but now that it's\n> gone, I think it makes sense to drop the macro.\n\nWill that apply to the _o() usage as well? (it doesn't have to)\n\n(Hence why I asked earlier if _o() should be templates too, but they\naren't used outside of the ::Private)\n\n\n\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>>>\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 407F2BD794\n\tfor ;\n\tWed, 7 Jul 2021 12:35:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD20368503;\n\tWed, 7 Jul 2021 14:35:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 184BD60284\n\tfor ;\n\tWed, 7 Jul 2021 14:35:36 +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 95B9C2E4;\n\tWed, 7 Jul 2021 14:35:35 +0200 (CEST)"],"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=\"nzJ+u7/Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625661335;\n\tbh=1KIPX6dbPHavXrmHaf69hpjqru6h9gSkXucMa0wZkUk=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=nzJ+u7/ZtwEIxwHnoCkccdwWhieCqoxsfazN2381NkXGas50Impjd6YORlqDFVAL7\n\toJNsYFXVdenMjA6/rReCCySIleSVT0YMYySnA1wyR17MfcUfMIk4BksSPn9xMoVk3f\n\tpPcDtEggSirZKyCqj9wlG5V2Gf3513MQl8HY/YVM=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>\n\t<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>\n\t","Message-ID":"","Date":"Wed, 7 Jul 2021 13:35:33 +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":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18074,"web_url":"https://patchwork.libcamera.org/comment/18074/","msgid":"","date":"2021-07-11T14:38:28","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make\n\tFrameBuffer class Extensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 07, 2021 at 01:35:33PM +0100, Kieran Bingham wrote:\n> On 07/07/2021 12:52, Laurent Pinchart wrote:\n> > On Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:\n> >> On 05/07/2021 00:28, 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> >>> ---\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, 47 insertions(+), 23 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> >>> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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> >>> +Request *FrameBuffer::request() const\n> >>> +{\n> >>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> >>\n> >> so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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> >>> +\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> >>\n> >> But anywhere else, is through the _d()?\n> >>\n> >> Is LIBCAMERA_D_PTR now redundant, that even internally the access could\n> >> be done through _d()?\n> >>\n> >> Though I find\n> >>\n> >> \tbuffer->_d()->setRequest(request);\n> >>\n> >> A bit more ugly to read... and now the read has to know which parts of\n> >> the object are accessible through buffer-> and which ones have to be\n> >> indirected - but I guess that's not really an issue. Just an adjustment\n> >> required on the developers.\n> > \n> > And that's also the whole point of the D-Pointer pattern, if we added\n> > FrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it\n> > wouldn't make much sense :-)\n> \n> I get that, I'm not suggesting wrapping it to be available in the\n> 'public' methods of the class...\n> \n> >> Does _d convey the right meaning to everyone for that? Or would the\n> >> accessor be better named as\n> >>\n> >> \tbuffer->internal()->setRequest(request) ?\n> > \n> > I'd like to keep the name as short as possible.\n> \n> What does _d actually mean?\n> \n> ->internal() I could understand.\n> ->private() I could understand (but it's a reserved keyword).\n> \n> If it was _p I could understand it was shorthand for either pointer or\n> private.\n> \n> But 'd' sounds like a random letter to me currently. it doesn't convey\n> the message \"Access the internal object\" in any way other than a magic\n> short letter.\n> \n> I.e. is there a distinction as to why it is a 'd' rather than a 'z'...\n> \n> https://en.wikipedia.org/wiki/Opaque_pointer tells me that it's simply\n> the pattern name ... so is _d data? or something else?\n\nIt's the design pattern name. I'm not sure what it meant in the first\nplace, it could be data.\n\n> >> I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\n> >> there to maintain the existing functionality anyway, but my question is\n> >> more on the \"Do we want two ways to access the same data\"\n> > \n> > We could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there\n> > originally to hide the template argument, but now that it's\n> > gone, I think it makes sense to drop the macro.\n> \n> Will that apply to the _o() usage as well? (it doesn't have to)\n> \n> (Hence why I asked earlier if _o() should be templates too, but they\n> aren't used outside of the ::Private)\n\nIf we want to drop the LIBCAMERA_O_PTR() then I think we should define\noverriden version of the _o() function to avoid having to write\n_o(). Anyone has a preference ?\n\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 DA89DC3224\n\tfor ;\n\tSun, 11 Jul 2021 14:39:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CB3868521;\n\tSun, 11 Jul 2021 16:39:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 72A2568519\n\tfor ;\n\tSun, 11 Jul 2021 16:39:14 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E8188CC;\n\tSun, 11 Jul 2021 16:39:13 +0200 (CEST)"],"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=\"razt81rB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626014354;\n\tbh=p0JW81tWj/XC70oHIkxIeyyb/ZqmPi5CGWu6J6zZ6Rk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=razt81rBCPi3wz9fEWAtS5ievBo3hcpDPW2gqrO28W0eq986L8C6Z+CzVCn4LWp1j\n\tCrIEoEKtVhOXkd4F6r9nZMG2/VYr4SUsBjzxvD+TsjD7fYsWstlKqQH8qNr2W3iGab\n\t+mC+LTSw5IChjVaZmKG7q/srH+z+MBLSntrI0n3s=","Date":"Sun, 11 Jul 2021 17:38:28 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>\n\t<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18098,"web_url":"https://patchwork.libcamera.org/comment/18098/","msgid":"<20210712081529.pyewguckn3bb7hqp@uno.localdomain>","date":"2021-07-12T08:15:29","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make\n\tFrameBuffer 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, Jul 11, 2021 at 05:38:28PM +0300, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> On Wed, Jul 07, 2021 at 01:35:33PM +0100, Kieran Bingham wrote:\n> > On 07/07/2021 12:52, Laurent Pinchart wrote:\n> > > On Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:\n> > >> On 05/07/2021 00:28, 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> > >>> ---\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, 47 insertions(+), 23 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > >>> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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> > >>> +Request *FrameBuffer::request() const\n> > >>> +{\n> > >>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> > >>\n> > >> so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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> > >>> +\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> > >>\n> > >> But anywhere else, is through the _d()?\n> > >>\n> > >> Is LIBCAMERA_D_PTR now redundant, that even internally the access could\n> > >> be done through _d()?\n> > >>\n> > >> Though I find\n> > >>\n> > >> \tbuffer->_d()->setRequest(request);\n> > >>\n> > >> A bit more ugly to read... and now the read has to know which parts of\n> > >> the object are accessible through buffer-> and which ones have to be\n> > >> indirected - but I guess that's not really an issue. Just an adjustment\n> > >> required on the developers.\n> > >\n> > > And that's also the whole point of the D-Pointer pattern, if we added\n> > > FrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it\n> > > wouldn't make much sense :-)\n> >\n> > I get that, I'm not suggesting wrapping it to be available in the\n> > 'public' methods of the class...\n> >\n> > >> Does _d convey the right meaning to everyone for that? Or would the\n> > >> accessor be better named as\n> > >>\n> > >> \tbuffer->internal()->setRequest(request) ?\n> > >\n> > > I'd like to keep the name as short as possible.\n> >\n> > What does _d actually mean?\n> >\n> > ->internal() I could understand.\n> > ->private() I could understand (but it's a reserved keyword).\n> >\n> > If it was _p I could understand it was shorthand for either pointer or\n> > private.\n> >\n> > But 'd' sounds like a random letter to me currently. it doesn't convey\n> > the message \"Access the internal object\" in any way other than a magic\n> > short letter.\n> >\n> > I.e. is there a distinction as to why it is a 'd' rather than a 'z'...\n> >\n> > https://en.wikipedia.org/wiki/Opaque_pointer tells me that it's simply\n> > the pattern name ... so is _d data? or something else?\n>\n> It's the design pattern name. I'm not sure what it meant in the first\n> place, it could be data.\n>\n> > >> I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\n> > >> there to maintain the existing functionality anyway, but my question is\n> > >> more on the \"Do we want two ways to access the same data\"\n> > >\n> > > We could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there\n> > > originally to hide the template argument, but now that it's\n> > > gone, I think it makes sense to drop the macro.\n> >\n> > Will that apply to the _o() usage as well? (it doesn't have to)\n> >\n> > (Hence why I asked earlier if _o() should be templates too, but they\n> > aren't used outside of the ::Private)\n>\n> If we want to drop the LIBCAMERA_O_PTR() then I think we should define\n> overriden version of the _o() function to avoid having to write\n> _o(). Anyone has a preference ?\n>\n\nI don't, but I also don't really think it's worth tbh.\n\nWhatever naming we chose, one has to read documentation to get what\n'internal' 'external' 'outer' or 'private' means in the context of\nExensible. Even if the macro it's not required, LIBCAMERA_D_PTR()\nmakes explicit what the reader is dealing with, more than just d_().\n\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> > >>>\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 ABED7C3224\n\tfor ;\n\tMon, 12 Jul 2021 08:14:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C84A68524;\n\tMon, 12 Jul 2021 10:14:42 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A442268506\n\tfor ;\n\tMon, 12 Jul 2021 10:14:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 8EC61100017;\n\tMon, 12 Jul 2021 08:14:39 +0000 (UTC)"],"Date":"Mon, 12 Jul 2021 10:15:29 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210712081529.pyewguckn3bb7hqp@uno.localdomain>","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>\n\t<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18100,"web_url":"https://patchwork.libcamera.org/comment/18100/","msgid":"<9e758d2c-a415-abfb-3caa-83654f43cddb@ideasonboard.com>","date":"2021-07-12T08:25:17","subject":"Re: [libcamera-devel] [PATCH 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":"Hi Jacopo/Laurent,\n\nOn 12/07/2021 09:15, Jacopo Mondi wrote:\n> Hi Laurent,\n> \n> On Sun, Jul 11, 2021 at 05:38:28PM +0300, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> On Wed, Jul 07, 2021 at 01:35:33PM +0100, Kieran Bingham wrote:\n>>> On 07/07/2021 12:52, Laurent Pinchart wrote:\n>>>> On Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:\n>>>>> On 05/07/2021 00:28, 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>>>>>> ---\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, 47 insertions(+), 23 deletions(-)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n>>>>>> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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>>>>>> +Request *FrameBuffer::request() const\n>>>>>> +{\n>>>>>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n>>>>>\n>>>>> so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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>>>>>> +\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>>>>>\n>>>>> But anywhere else, is through the _d()?\n>>>>>\n>>>>> Is LIBCAMERA_D_PTR now redundant, that even internally the access could\n>>>>> be done through _d()?\n>>>>>\n>>>>> Though I find\n>>>>>\n>>>>> \tbuffer->_d()->setRequest(request);\n>>>>>\n>>>>> A bit more ugly to read... and now the read has to know which parts of\n>>>>> the object are accessible through buffer-> and which ones have to be\n>>>>> indirected - but I guess that's not really an issue. Just an adjustment\n>>>>> required on the developers.\n>>>>\n>>>> And that's also the whole point of the D-Pointer pattern, if we added\n>>>> FrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it\n>>>> wouldn't make much sense :-)\n>>>\n>>> I get that, I'm not suggesting wrapping it to be available in the\n>>> 'public' methods of the class...\n>>>\n>>>>> Does _d convey the right meaning to everyone for that? Or would the\n>>>>> accessor be better named as\n>>>>>\n>>>>> \tbuffer->internal()->setRequest(request) ?\n>>>>\n>>>> I'd like to keep the name as short as possible.\n>>>\n>>> What does _d actually mean?\n>>>\n>>> ->internal() I could understand.\n>>> ->private() I could understand (but it's a reserved keyword).\n>>>\n>>> If it was _p I could understand it was shorthand for either pointer or\n>>> private.\n>>>\n>>> But 'd' sounds like a random letter to me currently. it doesn't convey\n>>> the message \"Access the internal object\" in any way other than a magic\n>>> short letter.\n>>>\n>>> I.e. is there a distinction as to why it is a 'd' rather than a 'z'...\n>>>\n>>> https://en.wikipedia.org/wiki/Opaque_pointer tells me that it's simply\n>>> the pattern name ... so is _d data? or something else?\n>>\n>> It's the design pattern name. I'm not sure what it meant in the first\n>> place, it could be data.\n>>\n>>>>> I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\n>>>>> there to maintain the existing functionality anyway, but my question is\n>>>>> more on the \"Do we want two ways to access the same data\"\n>>>>\n>>>> We could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there\n>>>> originally to hide the template argument, but now that it's\n>>>> gone, I think it makes sense to drop the macro.\n>>>\n>>> Will that apply to the _o() usage as well? (it doesn't have to)\n>>>\n>>> (Hence why I asked earlier if _o() should be templates too, but they\n>>> aren't used outside of the ::Private)\n>>\n>> If we want to drop the LIBCAMERA_O_PTR() then I think we should define\n>> overriden version of the _o() function to avoid having to write\n>> _o(). Anyone has a preference ?\n>>\n> \n> I don't, but I also don't really think it's worth tbh.\n> \n> Whatever naming we chose, one has to read documentation to get what\n> 'internal' 'external' 'outer' or 'private' means in the context of\n> Exensible. Even if the macro it's not required, LIBCAMERA_D_PTR()\n> makes explicit what the reader is dealing with, more than just d_().\n> \n\n\nQuoting my earlier statement:\n\n>> more on the \"Do we want two ways to access the same data\"\n\nThe part of this patch I disliked is that we can now access the internal\ndata with either\n\n blah->_d()->foo()\n\nor\n\n const mydata *d = LIBCAMERA_D_PTR();\n d->foo();\n\nwhich simply calls _d()\n\nI don't mind if we keep LIBCAMERA_D_PTR as the style - but I'd prefer\nconsistency ...\n\nAs LIBCAMERA_D_PTR isn't really needed, that's why I asked if it was\nredundant.\n\nIf using it would be more explicit - then it should be used externally too.\n\n\tbuffer->LIBCAMERA_D_PTR()->setRequest(request);\n\nwould technically still work - but it doesn't look right ;-)\n\n\n\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>>>>>>\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 9FF65C3224\n\tfor ;\n\tMon, 12 Jul 2021 08:25:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 099D068526;\n\tMon, 12 Jul 2021 10:25:22 +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 6411768506\n\tfor ;\n\tMon, 12 Jul 2021 10:25:20 +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 C49CFCC;\n\tMon, 12 Jul 2021 10:25:19 +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=\"hsmrn3gy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626078320;\n\tbh=e28uppOYenEph6ee9uLkLap65+3uAHc5o1N7qJJOQJ0=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=hsmrn3gygkKszfP5D4FroU125Pi8D0KxIEVAHF7SrhhbEl5QqpsiSJhHby29CaGcD\n\tkFI0/FkE8UCmjWuiuFtywaumsuXZRvkd1+mX4PydQS/EHXBkGeUn/InXzIKCHXomMZ\n\tiDoDrjCr7ab4lsnkR+NB/oDHOHAZIJW3E5cyJTTg=","From":"Kieran Bingham ","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>\n\t<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>\n\t\n\t\n\t\n\t<20210712081529.pyewguckn3bb7hqp@uno.localdomain>","Message-ID":"<9e758d2c-a415-abfb-3caa-83654f43cddb@ideasonboard.com>","Date":"Mon, 12 Jul 2021 09:25:17 +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":"<20210712081529.pyewguckn3bb7hqp@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18110,"web_url":"https://patchwork.libcamera.org/comment/18110/","msgid":"","date":"2021-07-12T13:03:50","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: framebuffer: Make\n\tFrameBuffer class Extensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jul 12, 2021 at 09:25:17AM +0100, Kieran Bingham wrote:\n> On 12/07/2021 09:15, Jacopo Mondi wrote:\n> > On Sun, Jul 11, 2021 at 05:38:28PM +0300, Laurent Pinchart wrote:\n> >> On Wed, Jul 07, 2021 at 01:35:33PM +0100, Kieran Bingham wrote:\n> >>> On 07/07/2021 12:52, Laurent Pinchart wrote:\n> >>>> On Wed, Jul 07, 2021 at 09:41:40AM +0100, Kieran Bingham wrote:\n> >>>>> On 05/07/2021 00:28, 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> >>>>>> ---\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, 47 insertions(+), 23 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> >>>>>> index baf22a466907..7265e816b036 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..5e13e281fb8c 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,12 @@ 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> >>>>>> +Request *FrameBuffer::request() const\n> >>>>>> +{\n> >>>>>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> >>>>>\n> >>>>> so internally in a class with a Private, it uses LIBCAMERA_D_PTR(),\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> >>>>>> +\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> >>>>>\n> >>>>> But anywhere else, is through the _d()?\n> >>>>>\n> >>>>> Is LIBCAMERA_D_PTR now redundant, that even internally the access could\n> >>>>> be done through _d()?\n> >>>>>\n> >>>>> Though I find\n> >>>>>\n> >>>>> \tbuffer->_d()->setRequest(request);\n> >>>>>\n> >>>>> A bit more ugly to read... and now the read has to know which parts of\n> >>>>> the object are accessible through buffer-> and which ones have to be\n> >>>>> indirected - but I guess that's not really an issue. Just an adjustment\n> >>>>> required on the developers.\n> >>>>\n> >>>> And that's also the whole point of the D-Pointer pattern, if we added\n> >>>> FrameBuffer::setRequest() as a wrapper around _d()->setRequest(), it\n> >>>> wouldn't make much sense :-)\n> >>>\n> >>> I get that, I'm not suggesting wrapping it to be available in the\n> >>> 'public' methods of the class...\n> >>>\n> >>>>> Does _d convey the right meaning to everyone for that? Or would the\n> >>>>> accessor be better named as\n> >>>>>\n> >>>>> \tbuffer->internal()->setRequest(request) ?\n> >>>>\n> >>>> I'd like to keep the name as short as possible.\n> >>>\n> >>> What does _d actually mean?\n> >>>\n> >>> ->internal() I could understand.\n> >>> ->private() I could understand (but it's a reserved keyword).\n> >>>\n> >>> If it was _p I could understand it was shorthand for either pointer or\n> >>> private.\n> >>>\n> >>> But 'd' sounds like a random letter to me currently. it doesn't convey\n> >>> the message \"Access the internal object\" in any way other than a magic\n> >>> short letter.\n> >>>\n> >>> I.e. is there a distinction as to why it is a 'd' rather than a 'z'...\n> >>>\n> >>> https://en.wikipedia.org/wiki/Opaque_pointer tells me that it's simply\n> >>> the pattern name ... so is _d data? or something else?\n> >>\n> >> It's the design pattern name. I'm not sure what it meant in the first\n> >> place, it could be data.\n> >>\n> >>>>> I see that LIBCAMERA_D_PTR does now indeed simply call _d() so it's\n> >>>>> there to maintain the existing functionality anyway, but my question is\n> >>>>> more on the \"Do we want two ways to access the same data\"\n> >>>>\n> >>>> We could drop LIBCAMERA_D_PTR() as it's easy to type _d(). It was there\n> >>>> originally to hide the template argument, but now that it's\n> >>>> gone, I think it makes sense to drop the macro.\n> >>>\n> >>> Will that apply to the _o() usage as well? (it doesn't have to)\n> >>>\n> >>> (Hence why I asked earlier if _o() should be templates too, but they\n> >>> aren't used outside of the ::Private)\n> >>\n> >> If we want to drop the LIBCAMERA_O_PTR() then I think we should define\n> >> overriden version of the _o() function to avoid having to write\n> >> _o(). Anyone has a preference ?\n> >>\n> > \n> > I don't, but I also don't really think it's worth tbh.\n> > \n> > Whatever naming we chose, one has to read documentation to get what\n> > 'internal' 'external' 'outer' or 'private' means in the context of\n> > Exensible. Even if the macro it's not required, LIBCAMERA_D_PTR()\n> > makes explicit what the reader is dealing with, more than just d_().\n> \n> Quoting my earlier statement:\n> \n> >> more on the \"Do we want two ways to access the same data\"\n> \n> The part of this patch I disliked is that we can now access the internal\n> data with either\n> \n> blah->_d()->foo()\n> \n> or\n> \n> const mydata *d = LIBCAMERA_D_PTR();\n> d->foo();\n> \n> which simply calls _d()\n> \n> I don't mind if we keep LIBCAMERA_D_PTR as the style - but I'd prefer\n> consistency ...\n> \n> As LIBCAMERA_D_PTR isn't really needed, that's why I asked if it was\n> redundant.\n> \n> If using it would be more explicit - then it should be used externally too.\n> \n> \tbuffer->LIBCAMERA_D_PTR()->setRequest(request);\n> \n> would technically still work - but it doesn't look right ;-)\n\nThat I think we all agree on :-)\n\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 6FAFEC3224\n\tfor ;\n\tMon, 12 Jul 2021 13:04:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE4EC68524;\n\tMon, 12 Jul 2021 15:04:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09BE768513\n\tfor ;\n\tMon, 12 Jul 2021 15:04:38 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79380CC;\n\tMon, 12 Jul 2021 15:04:37 +0200 (CEST)"],"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=\"tnQRGTIg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626095077;\n\tbh=ybLLxFImGN2LZV36n8bly3SxhEZdstgRZnf/3AYoWHQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tnQRGTIgRMNVzwtv8JmYN3JJ0PKx2EakqAQgAGXh3izXXNEhTUB9+o8R4h7pm8rf/\n\tcniEuritBkgUZOwOt04BB1pztzvOO06EhZr9i8P37D/L+S7Tcc3vN1dlCML7lhf2aK\n\t81bzs2socLwBkBcLzuMTZfoGNoUpwsJz0LMBLot0=","Date":"Mon, 12 Jul 2021 16:03:50 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-4-laurent.pinchart@ideasonboard.com>\n\t<0c261e4d-5ab1-ace9-ce12-82b574d58203@ideasonboard.com>\n\t\n\t\n\t\n\t<20210712081529.pyewguckn3bb7hqp@uno.localdomain>\n\t<9e758d2c-a415-abfb-3caa-83654f43cddb@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9e758d2c-a415-abfb-3caa-83654f43cddb@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]