[{"id":14306,"web_url":"https://patchwork.libcamera.org/comment/14306/","msgid":"","date":"2020-12-22T11:35:11","subject":"Re: [libcamera-devel] [PATCH v2 3/9] libcamera: bayer_format: Add\n\tsupport for mbus codes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Dec 18, 2020 at 05:47:48PM +0100, Jacopo Mondi wrote:\n> The existing implementation of the BayerFormat class supports\n> converting a V4L2PixelFormat to a BayerFormat and vice-versa.\n> \n> Expand the class by adding support for converting a media bus code\n> to a BayerFormat instance, by providing a conversion table and a\n> dedicated constructor.\n> \n> Expand the number of supported compression and expand the documentation.\n> \n> Do not provide support for converting a BayerFormat to a media bus code\n> as the feature is currently not required.\n\nAnd the mapping would be 1:1 anyway. That's the issue with media bus\ncodes, they don't have a 1:1 mapping to pixel formats.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/bayer_format.h | 3 ++\n> src/libcamera/bayer_format.cpp | 66 ++++++++++++++++++++++-\n> 2 files changed, 68 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> index 4280b76b016f..7d7539e275ff 100644\n> --- a/include/libcamera/internal/bayer_format.h\n> +++ b/include/libcamera/internal/bayer_format.h\n> @@ -30,6 +30,8 @@ public:\n> \t\tNone = 0,\n> \t\tCSI2Packed = 1,\n> \t\tIPU3Packed = 2,\n> +\t\tALAW8Compression = 3,\n> +\t\tDPCM8Compression = 4,\n\nCompression and packing are not mutually exclusive, this would need to\nbecome flags, or possibly get moved to a separate enum.\n\n> \t};\n> \n> \tconstexpr BayerFormat()\n> @@ -43,6 +45,7 @@ public:\n> \t}\n> \n> \texplicit BayerFormat(V4L2PixelFormat v4l2Format);\n> +\texplicit BayerFormat(unsigned int mbusCode);\n> \tbool isValid() const { return bitDepth != 0; }\n> \n> \tstd::string toString() const;\n> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> index c42792ff1948..a4eba4592bfe 100644\n> --- a/src/libcamera/bayer_format.cpp\n> +++ b/src/libcamera/bayer_format.cpp\n> @@ -8,6 +8,9 @@\n> #include \"libcamera/internal/bayer_format.h\"\n> \n> #include \n> +#include \n> +\n> +#include \n> \n> #include \n> \n> @@ -45,7 +48,8 @@ namespace libcamera {\n> \n> /**\n> * \\enum BayerFormat::Packing\n> - * \\brief Different types of packing that can be applied to a BayerFormat\n> + * \\brief Different types of packing or compressions that can be applied to a\n> + * BayerFormat\n> *\n> * \\var BayerFormat::None\n> * \\brief No packing\n> @@ -53,6 +57,10 @@ namespace libcamera {\n> * \\brief Format uses MIPI CSI-2 style packing\n> * \\var BayerFormat::IPU3Packed\n> * \\brief Format uses IPU3 style packing\n> + * \\var BayerFormat::ALAW8Compression\n> + * \\brief Format uses ALAW8 compression\n> + * \\var BayerFormat::DPCM8Compression\n> + * \\brief Format uses DPCM8 compression\n> */\n> \n> namespace {\n> @@ -140,6 +148,41 @@ const std::map bayerToV4l2{\n> \t{ { BayerFormat::RGGB, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) },\n> };\n> \n> +const std::unordered_map mbusCodeToBayer{\n> +\t{ MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, { BayerFormat::BGGR, 8, BayerFormat::ALAW8Compression } },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, { BayerFormat::GBRG, 8, BayerFormat::ALAW8Compression } },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, { BayerFormat::GRBG, 8, BayerFormat::ALAW8Compression } },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, { BayerFormat::RGGB, 8, BayerFormat::ALAW8Compression } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, { BayerFormat::BGGR, 8, BayerFormat::DPCM8Compression } },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, { BayerFormat::GBRG, 8, BayerFormat::DPCM8Compression } },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, { BayerFormat::GRBG, 8, BayerFormat::DPCM8Compression } },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, { BayerFormat::RGGB, 8, BayerFormat::DPCM8Compression } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> +\t{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> +};\n> +\n> } /* namespace */\n> \n> /**\n> @@ -169,6 +212,22 @@ BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)\n> \t\t*this = it->second;\n> }\n> \n> +/**\n> + * \\brief Construct a BayerFormat from a media bus code\n> + * \\param[in] mbusCode The media bus code to convert into a BayerFormat\n> + *\n> + * The media bus code numeric identifiers are defined by the V4L2 specification.\n> + */\n> +BayerFormat::BayerFormat(unsigned int mbusCode)\n> +\t: order(BGGR), packing(None)\n> +{\n> +\tconst auto it = mbusCodeToBayer.find(mbusCode);\n> +\tif (it == mbusCodeToBayer.end())\n> +\t\tbitDepth = 0;\n> +\telse\n> +\t\t*this = it->second;\n> +}\n\nInstead of a constructor, wouldn't a static\nBayerFormat::fromMediaBusCode() function be better ? The trouble with a\nconstructor is that, as it has to take an unsigned int argument (we\ndon't have a media bus code class), one could write\n\n\tBayerFormat format(V4L2_PIX_FMT_YUYV);\n\nand the compiler wouldn't complain. Worse, PixelFormat::operator\nuint32_t() isn't explicit, so\n\n\tPixelFormat pf(...);\n\tBayerFormat format(pf);\n\nwill also compile.\n\nObviously,\n\n\tPixelFormat pf(...);\n\tBayerFormat format = BayerFormat::fromMediaBusCode(pf);\n\nwill also compile, but it will be easier to spot from the code that\nsomething is wrong.\n\n> +\n> /**\n> * \\fn BayerFormat::isValid()\n> * \\brief Return whether a BayerFormat is valid\n> @@ -258,6 +317,11 @@ BayerFormat BayerFormat::transform(Transform t) const\n> /**\n> * \\var BayerFormat::bitDepth\n> * \\brief The bit depth of the samples in the Bayer pattern\n> + *\n> + * For formats using compression schemes such as Packing::ALAW8Compression or\n> + * Packing::DPCM8Compression, the bitDepth value refers to the size of the\n> + * sample when transmitted on the wire or stored into memory. The resulting\n> + * images need to be de-compressed to be viewed properly.\n> */\n> \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 05C1EC0F1A\n\tfor ;\n\tTue, 22 Dec 2020 11:35:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80DE9615AC;\n\tTue, 22 Dec 2020 12:35:20 +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 EAF7860528\n\tfor ;\n\tTue, 22 Dec 2020 12:35:19 +0100 (CET)","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 56C759E6;\n\tTue, 22 Dec 2020 12:35:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"io9B8srY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608636919;\n\tbh=ReW4tVyFKrNIWfmB8/o26U23Obi97VMoBoHyNQQSf8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=io9B8srYxXQ0ADkahqIXaqd+vZNlbYDm3HqcWHieNKfVA/YcTzXZm9rAHVrxAZN8g\n\tFtyRunN+ao5CZVKKKtafUYh7oFm9zybnmagC7SHS9mIw6S7ZhTxrqc6girm84ED2zQ\n\tRbrZGwVjZK5TtevGiO3NOwZVkgucE2LgWyUXRtKg=","Date":"Tue, 22 Dec 2020 13:35:11 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201218164754.81422-1-jacopo@jmondi.org>\n\t<20201218164754.81422-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201218164754.81422-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/9] libcamera: bayer_format: Add\n\tsupport for mbus codes","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14307,"web_url":"https://patchwork.libcamera.org/comment/14307/","msgid":"<20201222125438.y2rmdxy35bectkzw@uno.localdomain>","date":"2020-12-22T12:54:38","subject":"Re: [libcamera-devel] [PATCH v2 3/9] libcamera: bayer_format: Add\n\tsupport for mbus codes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Dec 22, 2020 at 01:35:11PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 18, 2020 at 05:47:48PM +0100, Jacopo Mondi wrote:\n> > The existing implementation of the BayerFormat class supports\n> > converting a V4L2PixelFormat to a BayerFormat and vice-versa.\n> >\n> > Expand the class by adding support for converting a media bus code\n> > to a BayerFormat instance, by providing a conversion table and a\n> > dedicated constructor.\n> >\n> > Expand the number of supported compression and expand the documentation.\n> >\n> > Do not provide support for converting a BayerFormat to a media bus code\n> > as the feature is currently not required.\n>\n> And the mapping would be 1:1 anyway. That's the issue with media bus\n> codes, they don't have a 1:1 mapping to pixel formats.\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/bayer_format.h | 3 ++\n> > src/libcamera/bayer_format.cpp | 66 ++++++++++++++++++++++-\n> > 2 files changed, 68 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > index 4280b76b016f..7d7539e275ff 100644\n> > --- a/include/libcamera/internal/bayer_format.h\n> > +++ b/include/libcamera/internal/bayer_format.h\n> > @@ -30,6 +30,8 @@ public:\n> > \t\tNone = 0,\n> > \t\tCSI2Packed = 1,\n> > \t\tIPU3Packed = 2,\n> > +\t\tALAW8Compression = 3,\n> > +\t\tDPCM8Compression = 4,\n>\n> Compression and packing are not mutually exclusive, this would need to\n> become flags, or possibly get moved to a separate enum.\n>\n\nI'll make these flags\n\n> > \t};\n> >\n> > \tconstexpr BayerFormat()\n> > @@ -43,6 +45,7 @@ public:\n> > \t}\n> >\n> > \texplicit BayerFormat(V4L2PixelFormat v4l2Format);\n> > +\texplicit BayerFormat(unsigned int mbusCode);\n> > \tbool isValid() const { return bitDepth != 0; }\n> >\n> > \tstd::string toString() const;\n> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > index c42792ff1948..a4eba4592bfe 100644\n> > --- a/src/libcamera/bayer_format.cpp\n> > +++ b/src/libcamera/bayer_format.cpp\n> > @@ -8,6 +8,9 @@\n> > #include \"libcamera/internal/bayer_format.h\"\n> >\n> > #include \n> > +#include \n> > +\n> > +#include \n> >\n> > #include \n> >\n> > @@ -45,7 +48,8 @@ namespace libcamera {\n> >\n> > /**\n> > * \\enum BayerFormat::Packing\n> > - * \\brief Different types of packing that can be applied to a BayerFormat\n> > + * \\brief Different types of packing or compressions that can be applied to a\n> > + * BayerFormat\n> > *\n> > * \\var BayerFormat::None\n> > * \\brief No packing\n> > @@ -53,6 +57,10 @@ namespace libcamera {\n> > * \\brief Format uses MIPI CSI-2 style packing\n> > * \\var BayerFormat::IPU3Packed\n> > * \\brief Format uses IPU3 style packing\n> > + * \\var BayerFormat::ALAW8Compression\n> > + * \\brief Format uses ALAW8 compression\n> > + * \\var BayerFormat::DPCM8Compression\n> > + * \\brief Format uses DPCM8 compression\n> > */\n> >\n> > namespace {\n> > @@ -140,6 +148,41 @@ const std::map bayerToV4l2{\n> > \t{ { BayerFormat::RGGB, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) },\n> > };\n> >\n> > +const std::unordered_map mbusCodeToBayer{\n> > +\t{ MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, { BayerFormat::BGGR, 8, BayerFormat::ALAW8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, { BayerFormat::GBRG, 8, BayerFormat::ALAW8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, { BayerFormat::GRBG, 8, BayerFormat::ALAW8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, { BayerFormat::RGGB, 8, BayerFormat::ALAW8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, { BayerFormat::BGGR, 8, BayerFormat::DPCM8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, { BayerFormat::GBRG, 8, BayerFormat::DPCM8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, { BayerFormat::GRBG, 8, BayerFormat::DPCM8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, { BayerFormat::RGGB, 8, BayerFormat::DPCM8Compression } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> > +};\n> > +\n> > } /* namespace */\n> >\n> > /**\n> > @@ -169,6 +212,22 @@ BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)\n> > \t\t*this = it->second;\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a BayerFormat from a media bus code\n> > + * \\param[in] mbusCode The media bus code to convert into a BayerFormat\n> > + *\n> > + * The media bus code numeric identifiers are defined by the V4L2 specification.\n> > + */\n> > +BayerFormat::BayerFormat(unsigned int mbusCode)\n> > +\t: order(BGGR), packing(None)\n> > +{\n> > +\tconst auto it = mbusCodeToBayer.find(mbusCode);\n> > +\tif (it == mbusCodeToBayer.end())\n> > +\t\tbitDepth = 0;\n> > +\telse\n> > +\t\t*this = it->second;\n> > +}\n>\n> Instead of a constructor, wouldn't a static\n> BayerFormat::fromMediaBusCode() function be better ? The trouble with a\n> constructor is that, as it has to take an unsigned int argument (we\n> don't have a media bus code class), one could write\n>\n> \tBayerFormat format(V4L2_PIX_FMT_YUYV);\n>\n> and the compiler wouldn't complain. Worse, PixelFormat::operator\n> uint32_t() isn't explicit, so\n>\n> \tPixelFormat pf(...);\n> \tBayerFormat format(pf);\n>\n> will also compile.\n>\n> Obviously,\n>\n> \tPixelFormat pf(...);\n> \tBayerFormat format = BayerFormat::fromMediaBusCode(pf);\n>\n> will also compile, but it will be easier to spot from the code that\n> something is wrong.\n>\n\nI went with a constructor for simmetry with the existing\n\texplicit BayerFormat(V4L2PixelFormat v4l2Format);\n\nI feel like the whole class should be better reworked to be similar to\nV4L2PixelFormat that provides a 'toPixelFormat' and one\n'fromPixelFormat'.\n\nIn the meantime, if a static methods makes it harder to get the\nconstuctor wrong, I'll use that.\n\nThanks\n j\n\n\n> > +\n> > /**\n> > * \\fn BayerFormat::isValid()\n> > * \\brief Return whether a BayerFormat is valid\n> > @@ -258,6 +317,11 @@ BayerFormat BayerFormat::transform(Transform t) const\n> > /**\n> > * \\var BayerFormat::bitDepth\n> > * \\brief The bit depth of the samples in the Bayer pattern\n> > + *\n> > + * For formats using compression schemes such as Packing::ALAW8Compression or\n> > + * Packing::DPCM8Compression, the bitDepth value refers to the size of the\n> > + * sample when transmitted on the wire or stored into memory. The resulting\n> > + * images need to be de-compressed to be viewed properly.\n> > */\n> >\n> > /**\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 158FAC0F1B\n\tfor ;\n\tTue, 22 Dec 2020 12:54:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BA1A6158A;\n\tTue, 22 Dec 2020 13:54:28 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A05EE60528\n\tfor ;\n\tTue, 22 Dec 2020 13:54:27 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 101F6240005;\n\tTue, 22 Dec 2020 12:54:26 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 22 Dec 2020 13:54:38 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201222125438.y2rmdxy35bectkzw@uno.localdomain>","References":"<20201218164754.81422-1-jacopo@jmondi.org>\n\t<20201218164754.81422-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/9] libcamera: bayer_format: Add\n\tsupport for mbus codes","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14310,"web_url":"https://patchwork.libcamera.org/comment/14310/","msgid":"","date":"2020-12-22T16:49:59","subject":"Re: [libcamera-devel] [PATCH v2 3/9] libcamera: bayer_format: Add\n\tsupport for mbus codes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Dec 22, 2020 at 01:54:38PM +0100, Jacopo Mondi wrote:\n> On Tue, Dec 22, 2020 at 01:35:11PM +0200, Laurent Pinchart wrote:\n> > On Fri, Dec 18, 2020 at 05:47:48PM +0100, Jacopo Mondi wrote:\n> > > The existing implementation of the BayerFormat class supports\n> > > converting a V4L2PixelFormat to a BayerFormat and vice-versa.\n> > >\n> > > Expand the class by adding support for converting a media bus code\n> > > to a BayerFormat instance, by providing a conversion table and a\n> > > dedicated constructor.\n> > >\n> > > Expand the number of supported compression and expand the documentation.\n> > >\n> > > Do not provide support for converting a BayerFormat to a media bus code\n> > > as the feature is currently not required.\n> >\n> > And the mapping would be 1:1 anyway. That's the issue with media bus\n> > codes, they don't have a 1:1 mapping to pixel formats.\n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/bayer_format.h | 3 ++\n> > > src/libcamera/bayer_format.cpp | 66 ++++++++++++++++++++++-\n> > > 2 files changed, 68 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h\n> > > index 4280b76b016f..7d7539e275ff 100644\n> > > --- a/include/libcamera/internal/bayer_format.h\n> > > +++ b/include/libcamera/internal/bayer_format.h\n> > > @@ -30,6 +30,8 @@ public:\n> > > \t\tNone = 0,\n> > > \t\tCSI2Packed = 1,\n> > > \t\tIPU3Packed = 2,\n> > > +\t\tALAW8Compression = 3,\n> > > +\t\tDPCM8Compression = 4,\n> >\n> > Compression and packing are not mutually exclusive, this would need to\n> > become flags, or possibly get moved to a separate enum.\n> >\n> \n> I'll make these flags\n\nActually, you don't seem to use these at the moment. Do you plan to use\nthose flags, or should we leave them out for now ?\n\n> > > \t};\n> > >\n> > > \tconstexpr BayerFormat()\n> > > @@ -43,6 +45,7 @@ public:\n> > > \t}\n> > >\n> > > \texplicit BayerFormat(V4L2PixelFormat v4l2Format);\n> > > +\texplicit BayerFormat(unsigned int mbusCode);\n> > > \tbool isValid() const { return bitDepth != 0; }\n> > >\n> > > \tstd::string toString() const;\n> > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp\n> > > index c42792ff1948..a4eba4592bfe 100644\n> > > --- a/src/libcamera/bayer_format.cpp\n> > > +++ b/src/libcamera/bayer_format.cpp\n> > > @@ -8,6 +8,9 @@\n> > > #include \"libcamera/internal/bayer_format.h\"\n> > >\n> > > #include \n> > > +#include \n> > > +\n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -45,7 +48,8 @@ namespace libcamera {\n> > >\n> > > /**\n> > > * \\enum BayerFormat::Packing\n> > > - * \\brief Different types of packing that can be applied to a BayerFormat\n> > > + * \\brief Different types of packing or compressions that can be applied to a\n> > > + * BayerFormat\n> > > *\n> > > * \\var BayerFormat::None\n> > > * \\brief No packing\n> > > @@ -53,6 +57,10 @@ namespace libcamera {\n> > > * \\brief Format uses MIPI CSI-2 style packing\n> > > * \\var BayerFormat::IPU3Packed\n> > > * \\brief Format uses IPU3 style packing\n> > > + * \\var BayerFormat::ALAW8Compression\n> > > + * \\brief Format uses ALAW8 compression\n> > > + * \\var BayerFormat::DPCM8Compression\n> > > + * \\brief Format uses DPCM8 compression\n> > > */\n> > >\n> > > namespace {\n> > > @@ -140,6 +148,41 @@ const std::map bayerToV4l2{\n> > > \t{ { BayerFormat::RGGB, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) },\n> > > };\n> > >\n> > > +const std::unordered_map mbusCodeToBayer{\n> > > +\t{ MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, { BayerFormat::BGGR, 8, BayerFormat::ALAW8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, { BayerFormat::GBRG, 8, BayerFormat::ALAW8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, { BayerFormat::GRBG, 8, BayerFormat::ALAW8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, { BayerFormat::RGGB, 8, BayerFormat::ALAW8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, { BayerFormat::BGGR, 8, BayerFormat::DPCM8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, { BayerFormat::GBRG, 8, BayerFormat::DPCM8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, { BayerFormat::GRBG, 8, BayerFormat::DPCM8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, { BayerFormat::RGGB, 8, BayerFormat::DPCM8Compression } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },\n> > > +};\n> > > +\n> > > } /* namespace */\n> > >\n> > > /**\n> > > @@ -169,6 +212,22 @@ BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)\n> > > \t\t*this = it->second;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Construct a BayerFormat from a media bus code\n> > > + * \\param[in] mbusCode The media bus code to convert into a BayerFormat\n> > > + *\n> > > + * The media bus code numeric identifiers are defined by the V4L2 specification.\n> > > + */\n> > > +BayerFormat::BayerFormat(unsigned int mbusCode)\n> > > +\t: order(BGGR), packing(None)\n> > > +{\n> > > +\tconst auto it = mbusCodeToBayer.find(mbusCode);\n> > > +\tif (it == mbusCodeToBayer.end())\n> > > +\t\tbitDepth = 0;\n> > > +\telse\n> > > +\t\t*this = it->second;\n> > > +}\n> >\n> > Instead of a constructor, wouldn't a static\n> > BayerFormat::fromMediaBusCode() function be better ? The trouble with a\n> > constructor is that, as it has to take an unsigned int argument (we\n> > don't have a media bus code class), one could write\n> >\n> > \tBayerFormat format(V4L2_PIX_FMT_YUYV);\n> >\n> > and the compiler wouldn't complain. Worse, PixelFormat::operator\n> > uint32_t() isn't explicit, so\n> >\n> > \tPixelFormat pf(...);\n> > \tBayerFormat format(pf);\n> >\n> > will also compile.\n> >\n> > Obviously,\n> >\n> > \tPixelFormat pf(...);\n> > \tBayerFormat format = BayerFormat::fromMediaBusCode(pf);\n> >\n> > will also compile, but it will be easier to spot from the code that\n> > something is wrong.\n> \n> I went with a constructor for simmetry with the existing\n> \texplicit BayerFormat(V4L2PixelFormat v4l2Format);\n> \n> I feel like the whole class should be better reworked to be similar to\n> V4L2PixelFormat that provides a 'toPixelFormat' and one\n> 'fromPixelFormat'.\n\nSeems like a a good idea. We already have\nBayerFormat::toV4L2PixelFormat(), it would make sense to add\nBayerFormat::fromV4L2PixelFormat(). It's not a prerequisite for this\nseries though.\n\n> In the meantime, if a static methods makes it harder to get the\n> constuctor wrong, I'll use that.\n> \n> > > +\n> > > /**\n> > > * \\fn BayerFormat::isValid()\n> > > * \\brief Return whether a BayerFormat is valid\n> > > @@ -258,6 +317,11 @@ BayerFormat BayerFormat::transform(Transform t) const\n> > > /**\n> > > * \\var BayerFormat::bitDepth\n> > > * \\brief The bit depth of the samples in the Bayer pattern\n> > > + *\n> > > + * For formats using compression schemes such as Packing::ALAW8Compression or\n> > > + * Packing::DPCM8Compression, the bitDepth value refers to the size of the\n> > > + * sample when transmitted on the wire or stored into memory. The resulting\n> > > + * images need to be de-compressed to be viewed properly.\n> > > */\n> > >\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 DB6C1C0F1A\n\tfor ;\n\tTue, 22 Dec 2020 16:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 74E496158A;\n\tTue, 22 Dec 2020 17:50:08 +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 AFBF760528\n\tfor ;\n\tTue, 22 Dec 2020 17:50:07 +0100 (CET)","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 1DEA29E6;\n\tTue, 22 Dec 2020 17:50:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aFuOyuke\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608655807;\n\tbh=ysONtnGtlHRhljdXp9kN4A6Xz10Ne0NfPkepQaqo4bs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aFuOyukekjIulh6HTyXaEgix8yQMA/XKX/ku/nmcc+ubLkUbBL9BK/oTqLK0wIN7m\n\t7kvOiDoVLg/vDDoqtaUTzLg/P0MT1b8BbgvYAc0SuZ7bss6LX71WDauTiaLEhwVaMg\n\t68cWsQnmXEhgv3kbYkKNkvktE/Y609X1JN9cVGN0=","Date":"Tue, 22 Dec 2020 18:49:59 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201218164754.81422-1-jacopo@jmondi.org>\n\t<20201218164754.81422-4-jacopo@jmondi.org>\n\t\n\t<20201222125438.y2rmdxy35bectkzw@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201222125438.y2rmdxy35bectkzw@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 3/9] libcamera: bayer_format: Add\n\tsupport for mbus codes","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]