[{"id":4596,"web_url":"https://patchwork.libcamera.org/comment/4596/","msgid":"<20200428000634.GI1165729@oden.dyn.berto.se>","date":"2020-04-28T00:06:34","subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat information","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-04-27 23:32:34 +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> While at there, remove a rouge map inclusion from header file.\n> \n> Signed-off-by: Jacopo Mondi \n> \n> ---\n> src/libcamera/include/v4l2_subdevice.h | 2 +-\n> src/libcamera/v4l2_subdevice.cpp | 104 +++++++++++++++++++++++++\n> 2 files changed, 105 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 27ba5b17f61e..d0e565dbdaab 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -7,7 +7,6 @@\n> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> \n> -#include \n> #include \n> #include \n> \n> @@ -27,6 +26,7 @@ struct V4L2SubdeviceFormat {\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 74788ce7cf4f..93fe4b8c3d32 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 bits per pixel to V4L2 media bus codes\n> + */\n> +const std::map formatInfoMap = {\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> @@ -81,6 +168,23 @@ 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 media bus code is not supported\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> +\t\t\t\t << toString() << \"'\";\n> +\t\treturn 0;\n\nThis does not match the documentation of defaulting to 8 if the format \nis not found. I'm wondering however if we shall not make the error fatal \nas a default value that is wrong could lead to really odd behaviors.\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> 2.26.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 29049603FB\n\tfor ;\n\tTue, 28 Apr 2020 02:06:36 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id b2so19521042ljp.4\n\tfor ;\n\tMon, 27 Apr 2020 17:06:36 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tt24sm14055905lfk.90.2020.04.27.17.06.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 27 Apr 2020 17:06:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"CSkoDBBQ\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=csPHYjL06gcWSDRBFZccFMELDWHEKxV5VceupZzyrkQ=;\n\tb=CSkoDBBQC+sm9scsdvbGvl2wYiiNv+P8rgdKviZSw6nIuG5X5ucOru4JvBnHlxY6hN\n\tHf9cdtc6a6hxmEvTqSSZ1rdrEk1kV6Fpby/8CcrpXwPhdc078mRr1tl12bT40ctRjjbA\n\tIXxBGL6h2e4v16iWs5RMWUkXJMsW74AWddXtmxrBr+o4okUhh0oy3q1AAAdI1LWYUtSC\n\tY7cWNfMooOCRpyerRbz6Bre6AbDU8tOBt8KWdI8UrtPzpnDBBHuBX07QfuvCpozUDZXe\n\t9Ji4rHICfvGfFI/E3WMV3/xTVrrJ2B3cUxJatX5McY2K3l3qZk//oPbc5ML2L5RU/2I1\n\tC3sQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=csPHYjL06gcWSDRBFZccFMELDWHEKxV5VceupZzyrkQ=;\n\tb=CRNuBfbn5QKFfGzflWpd5TUV50r6sfEIqv1zB0k8BIXtdbjBb6wZ7kbDIS3RQ+ULeC\n\tj2ZFcPIYoQA4IhZiuystcoXMv6Sy+V2s/kTjTpGrLZeAecjN5eccki83jqhlwhUIBEfW\n\t5PNb87wdxB48Nu7B1CZ98S3Mxc9rCcjNvMO2qx2Sv9e4CR272j9mMtyt0d9cx6pwp92M\n\tTUJ3Fn+YSilXV+fVGDtABGUOzrdfigh8t9xEdojGx+hXBSb/qgtq1arx/ZkTXP94vVmK\n\tQKK93pdNR3RFZcCARv0mWmfspUr4cE2TTR29hSWu4jm1igCPGYf9iKm8A5GiGhbkdB1z\n\t9bfw==","X-Gm-Message-State":"AGi0PuZwoypXMQSm4zyTMmRMLJ2fFKvLbsC7KhWjuGURxFSkUYQcmOVM\n\tsgtK45MDKGS1ikvKva3C13Nhpr3lKqE=","X-Google-Smtp-Source":"APiQypLxJ7UiTGzPiv7FbPrwUiM6LkxpvTQgnFUV6DRf63Xqwxp61x+Fm9g/IgDiXdF8Su0PjK+Bcw==","X-Received":"by 2002:a2e:a37b:: with SMTP id\n\ti27mr15001049ljn.36.1588032395442; \n\tMon, 27 Apr 2020 17:06:35 -0700 (PDT)","Date":"Tue, 28 Apr 2020 02:06:34 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200428000634.GI1165729@oden.dyn.berto.se>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200427213236.333777-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat 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":"Tue, 28 Apr 2020 00:06:36 -0000"}},{"id":4602,"web_url":"https://patchwork.libcamera.org/comment/4602/","msgid":"<20200428020123.GE3579@pendragon.ideasonboard.com>","date":"2020-04-28T02:01:23","subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Niklas,\n\nOn Tue, Apr 28, 2020 at 02:06:34AM +0200, Niklas Söderlund wrote:\n> On 2020-04-27 23:32:34 +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> > While at there, remove a rouge map inclusion from header file.\n\ns/there/it/ (or \"while there\")\ns/rouge/rogue/ (unless you really mean red :-))\n\n> > \n> > Signed-off-by: Jacopo Mondi \n> > \n> > ---\n> > src/libcamera/include/v4l2_subdevice.h | 2 +-\n> > src/libcamera/v4l2_subdevice.cpp | 104 +++++++++++++++++++++++++\n> > 2 files changed, 105 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 27ba5b17f61e..d0e565dbdaab 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -7,7 +7,6 @@\n> > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > \n> > -#include \n> > #include \n> > #include \n> > \n> > @@ -27,6 +26,7 @@ struct V4L2SubdeviceFormat {\n> > \tSize size;\n> > \n> > \tconst std::string toString() const;\n> > +\tuint8_t bitsPerPixel() const;\n\nI think unsigned int would be more efficient (or at least not less). It\nwould also avoid interesting issues when writing\n\n\tLOG(Info) << \"bits per pixel: \" << format.bitsPerPixel();\n\nas with uint8_t it will be considered as a char and not a number (I\nknow...).\n\n> > };\n> > \n> > class V4L2Subdevice : public V4L2Device\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 74788ce7cf4f..93fe4b8c3d32 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 bits per pixel to V4L2 media bus codes\n> > + */\n> > +const std::map formatInfoMap = {\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\n1_5X8 means 1.5x8 = 12 :-)\n\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\nThese last four tshould be 12 bits per pixel.\n\n> > +\t{ V4L2_MBUS_FMT_JPEG_1X8, 8 },\n> > +\t{ V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },\n\nI'd drop these two as bits per pixel is really ill-defined for JPEG.\n\nI think the bits per pixel concept is ill-defined in general. Our\ncurrent use case for this is limited to Bayer formats, to know the\ndynamic range of the data. When transmitting, let's say,\nV4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, I can imagine the pipeline handler\npossibly being interested in knowing that the bus width is 16 bits in\norder to configure the pipeline, and the IPA being interested in knowing\nthe dynamic range is 10 bits. We'll have to revisiti all this later,\nthere's no reason to block this patch for this, but please remember this\nissue in the future as I'll want to refactor this before getting more\nusers of this API.\n\n> > +\t{ V4L2_MBUS_FMT_AHSV8888_1X32, 32 },\n> > +};\n> > +\n> > +}; /* namespace */\n\ns/};/}/\n\n> > +\n> > /**\n> > * \\struct V4L2SubdeviceFormat\n> > * \\brief The V4L2 sub-device image format and sizes\n> > @@ -81,6 +168,23 @@ 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 media bus code is not supported\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> > +\t\t\t\t << toString() << \"'\";\n> > +\t\treturn 0;\n> \n> This does not match the documentation of defaulting to 8 if the format \n> is not found. I'm wondering however if we shall not make the error fatal \n> as a default value that is wrong could lead to really odd behaviors.\n\nThe code was updated but the documentation stayed as-is. s/or default it\nto 8/or 0/.\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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F3B1603F5\n\tfor ;\n\tTue, 28 Apr 2020 04:01:39 +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 E897972C;\n\tTue, 28 Apr 2020 04:01:38 +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=\"cMOesK0N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588039299;\n\tbh=O9nDcauzXNmOPinH//td0daofQNdYeX+DYGkGzcNGPU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cMOesK0NUsnFr3GTzK5n3sy8AxSAt1aB5lb/YZGThWB4aBeUnhKVG4ss8bbJJVC7U\n\tkSrTleV15C06LXHOztqamRAxPUJpFFjTY32yzDC9Enk9APBZyY8q4leo2MfN51TLlc\n\tiDeeJDsSUUAuiEYKQJCK3udzqgcKx8dlml6Ujb7A=","Date":"Tue, 28 Apr 2020 05:01:23 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200428020123.GE3579@pendragon.ideasonboard.com>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-6-jacopo@jmondi.org>\n\t<20200428000634.GI1165729@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200428000634.GI1165729@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat 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":"Tue, 28 Apr 2020 02:01:39 -0000"}},{"id":4610,"web_url":"https://patchwork.libcamera.org/comment/4610/","msgid":"<20200428071848.hnbdqwf7zmlbrmui@uno.localdomain>","date":"2020-04-28T07:18:48","subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat information","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n\nOn Tue, Apr 28, 2020 at 05:01:23AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo and Niklas,\n>\n> On Tue, Apr 28, 2020 at 02:06:34AM +0200, Niklas Söderlund wrote:\n> > On 2020-04-27 23:32:34 +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> > > While at there, remove a rouge map inclusion from header file.\n>\n> s/there/it/ (or \"while there\")\n> s/rouge/rogue/ (unless you really mean red :-))\n>\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > >\n> > > ---\n> > > src/libcamera/include/v4l2_subdevice.h | 2 +-\n> > > src/libcamera/v4l2_subdevice.cpp | 104 +++++++++++++++++++++++++\n> > > 2 files changed, 105 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > > index 27ba5b17f61e..d0e565dbdaab 100644\n> > > --- a/src/libcamera/include/v4l2_subdevice.h\n> > > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > > @@ -7,7 +7,6 @@\n> > > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > > #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > >\n> > > -#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -27,6 +26,7 @@ struct V4L2SubdeviceFormat {\n> > > \tSize size;\n> > >\n> > > \tconst std::string toString() const;\n> > > +\tuint8_t bitsPerPixel() const;\n>\n> I think unsigned int would be more efficient (or at least not less). It\n> would also avoid interesting issues when writing\n>\n> \tLOG(Info) << \"bits per pixel: \" << format.bitsPerPixel();\n>\n> as with uint8_t it will be considered as a char and not a number (I\n> know...).\n\nI stumbled into this a few days ago, yes, I know :(\n\nI can change this to unsigned int, sure\n\n>\n> > > };\n> > >\n> > > class V4L2Subdevice : public V4L2Device\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 74788ce7cf4f..93fe4b8c3d32 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 bits per pixel to V4L2 media bus codes\n> > > + */\n> > > +const std::map formatInfoMap = {\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>\n> 1_5X8 means 1.5x8 = 12 :-)\n>\n\nAh!\n\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>\n> These last four tshould be 12 bits per pixel.\n>\n\nOuch, sorry, I re-checked this twice but I've missed this it seems\n\n> > > +\t{ V4L2_MBUS_FMT_JPEG_1X8, 8 },\n> > > +\t{ V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },\n>\n> I'd drop these two as bits per pixel is really ill-defined for JPEG.\n>\n\nAck\n\n> I think the bits per pixel concept is ill-defined in general. Our\n> current use case for this is limited to Bayer formats, to know the\n> dynamic range of the data. When transmitting, let's say,\n> V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, I can imagine the pipeline handler\n> possibly being interested in knowing that the bus width is 16 bits in\n> order to configure the pipeline, and the IPA being interested in knowing\n> the dynamic range is 10 bits. We'll have to revisiti all this later,\n> there's no reason to block this patch for this, but please remember this\n> issue in the future as I'll want to refactor this before getting more\n> users of this API.\n\nI agree... should we start thinking of a 'sample width' vs 'bus width'\n?\n\n>\n> > > +\t{ V4L2_MBUS_FMT_AHSV8888_1X32, 32 },\n> > > +};\n> > > +\n> > > +}; /* namespace */\n>\n> s/};/}/\n>\n> > > +\n> > > /**\n> > > * \\struct V4L2SubdeviceFormat\n> > > * \\brief The V4L2 sub-device image format and sizes\n> > > @@ -81,6 +168,23 @@ 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 media bus code is not supported\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> > > +\t\t\t\t << toString() << \"'\";\n> > > +\t\treturn 0;\n> >\n> > This does not match the documentation of defaulting to 8 if the format\n> > is not found. I'm wondering however if we shall not make the error fatal\n> > as a default value that is wrong could lead to really odd behaviors.\n>\n> The code was updated but the documentation stayed as-is. s/or default it\n> to 8/or 0/.\n\nYeah, leftover\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nThanks\n j\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 relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9345D603F7\n\tfor ;\n\tTue, 28 Apr 2020 09:15:39 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 2AE2C60002;\n\tTue, 28 Apr 2020 07:15:37 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Tue, 28 Apr 2020 09:18:48 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200428071848.hnbdqwf7zmlbrmui@uno.localdomain>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-6-jacopo@jmondi.org>\n\t<20200428000634.GI1165729@oden.dyn.berto.se>\n\t<20200428020123.GE3579@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200428020123.GE3579@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat 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":"Tue, 28 Apr 2020 07:15:39 -0000"}},{"id":4615,"web_url":"https://patchwork.libcamera.org/comment/4615/","msgid":"<20200428112054.GD5859@pendragon.ideasonboard.com>","date":"2020-04-28T11:20:54","subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat 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 Tue, Apr 28, 2020 at 09:18:48AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 28, 2020 at 05:01:23AM +0300, Laurent Pinchart wrote:\n> > On Tue, Apr 28, 2020 at 02:06:34AM +0200, Niklas Söderlund wrote:\n> > > On 2020-04-27 23:32:34 +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> > > > While at there, remove a rouge map inclusion from header file.\n> >\n> > s/there/it/ (or \"while there\")\n> > s/rouge/rogue/ (unless you really mean red :-))\n> >\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > >\n> > > > ---\n> > > > src/libcamera/include/v4l2_subdevice.h | 2 +-\n> > > > src/libcamera/v4l2_subdevice.cpp | 104 +++++++++++++++++++++++++\n> > > > 2 files changed, 105 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > > > index 27ba5b17f61e..d0e565dbdaab 100644\n> > > > --- a/src/libcamera/include/v4l2_subdevice.h\n> > > > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > > > @@ -7,7 +7,6 @@\n> > > > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > > > #define __LIBCAMERA_V4L2_SUBDEVICE_H__\n> > > >\n> > > > -#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -27,6 +26,7 @@ struct V4L2SubdeviceFormat {\n> > > > \tSize size;\n> > > >\n> > > > \tconst std::string toString() const;\n> > > > +\tuint8_t bitsPerPixel() const;\n> >\n> > I think unsigned int would be more efficient (or at least not less). It\n> > would also avoid interesting issues when writing\n> >\n> > \tLOG(Info) << \"bits per pixel: \" << format.bitsPerPixel();\n> >\n> > as with uint8_t it will be considered as a char and not a number (I\n> > know...).\n> \n> I stumbled into this a few days ago, yes, I know :(\n> \n> I can change this to unsigned int, sure\n> \n> > > > };\n> > > >\n> > > > class V4L2Subdevice : public V4L2Device\n> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > > index 74788ce7cf4f..93fe4b8c3d32 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 bits per pixel to V4L2 media bus codes\n> > > > + */\n> > > > +const std::map formatInfoMap = {\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> >\n> > 1_5X8 means 1.5x8 = 12 :-)\n> \n> Ah!\n> \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> >\n> > These last four tshould be 12 bits per pixel.\n> \n> Ouch, sorry, I re-checked this twice but I've missed this it seems\n> \n> > > > +\t{ V4L2_MBUS_FMT_JPEG_1X8, 8 },\n> > > > +\t{ V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },\n> >\n> > I'd drop these two as bits per pixel is really ill-defined for JPEG.\n> \n> Ack\n> \n> > I think the bits per pixel concept is ill-defined in general. Our\n> > current use case for this is limited to Bayer formats, to know the\n> > dynamic range of the data. When transmitting, let's say,\n> > V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, I can imagine the pipeline handler\n> > possibly being interested in knowing that the bus width is 16 bits in\n> > order to configure the pipeline, and the IPA being interested in knowing\n> > the dynamic range is 10 bits. We'll have to revisiti all this later,\n> > there's no reason to block this patch for this, but please remember this\n> > issue in the future as I'll want to refactor this before getting more\n> > users of this API.\n> \n> I agree... should we start thinking of a 'sample width' vs 'bus width'\n> ?\n\nThat's the idea, yes. I'm sure we'll add more parameters later. Your\npatch is a good first step :-)\n\n> > > > +\t{ V4L2_MBUS_FMT_AHSV8888_1X32, 32 },\n> > > > +};\n> > > > +\n> > > > +}; /* namespace */\n> >\n> > s/};/}/\n> >\n> > > > +\n> > > > /**\n> > > > * \\struct V4L2SubdeviceFormat\n> > > > * \\brief The V4L2 sub-device image format and sizes\n> > > > @@ -81,6 +168,23 @@ 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 media bus code is not supported\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> > > > +\t\t\t\t << toString() << \"'\";\n> > > > +\t\treturn 0;\n> > >\n> > > This does not match the documentation of defaulting to 8 if the format\n> > > is not found. I'm wondering however if we shall not make the error fatal\n> > > as a default value that is wrong could lead to really odd behaviors.\n> >\n> > The code was updated but the documentation stayed as-is. s/or default it\n> > to 8/or 0/.\n> \n> Yeah, leftover\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 1597360AF5\n\tfor ;\n\tTue, 28 Apr 2020 13:21:11 +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 759CE72C;\n\tTue, 28 Apr 2020 13:21:10 +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=\"EgdMqB1F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588072870;\n\tbh=B4TYOKNdbZZNtNBw1fvhI8PMoxy13OrHCyJwSY2azVA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EgdMqB1FYUTx9/MleysO60fN+x8qzeWOdtjd/eKFsvSBgYPI6LQz9dCkJKNpborow\n\tNS+2byNtY+mr/sT3m38YxezKxfZxSEiixki8kkY5ZGZGSFIoqbetymnoqwxYwMnQjC\n\t9GJsr3vuCKwkiPjlnXE7iOd/BvSwWzNsSpsiklB8=","Date":"Tue, 28 Apr 2020 14:20:54 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200428112054.GD5859@pendragon.ideasonboard.com>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-6-jacopo@jmondi.org>\n\t<20200428000634.GI1165729@oden.dyn.berto.se>\n\t<20200428020123.GE3579@pendragon.ideasonboard.com>\n\t<20200428071848.hnbdqwf7zmlbrmui@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200428071848.hnbdqwf7zmlbrmui@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add\n\tformat 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":"Tue, 28 Apr 2020 11:21:11 -0000"}}]