[{"id":25281,"web_url":"https://patchwork.libcamera.org/comment/25281/","msgid":"<79b04665-fa23-c9ff-38c0-c27ef0aa0e09@ideasonboard.com>","date":"2022-10-05T06:21:36","subject":"Re: [libcamera-devel] [PATCH v2 1/4] libcamera: framebuffer: Move\n\tremaining private data to Private class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch\n\nOn 10/5/22 3:59 AM, Laurent Pinchart via libcamera-devel wrote:\n> Private members of the FrameBuffer class are split between FrameBuffer\n> and FrameBuffer::Private. There was no real justification for this\n> split, and keeping some members private in the FrameBuffer class causes\n> multiple issues:\n>\n> - Future modifications of the FrameBuffer class without breaking the ABI\n> may be more difficult.\n> - Mutable access to members that should not be modified by applications\n> require a friend statement, or going through the Private class.\n>\n> Move all remaining private members to the Private class to address the\n> first issue, and add a Private::metadata() function to address the\n> second problem.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Jacopo Mondi \n\nReviewed-by: Umang Jain \n\n> ---\n> Changes since v1:\n>\n> - Add commit message\n> - Fix compilation issue\n> ---\n> include/libcamera/framebuffer.h | 19 ++----\n> include/libcamera/internal/framebuffer.h | 10 +++-\n> .../mm/cros_frame_buffer_allocator.cpp | 8 +--\n> .../mm/generic_frame_buffer_allocator.cpp | 9 +--\n> src/libcamera/framebuffer.cpp | 58 ++++++++++++-------\n> src/libcamera/v4l2_videodevice.cpp | 20 ++++---\n> 6 files changed, 71 insertions(+), 53 deletions(-)\n>\n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 36b91d11ed79..695539993f96 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -59,28 +59,19 @@ public:\n> \t};\n> \n> \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> -\tFrameBuffer(std::unique_ptr d,\n> -\t\t const std::vector &planes, unsigned int cookie = 0);\n> +\tFrameBuffer(std::unique_ptr d);\n> \n> -\tconst std::vector &planes() const { return planes_; }\n> +\tconst std::vector &planes() const;\n> \tRequest *request() const;\n> -\tconst FrameMetadata &metadata() const { return metadata_; }\n> +\tconst FrameMetadata &metadata() const;\n> \n> -\tuint64_t cookie() const { return cookie_; }\n> -\tvoid setCookie(uint64_t cookie) { cookie_ = cookie; }\n> +\tuint64_t cookie() const;\n> +\tvoid setCookie(uint64_t cookie);\n> \n> \tstd::unique_ptr releaseFence();\n> \n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> -\n> -\tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> -\n> -\tstd::vector planes_;\n> -\n> -\tFrameMetadata metadata_;\n> -\n> -\tuint64_t cookie_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index 8a9cc98e22e3..1f42a4fcc865 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -22,7 +22,7 @@ class FrameBuffer::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> \n> public:\n> -\tPrivate();\n> +\tPrivate(const std::vector &planes, uint64_t cookie = 0);\n> \tvirtual ~Private();\n> \n> \tvoid setRequest(Request *request) { request_ = request; }\n> @@ -31,9 +31,15 @@ public:\n> \tFence *fence() const { return fence_.get(); }\n> \tvoid setFence(std::unique_ptr fence) { fence_ = std::move(fence); }\n> \n> -\tvoid cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; }\n> +\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> +\n> +\tFrameMetadata &metadata() { return metadata_; }\n> \n> private:\n> +\tstd::vector planes_;\n> +\tFrameMetadata metadata_;\n> +\tuint64_t cookie_;\n> +\n> \tstd::unique_ptr fence_;\n> \tRequest *request_;\n> \tbool isContiguous_;\n> diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\n> index 52e8c180e3db..1e6f1ef55434 100644\n> --- a/src/android/mm/cros_frame_buffer_allocator.cpp\n> +++ b/src/android/mm/cros_frame_buffer_allocator.cpp\n> @@ -28,8 +28,9 @@ class CrosFrameBufferData : public FrameBuffer::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> \n> public:\n> -\tCrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)\n> -\t\t: FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))\n> +\tCrosFrameBufferData(cros::ScopedBufferHandle scopedHandle,\n> +\t\t\t const std::vector &planes)\n> +\t\t: FrameBuffer::Private(planes), scopedHandle_(std::move(scopedHandle))\n> \t{\n> \t}\n> \n> @@ -81,8 +82,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,\n> \t}\n> \n> \treturn std::make_unique(\n> -\t\tstd::make_unique(std::move(scopedHandle)),\n> -\t\tplanes);\n> +\t\tstd::make_unique(std::move(scopedHandle), planes));\n> }\n> \n> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\n> index acb2fa2b699d..956623df1f80 100644\n> --- a/src/android/mm/generic_frame_buffer_allocator.cpp\n> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp\n> @@ -32,8 +32,10 @@ class GenericFrameBufferData : public FrameBuffer::Private\n> \n> public:\n> \tGenericFrameBufferData(struct alloc_device_t *allocDevice,\n> -\t\t\t buffer_handle_t handle)\n> -\t\t: allocDevice_(allocDevice), handle_(handle)\n> +\t\t\t buffer_handle_t handle,\n> +\t\t\t const std::vector &planes)\n> +\t\t: FrameBuffer::Private(planes), allocDevice_(allocDevice),\n> +\t\t handle_(handle)\n> \t{\n> \t\tASSERT(allocDevice_);\n> \t\tASSERT(handle_);\n> @@ -136,8 +138,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,\n> \t}\n> \n> \treturn std::make_unique(\n> -\t\tstd::make_unique(allocDevice_, handle),\n> -\t\tplanes);\n> +\t\tstd::make_unique(allocDevice_, handle, planes));\n> }\n> \n> PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 7be18560417c..5a7f3c0b5f9a 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -114,9 +114,16 @@ LOG_DEFINE_CATEGORY(Buffer)\n> * pipeline handlers.\n> */\n> \n> -FrameBuffer::Private::Private()\n> -\t: request_(nullptr), isContiguous_(true)\n> +/**\n> + * \\brief Construct a FrameBuffer::Private instance\n> + * \\param[in] planes The frame memory planes\n> + * \\param[in] cookie Cookie\n> + */\n> +FrameBuffer::Private::Private(const std::vector &planes, uint64_t cookie)\n> +\t: planes_(planes), cookie_(cookie), request_(nullptr),\n> +\t isContiguous_(true)\n> {\n> +\tmetadata_.planes_.resize(planes_.size());\n> }\n> \n> /**\n> @@ -194,6 +201,12 @@ FrameBuffer::Private::~Private()\n> * indicate that the metadata is invalid.\n> */\n> \n> +/**\n> + * \\fn FrameBuffer::Private::metadata()\n> + * \\brief Retrieve the dynamic metadata\n> + * \\return Dynamic metadata for the frame contained in the buffer\n> + */\n> +\n> /**\n> * \\class FrameBuffer\n> * \\brief Frame buffer data and its associated dynamic metadata\n> @@ -291,29 +304,22 @@ ino_t fileDescriptorInode(const SharedFD &fd)\n> * \\param[in] cookie Cookie\n> */\n> FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> -\t: FrameBuffer(std::make_unique(), planes, cookie)\n> +\t: FrameBuffer(std::make_unique(planes, cookie))\n> {\n> }\n> \n> /**\n> - * \\brief Construct a FrameBuffer with an extensible private class and an array\n> - * of planes\n> + * \\brief Construct a FrameBuffer with an extensible private class\n> * \\param[in] d The extensible private class\n> - * \\param[in] planes The frame memory planes\n> - * \\param[in] cookie Cookie\n> */\n> -FrameBuffer::FrameBuffer(std::unique_ptr d,\n> -\t\t\t const std::vector &planes,\n> -\t\t\t unsigned int cookie)\n> -\t: Extensible(std::move(d)), planes_(planes), cookie_(cookie)\n> +FrameBuffer::FrameBuffer(std::unique_ptr d)\n> +\t: Extensible(std::move(d))\n> {\n> -\tmetadata_.planes_.resize(planes_.size());\n> -\n> \tunsigned int offset = 0;\n> \tbool isContiguous = true;\n> \tino_t inode = 0;\n> \n> -\tfor (const auto &plane : planes_) {\n> +\tfor (const auto &plane : _d()->planes_) {\n> \t\tASSERT(plane.offset != Plane::kInvalidOffset);\n> \n> \t\tif (plane.offset != offset) {\n> @@ -325,9 +331,9 @@ FrameBuffer::FrameBuffer(std::unique_ptr d,\n> \t\t * Two different dmabuf file descriptors may still refer to the\n> \t\t * same dmabuf instance. Check this using inodes.\n> \t\t */\n> -\t\tif (plane.fd != planes_[0].fd) {\n> +\t\tif (plane.fd != _d()->planes_[0].fd) {\n> \t\t\tif (!inode)\n> -\t\t\t\tinode = fileDescriptorInode(planes_[0].fd);\n> +\t\t\t\tinode = fileDescriptorInode(_d()->planes_[0].fd);\n> \t\t\tif (fileDescriptorInode(plane.fd) != inode) {\n> \t\t\t\tisContiguous = false;\n> \t\t\t\tbreak;\n> @@ -344,10 +350,13 @@ FrameBuffer::FrameBuffer(std::unique_ptr d,\n> }\n> \n> /**\n> - * \\fn FrameBuffer::planes()\n> * \\brief Retrieve the static plane descriptors\n> * \\return Array of plane descriptors\n> */\n> +const std::vector &FrameBuffer::planes() const\n> +{\n> +\treturn _d()->planes_;\n> +}\n> \n> /**\n> * \\brief Retrieve the request this buffer belongs to\n> @@ -368,13 +377,15 @@ Request *FrameBuffer::request() const\n> }\n> \n> /**\n> - * \\fn FrameBuffer::metadata()\n> * \\brief Retrieve the dynamic metadata\n> * \\return Dynamic metadata for the frame contained in the buffer\n> */\n> +const FrameMetadata &FrameBuffer::metadata() const\n> +{\n> +\treturn _d()->metadata_;\n> +}\n> \n> /**\n> - * \\fn FrameBuffer::cookie()\n> * \\brief Retrieve the cookie\n> *\n> * The cookie belongs to the creator of the FrameBuffer, which controls its\n> @@ -384,9 +395,12 @@ Request *FrameBuffer::request() const\n> *\n> * \\return The cookie\n> */\n> +uint64_t FrameBuffer::cookie() const\n> +{\n> +\treturn _d()->cookie_;\n> +}\n> \n> /**\n> - * \\fn FrameBuffer::setCookie()\n> * \\brief Set the cookie\n> * \\param[in] cookie Cookie to set\n> *\n> @@ -395,6 +409,10 @@ Request *FrameBuffer::request() const\n> * modify the cookie value of buffers they haven't created themselves. The\n> * libcamera core never modifies the buffer cookie.\n> */\n> +void FrameBuffer::setCookie(uint64_t cookie)\n> +{\n> +\t_d()->cookie_ = cookie;\n> +}\n> \n> /**\n> * \\brief Extract the Fence associated with this Framebuffer\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 955e150867ef..4d846f6be7fa 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1791,12 +1791,14 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\twatchdog_.start(std::chrono::duration_cast(watchdogDuration_));\n> \t}\n> \n> -\tbuffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> -\t\t\t\t ? FrameMetadata::FrameError\n> -\t\t\t\t : FrameMetadata::FrameSuccess;\n> -\tbuffer->metadata_.sequence = buf.sequence;\n> -\tbuffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> -\t\t\t\t + buf.timestamp.tv_usec * 1000ULL;\n> +\tFrameMetadata &metadata = buffer->_d()->metadata();\n> +\n> +\tmetadata.status = buf.flags & V4L2_BUF_FLAG_ERROR\n> +\t\t\t? FrameMetadata::FrameError\n> +\t\t\t: FrameMetadata::FrameSuccess;\n> +\tmetadata.sequence = buf.sequence;\n> +\tmetadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> +\t\t\t + buf.timestamp.tv_usec * 1000ULL;\n> \n> \tif (V4L2_TYPE_IS_OUTPUT(buf.type))\n> \t\treturn buffer;\n> @@ -1812,10 +1814,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\t\t\t<< buf.sequence << \")\";\n> \t\tfirstFrame_ = buf.sequence;\n> \t}\n> -\tbuffer->metadata_.sequence -= firstFrame_.value();\n> +\tmetadata.sequence -= firstFrame_.value();\n> \n> \tunsigned int numV4l2Planes = multiPlanar ? buf.length : 1;\n> -\tFrameMetadata &metadata = buffer->metadata_;\n> \n> \tif (numV4l2Planes != buffer->planes().size()) {\n> \t\t/*\n> @@ -1941,9 +1942,10 @@ int V4L2VideoDevice::streamOff()\n> \t/* Send back all queued buffers. */\n> \tfor (auto it : queuedBuffers_) {\n> \t\tFrameBuffer *buffer = it.second;\n> +\t\tFrameMetadata &metadata = buffer->_d()->metadata();\n> \n> \t\tcache_->put(it.first);\n> -\t\tbuffer->metadata_.status = FrameMetadata::FrameCancelled;\n> +\t\tmetadata.status = FrameMetadata::FrameCancelled;\n> \t\tbufferReady.emit(buffer);\n> \t}\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 1BB7EBD16B\n\tfor ;\n\tWed, 5 Oct 2022 06:21:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 710A961F7B;\n\tWed, 5 Oct 2022 08:21:46 +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 C07D1603F3\n\tfor ;\n\tWed, 5 Oct 2022 08:21:44 +0200 (CEST)","from [192.168.1.103] (unknown [103.251.226.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1946E415;\n\tWed, 5 Oct 2022 08:21:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664950906;\n\tbh=nZKeAQkoNbnuC6UqXhr3WRy4GNJcMXips+VwrZWwsQQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=v4AozUnv7Da5THTwUkTqvb8+Es6kaXQOMEBYagJDCXMv8nMjbEA4tVwnO+wowT9+9\n\ts39LmANjknJMusES71LWllElWN4MviSoAfT0mwVUqDGzif7vkkILLHeuAcfB45MBXD\n\tWmXGsK0H171R/PCYQpglxvwAZN2ox1Mkrb9Uen8CGvYO/pePL9MOJKCmaOaYQ+7rJM\n\tVCnBrfVPtOia3J1nXa0pdSf1Djk7hLfLi6CJBE2yMi/+ijg3t3y5vjsGVKknEolowd\n\tt+e3BTcUKc7CUf0AhGpfEFKaQfZtG4e0XlyEqFa0ytjDNZgSA0EJW1O1PiYuCrb+Cv\n\t+klnXC/yrH+gQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664950904;\n\tbh=nZKeAQkoNbnuC6UqXhr3WRy4GNJcMXips+VwrZWwsQQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=oJb22hNl2jvGcvAiNaZQpa3Wmt0k9XUtltTVCHommBUEG6cb2UxsOKF+i6LJIfY26\n\tmVuem+Pf5riPbh0yrbwj6W6DUGyU2tkAoS0Wk3pBr2k5Xdx1/ELqEBtjn9OLDGg3Mr\n\tJO8JZKyg1hx8ykAwVkXqO2FHpAvrd3xbgWkk5z+w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oJb22hNl\"; dkim-atps=neutral","Message-ID":"<79b04665-fa23-c9ff-38c0-c27ef0aa0e09@ideasonboard.com>","Date":"Wed, 5 Oct 2022 11:51:36 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221004222903.6393-1-laurent.pinchart@ideasonboard.com>\n\t<20221004222903.6393-2-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20221004222903.6393-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] libcamera: framebuffer: Move\n\tremaining private data to Private class","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]