[{"id":28777,"web_url":"https://patchwork.libcamera.org/comment/28777/","msgid":"","date":"2024-02-28T08:32:29","subject":"Re: [PATCH 3/9] libcamera: v4l2_subdevice: Expose media bus format\n\tinfo as internal API","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Tue, Feb 27, 2024 at 04:09:47PM +0200, Laurent Pinchart wrote:\n> The V4L2SubdeviceFormatInfo structure, internal to the\n> v4l2_subdevice.cpp compilation unit, contains information about media\n> bus formats that will be useful in other parts of libcamera. To prepare\n> for this, expose the structure in the v4l2_subdevice.h header and turn\n> it into a class with a similar design as PixelFormatInfo.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/v4l2_subdevice.h | 13 +++\n> src/libcamera/v4l2_subdevice.cpp | 94 ++++++++++++++-------\n> 2 files changed, 76 insertions(+), 31 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 17db311bcfb3..a4df9ddfd322 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -29,6 +29,19 @@ namespace libcamera {\n>\n> class MediaDevice;\n>\n> +class MediaBusFormatInfo\n> +{\n> +public:\n> +\tbool isValid() const { return code != 0; }\n> +\n> +\tstatic const MediaBusFormatInfo &info(uint32_t code);\n> +\n> +\tconst char *name;\n> +\tuint32_t code;\n> +\tunsigned int bitsPerPixel;\n> +\tPixelFormatInfo::ColourEncoding colourEncoding;\n> +};\n> +\n> struct V4L2SubdeviceCapability final : v4l2_subdev_capability {\n> \tbool isReadOnly() const\n> \t{\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index a74b8362b6d1..fd289ae9ae6f 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -36,28 +36,40 @@ namespace libcamera {\n>\n> LOG_DECLARE_CATEGORY(V4L2)\n>\n> +/**\n> + * \\class MediaBusFormatInfo\n> + * \\brief Information about media bus formats\n> + *\n> + * The MediaBusFormatInfo class groups together information describing a media\n> + * bus format. It facilitates handling of media bus formats by providing data\n> + * commonly used in pipeline handlers.\n> + *\n> + * \\var MediaBusFormatInfo::name\n> + * \\brief The format name as a human-readable string, used as the text\n> + * representation of the format\n> + *\n> + * \\var MediaBusFormatInfo::code\n> + * \\brief The media bus format code described by this instance\n\nIs it worth saying it's one of the MEDIA_BUS_FMT_ entries defined by\nV4L2 ?\n\n> + *\n> + * \\var MediaBusFormatInfo::bitsPerPixel\n> + * \\brief The average number of bits per pixel\n> + *\n> + * The number of bits per pixel averages the total number of bits for all\n> + * colour components over the whole image, excluding any padding bits or\n> + * padding pixels.\n\nNot sure I got why it's an average here\n\n> + *\n> + * For formats that transmit multiple or fractional pixels per sample, the\n> + * value will differ from the bus width.\n\nIs this relevant ? Isn't this related to the HW bus width ?\n\n> + *\n> + * Formats that don't have a fixed number of bits per pixel, such as compressed\n> + * formats, report 0 in this field.\n> + *\n> + * \\var MediaBusFormatInfo::colourEncoding\n> + * \\brief The colour encoding type\n> + */\n> +\n> namespace {\n>\n> -/*\n> - * \\struct MediaBusFormatInfo\n> - * \\brief Information about media bus formats\n> - * \\param name Name of MBUS format\n> - * \\param code The media bus format code\n> - * \\param bitsPerPixel Bits per pixel\n> - * \\param colourEncoding Type of colour encoding\n> - */\n> -struct MediaBusFormatInfo {\n> -\tconst char *name;\n> -\tuint32_t code;\n> -\tunsigned int bitsPerPixel;\n> -\tPixelFormatInfo::ColourEncoding colourEncoding;\n> -};\n> -\n> -/*\n> - * \\var mediaBusFormatInfo\n> - * \\brief A map that associates MediaBusFormatInfo struct to V4L2 media\n> - * bus codes\n> - */\n> const std::map mediaBusFormatInfo{\n> \t/* This table is sorted to match the order in linux/media-bus-format.h */\n> \t{ MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE, {\n> @@ -533,6 +545,33 @@ const std::map mediaBusFormatInfo{\n>\n> } /* namespace */\n>\n> +/**\n> + * \\fn bool MediaBusFormatInfo::isValid() const\n> + * \\brief Check if the media bus format info is valid\n> + * \\return True if the media bus format info is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Retrieve information about a media bus format\n> + * \\param[in] code The media bus format code\n> + * \\return The MediaBusFormatInfo describing the \\a code if known, or an invalid\n> + * MediaBusFormatInfo otherwise\n> + */\n> +const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code)\n> +{\n> +\tstatic const MediaBusFormatInfo invalid{};\n> +\n> +\tconst auto it = mediaBusFormatInfo.find(code);\n> +\tif (it == mediaBusFormatInfo.end()) {\n> +\t\tLOG(V4L2, Warning)\n> +\t\t\t<< \"Unsupported media bus format \"\n\nFitst on the previous line\n\n> +\t\t\t<< utils::hex(code, 4);\n> +\t\treturn invalid;\n> +\t}\n> +\n> +\treturn it->second;\n> +}\n> +\n> /**\n> * \\struct V4L2SubdeviceCapability\n> * \\brief struct v4l2_subdev_capability object wrapper and helpers\n> @@ -629,14 +668,7 @@ const std::string V4L2SubdeviceFormat::toString() const\n> */\n> uint8_t V4L2SubdeviceFormat::bitsPerPixel() const\n> {\n> -\tconst auto it = mediaBusFormatInfo.find(mbus_code);\n> -\tif (it == mediaBusFormatInfo.end()) {\n> -\t\tLOG(V4L2, Error) << \"No information available for format '\"\n> -\t\t\t\t << *this << \"'\";\n> -\t\treturn 0;\n> -\t}\n> -\n> -\treturn it->second.bitsPerPixel;\n> +\treturn MediaBusFormatInfo::info(mbus_code).bitsPerPixel;\n> }\n>\n> /**\n> @@ -903,9 +935,9 @@ std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> \t\treturn std::nullopt;\n>\n> \tPixelFormatInfo::ColourEncoding colourEncoding;\n> -\tauto iter = mediaBusFormatInfo.find(format.code);\n> -\tif (iter != mediaBusFormatInfo.end()) {\n> -\t\tcolourEncoding = iter->second.colourEncoding;\n> +\tconst MediaBusFormatInfo &info = MediaBusFormatInfo::info(format.code);\n> +\tif (info.isValid()) {\n> +\t\tcolourEncoding = info.colourEncoding;\n> \t} else {\n> \t\tLOG(V4L2, Warning)\n> \t\t\t<< \"Unknown subdev format \"\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 6E173BD80A\n\tfor ;\n\tWed, 28 Feb 2024 08:32:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3EAF62868;\n\tWed, 28 Feb 2024 09:32:33 +0100 (CET)","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 252B562867\n\tfor ;\n\tWed, 28 Feb 2024 09:32:32 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5A3C1672;\n\tWed, 28 Feb 2024 09:32:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FmLOW/ux\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709109139;\n\tbh=jr/kcQRNbvjLTwiYWYzGhKEHAnRPVJuJIrCJxQvRp84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FmLOW/uxr84DFVojTQH5YMbF0aiHVdW9/pjbNjYWpAbXuHcqLkDpNCAuV8VgblRO5\n\tDdhynoSIpQwVWKZwlmwtRlrPoJ2X8Lrh582uNKMsabeGvw8yAFWlld/lDPKcbi7PTl\n\t/3dheFbKiF1Q+WXmypJjlwNaJmMGAJ9huAQDNYKs=","Date":"Wed, 28 Feb 2024 09:32:29 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Subject":"Re: [PATCH 3/9] libcamera: v4l2_subdevice: Expose media bus format\n\tinfo as internal API","Message-ID":"","References":"<20240227140953.26093-1-laurent.pinchart@ideasonboard.com>\n\t<20240227140953.26093-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240227140953.26093-4-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28783,"web_url":"https://patchwork.libcamera.org/comment/28783/","msgid":"<20240228102039.GE3419@pendragon.ideasonboard.com>","date":"2024-02-28T10:20:39","subject":"Re: [PATCH 3/9] libcamera: v4l2_subdevice: Expose media bus format\n\tinfo as internal API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Feb 28, 2024 at 09:32:29AM +0100, Jacopo Mondi wrote:\n> On Tue, Feb 27, 2024 at 04:09:47PM +0200, Laurent Pinchart wrote:\n> > The V4L2SubdeviceFormatInfo structure, internal to the\n> > v4l2_subdevice.cpp compilation unit, contains information about media\n> > bus formats that will be useful in other parts of libcamera. To prepare\n> > for this, expose the structure in the v4l2_subdevice.h header and turn\n> > it into a class with a similar design as PixelFormatInfo.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/v4l2_subdevice.h | 13 +++\n> > src/libcamera/v4l2_subdevice.cpp | 94 ++++++++++++++-------\n> > 2 files changed, 76 insertions(+), 31 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > index 17db311bcfb3..a4df9ddfd322 100644\n> > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > @@ -29,6 +29,19 @@ namespace libcamera {\n> >\n> > class MediaDevice;\n> >\n> > +class MediaBusFormatInfo\n> > +{\n> > +public:\n> > +\tbool isValid() const { return code != 0; }\n> > +\n> > +\tstatic const MediaBusFormatInfo &info(uint32_t code);\n> > +\n> > +\tconst char *name;\n> > +\tuint32_t code;\n> > +\tunsigned int bitsPerPixel;\n> > +\tPixelFormatInfo::ColourEncoding colourEncoding;\n> > +};\n> > +\n> > struct V4L2SubdeviceCapability final : v4l2_subdev_capability {\n> > \tbool isReadOnly() const\n> > \t{\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index a74b8362b6d1..fd289ae9ae6f 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -36,28 +36,40 @@ namespace libcamera {\n> >\n> > LOG_DECLARE_CATEGORY(V4L2)\n> >\n> > +/**\n> > + * \\class MediaBusFormatInfo\n> > + * \\brief Information about media bus formats\n> > + *\n> > + * The MediaBusFormatInfo class groups together information describing a media\n> > + * bus format. It facilitates handling of media bus formats by providing data\n> > + * commonly used in pipeline handlers.\n> > + *\n> > + * \\var MediaBusFormatInfo::name\n> > + * \\brief The format name as a human-readable string, used as the text\n> > + * representation of the format\n> > + *\n> > + * \\var MediaBusFormatInfo::code\n> > + * \\brief The media bus format code described by this instance\n> \n> Is it worth saying it's one of the MEDIA_BUS_FMT_ entries defined by\n> V4L2 ?\n\nGood idea, I'll add it.\n\n> > + *\n> > + * \\var MediaBusFormatInfo::bitsPerPixel\n> > + * \\brief The average number of bits per pixel\n> > + *\n> > + * The number of bits per pixel averages the total number of bits for all\n> > + * colour components over the whole image, excluding any padding bits or\n> > + * padding pixels.\n> \n> Not sure I got why it's an average here\n\nSee MEDIA_BUS_FMT_UYVY8_1_5X8 in\nhttps://docs.kernel.org/userspace-api/media/v4l/subdev-formats.html#v4l2-mbus-pixelcode.\nThe chroma samples are shared between multiple pixels.\n\n> > + *\n> > + * For formats that transmit multiple or fractional pixels per sample, the\n> > + * value will differ from the bus width.\n> \n> Is this relevant ? Isn't this related to the HW bus width ?\n\nYes it's related to the hardware bus width. The point of this comment is\nto indicate to the reader that bitsPerPixel is not the bus width.\n\n> > + *\n> > + * Formats that don't have a fixed number of bits per pixel, such as compressed\n> > + * formats, report 0 in this field.\n> > + *\n> > + * \\var MediaBusFormatInfo::colourEncoding\n> > + * \\brief The colour encoding type\n> > + */\n> > +\n> > namespace {\n> >\n> > -/*\n> > - * \\struct MediaBusFormatInfo\n> > - * \\brief Information about media bus formats\n> > - * \\param name Name of MBUS format\n> > - * \\param code The media bus format code\n> > - * \\param bitsPerPixel Bits per pixel\n> > - * \\param colourEncoding Type of colour encoding\n> > - */\n> > -struct MediaBusFormatInfo {\n> > -\tconst char *name;\n> > -\tuint32_t code;\n> > -\tunsigned int bitsPerPixel;\n> > -\tPixelFormatInfo::ColourEncoding colourEncoding;\n> > -};\n> > -\n> > -/*\n> > - * \\var mediaBusFormatInfo\n> > - * \\brief A map that associates MediaBusFormatInfo struct to V4L2 media\n> > - * bus codes\n> > - */\n> > const std::map mediaBusFormatInfo{\n> > \t/* This table is sorted to match the order in linux/media-bus-format.h */\n> > \t{ MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE, {\n> > @@ -533,6 +545,33 @@ const std::map mediaBusFormatInfo{\n> >\n> > } /* namespace */\n> >\n> > +/**\n> > + * \\fn bool MediaBusFormatInfo::isValid() const\n> > + * \\brief Check if the media bus format info is valid\n> > + * \\return True if the media bus format info is valid, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Retrieve information about a media bus format\n> > + * \\param[in] code The media bus format code\n> > + * \\return The MediaBusFormatInfo describing the \\a code if known, or an invalid\n> > + * MediaBusFormatInfo otherwise\n> > + */\n> > +const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code)\n> > +{\n> > +\tstatic const MediaBusFormatInfo invalid{};\n> > +\n> > +\tconst auto it = mediaBusFormatInfo.find(code);\n> > +\tif (it == mediaBusFormatInfo.end()) {\n> > +\t\tLOG(V4L2, Warning)\n> > +\t\t\t<< \"Unsupported media bus format \"\n> \n> Fitst on the previous line\n\nNot the full statement:\n\n\t\tLOG(V4L2, Warning) << \"Unsupported media bus format \" << utils::hex(code, 4);\n\nand our usual coding style is to wrap just after LOG() in that case.\n\n> > +\t\t\t<< utils::hex(code, 4);\n> > +\t\treturn invalid;\n> > +\t}\n> > +\n> > +\treturn it->second;\n> > +}\n> > +\n> > /**\n> > * \\struct V4L2SubdeviceCapability\n> > * \\brief struct v4l2_subdev_capability object wrapper and helpers\n> > @@ -629,14 +668,7 @@ const std::string V4L2SubdeviceFormat::toString() const\n> > */\n> > uint8_t V4L2SubdeviceFormat::bitsPerPixel() const\n> > {\n> > -\tconst auto it = mediaBusFormatInfo.find(mbus_code);\n> > -\tif (it == mediaBusFormatInfo.end()) {\n> > -\t\tLOG(V4L2, Error) << \"No information available for format '\"\n> > -\t\t\t\t << *this << \"'\";\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > -\treturn it->second.bitsPerPixel;\n> > +\treturn MediaBusFormatInfo::info(mbus_code).bitsPerPixel;\n> > }\n> >\n> > /**\n> > @@ -903,9 +935,9 @@ std::optional V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> > \t\treturn std::nullopt;\n> >\n> > \tPixelFormatInfo::ColourEncoding colourEncoding;\n> > -\tauto iter = mediaBusFormatInfo.find(format.code);\n> > -\tif (iter != mediaBusFormatInfo.end()) {\n> > -\t\tcolourEncoding = iter->second.colourEncoding;\n> > +\tconst MediaBusFormatInfo &info = MediaBusFormatInfo::info(format.code);\n> > +\tif (info.isValid()) {\n> > +\t\tcolourEncoding = info.colourEncoding;\n> > \t} else {\n> > \t\tLOG(V4L2, Warning)\n> > \t\t\t<< \"Unknown subdev format \"","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 5E945BD160\n\tfor ;\n\tWed, 28 Feb 2024 10:20:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D18D62868;\n\tWed, 28 Feb 2024 11:20:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D50F5627F9\n\tfor ;\n\tWed, 28 Feb 2024 11:20:37 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E20E8672;\n\tWed, 28 Feb 2024 11:20:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kta+yYVg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709115625;\n\tbh=eVXro3JuqFMEoh0OIrj4G9IUIdiAngzkB3jD+kjYmXg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kta+yYVgvEpmM+NnoefEJrRNXXfsuNvn0tKTOhbnhMTh/QXsO4C6a4EcR4qmp41Tq\n\tBnN3jsY6gK+DwRglg4Epkh7SJd95FTrHS+1I5TU82qLsfv/BpGo4fkOkB3jEZ+Ar/l\n\tmuR1cva0QioM5ULeWg04LxR+oKWHv0LcBv0fGHnQ=","Date":"Wed, 28 Feb 2024 12:20:39 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [PATCH 3/9] libcamera: v4l2_subdevice: Expose media bus format\n\tinfo as internal API","Message-ID":"<20240228102039.GE3419@pendragon.ideasonboard.com>","References":"<20240227140953.26093-1-laurent.pinchart@ideasonboard.com>\n\t<20240227140953.26093-4-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]