[{"id":18585,"web_url":"https://patchwork.libcamera.org/comment/18585/","msgid":"<1866183b-8399-cf51-95c5-58297c632add@ideasonboard.com>","date":"2021-08-06T09:48:21","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat class","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/08/2021 19:22, Laurent Pinchart wrote:\n> The FrameFormat class describes how a frame is stored in memory. It\n> groups a pixel format, size in pixels, and per-plane stride and size.\n> \n> This new class will be used in the configuration API rework, but could\n> already be put in use if desired to replace the various data structures\n> used to carry the same information.\n\nI think that second paragraph can go under the ---\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> \n> I've developed this as part of the configuration API rework, and\n> realized it could already be useful, in particular if we want to start\n> cleaning up support for multi-planar formats. I'm thus sending it as an\n> RFC.\n\nThis looks like a good idea. I was considering what information such as\npixel-format we might have to add to FrameBuffer to convey the\ninformation required, but we could equally keep it separate.\n\n> \n> ---\n> include/libcamera/format.h | 43 ++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/format.cpp | 142 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 187 insertions(+)\n> create mode 100644 include/libcamera/format.h\n> create mode 100644 src/libcamera/format.cpp\n> \n> diff --git a/include/libcamera/format.h b/include/libcamera/format.h\n> new file mode 100644\n> index 000000000000..4982ab8230dc\n> --- /dev/null\n> +++ b/include/libcamera/format.h\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * format.h - Frame Format\n\nShould this be named frame_format.h?\n\nOtherwise it might be confused the Stream format or others?\n\n> + */\n> +#ifndef __LIBCAMERA_FORMAT_H__\n> +#define __LIBCAMERA_FORMAT_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class FrameFormat\n> +{\n> +public:\n> +\tstatic constexpr unsigned int kMaxPlanes = 4;\n> +\n> +\tstruct Plane {\n> +\t\tstd::size_t stride;\n> +\t\tstd::size_t size;\n> +\t};\n> +\n> +\tFrameFormat();\n> +\tFrameFormat(PixelFormat format, const Size &size);\n> +\n> +\tstd::size_t stride() const;\n> +\tstd::size_t frameSize() const;\n> +\tstd::string toString() const;\n> +\n> +\tPixelFormat format_;\n> +\tSize size_;\n> +\tstd::array planes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_FORMAT_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 5b25ef847ed4..dcfadb381874 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -6,6 +6,7 @@ libcamera_public_headers = files([\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> + 'format.h',\n> 'framebuffer.h',\n> 'framebuffer_allocator.h',\n> 'geometry.h',\n> diff --git a/src/libcamera/format.cpp b/src/libcamera/format.cpp\n> new file mode 100644\n> index 000000000000..f77413781de1\n> --- /dev/null\n> +++ b/src/libcamera/format.cpp\n> @@ -0,0 +1,142 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * format.cpp - Frame Format\n> + */\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file libcamera/format.h\n> + * \\brief Frame Format\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class FrameFormat\n> + * \\brief Format of a frame as stored in memory\n> + *\n> + * The FrameFormat class fully describes how a frame is stored in memory. It\n> + * combines a pixel format, a size in pixels, and memory layout information for\n> + * planes.\n> + *\n> + * Frames are stored in memory in one or multiple planes, as specified by the\n> + * pixel format. A plane is a two-dimensional memory area organized as lines of\n> + * samples. A sample typically stores one or multiple components of a pixel (for\n> + * instance, the NV12 format uses two planes, with the first plane storing only\n> + * the luminance Y component of the pixels, and the second plane storing both\n> + * the chroma Cb and Cr components).\n> + *\n> + * Planes cover the whole frame. Their number of lines and sample per lines\n> + * correspond to the frame width and height in pixels, possibly divided by\n> + * horizontal and vertical subsampling factors as specified by the pixel\n> + * format.\n> + *\n> + * A plane may contain padding at the end of line, to support requirements of\n> + * applications and cameras, such as alignment constraints. This is specified\n> + * as a line stride value, equal to the distance in bytes between the first\n> + * sample of successive lines in memory, including pixel data and padding. All\n> + * lines in a plane have the same stride value, including the last line.\n> + * Additionally, padding may also occur at the end of the plane, specified as\n> + * the total plane size in bytes.\n> + *\n> + * \\var FrameFormat::kMaxPlanes\n> + * \\brief Maximum number of data planes for a frame\n> + *\n> + * \\var FrameFormat::format_\n> + * \\brief Frame pixel format, defines how pixel data is packed and stored in\n> + * memory\n> + *\n> + * \\var FrameFormat::size_\n> + * \\brief Frame size, defines the horizontal and vertical dimensions of the\n> + * frame in pixels (excluding any padding)\n> + *\n> + * \\var FrameFormat::planes_\n> + * \\brief Frame data planes, defines the memory layout for each data plane of\n> + * the frame\n> + *\n> + * Unused entries in the planes array shall be zero-initialized.\n> + */\n> +\n> +/**\n> + * \\class FrameFormat::Plane\n> + * \\brief Memory layout configuration for one data plane of a frame\n> + *\n> + * \\var FrameFormat::Plane::stride\n> + * \\brief Distance in bytes between the beginning of two successive lines\n> + *\n> + * If the strides of different planes can't be set separately, the stride of the\n> + * first plane takes precedence and is used by the camera.\n> + *\n> + * \\var FrameFormat::Plane::size\n> + * \\brief Maximum number of bytes required to store the plane\n> + *\n> + * The \\a size reports the number of bytes required to store data for plane.\n> + * For uncompressed formats, it takes into account the plane's stride, the\n> + * frame height, the plane's vertical subsampling and the overall alignment\n> + * constraints of the camera (for instance many cameras require buffer sizes\n> + * to be a multiple of the page size). For compressed formats, it indicates the\n> + * maximum size of the compressed data.\n> + */\n> +\n> +/**\n> + * \\brief Construct a zero-initialized FrameFormat\n> + */\n> +FrameFormat::FrameFormat() = default;\n> +\n> +/**\n> + * \\brief Construct a FrameFormat\n> + * \\param[in] format The pixel format\n> + * \\param[in] size The frame size in pixels\n> + */\n> +FrameFormat::FrameFormat(PixelFormat format, const Size &size)\n> +\t: format_(format), size_(size)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the frame stride in bytes\n> + *\n> + * The stride() function is a convenience helper to retrieve the stride of the\n> + * first plane. For multi-planar formats that have different stride values for\n> + * each plane, applications shall access the stride from the \\ref planes_\n> + * instead.\n> + *\n> + * \\return The frame stride in bytes\n> + */\n> +std::size_t FrameFormat::stride() const\n> +{\n> +\treturn planes_[0].stride;\n> +}\n> +\n\nWhat about having the plane as a parameter, defaulting to 0.\n\nThen the access to reading the strides can be done through a const\naccessor rather than digging into the planes_ directly.\n\nWith a similar accessor for size - could the Plane could be made private\nand populated during the constructor only?\n\nWith those considered:\n\nReviewed-by: Kieran Bingham \n\n\n> +/**\n> + * \\brief Retrieve the full frame size\n> + *\n> + * The frameSize() function is a convenience helper to retrieve the full frame\n> + * size, defined as the sum of the sizes of all planes.\n> + *\n> + * \\return The full frame size, in bytes\n> + */\n> +std::size_t FrameFormat::frameSize() const\n> +{\n> +\treturn std::accumulate(planes_.begin(), planes_.end(), 0,\n> +\t\t\t [](std::size_t a, const Plane &b) {\n> +\t\t\t\t return a + b.size;\n> +\t\t\t });\n> +}\n> +\n> +/**\n> + * \\brief Assemble and return a string describing the frame format\n> + *\n> + * \\return A string describing the FrameFormat\n> + */\n> +std::string FrameFormat::toString() const\n> +{\n> +\treturn size_.toString() + \"-\" + format_.toString();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 4f08580157f9..7cf17ec2c628 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -15,6 +15,7 @@ libcamera_sources = files([\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'file_descriptor.cpp',\n> + 'format.cpp',\n> 'formats.cpp',\n> 'framebuffer.cpp',\n> 'framebuffer_allocator.cpp',\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 DBDA9C3235\n\tfor ;\n\tFri, 6 Aug 2021 09:48:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43ACB6881B;\n\tFri, 6 Aug 2021 11:48:26 +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 8750C687D0\n\tfor ;\n\tFri, 6 Aug 2021 11:48:24 +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 0B73F4FB;\n\tFri, 6 Aug 2021 11:48:24 +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=\"B3VFYMUN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628243304;\n\tbh=p+u8Vw792FIqsiI0ba1NkFmh3O8wHMuDXTRa3pFd2NU=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=B3VFYMUNeMoldaAGrgeckN1xMJaH4qb6vW3cyCkzgEtmn1X+7YondEBljBKiPVDad\n\t4a2J/X0MXyBFWyXxZgHbuA9N5YYL2QexrKhndSiH51N4oXpR3HkqdoNpSap0salw+j\n\tJwczQ0h2RY7KXYHumjcXtJv9RVCNJkzZ5KyJQtmc=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210805182203.1297-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<1866183b-8399-cf51-95c5-58297c632add@ideasonboard.com>","Date":"Fri, 6 Aug 2021 10:48:21 +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":"<20210805182203.1297-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18653,"web_url":"https://patchwork.libcamera.org/comment/18653/","msgid":"","date":"2021-08-10T03:10:17","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat class","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Fri, Aug 6, 2021 at 6:48 PM Kieran Bingham\n wrote:\n>\n> Hi Laurent,\n>\n> On 05/08/2021 19:22, Laurent Pinchart wrote:\n> > The FrameFormat class describes how a frame is stored in memory. It\n> > groups a pixel format, size in pixels, and per-plane stride and size.\n> >\n> > This new class will be used in the configuration API rework, but could\n> > already be put in use if desired to replace the various data structures\n> > used to carry the same information.\n>\n> I think that second paragraph can go under the ---\n>\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> >\n> > I've developed this as part of the configuration API rework, and\n> > realized it could already be useful, in particular if we want to start\n> > cleaning up support for multi-planar formats. I'm thus sending it as an\n> > RFC.\n>\n> This looks like a good idea. I was considering what information such as\n> pixel-format we might have to add to FrameBuffer to convey the\n> information required, but we could equally keep it separate.\n>\n> >\n> > ---\n> > include/libcamera/format.h | 43 ++++++++++\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/format.cpp | 142 ++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 187 insertions(+)\n> > create mode 100644 include/libcamera/format.h\n> > create mode 100644 src/libcamera/format.cpp\n> >\n> > diff --git a/include/libcamera/format.h b/include/libcamera/format.h\n> > new file mode 100644\n> > index 000000000000..4982ab8230dc\n> > --- /dev/null\n> > +++ b/include/libcamera/format.h\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * format.h - Frame Format\n>\n> Should this be named frame_format.h?\n>\n> Otherwise it might be confused the Stream format or others?\n>\n> > + */\n> > +#ifndef __LIBCAMERA_FORMAT_H__\n> > +#define __LIBCAMERA_FORMAT_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class FrameFormat\n> > +{\n> > +public:\n> > + static constexpr unsigned int kMaxPlanes = 4;\n> > +\n> > + struct Plane {\n> > + std::size_t stride;\n> > + std::size_t size;\n> > + };\n> > +\n> > + FrameFormat();\n> > + FrameFormat(PixelFormat format, const Size &size);\n> > +\n> > + std::size_t stride() const;\n> > + std::size_t frameSize() const;\n> > + std::string toString() const;\n> > +\n> > + PixelFormat format_;\n> > + Size size_;\n> > + std::array planes_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_FORMAT_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 5b25ef847ed4..dcfadb381874 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -6,6 +6,7 @@ libcamera_public_headers = files([\n> > 'compiler.h',\n> > 'controls.h',\n> > 'file_descriptor.h',\n> > + 'format.h',\n> > 'framebuffer.h',\n> > 'framebuffer_allocator.h',\n> > 'geometry.h',\n> > diff --git a/src/libcamera/format.cpp b/src/libcamera/format.cpp\n> > new file mode 100644\n> > index 000000000000..f77413781de1\n> > --- /dev/null\n> > +++ b/src/libcamera/format.cpp\n> > @@ -0,0 +1,142 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * format.cpp - Frame Format\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file libcamera/format.h\n> > + * \\brief Frame Format\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class FrameFormat\n> > + * \\brief Format of a frame as stored in memory\n> > + *\n> > + * The FrameFormat class fully describes how a frame is stored in memory. It\n> > + * combines a pixel format, a size in pixels, and memory layout information for\n> > + * planes.\n> > + *\n> > + * Frames are stored in memory in one or multiple planes, as specified by the\n> > + * pixel format. A plane is a two-dimensional memory area organized as lines of\n> > + * samples. A sample typically stores one or multiple components of a pixel (for\n> > + * instance, the NV12 format uses two planes, with the first plane storing only\n> > + * the luminance Y component of the pixels, and the second plane storing both\n> > + * the chroma Cb and Cr components).\n> > + *\n> > + * Planes cover the whole frame. Their number of lines and sample per lines\n> > + * correspond to the frame width and height in pixels, possibly divided by\n> > + * horizontal and vertical subsampling factors as specified by the pixel\n> > + * format.\n> > + *\n> > + * A plane may contain padding at the end of line, to support requirements of\n> > + * applications and cameras, such as alignment constraints. This is specified\n> > + * as a line stride value, equal to the distance in bytes between the first\n> > + * sample of successive lines in memory, including pixel data and padding. All\n> > + * lines in a plane have the same stride value, including the last line.\n> > + * Additionally, padding may also occur at the end of the plane, specified as\n> > + * the total plane size in bytes.\n> > + *\n> > + * \\var FrameFormat::kMaxPlanes\n> > + * \\brief Maximum number of data planes for a frame\n> > + *\n> > + * \\var FrameFormat::format_\n> > + * \\brief Frame pixel format, defines how pixel data is packed and stored in\n> > + * memory\n> > + *\n> > + * \\var FrameFormat::size_\n> > + * \\brief Frame size, defines the horizontal and vertical dimensions of the\n> > + * frame in pixels (excluding any padding)\n> > + *\n> > + * \\var FrameFormat::planes_\n> > + * \\brief Frame data planes, defines the memory layout for each data plane of\n> > + * the frame\n> > + *\n> > + * Unused entries in the planes array shall be zero-initialized.\n> > + */\n> > +\n> > +/**\n> > + * \\class FrameFormat::Plane\n> > + * \\brief Memory layout configuration for one data plane of a frame\n> > + *\n> > + * \\var FrameFormat::Plane::stride\n> > + * \\brief Distance in bytes between the beginning of two successive lines\n> > + *\n> > + * If the strides of different planes can't be set separately, the stride of the\n> > + * first plane takes precedence and is used by the camera.\n> > + *\n> > + * \\var FrameFormat::Plane::size\n> > + * \\brief Maximum number of bytes required to store the plane\n> > + *\n\nReviewed-by: Hirokazu Honda \n\nIf there is some extra data between planes. Does the size cover the\nextra data size?\nThat is, plene[0].size is possibly not the offset of plane[1]?\n\n-Hiro\n> > + * The \\a size reports the number of bytes required to store data for plane.\n> > + * For uncompressed formats, it takes into account the plane's stride, the\n> > + * frame height, the plane's vertical subsampling and the overall alignment\n> > + * constraints of the camera (for instance many cameras require buffer sizes\n> > + * to be a multiple of the page size). For compressed formats, it indicates the\n> > + * maximum size of the compressed data.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a zero-initialized FrameFormat\n> > + */\n> > +FrameFormat::FrameFormat() = default;\n> > +\n> > +/**\n> > + * \\brief Construct a FrameFormat\n> > + * \\param[in] format The pixel format\n> > + * \\param[in] size The frame size in pixels\n> > + */\n> > +FrameFormat::FrameFormat(PixelFormat format, const Size &size)\n> > + : format_(format), size_(size)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the frame stride in bytes\n> > + *\n> > + * The stride() function is a convenience helper to retrieve the stride of the\n> > + * first plane. For multi-planar formats that have different stride values for\n> > + * each plane, applications shall access the stride from the \\ref planes_\n> > + * instead.\n> > + *\n> > + * \\return The frame stride in bytes\n> > + */\n> > +std::size_t FrameFormat::stride() const\n> > +{\n> > + return planes_[0].stride;\n> > +}\n> > +\n>\n> What about having the plane as a parameter, defaulting to 0.\n>\n> Then the access to reading the strides can be done through a const\n> accessor rather than digging into the planes_ directly.\n>\n> With a similar accessor for size - could the Plane could be made private\n> and populated during the constructor only?\n>\n> With those considered:\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > +/**\n> > + * \\brief Retrieve the full frame size\n> > + *\n> > + * The frameSize() function is a convenience helper to retrieve the full frame\n> > + * size, defined as the sum of the sizes of all planes.\n> > + *\n> > + * \\return The full frame size, in bytes\n> > + */\n> > +std::size_t FrameFormat::frameSize() const\n> > +{\n> > + return std::accumulate(planes_.begin(), planes_.end(), 0,\n> > + [](std::size_t a, const Plane &b) {\n> > + return a + b.size;\n> > + });\n> > +}\n> > +\n> > +/**\n> > + * \\brief Assemble and return a string describing the frame format\n> > + *\n> > + * \\return A string describing the FrameFormat\n> > + */\n> > +std::string FrameFormat::toString() const\n> > +{\n> > + return size_.toString() + \"-\" + format_.toString();\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 4f08580157f9..7cf17ec2c628 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -15,6 +15,7 @@ libcamera_sources = files([\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'file_descriptor.cpp',\n> > + 'format.cpp',\n> > 'formats.cpp',\n> > 'framebuffer.cpp',\n> > 'framebuffer_allocator.cpp',\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 CE4BAC3240\n\tfor ;\n\tTue, 10 Aug 2021 03:10:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1AAB46884F;\n\tTue, 10 Aug 2021 05:10:31 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61F53687EB\n\tfor ;\n\tTue, 10 Aug 2021 05:10:29 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id g21so27991086edb.4\n\tfor ;\n\tMon, 09 Aug 2021 20:10:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"U4ZANJ9e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Smy51pSUjlMBB71i+dSW+0tuOKhI9hLVG77pCASiqok=;\n\tb=U4ZANJ9ejVNmSezLR1xTiho3jG7PLS/CXphCCEOfzDVVoHLrwfT/JKhiH3HmXz3q3s\n\tJAZw1ER1DsjwQRB4ISRIyGsrAOgKQh225qXtnL1JxrigMyQiYimlbWf/AAbrQnqrcNlO\n\t8B487F5uPGKf1yJ9xgI43UI+xPeQ1V1nylAQI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Smy51pSUjlMBB71i+dSW+0tuOKhI9hLVG77pCASiqok=;\n\tb=tpCbO73NqgKAdVwXRF4Ig4b0+Ny6kezFhueN6kP7HkLMUi2Yb3XtftUiSpWld7ZwnP\n\tFPzX1Hv3AMuEQU9FoyjkQtScGIOpKZYWYB8ah8DQ9mF19thyZlN83sPQMqfHeO5es5Dw\n\tvf6KawBtHHZVK+iPnBO0ZsL35xQGZJJ4Idi54NRkIoYaidZN5gnjKuknHFh0AviFXNZ4\n\tqfj9aCS1s7plW7vCYjUN1qwfH4VANleD8Bcgg5e/w12AseeuJT30wrA6hUmWix2pP2NG\n\tevtLT2+LUDQDkgkiomCD8idrL+lR3Y4fGcASo8X0Mf6TsWXEuwmsnlVreEmTSlOolg0S\n\tmaYg==","X-Gm-Message-State":"AOAM531qCHFH4Idv5qf14C4qYBNa5JocbqWE3NEa5myZkM60xIpioGxV\n\ticPs3tUJUebC+b/fHBFxYfpjAZjtto8RJx8H3nVsgg==","X-Google-Smtp-Source":"ABdhPJwujCodW6odrlqNujgc2gAEUaxDvLGhSr5iEtY09GPzwVMoRHAZZRfrxX7p7aww+sQK9BQzA3Q/6n71UGFfJDI=","X-Received":"by 2002:a05:6402:202:: with SMTP id\n\tt2mr2149008edv.116.1628565028978; \n\tMon, 09 Aug 2021 20:10:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20210805182203.1297-1-laurent.pinchart@ideasonboard.com>\n\t<1866183b-8399-cf51-95c5-58297c632add@ideasonboard.com>","In-Reply-To":"<1866183b-8399-cf51-95c5-58297c632add@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 10 Aug 2021 12:10:17 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat 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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18686,"web_url":"https://patchwork.libcamera.org/comment/18686/","msgid":"<20210810154823.uth3yzngxjcmjqbd@uno.localdomain>","date":"2021-08-10T15:48:23","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Thu, Aug 05, 2021 at 09:22:03PM +0300, Laurent Pinchart wrote:\n> The FrameFormat class describes how a frame is stored in memory. It\n> groups a pixel format, size in pixels, and per-plane stride and size.\n>\n> This new class will be used in the configuration API rework, but could\n> already be put in use if desired to replace the various data structures\n> used to carry the same information.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n>\n> I've developed this as part of the configuration API rework, and\n> realized it could already be useful, in particular if we want to start\n> cleaning up support for multi-planar formats. I'm thus sending it as an\n> RFC.\n>\n> ---\n> include/libcamera/format.h | 43 ++++++++++\n\nWe do have a include/libcamera/formats.h.in which generate\npixelformats. Do you think is there a naming conflict ? Once built\nformats.h.in get generated into formats.h\n\n> include/libcamera/meson.build | 1 +\n> src/libcamera/format.cpp | 142 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 187 insertions(+)\n> create mode 100644 include/libcamera/format.h\n> create mode 100644 src/libcamera/format.cpp\n>\n> diff --git a/include/libcamera/format.h b/include/libcamera/format.h\n> new file mode 100644\n> index 000000000000..4982ab8230dc\n> --- /dev/null\n> +++ b/include/libcamera/format.h\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * format.h - Frame Format\n> + */\n> +#ifndef __LIBCAMERA_FORMAT_H__\n> +#define __LIBCAMERA_FORMAT_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class FrameFormat\n> +{\n> +public:\n> +\tstatic constexpr unsigned int kMaxPlanes = 4;\n\nDo we use the kConstant convention ?\n\n> +\n> +\tstruct Plane {\n> +\t\tstd::size_t stride;\n> +\t\tstd::size_t size;\n> +\t};\n\nWe do have so many items named Plane :(\n\n> +\n> +\tFrameFormat();\n> +\tFrameFormat(PixelFormat format, const Size &size);\n> +\n> +\tstd::size_t stride() const;\n> +\tstd::size_t frameSize() const;\n> +\tstd::string toString() const;\n> +\n> +\tPixelFormat format_;\n> +\tSize size_;\n> +\tstd::array planes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_FORMAT_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 5b25ef847ed4..dcfadb381874 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -6,6 +6,7 @@ libcamera_public_headers = files([\n> 'compiler.h',\n> 'controls.h',\n> 'file_descriptor.h',\n> + 'format.h',\n> 'framebuffer.h',\n> 'framebuffer_allocator.h',\n> 'geometry.h',\n> diff --git a/src/libcamera/format.cpp b/src/libcamera/format.cpp\n> new file mode 100644\n> index 000000000000..f77413781de1\n> --- /dev/null\n> +++ b/src/libcamera/format.cpp\n> @@ -0,0 +1,142 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * format.cpp - Frame Format\n> + */\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file libcamera/format.h\n\nDo we prefix the file name with libcamera/ ?\n\n> + * \\brief Frame Format\n\nQuite brief :)\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class FrameFormat\n> + * \\brief Format of a frame as stored in memory\n> + *\n> + * The FrameFormat class fully describes how a frame is stored in memory. It\n> + * combines a pixel format, a size in pixels, and memory layout information for\n\nper plane ?\n\n> + * planes.\n> + *\n> + * Frames are stored in memory in one or multiple planes, as specified by the\n> + * pixel format. A plane is a two-dimensional memory area organized as lines of\n> + * samples. A sample typically stores one or multiple components of a pixel (for\n> + * instance, the NV12 format uses two planes, with the first plane storing only\n> + * the luminance Y component of the pixels, and the second plane storing both\n> + * the chroma Cb and Cr components).\n\nVery long (). Maybe as an example paragraph ?\n\n> + *\n> + * Planes cover the whole frame. Their number of lines and sample per lines\n\nper line ?\n\n> + * correspond to the frame width and height in pixels, possibly divided by\n\nnumber -> correspond\n\n> + * horizontal and vertical subsampling factors as specified by the pixel\n> + * format.\n> + *\n> + * A plane may contain padding at the end of line, to support requirements of\n> + * applications and cameras, such as alignment constraints. This is specified\n> + * as a line stride value, equal to the distance in bytes between the first\n> + * sample of successive lines in memory, including pixel data and padding. All\n> + * lines in a plane have the same stride value, including the last line.\n> + * Additionally, padding may also occur at the end of the plane, specified as\n> + * the total plane size in bytes.\n> + *\n> + * \\var FrameFormat::kMaxPlanes\n> + * \\brief Maximum number of data planes for a frame\n> + *\n> + * \\var FrameFormat::format_\n> + * \\brief Frame pixel format, defines how pixel data is packed and stored in\n> + * memory\n> + *\n> + * \\var FrameFormat::size_\n> + * \\brief Frame size, defines the horizontal and vertical dimensions of the\n> + * frame in pixels (excluding any padding)\n> + *\n> + * \\var FrameFormat::planes_\n> + * \\brief Frame data planes, defines the memory layout for each data plane of\n> + * the frame\n> + *\n> + * Unused entries in the planes array shall be zero-initialized.\n> + */\n> +\n> +/**\n> + * \\class FrameFormat::Plane\n> + * \\brief Memory layout configuration for one data plane of a frame\n> + *\n> + * \\var FrameFormat::Plane::stride\n> + * \\brief Distance in bytes between the beginning of two successive lines\n> + *\n> + * If the strides of different planes can't be set separately, the stride of the\n> + * first plane takes precedence and is used by the camera.\n> + *\n> + * \\var FrameFormat::Plane::size\n> + * \\brief Maximum number of bytes required to store the plane\n> + *\n> + * The \\a size reports the number of bytes required to store data for plane.\n> + * For uncompressed formats, it takes into account the plane's stride, the\n> + * frame height, the plane's vertical subsampling and the overall alignment\n> + * constraints of the camera (for instance many cameras require buffer sizes\n> + * to be a multiple of the page size). For compressed formats, it indicates the\n> + * maximum size of the compressed data.\n> + */\n> +\n> +/**\n> + * \\brief Construct a zero-initialized FrameFormat\n> + */\n> +FrameFormat::FrameFormat() = default;\n\nMAybe it's me but = default is usually in the header file\n\n> +\n> +/**\n> + * \\brief Construct a FrameFormat\n> + * \\param[in] format The pixel format\n> + * \\param[in] size The frame size in pixels\n> + */\n> +FrameFormat::FrameFormat(PixelFormat format, const Size &size)\n> +\t: format_(format), size_(size)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the frame stride in bytes\n> + *\n> + * The stride() function is a convenience helper to retrieve the stride of the\n> + * first plane. For multi-planar formats that have different stride values for\n> + * each plane, applications shall access the stride from the \\ref planes_\n> + * instead.\n> + *\n> + * \\return The frame stride in bytes\n> + */\n> +std::size_t FrameFormat::stride() const\n\nWhy can't it take an index ? I mean, it's either an accessor to each\nsingle plane stride or application should go through the planes_\nvector and find stride there. I understand planes_[0] is special, but\nconsidering we don't have a variable to express how many planes a\nformat has, requesting application to poke into planes_ might not be\nideal ?\n\n> +{\n> +\treturn planes_[0].stride;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the full frame size\n> + *\n> + * The frameSize() function is a convenience helper to retrieve the full frame\n> + * size, defined as the sum of the sizes of all planes.\n> + *\n> + * \\return The full frame size, in bytes\n> + */\n> +std::size_t FrameFormat::frameSize() const\n> +{\n> +\treturn std::accumulate(planes_.begin(), planes_.end(), 0,\n> +\t\t\t [](std::size_t a, const Plane &b) {\n> +\t\t\t\t return a + b.size;\n> +\t\t\t });\n> +}\n> +\n> +/**\n> + * \\brief Assemble and return a string describing the frame format\n> + *\n> + * \\return A string describing the FrameFormat\n> + */\n> +std::string FrameFormat::toString() const\n> +{\n> +\treturn size_.toString() + \"-\" + format_.toString();\n> +}\n> +\n> +} /* namespace libcamera */\n\nNits apart, I am missing how this is meant to be used...\nA frame format is a property of the pixel format. I would expect each\nPixelFormat to have a FrameFormat associated to describe its layout in\nmemory.\n\nHow do expect this class to be used ?\n\nThanks\n j\n\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 4f08580157f9..7cf17ec2c628 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -15,6 +15,7 @@ libcamera_sources = files([\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'file_descriptor.cpp',\n> + 'format.cpp',\n> 'formats.cpp',\n> 'framebuffer.cpp',\n> 'framebuffer_allocator.cpp',\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 66DA1BD87D\n\tfor ;\n\tTue, 10 Aug 2021 15:47:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2A1E6884F;\n\tTue, 10 Aug 2021 17:47:37 +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 60FC4687F0\n\tfor ;\n\tTue, 10 Aug 2021 17:47:36 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id D6490240003;\n\tTue, 10 Aug 2021 15:47:35 +0000 (UTC)"],"Date":"Tue, 10 Aug 2021 17:48:23 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210810154823.uth3yzngxjcmjqbd@uno.localdomain>","References":"<20210805182203.1297-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210805182203.1297-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]