[{"id":25224,"web_url":"https://patchwork.libcamera.org/comment/25224/","msgid":"<20221003083235.l6eqr53vpzvpcejh@uno.localdomain>","date":"2022-10-03T08:32:35","subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: framebuffer: Move\n\tremaining private data to Private class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Sun, Oct 02, 2022 at 03:36:10AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Signed-off-by: Laurent Pinchart \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..8ff4241f1506 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 1dbb839ed456..d25a785adeea 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1793,12 +1793,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\nA very minor nit: in queueBuffer() we use the public FrameBuffer::metadata()\nfunction. Might be nice to use the same pattern\n\nThat apart\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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> @@ -1814,10 +1816,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> @@ -1943,9 +1944,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> --\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 D7868BD16B\n\tfor ;\n\tMon, 3 Oct 2022 08:32:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C299062CD4;\n\tMon, 3 Oct 2022 10:32:39 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4946961F74\n\tfor ;\n\tMon, 3 Oct 2022 10:32:38 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7E5D5240011;\n\tMon, 3 Oct 2022 08:32:37 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664785959;\n\tbh=1xa4Iek3W8NBKlE/bNLnPzyysAgwDSspskeg+mgdKlM=;\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:Cc:\n\tFrom;\n\tb=S6v5+dMZh3JvjNjZG0UskWQnVbevRS+YPe6MjVzw5P2DRjV15DvBR1m/o0+xuhsFC\n\trCRLxrDASkRz6WGd1J7sM4pd3puC22WAs6OGVUMIXNBRYSby0RDXcJeL5+Q5CWL0O4\n\tVjmYqdmBv/iq/doYKqiKWP5E6fCjrAYuJbhB9AkwPJnj92EwLG8Q73LVHpx29segsH\n\t2ZjKdGyPxDF6myWyYG83GVjr9pqupdRZz6LITP+0YmXJ0EfuqAZ7EF+o4fo67Quc/U\n\t0Cbp1PUBEn6XkKzOuxU6FpTS6HwQhmT70Zp6E/8TQcQ6ZSEamMRC2kIDNoONdgBtrM\n\tO5V/GM2ZitMpg==","Date":"Mon, 3 Oct 2022 10:32:35 +0200","To":"Laurent Pinchart ","Message-ID":"<20221003083235.l6eqr53vpzvpcejh@uno.localdomain>","References":"<20221002003612.13603-1-laurent.pinchart@ideasonboard.com>\n\t<20221002003612.13603-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221002003612.13603-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25226,"web_url":"https://patchwork.libcamera.org/comment/25226/","msgid":"","date":"2022-10-03T08:38:10","subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: framebuffer: Move\n\tremaining private data to Private class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 03, 2022 at 10:32:35AM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Sun, Oct 02, 2022 at 03:36:10AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > Signed-off-by: Laurent Pinchart \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..8ff4241f1506 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 1dbb839ed456..d25a785adeea 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1793,12 +1793,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> A very minor nit: in queueBuffer() we use the public FrameBuffer::metadata()\n> function. Might be nice to use the same pattern\n\nFrameBuffer::metadata() returns a const reference.\n\n> That apart\n> Reviewed-by: Jacopo Mondi \n> \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> > @@ -1814,10 +1816,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> > @@ -1943,9 +1944,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 BEE46C0DA4\n\tfor ;\n\tMon, 3 Oct 2022 08:38:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 315E362CD5;\n\tMon, 3 Oct 2022 10:38:14 +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 0A9CA61F74\n\tfor ;\n\tMon, 3 Oct 2022 10:38:13 +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 71120FFD;\n\tMon, 3 Oct 2022 10:38:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664786294;\n\tbh=3lCtMNW36LC8fxK6yp9r6xbwyReDXU5xGwMKq9fnJRo=;\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:Cc:\n\tFrom;\n\tb=cXTzRQ7QwRkrsxmAf3iX2M/e2YrVQskgZmCA0OUBrA3vuTLw1WqbgY8Ut8CRnuOv4\n\tmfE31cFYIs1n7wUPD1tx9+z7d5pDqbjmbie5Q9Annc474hb0AMLjAA6LokDROqqALj\n\thsf3jaXClu8mTU1EdfDEk68ITynJDwQdYOf/W7dROd97fSXXHP2rCX54R0kbx4lKH8\n\twXzF/FL86d6UNRkOAvYxXpWGewotgIn4OwUSklKftKBPJStBNPFDfSLQTlpevqHSQw\n\tqfLUvZ+NGxU+vlzg2CniH6mDrVR2pz3M4te4ZMl6N8qTCDJ1N+lG2Z3B/exZ/6mF17\n\tvllJtJKZTfeGw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664786292;\n\tbh=3lCtMNW36LC8fxK6yp9r6xbwyReDXU5xGwMKq9fnJRo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F7ASBnpdDs7wzO9eU9Up/doEz/MClu0nMH3THG7kpPAoCZDNzTrsorkJSgSvrXhXK\n\tRDvssvdBcaloMdrVBw/8ecNnb3lq5QnQDmmAcYJ9IytEXrq49O9uqg+5exxS1KJNiK\n\tJ5TbL9jzupl1kTFn1kTBgnJQFcJOUCvogQ2ZDM+o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"F7ASBnpd\"; dkim-atps=neutral","Date":"Mon, 3 Oct 2022 11:38:10 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221002003612.13603-1-laurent.pinchart@ideasonboard.com>\n\t<20221002003612.13603-3-laurent.pinchart@ideasonboard.com>\n\t<20221003083235.l6eqr53vpzvpcejh@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221003083235.l6eqr53vpzvpcejh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/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 ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]