[{"id":4083,"web_url":"https://patchwork.libcamera.org/comment/4083/","msgid":"<20200319000211.GC3042581@oden.dyn.berto.se>","date":"2020-03-19T00:02:11","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add\n\tV4L2PixelFormat class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2020-03-17 01:46:48 +0200, Laurent Pinchart wrote:\n> The V4L2PixelFormat class describes the pixel format of a V4L2 buffer.\n> It wraps the V4L2 numerical FourCC, and shall be used in all APIs that\n> deal with V4L2 pixel formats. Its purpose is to prevent unintentional\n> confusion of V4L2 and DRM FourCCs in code by catching implicit\n> conversion attempts at compile time.\n> \n> The constructor taking a V4L2 FourCC integer value will be made explicit\n> in a further commit to minimize the size of this change and keep it\n> reviewable.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++---\n> src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++-----\n> 2 files changed, 79 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index b3000f3c5133..49d2ca357efa 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -149,10 +149,31 @@ private:\n> \tunsigned int missCounter_;\n> };\n> \n> +class V4L2PixelFormat\n> +{\n> +public:\n> +\tV4L2PixelFormat()\n> +\t\t: fourcc_(0)\n> +\t{\n> +\t}\n> +\n> +\tV4L2PixelFormat(uint32_t fourcc)\n> +\t\t: fourcc_(fourcc)\n> +\t{\n> +\t}\n> +\n> +\tbool isValid() const { return fourcc_ != 0; }\n> +\tuint32_t value() const { return fourcc_; }\n> +\toperator uint32_t() const { return fourcc_; }\n> +\n> +private:\n> +\tuint32_t fourcc_;\n> +};\n> +\n> class V4L2DeviceFormat\n> {\n> public:\n> -\tuint32_t fourcc;\n> +\tV4L2PixelFormat fourcc;\n> \tSize size;\n> \n> \tstruct {\n> @@ -184,7 +205,7 @@ public:\n> \n> \tint getFormat(V4L2DeviceFormat *format);\n> \tint setFormat(V4L2DeviceFormat *format);\n> -\tstd::map> formats();\n> +\tstd::map> formats();\n> \n> \tint setCrop(Rectangle *rect);\n> \tint setCompose(Rectangle *rect);\n> @@ -203,9 +224,9 @@ public:\n> \tstatic V4L2VideoDevice *fromEntityName(const MediaDevice *media,\n> \t\t\t\t\t const std::string &entity);\n> \n> -\tstatic PixelFormat toPixelFormat(uint32_t v4l2Fourcc);\n> -\tuint32_t toV4L2Fourcc(PixelFormat pixelFormat);\n> -\tstatic uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar);\n> +\tstatic PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc);\n> +\tV4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat);\n> +\tstatic V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar);\n> \n> protected:\n> \tstd::string logPrefix() const;\n> @@ -220,8 +241,8 @@ private:\n> \tint getFormatSingleplane(V4L2DeviceFormat *format);\n> \tint setFormatSingleplane(V4L2DeviceFormat *format);\n> \n> -\tstd::vector enumPixelformats();\n> -\tstd::vector enumSizes(unsigned int pixelFormat);\n> +\tstd::vector enumPixelformats();\n> +\tstd::vector enumSizes(V4L2PixelFormat pixelFormat);\n> \n> \tint setSelection(unsigned int target, Rectangle *rect);\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 81911e764fde..40396c22aa45 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> \treturn true;\n> }\n> \n> +/**\n> + * \\class V4L2PixelFormat\n> + * \\brief V4L2 pixel format wrapper\n> + *\n> + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It\n> + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with\n> + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of\n> + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at\n> + * compile time.\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::V4L2PixelFormat()\n> + * \\brief Construct an invalid V4L2 pixel format with a numerical value of 0\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc)\n> + * \\brief Construct a V4L2 pixel format from a FourCC value\n> + * \\param[in] fourcc The pixel format numerical value\n> + */\n> +\n> +/**\n> + * \\fn bool V4L2PixelFormat::isValid() const\n> + * \\brief Check if the pixel format is valid\n> + * \\return True if the pixel format has a non-zero value, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn uint32_t V4L2PixelFormat::value() const\n> + * \\brief Retrieve the pixel format numerical value\n> + * \\return The pixel format numerical value\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::operator uint32_t() const\n> + * \\brief Convert the the pixel format numerical value\n> + * \\return The pixel format numerical value\n> + */\n> +\n> /**\n> * \\class V4L2DeviceFormat\n> * \\brief The V4L2 video device image format and sizes\n> @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> const std::string V4L2DeviceFormat::toString() const\n> {\n> \tstd::stringstream ss;\n> -\tss << size.toString() << \"-\" << utils::hex(fourcc);\n> +\tss << size.toString() << \"-\" << utils::hex(fourcc.value());\n\nAs you state in the cover letter a toString() method should be added. I \nthink for now it can just convert the FourCC to a hex value and return \nit as a string. The important thing I think is to convert this and the \nother utils::hex() callsite bellow to use a toString() method.\n\nWhit this fixed,\n\nReviewed-by: Niklas Söderlund \n\n> \treturn ss.str();\n> }\n> \n> @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> *\n> * \\return A list of the supported video device formats\n> */\n> -std::map> V4L2VideoDevice::formats()\n> +std::map> V4L2VideoDevice::formats()\n> {\n> -\tstd::map> formats;\n> +\tstd::map> formats;\n> \n> -\tfor (unsigned int pixelformat : enumPixelformats()) {\n> +\tfor (V4L2PixelFormat pixelformat : enumPixelformats()) {\n> \t\tstd::vector sizes = enumSizes(pixelformat);\n> \t\tif (sizes.empty())\n> \t\t\treturn {};\n> @@ -859,9 +899,9 @@ std::map> V4L2VideoDevice::formats()\n> \treturn formats;\n> }\n> \n> -std::vector V4L2VideoDevice::enumPixelformats()\n> +std::vector V4L2VideoDevice::enumPixelformats()\n> {\n> -\tstd::vector formats;\n> +\tstd::vector formats;\n> \tint ret;\n> \n> \tfor (unsigned int index = 0; ; index++) {\n> @@ -886,7 +926,7 @@ std::vector V4L2VideoDevice::enumPixelformats()\n> \treturn formats;\n> }\n> \n> -std::vector V4L2VideoDevice::enumSizes(unsigned int pixelFormat)\n> +std::vector V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)\n> {\n> \tstd::vector sizes;\n> \tint ret;\n> @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> * \\param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*)\n> * \\return The PixelFormat corresponding to \\a v4l2Fourcc\n> */\n> -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc)\n> {\n> \tswitch (v4l2Fourcc) {\n> \t/* RGB formats. */\n> @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> \t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> \t\t\t\tLogError).stream()\n> \t\t\t<< \"Unsupported V4L2 pixel format \"\n> -\t\t\t<< utils::hex(v4l2Fourcc);\n> +\t\t\t<< utils::hex(v4l2Fourcc.value());\n> \t\treturn PixelFormat();\n> \t}\n> }\n> @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> *\n> * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> */\n> -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> {\n> \treturn V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar());\n> }\n> @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> *\n> * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> */\n> -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> {\n> \tswitch (pixelFormat.fourcc()) {\n> \t/* RGB formats. */\n> -- \n> Regards,\n> \n> Laurent Pinchart\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-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0635260419\n\tfor ;\n\tThu, 19 Mar 2020 01:02:14 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id j17so151805lfe.7\n\tfor ;\n\tWed, 18 Mar 2020 17:02:13 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tn1sm130789lfl.56.2020.03.18.17.02.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 18 Mar 2020 17:02:12 -0700 (PDT)"],"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=pdBzmJ9+juIEpzCAdvUoOwMJ6+4QO13OuuN5urt2YUY=;\n\tb=ePB2HhXE/vMdw3yRu7jDmNJhCOUVjSnhg3ugKWpWPRgAXiXafQPhQE6eFqPei5Qh7E\n\tZKlzwy+uGXNKZRNGRQS9EDNBBmUnhApRdIsIS/t9Iv456nrrcCA92GmnXpSxZcg6l4n0\n\tX8T42IhVtkouiTxgvJhGjUJ3/Q5rf/gHIQ90aeNY/W4T4tEzjMEu6ALSvorRvwF9tOBC\n\tETgj5fBrCZydHBWus8lJwiMFb0A9s56BCTQgt91TsrASoGeU+CD2owr++kS4IkSLl0hX\n\tSRKBGw8wPA7ZGFpurTPIYHEqpPHC+TL3NDSpmYmf8fIHKNR9SgKv9kOgMLs6W8r1SlvO\n\top5Q==","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=pdBzmJ9+juIEpzCAdvUoOwMJ6+4QO13OuuN5urt2YUY=;\n\tb=UbYR7y2A8Ow5K71qJkPICpSjzncJGa14BCcz6w6dEMj7OJnxeD/EFoHcei5DeLn3HW\n\trjT4TOjzeOcNYwyBFBs4w70eNGd1e0bzIbeN38aLZJm9tiO/tKjrukwjZWIQy0GiLTV+\n\tbG4mKoNoShkIZOzmTEUbkTiuLSTakgMj8geuxsfUtaep6xxmDB+OIo4jhX3MRXqjJjnC\n\t5BD5xGvLkVGpW9hwjTvdUcjdqKehrBlao3BBc4VIYzY06lZWBJj4B/5y9ujaBFeiUrhJ\n\te7Hvsbp3Ld6G1L6zniI1sF3GrbuTViPtVaLHDtVlFJ9kX2kJKNnPU4YlrFavNHk6m6Gw\n\tlgGg==","X-Gm-Message-State":"ANhLgQ0qIk/Naov+GHWyo4j+uGJm7iqU4YZXLQjNpY+qzfcdRauhXFkL\n\tH+l/3oCvK4k70H36plZ5F21+gA==","X-Google-Smtp-Source":"ADFU+vuzDSDLFK3lwDEXSUvWRomsDjrOUFDhwHdn0ZPlArokJvbjixxhrvI+NB2xkAyX01JgWsOd3Q==","X-Received":"by 2002:ac2:5212:: with SMTP id a18mr350337lfl.133.1584576132979;\n\tWed, 18 Mar 2020 17:02:12 -0700 (PDT)","Date":"Thu, 19 Mar 2020 01:02:11 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200319000211.GC3042581@oden.dyn.berto.se>","References":"<20200316234649.2545-1-laurent.pinchart@ideasonboard.com>\n\t<20200316234649.2545-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200316234649.2545-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add\n\tV4L2PixelFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 19 Mar 2020 00:02:14 -0000"}},{"id":4091,"web_url":"https://patchwork.libcamera.org/comment/4091/","msgid":"<20200319123241.d2g3xneeeolzqedl@uno.localdomain>","date":"2020-03-19T12:32:41","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add\n\tV4L2PixelFormat class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for the patch\n\nOn Tue, Mar 17, 2020 at 01:46:48AM +0200, Laurent Pinchart wrote:\n> The V4L2PixelFormat class describes the pixel format of a V4L2 buffer.\n> It wraps the V4L2 numerical FourCC, and shall be used in all APIs that\n> deal with V4L2 pixel formats. Its purpose is to prevent unintentional\n> confusion of V4L2 and DRM FourCCs in code by catching implicit\n> conversion attempts at compile time.\n>\n> The constructor taking a V4L2 FourCC integer value will be made explicit\n> in a further commit to minimize the size of this change and keep it\n> reviewable.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++---\n> src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++-----\n> 2 files changed, 79 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index b3000f3c5133..49d2ca357efa 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -149,10 +149,31 @@ private:\n> \tunsigned int missCounter_;\n> };\n>\n> +class V4L2PixelFormat\n> +{\n> +public:\n> +\tV4L2PixelFormat()\n> +\t\t: fourcc_(0)\n> +\t{\n> +\t}\n> +\n> +\tV4L2PixelFormat(uint32_t fourcc)\n> +\t\t: fourcc_(fourcc)\n> +\t{\n> +\t}\n> +\n> +\tbool isValid() const { return fourcc_ != 0; }\n> +\tuint32_t value() const { return fourcc_; }\n\ns/value/fourcc ?\n\n> +\toperator uint32_t() const { return fourcc_; }\n\nIf I remove this I get errors in all pipelines when comparing\nV4L2PixelFormat instances. Is this a shortcut to allow implicit cast\nto uin32_t so that operator==() and operator!=() are happy ? Isn't it\nbetter to defined those operator instead ?\n\n> +\n> +private:\n> +\tuint32_t fourcc_;\n> +};\n> +\n> class V4L2DeviceFormat\n> {\n> public:\n> -\tuint32_t fourcc;\n> +\tV4L2PixelFormat fourcc;\n> \tSize size;\n>\n> \tstruct {\n> @@ -184,7 +205,7 @@ public:\n>\n> \tint getFormat(V4L2DeviceFormat *format);\n> \tint setFormat(V4L2DeviceFormat *format);\n> -\tstd::map> formats();\n> +\tstd::map> formats();\n>\n> \tint setCrop(Rectangle *rect);\n> \tint setCompose(Rectangle *rect);\n> @@ -203,9 +224,9 @@ public:\n> \tstatic V4L2VideoDevice *fromEntityName(const MediaDevice *media,\n> \t\t\t\t\t const std::string &entity);\n>\n> -\tstatic PixelFormat toPixelFormat(uint32_t v4l2Fourcc);\n> -\tuint32_t toV4L2Fourcc(PixelFormat pixelFormat);\n> -\tstatic uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar);\n> +\tstatic PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc);\n> +\tV4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat);\n> +\tstatic V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar);\n\ns/toV4L2PixelFormat ?\n\nto be consistent with the new class name and leave froucc out of the\nAPI\n\n>\n> protected:\n> \tstd::string logPrefix() const;\n> @@ -220,8 +241,8 @@ private:\n> \tint getFormatSingleplane(V4L2DeviceFormat *format);\n> \tint setFormatSingleplane(V4L2DeviceFormat *format);\n>\n> -\tstd::vector enumPixelformats();\n> -\tstd::vector enumSizes(unsigned int pixelFormat);\n> +\tstd::vector enumPixelformats();\n> +\tstd::vector enumSizes(V4L2PixelFormat pixelFormat);\n>\n> \tint setSelection(unsigned int target, Rectangle *rect);\n>\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 81911e764fde..40396c22aa45 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> \treturn true;\n> }\n>\n> +/**\n> + * \\class V4L2PixelFormat\n> + * \\brief V4L2 pixel format wrapper\n\nIn documentation I would instead mention fourcc. Seems there is a bit\nof confusion between fourcc and pixel format.\n\nTo me v4l2 pixel format wraps a V4L2 fourcc code\nHere v4l2 pixel format wraps a v4l2 pixel format :)\n\n> + *\n> + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It\n> + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with\n> + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of\n> + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at\n> + * compile time.\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::V4L2PixelFormat()\n> + * \\brief Construct an invalid V4L2 pixel format with a numerical value of 0\n\nAlso here and below, the \"numerical values\" are fourcc codes, which\nare indeed numerical values, but from a specific range :)\n\nAll minors though\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc)\n> + * \\brief Construct a V4L2 pixel format from a FourCC value\n> + * \\param[in] fourcc The pixel format numerical value\n> + */\n> +\n> +/**\n> + * \\fn bool V4L2PixelFormat::isValid() const\n> + * \\brief Check if the pixel format is valid\n> + * \\return True if the pixel format has a non-zero value, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn uint32_t V4L2PixelFormat::value() const\n> + * \\brief Retrieve the pixel format numerical value\n> + * \\return The pixel format numerical value\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::operator uint32_t() const\n> + * \\brief Convert the the pixel format numerical value\n> + * \\return The pixel format numerical value\n> + */\n> +\n> /**\n> * \\class V4L2DeviceFormat\n> * \\brief The V4L2 video device image format and sizes\n> @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> const std::string V4L2DeviceFormat::toString() const\n> {\n> \tstd::stringstream ss;\n> -\tss << size.toString() << \"-\" << utils::hex(fourcc);\n> +\tss << size.toString() << \"-\" << utils::hex(fourcc.value());\n> \treturn ss.str();\n> }\n>\n> @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> *\n> * \\return A list of the supported video device formats\n> */\n> -std::map> V4L2VideoDevice::formats()\n> +std::map> V4L2VideoDevice::formats()\n> {\n> -\tstd::map> formats;\n> +\tstd::map> formats;\n>\n> -\tfor (unsigned int pixelformat : enumPixelformats()) {\n> +\tfor (V4L2PixelFormat pixelformat : enumPixelformats()) {\n> \t\tstd::vector sizes = enumSizes(pixelformat);\n> \t\tif (sizes.empty())\n> \t\t\treturn {};\n> @@ -859,9 +899,9 @@ std::map> V4L2VideoDevice::formats()\n> \treturn formats;\n> }\n>\n> -std::vector V4L2VideoDevice::enumPixelformats()\n> +std::vector V4L2VideoDevice::enumPixelformats()\n> {\n> -\tstd::vector formats;\n> +\tstd::vector formats;\n> \tint ret;\n>\n> \tfor (unsigned int index = 0; ; index++) {\n> @@ -886,7 +926,7 @@ std::vector V4L2VideoDevice::enumPixelformats()\n> \treturn formats;\n> }\n>\n> -std::vector V4L2VideoDevice::enumSizes(unsigned int pixelFormat)\n> +std::vector V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)\n> {\n> \tstd::vector sizes;\n> \tint ret;\n> @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> * \\param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*)\n> * \\return The PixelFormat corresponding to \\a v4l2Fourcc\n> */\n> -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc)\n> {\n> \tswitch (v4l2Fourcc) {\n> \t/* RGB formats. */\n> @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> \t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> \t\t\t\tLogError).stream()\n> \t\t\t<< \"Unsupported V4L2 pixel format \"\n> -\t\t\t<< utils::hex(v4l2Fourcc);\n> +\t\t\t<< utils::hex(v4l2Fourcc.value());\n> \t\treturn PixelFormat();\n> \t}\n> }\n> @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> *\n> * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> */\n> -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> {\n> \treturn V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar());\n> }\n> @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> *\n> * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> */\n> -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> {\n> \tswitch (pixelFormat.fourcc()) {\n> \t/* RGB formats. */\n> --\n> Regards,\n>\n> Laurent Pinchart\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 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 3A493629AC\n\tfor ;\n\tThu, 19 Mar 2020 13:29:45 +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 relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 97F9B60003;\n\tThu, 19 Mar 2020 12:29:44 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 19 Mar 2020 13:32:41 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200319123241.d2g3xneeeolzqedl@uno.localdomain>","References":"<20200316234649.2545-1-laurent.pinchart@ideasonboard.com>\n\t<20200316234649.2545-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200316234649.2545-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add\n\tV4L2PixelFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 19 Mar 2020 12:29:45 -0000"}},{"id":4095,"web_url":"https://patchwork.libcamera.org/comment/4095/","msgid":"<20200319125611.GA4872@pendragon.ideasonboard.com>","date":"2020-03-19T12:56:11","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add\n\tV4L2PixelFormat class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Mar 19, 2020 at 01:32:41PM +0100, Jacopo Mondi wrote:\n> On Tue, Mar 17, 2020 at 01:46:48AM +0200, Laurent Pinchart wrote:\n> > The V4L2PixelFormat class describes the pixel format of a V4L2 buffer.\n> > It wraps the V4L2 numerical FourCC, and shall be used in all APIs that\n> > deal with V4L2 pixel formats. Its purpose is to prevent unintentional\n> > confusion of V4L2 and DRM FourCCs in code by catching implicit\n> > conversion attempts at compile time.\n> >\n> > The constructor taking a V4L2 FourCC integer value will be made explicit\n> > in a further commit to minimize the size of this change and keep it\n> > reviewable.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++---\n> > src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++-----\n> > 2 files changed, 79 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index b3000f3c5133..49d2ca357efa 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -149,10 +149,31 @@ private:\n> > \tunsigned int missCounter_;\n> > };\n> >\n> > +class V4L2PixelFormat\n> > +{\n> > +public:\n> > +\tV4L2PixelFormat()\n> > +\t\t: fourcc_(0)\n> > +\t{\n> > +\t}\n> > +\n> > +\tV4L2PixelFormat(uint32_t fourcc)\n> > +\t\t: fourcc_(fourcc)\n> > +\t{\n> > +\t}\n> > +\n> > +\tbool isValid() const { return fourcc_ != 0; }\n> > +\tuint32_t value() const { return fourcc_; }\n> \n> s/value/fourcc ?\n\nI've considered that, I found pros and cons, and would have preferred\nusing value() for PixelFormat too, but we have modifiers too there, so\nthat's not an option. I'll change it to fourcc() here.\n\n> > +\toperator uint32_t() const { return fourcc_; }\n> \n> If I remove this I get errors in all pipelines when comparing\n> V4L2PixelFormat instances. Is this a shortcut to allow implicit cast\n> to uin32_t so that operator==() and operator!=() are happy ? Isn't it\n> better to defined those operator instead ?\n\nIt's not just comparison, it also makes it possible to assign a\nV4L2PixelFormat to a uint32_t, as well as pass a V4L2PixelFormat to a\nfunction that expects a uint32_t. Please see the related discussions on\nthe PixelFormat series.\n\n> > +\n> > +private:\n> > +\tuint32_t fourcc_;\n> > +};\n> > +\n> > class V4L2DeviceFormat\n> > {\n> > public:\n> > -\tuint32_t fourcc;\n> > +\tV4L2PixelFormat fourcc;\n> > \tSize size;\n> >\n> > \tstruct {\n> > @@ -184,7 +205,7 @@ public:\n> >\n> > \tint getFormat(V4L2DeviceFormat *format);\n> > \tint setFormat(V4L2DeviceFormat *format);\n> > -\tstd::map> formats();\n> > +\tstd::map> formats();\n> >\n> > \tint setCrop(Rectangle *rect);\n> > \tint setCompose(Rectangle *rect);\n> > @@ -203,9 +224,9 @@ public:\n> > \tstatic V4L2VideoDevice *fromEntityName(const MediaDevice *media,\n> > \t\t\t\t\t const std::string &entity);\n> >\n> > -\tstatic PixelFormat toPixelFormat(uint32_t v4l2Fourcc);\n> > -\tuint32_t toV4L2Fourcc(PixelFormat pixelFormat);\n> > -\tstatic uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar);\n> > +\tstatic PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc);\n> > +\tV4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat);\n> > +\tstatic V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar);\n> \n> s/toV4L2PixelFormat ?\n> \n> to be consistent with the new class name and leave froucc out of the\n> API\n\nGood point, I'll fix that.\n\n> >\n> > protected:\n> > \tstd::string logPrefix() const;\n> > @@ -220,8 +241,8 @@ private:\n> > \tint getFormatSingleplane(V4L2DeviceFormat *format);\n> > \tint setFormatSingleplane(V4L2DeviceFormat *format);\n> >\n> > -\tstd::vector enumPixelformats();\n> > -\tstd::vector enumSizes(unsigned int pixelFormat);\n> > +\tstd::vector enumPixelformats();\n> > +\tstd::vector enumSizes(V4L2PixelFormat pixelFormat);\n> >\n> > \tint setSelection(unsigned int target, Rectangle *rect);\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 81911e764fde..40396c22aa45 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > \treturn true;\n> > }\n> >\n> > +/**\n> > + * \\class V4L2PixelFormat\n> > + * \\brief V4L2 pixel format wrapper\n> \n> In documentation I would instead mention fourcc. Seems there is a bit\n> of confusion between fourcc and pixel format.\n\nGood idea, I'll fix that by writing \"V4L2 pixel format FourCC wrapper\".\n\n> To me v4l2 pixel format wraps a V4L2 fourcc code\n> Here v4l2 pixel format wraps a v4l2 pixel format :)\n> \n> > + *\n> > + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It\n> > + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with\n> > + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of\n> > + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at\n> > + * compile time.\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2PixelFormat::V4L2PixelFormat()\n> > + * \\brief Construct an invalid V4L2 pixel format with a numerical value of 0\n> \n> Also here and below, the \"numerical values\" are fourcc codes, which\n> are indeed numerical values, but from a specific range :)\n> \n> All minors though\n> Reviewed-by: Jacopo Mondi \n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc)\n> > + * \\brief Construct a V4L2 pixel format from a FourCC value\n> > + * \\param[in] fourcc The pixel format numerical value\n> > + */\n> > +\n> > +/**\n> > + * \\fn bool V4L2PixelFormat::isValid() const\n> > + * \\brief Check if the pixel format is valid\n> > + * \\return True if the pixel format has a non-zero value, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn uint32_t V4L2PixelFormat::value() const\n> > + * \\brief Retrieve the pixel format numerical value\n> > + * \\return The pixel format numerical value\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2PixelFormat::operator uint32_t() const\n> > + * \\brief Convert the the pixel format numerical value\n> > + * \\return The pixel format numerical value\n> > + */\n> > +\n> > /**\n> > * \\class V4L2DeviceFormat\n> > * \\brief The V4L2 video device image format and sizes\n> > @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > const std::string V4L2DeviceFormat::toString() const\n> > {\n> > \tstd::stringstream ss;\n> > -\tss << size.toString() << \"-\" << utils::hex(fourcc);\n> > +\tss << size.toString() << \"-\" << utils::hex(fourcc.value());\n> > \treturn ss.str();\n> > }\n> >\n> > @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)\n> > *\n> > * \\return A list of the supported video device formats\n> > */\n> > -std::map> V4L2VideoDevice::formats()\n> > +std::map> V4L2VideoDevice::formats()\n> > {\n> > -\tstd::map> formats;\n> > +\tstd::map> formats;\n> >\n> > -\tfor (unsigned int pixelformat : enumPixelformats()) {\n> > +\tfor (V4L2PixelFormat pixelformat : enumPixelformats()) {\n> > \t\tstd::vector sizes = enumSizes(pixelformat);\n> > \t\tif (sizes.empty())\n> > \t\t\treturn {};\n> > @@ -859,9 +899,9 @@ std::map> V4L2VideoDevice::formats()\n> > \treturn formats;\n> > }\n> >\n> > -std::vector V4L2VideoDevice::enumPixelformats()\n> > +std::vector V4L2VideoDevice::enumPixelformats()\n> > {\n> > -\tstd::vector formats;\n> > +\tstd::vector formats;\n> > \tint ret;\n> >\n> > \tfor (unsigned int index = 0; ; index++) {\n> > @@ -886,7 +926,7 @@ std::vector V4L2VideoDevice::enumPixelformats()\n> > \treturn formats;\n> > }\n> >\n> > -std::vector V4L2VideoDevice::enumSizes(unsigned int pixelFormat)\n> > +std::vector V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)\n> > {\n> > \tstd::vector sizes;\n> > \tint ret;\n> > @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> > * \\param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*)\n> > * \\return The PixelFormat corresponding to \\a v4l2Fourcc\n> > */\n> > -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> > +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc)\n> > {\n> > \tswitch (v4l2Fourcc) {\n> > \t/* RGB formats. */\n> > @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> > \t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> > \t\t\t\tLogError).stream()\n> > \t\t\t<< \"Unsupported V4L2 pixel format \"\n> > -\t\t\t<< utils::hex(v4l2Fourcc);\n> > +\t\t\t<< utils::hex(v4l2Fourcc.value());\n> > \t\treturn PixelFormat();\n> > \t}\n> > }\n> > @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> > *\n> > * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> > */\n> > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> > {\n> > \treturn V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar());\n> > }\n> > @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> > *\n> > * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> > */\n> > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> > {\n> > \tswitch (pixelFormat.fourcc()) {\n> > \t/* RGB formats. */","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 74FEB629AC\n\tfor ;\n\tThu, 19 Mar 2020 13:56:29 +0100 (CET)","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 C1C22A53;\n\tThu, 19 Mar 2020 13:56:28 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584622589;\n\tbh=yC04uUZfQmHad4KPoLLyObXs8EbwG6hTjBhukzNVrb4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tInKOc7Tug36C3em7eMQNjlnc36rX4b6ltCLKJoHy1lQpqWKvPCwcwXLBogCP2yMF\n\tUNh6XWPpGCbPI+F2WroPYCEHd7064vpyVB7+C9UL7/iuG1gn5xQcDJHkI7CBNN5Yrn\n\tvg4zNMoLtq8KtkEokeRqrEy3dwLKXXNuEjYeuO+E=","Date":"Thu, 19 Mar 2020 14:56:11 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200319125611.GA4872@pendragon.ideasonboard.com>","References":"<20200316234649.2545-1-laurent.pinchart@ideasonboard.com>\n\t<20200316234649.2545-2-laurent.pinchart@ideasonboard.com>\n\t<20200319123241.d2g3xneeeolzqedl@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200319123241.d2g3xneeeolzqedl@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add\n\tV4L2PixelFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 19 Mar 2020 12:56:29 -0000"}}]