{"id":17524,"url":"https://patchwork.libcamera.org/api/patches/17524/?format=json","web_url":"https://patchwork.libcamera.org/patch/17524/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20221004222903.6393-2-laurent.pinchart@ideasonboard.com>","date":"2022-10-04T22:29:00","name":"[libcamera-devel,v2,1/4] libcamera: framebuffer: Move remaining private data to Private class","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"b83db8e64fee7c97fd0f198fab96585b5a21ff1b","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/?format=json","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/17524/mbox/","series":[{"id":3531,"url":"https://patchwork.libcamera.org/api/series/3531/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=3531","date":"2022-10-04T22:28:59","name":"libcamera: Fix kernel deprecation warning with output buffers","version":2,"mbox":"https://patchwork.libcamera.org/series/3531/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/17524/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/17524/checks/","tags":{},"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 BE008C327C\n\tfor ;\n\tTue, 4 Oct 2022 22:29:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 78FA660ACB;\n\tWed, 5 Oct 2022 00:29:10 +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 9F93360AA5\n\tfor ;\n\tWed, 5 Oct 2022 00:29:08 +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 2A399997;\n\tWed, 5 Oct 2022 00:29:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664922550;\n\tbh=8e63qUQWXCtxp8pTQYx+QAgcCAOXD4p/LIiz/Ofkt9g=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=m3ouXR/h8baHIIenGeWPdfo/HuUi6zj9sogIK12AoeoqL1MU/Cf1hqwvbjEFywl/D\n\tfP5b1y+QCivfY813XwPfZ+9aGX7n6tRHoji4K7GbXq6V6U1/AFq6g8yZ5ssnLDzz4S\n\tas1c8vgh7FFjvZeXarHvi1YtE8vKYYfZ6HBmnLvKzunAhLxlTfgafAt17L8s0cLld4\n\tV9ZbDF31irQqsrg8KLeAUFQWZUNUhVDYlXId1GRYwI07lbAbUiMqAxiBmiNrDQAibo\n\tI1QLv2sAREdB4ewVGW+AOEvIuX5cNrVmnP73t2/kT67YGWl7N/n+N7Hgy/uH5hN+QN\n\tRzyJ1Xph9Tu6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664922548;\n\tbh=8e63qUQWXCtxp8pTQYx+QAgcCAOXD4p/LIiz/Ofkt9g=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=s70CWPKY/w/fBw+NH7M8On//X8QlXdI1OEmdUK/XFzjzHx2cmB6FG6tziL5ZJjvu7\n\tQKRLREdPyTjdIzxNwFq7OIIE0q0eb0/RF7N9gxfTYFD3foX52aIetjkFawUhuXgiJ5\n\tRq08cORqB7FU9vNyt+QkvFCqyX8UIvCNv09T/RR4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"s70CWPKY\"; dkim-atps=neutral","To":"libcamera-devel@lists.libcamera.org","Date":"Wed, 5 Oct 2022 01:29:00 +0300","Message-Id":"<20221004222903.6393-2-laurent.pinchart@ideasonboard.com>","X-Mailer":"git-send-email 2.35.1","In-Reply-To":"<20221004222903.6393-1-laurent.pinchart@ideasonboard.com>","References":"<20221004222903.6393-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"[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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"content":"Private members of the FrameBuffer class are split between FrameBuffer\nand FrameBuffer::Private. There was no real justification for this\nsplit, and keeping some members private in the FrameBuffer class causes\nmultiple 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\nMove all remaining private members to the Private class to address the\nfirst issue, and add a Private::metadata() function to address the\nsecond problem.\n\nSigned-off-by: Laurent Pinchart \nReviewed-by: Jacopo Mondi \n---\nChanges 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(-)","diff":"diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\nindex 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 */\ndiff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\nindex 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_;\ndiff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp\nindex 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\ndiff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp\nindex 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\ndiff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\nindex 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\ndiff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\nindex 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 \n","prefixes":["libcamera-devel","v2","1/4"]}