[{"id":4519,"web_url":"https://patchwork.libcamera.org/comment/4519/","msgid":"<20200425202903.GD10975@pendragon.ideasonboard.com>","date":"2020-04-25T20:29:03","subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice:\n\tAdd format information","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, Apr 24, 2020 at 11:52:59PM +0200, Jacopo Mondi wrote:\n> Define a map of static information associated with a media bus code.\n> Start by reporting the bits-per-pixel for each media bus code defined by\n> the V4L2 kernel API, to later expand it when necessary.\n> \n> Add to the V4L2SubdeviceFormat class a method to return the bits per\n> pixel, retrieved by inspecting the static map of format information.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_subdevice.h | 3 +\n> src/libcamera/v4l2_subdevice.cpp | 113 +++++++++++++++++++++++++\n> 2 files changed, 116 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 763ffadb61fb..43953a0a5a6c 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -23,10 +23,13 @@ namespace libcamera {\n> class MediaDevice;\n> \n> struct V4L2SubdeviceFormat {\n> +\tusing infoMap = std::map;\n\nThere's no need to define a type here as it's not used in the API, you\ncan use it directly below.\n\n> +\n> \tuint32_t mbus_code;\n> \tSize size;\n> \n> \tconst std::string toString() const;\n> +\tuint8_t bitsPerPixel() const;\n> };\n> \n> class V4L2Subdevice : public V4L2Device\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 4dcf8ce48754..6f72c363fc7f 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -32,6 +33,92 @@ namespace libcamera {\n> \n> LOG_DECLARE_CATEGORY(V4L2)\n> \n> +namespace {\n> +\n> +/*\n> + * \\var formatInfoMap\n> + * \\brief A map that associates information to V4L2 media bus codes\n> + */\n\nNo need for doxygen doc (that would require /** instead of /*) as it's\npurely internal.\n\n> +const V4L2SubdeviceFormat::infoMap formatInfoMap = {\n\nSo here we would have\n\nconst std::map formatInfoMap = {\n\n> +\t{ V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE, 16 },\n> +\t{ V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE, 16 },\n> +\t{ V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, 16 },\n> +\t{ V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, 16 },\n> +\t{ V4L2_MBUS_FMT_BGR565_2X8_BE, 16 },\n> +\t{ V4L2_MBUS_FMT_BGR565_2X8_LE, 16 },\n> +\t{ V4L2_MBUS_FMT_RGB565_2X8_BE, 16 },\n> +\t{ V4L2_MBUS_FMT_RGB565_2X8_LE, 16 },\n> +\t{ V4L2_MBUS_FMT_RGB666_1X18, 18 },\n> +\t{ V4L2_MBUS_FMT_RGB888_1X24, 24 },\n> +\t{ V4L2_MBUS_FMT_RGB888_2X12_BE, 24 },\n> +\t{ V4L2_MBUS_FMT_RGB888_2X12_LE, 24 },\n> +\t{ V4L2_MBUS_FMT_ARGB8888_1X32, 32 },\n> +\t{ V4L2_MBUS_FMT_Y8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_UV8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_UYVY8_1_5X8, 40 },\n> +\t{ V4L2_MBUS_FMT_VYUY8_1_5X8, 40 },\n> +\t{ V4L2_MBUS_FMT_YUYV8_1_5X8, 40 },\n> +\t{ V4L2_MBUS_FMT_YVYU8_1_5X8, 40 },\n> +\t{ V4L2_MBUS_FMT_UYVY8_2X8, 16 },\n> +\t{ V4L2_MBUS_FMT_VYUY8_2X8, 16 },\n> +\t{ V4L2_MBUS_FMT_YUYV8_2X8, 16 },\n> +\t{ V4L2_MBUS_FMT_YVYU8_2X8, 16 },\n> +\t{ V4L2_MBUS_FMT_Y10_1X10, 10 },\n> +\t{ V4L2_MBUS_FMT_UYVY10_2X10, 20 },\n> +\t{ V4L2_MBUS_FMT_VYUY10_2X10, 20 },\n> +\t{ V4L2_MBUS_FMT_YUYV10_2X10, 20 },\n> +\t{ V4L2_MBUS_FMT_YVYU10_2X10, 20 },\n> +\t{ V4L2_MBUS_FMT_Y12_1X12, 12 },\n> +\t{ V4L2_MBUS_FMT_UYVY8_1X16, 16 },\n> +\t{ V4L2_MBUS_FMT_VYUY8_1X16, 16 },\n> +\t{ V4L2_MBUS_FMT_YUYV8_1X16, 16 },\n> +\t{ V4L2_MBUS_FMT_YVYU8_1X16, 16 },\n> +\t{ V4L2_MBUS_FMT_YDYUYDYV8_1X16, 16 },\n> +\t{ V4L2_MBUS_FMT_UYVY10_1X20, 20 },\n> +\t{ V4L2_MBUS_FMT_VYUY10_1X20, 20 },\n> +\t{ V4L2_MBUS_FMT_YUYV10_1X20, 20 },\n> +\t{ V4L2_MBUS_FMT_YVYU10_1X20, 20 },\n> +\t{ V4L2_MBUS_FMT_YUV10_1X30, 30 },\n> +\t{ V4L2_MBUS_FMT_AYUV8_1X32, 32 },\n> +\t{ V4L2_MBUS_FMT_UYVY12_2X12, 24 },\n> +\t{ V4L2_MBUS_FMT_VYUY12_2X12, 24 },\n> +\t{ V4L2_MBUS_FMT_YUYV12_2X12, 24 },\n> +\t{ V4L2_MBUS_FMT_YVYU12_2X12, 24 },\n> +\t{ V4L2_MBUS_FMT_UYVY12_1X24, 24 },\n> +\t{ V4L2_MBUS_FMT_VYUY12_1X24, 24 },\n> +\t{ V4L2_MBUS_FMT_YUYV12_1X24, 24 },\n> +\t{ V4L2_MBUS_FMT_YVYU12_1X24, 24 },\n> +\t{ V4L2_MBUS_FMT_SBGGR8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SGBRG8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SGRBG8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SRGGB8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, 16 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, 16 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE, 16 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE, 16 },\n> +\t{ V4L2_MBUS_FMT_SBGGR10_1X10, 10 },\n> +\t{ V4L2_MBUS_FMT_SGBRG10_1X10, 10 },\n> +\t{ V4L2_MBUS_FMT_SGRBG10_1X10, 10 },\n> +\t{ V4L2_MBUS_FMT_SRGGB10_1X10, 10 },\n> +\t{ V4L2_MBUS_FMT_SBGGR12_1X12, 24 },\n> +\t{ V4L2_MBUS_FMT_SGBRG12_1X12, 24 },\n> +\t{ V4L2_MBUS_FMT_SGRBG12_1X12, 24 },\n> +\t{ V4L2_MBUS_FMT_SRGGB12_1X12, 24 },\n> +\t{ V4L2_MBUS_FMT_JPEG_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },\n> +\t{ V4L2_MBUS_FMT_AHSV8888_1X32, 32 },\n> +};\n> +\n> +}; /* namespace */\n> +\n> /**\n> * \\struct V4L2SubdeviceFormat\n> * \\brief The V4L2 sub-device image format and sizes\n> @@ -60,6 +147,14 @@ LOG_DECLARE_CATEGORY(V4L2)\n> * Section 4.15.3.1 of the above mentioned Linux kernel documentation section.\n> */\n> \n> +/**\n> + * \\typedef V4L2SubdeviceFormat::infoMap\n> + * \\brief Map of static information associated with a V4L2 subdevice format\n> + *\n> + * \\todo Expand with additional information. At the moment only the\n> + * bits-per-pixel information is reported.\n> + */\n> +\n\nAnd you can drop this too as the type doesn't need to be documented.\nWhat we may document later is a format info structure that would contain\nmore than just the bpp, but there's no need to introduce that yet.\n\n> /**\n> * \\var V4L2SubdeviceFormat::mbus_code\n> * \\brief The image format bus code\n> @@ -81,6 +176,24 @@ const std::string V4L2SubdeviceFormat::toString() const\n> \treturn ss.str();\n> }\n> \n> +/**\n> + * \\brief Retrieve the number of bits per pixel for the V4L2 subdevice format\n> + * \\return The number of bits per pixel for the format, or default it to 8 if\n> + * the format's mbus code is not supported\n\ns/mbus/media bus/\n\n> + */\n> +uint8_t V4L2SubdeviceFormat::bitsPerPixel() const\n> +{\n> +\tconst auto it = formatInfoMap.find(mbus_code);\n> +\tif (it == formatInfoMap.end()) {\n> +\t\tLOG(V4L2, Error) << \"No information available for format: '\"\n\ns/://\n\n> +\t\t\t\t << toString() << \"'\";\n> +\t\t/* Return 8 to avoid divisions by 0. */\n> +\t\treturn 8;\n\nI think we should return 0 and check for this condition in the caller.\nWhen we'll switch to a structure that will contain information about\nformats, the formatInfo() function will return a pointer, which will be\nnull when the format is not known. I would prefer preparing the callers\nfor this already.\n\nWith these small changes,\n\nReviewed-by: Laurent Pinchart \n\n> +\t}\n> +\n> +\treturn it->second;\n> +}\n> +\n> /**\n> * \\class V4L2Subdevice\n> * \\brief A V4L2 subdevice as exposed by the Linux kernel","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0757603FC\n\tfor ;\n\tSat, 25 Apr 2020 22:29:18 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 429924F7;\n\tSat, 25 Apr 2020 22:29:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eH4FAWUg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587846558;\n\tbh=IroZI4Ajks9N6kvnP62HVQD9dIncQ/37UncbRMqaSog=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eH4FAWUg3Ro58i1w97C+KCM2HHgsX3Rq7uCQhxhkPU9Zex00WNATxNb6JwTeNjYFv\n\t0FmL0FzjiylR7GvhAyDV5cs/y520vGTkcmqGYdkaX5PMTKWR7XF+66B3yAUM54anN3\n\t5oYjp+LKDbg9AW/NWpxKcsspg9k0LINXYernSHCk=","Date":"Sat, 25 Apr 2020 23:29:03 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425202903.GD10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-9-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200424215304.558317-9-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice:\n\tAdd format information","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","X-List-Received-Date":"Sat, 25 Apr 2020 20:29:19 -0000"}},{"id":4575,"web_url":"https://patchwork.libcamera.org/comment/4575/","msgid":"<20200427125406.tpfmduckjgkwdiey@uno.localdomain>","date":"2020-04-27T12:54:06","subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice:\n\tAdd format information","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Apr 25, 2020 at 11:29:03PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 24, 2020 at 11:52:59PM +0200, Jacopo Mondi wrote:\n> > Define a map of static information associated with a media bus code.\n> > Start by reporting the bits-per-pixel for each media bus code defined by\n> > the V4L2 kernel API, to later expand it when necessary.\n> >\n> > Add to the V4L2SubdeviceFormat class a method to return the bits per\n> > pixel, retrieved by inspecting the static map of format information.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_subdevice.h | 3 +\n> > src/libcamera/v4l2_subdevice.cpp | 113 +++++++++++++++++++++++++\n> > 2 files changed, 116 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 763ffadb61fb..43953a0a5a6c 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -23,10 +23,13 @@ namespace libcamera {\n> > class MediaDevice;\n> >\n> > struct V4L2SubdeviceFormat {\n> > +\tusing infoMap = std::map;\n>\n> There's no need to define a type here as it's not used in the API, you\n> can use it directly below.\n>\n\nI like to shorten the type though, and it does not actually brings\nmuch obfuscation\n\n> > +\n> > \tuint32_t mbus_code;\n> > \tSize size;\n> >\n> > \tconst std::string toString() const;\n> > +\tuint8_t bitsPerPixel() const;\n> > };\n> >\n> > class V4L2Subdevice : public V4L2Device\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 4dcf8ce48754..6f72c363fc7f 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -32,6 +33,92 @@ namespace libcamera {\n> >\n> > LOG_DECLARE_CATEGORY(V4L2)\n> >\n> > +namespace {\n> > +\n> > +/*\n> > + * \\var formatInfoMap\n> > + * \\brief A map that associates information to V4L2 media bus codes\n> > + */\n>\n> No need for doxygen doc (that would require /** instead of /*) as it's\n> purely internal.\n\nThis is intended. In other places for documentation not parsed by\ndoxygen we decided to maintain the doxygen style for consistency if\nI'm not mistaken\n\n>\n> > +const V4L2SubdeviceFormat::infoMap formatInfoMap = {\n>\n> So here we would have\n>\n> const std::map formatInfoMap = {\n>\n\nmeh. I can drop it, but we used type shortcuts in other places.\n\n> > +\t{ V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE, 16 },\n> > +\t{ V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE, 16 },\n> > +\t{ V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, 16 },\n> > +\t{ V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, 16 },\n> > +\t{ V4L2_MBUS_FMT_BGR565_2X8_BE, 16 },\n> > +\t{ V4L2_MBUS_FMT_BGR565_2X8_LE, 16 },\n> > +\t{ V4L2_MBUS_FMT_RGB565_2X8_BE, 16 },\n> > +\t{ V4L2_MBUS_FMT_RGB565_2X8_LE, 16 },\n> > +\t{ V4L2_MBUS_FMT_RGB666_1X18, 18 },\n> > +\t{ V4L2_MBUS_FMT_RGB888_1X24, 24 },\n> > +\t{ V4L2_MBUS_FMT_RGB888_2X12_BE, 24 },\n> > +\t{ V4L2_MBUS_FMT_RGB888_2X12_LE, 24 },\n> > +\t{ V4L2_MBUS_FMT_ARGB8888_1X32, 32 },\n> > +\t{ V4L2_MBUS_FMT_Y8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_UV8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_UYVY8_1_5X8, 40 },\n> > +\t{ V4L2_MBUS_FMT_VYUY8_1_5X8, 40 },\n> > +\t{ V4L2_MBUS_FMT_YUYV8_1_5X8, 40 },\n> > +\t{ V4L2_MBUS_FMT_YVYU8_1_5X8, 40 },\n> > +\t{ V4L2_MBUS_FMT_UYVY8_2X8, 16 },\n> > +\t{ V4L2_MBUS_FMT_VYUY8_2X8, 16 },\n> > +\t{ V4L2_MBUS_FMT_YUYV8_2X8, 16 },\n> > +\t{ V4L2_MBUS_FMT_YVYU8_2X8, 16 },\n> > +\t{ V4L2_MBUS_FMT_Y10_1X10, 10 },\n> > +\t{ V4L2_MBUS_FMT_UYVY10_2X10, 20 },\n> > +\t{ V4L2_MBUS_FMT_VYUY10_2X10, 20 },\n> > +\t{ V4L2_MBUS_FMT_YUYV10_2X10, 20 },\n> > +\t{ V4L2_MBUS_FMT_YVYU10_2X10, 20 },\n> > +\t{ V4L2_MBUS_FMT_Y12_1X12, 12 },\n> > +\t{ V4L2_MBUS_FMT_UYVY8_1X16, 16 },\n> > +\t{ V4L2_MBUS_FMT_VYUY8_1X16, 16 },\n> > +\t{ V4L2_MBUS_FMT_YUYV8_1X16, 16 },\n> > +\t{ V4L2_MBUS_FMT_YVYU8_1X16, 16 },\n> > +\t{ V4L2_MBUS_FMT_YDYUYDYV8_1X16, 16 },\n> > +\t{ V4L2_MBUS_FMT_UYVY10_1X20, 20 },\n> > +\t{ V4L2_MBUS_FMT_VYUY10_1X20, 20 },\n> > +\t{ V4L2_MBUS_FMT_YUYV10_1X20, 20 },\n> > +\t{ V4L2_MBUS_FMT_YVYU10_1X20, 20 },\n> > +\t{ V4L2_MBUS_FMT_YUV10_1X30, 30 },\n> > +\t{ V4L2_MBUS_FMT_AYUV8_1X32, 32 },\n> > +\t{ V4L2_MBUS_FMT_UYVY12_2X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_VYUY12_2X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_YUYV12_2X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_YVYU12_2X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_UYVY12_1X24, 24 },\n> > +\t{ V4L2_MBUS_FMT_VYUY12_1X24, 24 },\n> > +\t{ V4L2_MBUS_FMT_YUYV12_1X24, 24 },\n> > +\t{ V4L2_MBUS_FMT_YVYU12_1X24, 24 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SGBRG8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SGRBG8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SRGGB8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, 16 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, 16 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE, 16 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE, 16 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR10_1X10, 10 },\n> > +\t{ V4L2_MBUS_FMT_SGBRG10_1X10, 10 },\n> > +\t{ V4L2_MBUS_FMT_SGRBG10_1X10, 10 },\n> > +\t{ V4L2_MBUS_FMT_SRGGB10_1X10, 10 },\n> > +\t{ V4L2_MBUS_FMT_SBGGR12_1X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_SGBRG12_1X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_SGRBG12_1X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_SRGGB12_1X12, 24 },\n> > +\t{ V4L2_MBUS_FMT_JPEG_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_AHSV8888_1X32, 32 },\n> > +};\n> > +\n> > +}; /* namespace */\n> > +\n> > /**\n> > * \\struct V4L2SubdeviceFormat\n> > * \\brief The V4L2 sub-device image format and sizes\n> > @@ -60,6 +147,14 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > * Section 4.15.3.1 of the above mentioned Linux kernel documentation section.\n> > */\n> >\n> > +/**\n> > + * \\typedef V4L2SubdeviceFormat::infoMap\n> > + * \\brief Map of static information associated with a V4L2 subdevice format\n> > + *\n> > + * \\todo Expand with additional information. At the moment only the\n> > + * bits-per-pixel information is reported.\n> > + */\n> > +\n>\n> And you can drop this too as the type doesn't need to be documented.\n> What we may document later is a format info structure that would contain\n> more than just the bpp, but there's no need to introduce that yet.\n>\n\nDoes it hurt ?\n\n> > /**\n> > * \\var V4L2SubdeviceFormat::mbus_code\n> > * \\brief The image format bus code\n> > @@ -81,6 +176,24 @@ const std::string V4L2SubdeviceFormat::toString() const\n> > \treturn ss.str();\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve the number of bits per pixel for the V4L2 subdevice format\n> > + * \\return The number of bits per pixel for the format, or default it to 8 if\n> > + * the format's mbus code is not supported\n>\n> s/mbus/media bus/\n>\n> > + */\n> > +uint8_t V4L2SubdeviceFormat::bitsPerPixel() const\n> > +{\n> > +\tconst auto it = formatInfoMap.find(mbus_code);\n> > +\tif (it == formatInfoMap.end()) {\n> > +\t\tLOG(V4L2, Error) << \"No information available for format: '\"\n>\n> s/://\n>\n> > +\t\t\t\t << toString() << \"'\";\n> > +\t\t/* Return 8 to avoid divisions by 0. */\n> > +\t\treturn 8;\n>\n> I think we should return 0 and check for this condition in the caller.\n> When we'll switch to a structure that will contain information about\n> formats, the formatInfo() function will return a pointer, which will be\n> null when the format is not known. I would prefer preparing the callers\n> for this already.\n\nAck\n\nThanks\n j\n\n>\n> With these small changes,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\t}\n> > +\n> > +\treturn it->second;\n> > +}\n> > +\n> > /**\n> > * \\class V4L2Subdevice\n> > * \\brief A V4L2 subdevice as exposed by the Linux kernel\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B168603F9\n\tfor ;\n\tMon, 27 Apr 2020 14:50:56 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 79478FF80F;\n\tMon, 27 Apr 2020 12:50:55 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Mon, 27 Apr 2020 14:54:06 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427125406.tpfmduckjgkwdiey@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-9-jacopo@jmondi.org>\n\t<20200425202903.GD10975@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425202903.GD10975@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice:\n\tAdd format information","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","X-List-Received-Date":"Mon, 27 Apr 2020 12:50:56 -0000"}},{"id":4576,"web_url":"https://patchwork.libcamera.org/comment/4576/","msgid":"<20200427125607.GG10040@pendragon.ideasonboard.com>","date":"2020-04-27T12:56:07","subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice:\n\tAdd format information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 27, 2020 at 02:54:06PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 25, 2020 at 11:29:03PM +0300, Laurent Pinchart wrote:\n> > On Fri, Apr 24, 2020 at 11:52:59PM +0200, Jacopo Mondi wrote:\n> > > Define a map of static information associated with a media bus code.\n> > > Start by reporting the bits-per-pixel for each media bus code defined by\n> > > the V4L2 kernel API, to later expand it when necessary.\n> > >\n> > > Add to the V4L2SubdeviceFormat class a method to return the bits per\n> > > pixel, retrieved by inspecting the static map of format information.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/include/v4l2_subdevice.h | 3 +\n> > > src/libcamera/v4l2_subdevice.cpp | 113 +++++++++++++++++++++++++\n> > > 2 files changed, 116 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > > index 763ffadb61fb..43953a0a5a6c 100644\n> > > --- a/src/libcamera/include/v4l2_subdevice.h\n> > > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > > @@ -23,10 +23,13 @@ namespace libcamera {\n> > > class MediaDevice;\n> > >\n> > > struct V4L2SubdeviceFormat {\n> > > +\tusing infoMap = std::map;\n> >\n> > There's no need to define a type here as it's not used in the API, you\n> > can use it directly below.\n> \n> I like to shorten the type though, and it does not actually brings\n> much obfuscation\n\nThat's fine, but then please move it to v4l2_subdev.cpp. Otherwise (as\nyou've noticed) you have to document it, and documenting a type that\nisn't part of the API is confusing for documentation readers.\n\n> > > +\n> > > \tuint32_t mbus_code;\n> > > \tSize size;\n> > >\n> > > \tconst std::string toString() const;\n> > > +\tuint8_t bitsPerPixel() const;\n> > > };\n> > >\n> > > class V4L2Subdevice : public V4L2Device\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 4dcf8ce48754..6f72c363fc7f 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -14,6 +14,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -32,6 +33,92 @@ namespace libcamera {\n> > >\n> > > LOG_DECLARE_CATEGORY(V4L2)\n> > >\n> > > +namespace {\n> > > +\n> > > +/*\n> > > + * \\var formatInfoMap\n> > > + * \\brief A map that associates information to V4L2 media bus codes\n> > > + */\n> >\n> > No need for doxygen doc (that would require /** instead of /*) as it's\n> > purely internal.\n> \n> This is intended. In other places for documentation not parsed by\n> doxygen we decided to maintain the doxygen style for consistency if\n> I'm not mistaken\n\nOK, I thought it wasn't intentional. You can drop the \\var though, as\nthis is documenting the next line, and in that case I wonder if \\brief\nis better than\n\n/* A map that associates information to V4L2 media bus codes. */\n\n:-)\n\n> > > +const V4L2SubdeviceFormat::infoMap formatInfoMap = {\n> >\n> > So here we would have\n> >\n> > const std::map formatInfoMap = {\n> \n> meh. I can drop it, but we used type shortcuts in other places.\n\nWe do when they're part of the API, or used in multiple places, or very\nlong I think. I'm fine keeping it, but I wonder if\n\nusing infoMap = std::map;\nconst V4L2SubdeviceFormat::infoMap formatInfoMap = {\n\nis really more readable.\n\n> > > +\t{ V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_BGR565_2X8_BE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_BGR565_2X8_LE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_RGB565_2X8_BE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_RGB565_2X8_LE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_RGB666_1X18, 18 },\n> > > +\t{ V4L2_MBUS_FMT_RGB888_1X24, 24 },\n> > > +\t{ V4L2_MBUS_FMT_RGB888_2X12_BE, 24 },\n> > > +\t{ V4L2_MBUS_FMT_RGB888_2X12_LE, 24 },\n> > > +\t{ V4L2_MBUS_FMT_ARGB8888_1X32, 32 },\n> > > +\t{ V4L2_MBUS_FMT_Y8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_UV8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY8_1_5X8, 40 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY8_1_5X8, 40 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV8_1_5X8, 40 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU8_1_5X8, 40 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY8_2X8, 16 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY8_2X8, 16 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV8_2X8, 16 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU8_2X8, 16 },\n> > > +\t{ V4L2_MBUS_FMT_Y10_1X10, 10 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY10_2X10, 20 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY10_2X10, 20 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV10_2X10, 20 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU10_2X10, 20 },\n> > > +\t{ V4L2_MBUS_FMT_Y12_1X12, 12 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY8_1X16, 16 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY8_1X16, 16 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV8_1X16, 16 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU8_1X16, 16 },\n> > > +\t{ V4L2_MBUS_FMT_YDYUYDYV8_1X16, 16 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY10_1X20, 20 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY10_1X20, 20 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV10_1X20, 20 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU10_1X20, 20 },\n> > > +\t{ V4L2_MBUS_FMT_YUV10_1X30, 30 },\n> > > +\t{ V4L2_MBUS_FMT_AYUV8_1X32, 32 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY12_2X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY12_2X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV12_2X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU12_2X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_UYVY12_1X24, 24 },\n> > > +\t{ V4L2_MBUS_FMT_VYUY12_1X24, 24 },\n> > > +\t{ V4L2_MBUS_FMT_YUYV12_1X24, 24 },\n> > > +\t{ V4L2_MBUS_FMT_YVYU12_1X24, 24 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SGBRG8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SGRBG8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SRGGB8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE, 16 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR10_1X10, 10 },\n> > > +\t{ V4L2_MBUS_FMT_SGBRG10_1X10, 10 },\n> > > +\t{ V4L2_MBUS_FMT_SGRBG10_1X10, 10 },\n> > > +\t{ V4L2_MBUS_FMT_SRGGB10_1X10, 10 },\n> > > +\t{ V4L2_MBUS_FMT_SBGGR12_1X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_SGBRG12_1X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_SGRBG12_1X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_SRGGB12_1X12, 24 },\n> > > +\t{ V4L2_MBUS_FMT_JPEG_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_AHSV8888_1X32, 32 },\n> > > +};\n> > > +\n> > > +}; /* namespace */\n> > > +\n> > > /**\n> > > * \\struct V4L2SubdeviceFormat\n> > > * \\brief The V4L2 sub-device image format and sizes\n> > > @@ -60,6 +147,14 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > > * Section 4.15.3.1 of the above mentioned Linux kernel documentation section.\n> > > */\n> > >\n> > > +/**\n> > > + * \\typedef V4L2SubdeviceFormat::infoMap\n> > > + * \\brief Map of static information associated with a V4L2 subdevice format\n> > > + *\n> > > + * \\todo Expand with additional information. At the moment only the\n> > > + * bits-per-pixel information is reported.\n> > > + */\n> > > +\n> >\n> > And you can drop this too as the type doesn't need to be documented.\n> > What we may document later is a format info structure that would contain\n> > more than just the bpp, but there's no need to introduce that yet.\n> \n> Does it hurt ?\n\nAs it's purely internal, I think it can be confusing.\n\n> > > /**\n> > > * \\var V4L2SubdeviceFormat::mbus_code\n> > > * \\brief The image format bus code\n> > > @@ -81,6 +176,24 @@ const std::string V4L2SubdeviceFormat::toString() const\n> > > \treturn ss.str();\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the number of bits per pixel for the V4L2 subdevice format\n> > > + * \\return The number of bits per pixel for the format, or default it to 8 if\n> > > + * the format's mbus code is not supported\n> >\n> > s/mbus/media bus/\n> >\n> > > + */\n> > > +uint8_t V4L2SubdeviceFormat::bitsPerPixel() const\n> > > +{\n> > > +\tconst auto it = formatInfoMap.find(mbus_code);\n> > > +\tif (it == formatInfoMap.end()) {\n> > > +\t\tLOG(V4L2, Error) << \"No information available for format: '\"\n> >\n> > s/://\n> >\n> > > +\t\t\t\t << toString() << \"'\";\n> > > +\t\t/* Return 8 to avoid divisions by 0. */\n> > > +\t\treturn 8;\n> >\n> > I think we should return 0 and check for this condition in the caller.\n> > When we'll switch to a structure that will contain information about\n> > formats, the formatInfo() function will return a pointer, which will be\n> > null when the format is not known. I would prefer preparing the callers\n> > for this already.\n> \n> Ack\n> \n> > With these small changes,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > +\t}\n> > > +\n> > > +\treturn it->second;\n> > > +}\n> > > +\n> > > /**\n> > > * \\class V4L2Subdevice\n> > > * \\brief A V4L2 subdevice as exposed by the Linux kernel","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D8AB603F9\n\tfor ;\n\tMon, 27 Apr 2020 14:56:23 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABBCD72C;\n\tMon, 27 Apr 2020 14:56:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"orO+UYBN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587992182;\n\tbh=GmFQQWxGCp+h+JA+iDy6L4w8sHUBXVA7NmShx7PzDKk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=orO+UYBN5c02WmTs/r36SYSo+Sv2jSguV6v7hdID1YgLDFxqji99tdXdY/JqD1Wbn\n\tFIhsUWm4GDov0r9B6mUz61wEFYF3iff4k+z+GP+qW1VJsy0e7qoUpBP0BeKkvLLBBx\n\tJiMqe16cnmxnwjMdGhQADX29Pn88cEWZjsrMdGDI=","Date":"Mon, 27 Apr 2020 15:56:07 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427125607.GG10040@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-9-jacopo@jmondi.org>\n\t<20200425202903.GD10975@pendragon.ideasonboard.com>\n\t<20200427125406.tpfmduckjgkwdiey@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200427125406.tpfmduckjgkwdiey@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice:\n\tAdd format information","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","X-List-Received-Date":"Mon, 27 Apr 2020 12:56:23 -0000"}}]