[{"id":24314,"web_url":"https://patchwork.libcamera.org/comment/24314/","msgid":"","date":"2022-08-02T19:47:13","subject":"Re: [libcamera-devel] [PATCH v4 5/6] libcamera: v4l2_videodevice:\n\tMatch formats supported by the device","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 Tue, Aug 02, 2022 at 06:01:35PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Now that V4L2PixelFormat::fromPixelFormat() returns a list of formats\n> to chose from, select the one supported by the video device by matching\n> against the list of supported pixel formats.\n> \n> The first format found to match one of the device supported ones is\n> returned.\n> \n> As the list of pixel formats supported by the video device does not\n> change at run-time, cache it at device open() time. To maximize the\n> lookup efficiency store the list of supported V4L2PixelFormat in an\n> std::unordered_set<> which requires the V4L2PixelFormat class to be\n> associated with an hash function object and to support the comparison\n\ns/an hash/a hash/\n\n> operator.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/v4l2_pixelformat.h | 15 +++++\n> include/libcamera/internal/v4l2_videodevice.h | 4 ++\n> src/libcamera/v4l2_pixelformat.cpp | 29 ++++++++++\n> src/libcamera/v4l2_videodevice.cpp | 58 ++++++++++++++-----\n> 4 files changed, 92 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_pixelformat.h b/include/libcamera/internal/v4l2_pixelformat.h\n> index d5400f90a67e..05069a30304d 100644\n> --- a/include/libcamera/internal/v4l2_pixelformat.h\n> +++ b/include/libcamera/internal/v4l2_pixelformat.h\n> @@ -8,6 +8,7 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -22,6 +23,15 @@ namespace libcamera {\n> class V4L2PixelFormat\n> {\n> public:\n> +\tclass Hasher\n> +\t{\n> +\tpublic:\n> +\t\tstd::size_t operator()(const V4L2PixelFormat &f) const\n> +\t\t{\n> +\t\t\treturn f.fourcc();\n> +\t\t}\n> +\t};\n\nIt would be simpler to inject a specialization of std::hash<> in the std\nnamespace, that would allow declaring std::unordered_set and\nstd::unordered_map instances without having to pass a hasher explicitly.\nI'll send a patch with this.\n\n> +\n> \tstruct Info {\n> \t\tPixelFormat format;\n> \t\tconst char *description;\n> @@ -37,6 +47,11 @@ public:\n> \t{\n> \t}\n> \n> +\tbool operator==(const V4L2PixelFormat &other) const\n> +\t{\n> +\t\treturn fourcc_ == other.fourcc_;\n> +\t}\n\nThis isn't needed, the default operator==() implementation generated by\nthe compiler should be enough.\n\n> +\n> \tbool isValid() const { return fourcc_ != 0; }\n> \tuint32_t fourcc() const { return fourcc_; }\n> \toperator uint32_t() const { return fourcc_; }\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 29fa0bbaf670..c9064291a669 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -242,6 +243,8 @@ private:\n> \t\tStopped,\n> \t};\n> \n> +\tint initFormats();\n> +\n> \tint getFormatMeta(V4L2DeviceFormat *format);\n> \tint trySetFormatMeta(V4L2DeviceFormat *format, bool set);\n> \n> @@ -268,6 +271,7 @@ private:\n> \tV4L2Capability caps_;\n> \tV4L2DeviceFormat format_;\n> \tconst PixelFormatInfo *formatInfo_;\n> +\tstd::unordered_set pixelFormats_;\n> \n> \tenum v4l2_buf_type bufferType_;\n> \tenum v4l2_memory memoryType_;\n> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp\n> index 90c8fa8d9aae..264ca80aab27 100644\n> --- a/src/libcamera/v4l2_pixelformat.cpp\n> +++ b/src/libcamera/v4l2_pixelformat.cpp\n> @@ -187,6 +187,28 @@ const std::map vpf2pf{\n> \n> } /* namespace */\n> \n> +/**\n> + * \\class V4L2PixelFormat::Hasher\n> + * \\brief Function object class that implement hash function for the\n> + * V4L2PixelFormat class\n> + *\n> + * The Hasher class is a function object class that that allows to use the\n> + * V4L2PixelFormat class with C++ STL unordered associative containers.\n> + *\n> + * The hash for a V4L2PixelFormat is obtained by using the function call\n> + * operator().\n> + */\n> +\n> +/**\n> + * \\fn V4L2PixelFormat::Hasher::operator()(const V4L2PixelFormat &f) const\n> + * \\brief Function call operator that computes the V4L2PixelFormat hash value\n> + * \\param[in] f The V4L2PixelFormat to compute the hash on\n> + *\n> + * Compute the hash value of \\a f, which simply is the numerical FourCC value.\n> + *\n> + * \\return The numerical FourCC value for \\a f\n> + */\n> +\n> /**\n> * \\struct V4L2PixelFormat::Info\n> * \\brief Information about a V4L2 pixel format\n> @@ -208,6 +230,13 @@ const std::map vpf2pf{\n> * invalid, calling the isValid() function returns false.\n> */\n> \n> +/**\n> + * \\fn V4L2PixelFormat::operator==()\n> + * \\brief Compare two V4L2PixelFormat by comparing their FourCC codes\n> + * \\param[in] other The other V4L2PixelFormat to compare with\n> + * \\return True if the FourCC codes are the same, false otherwise\n> + */\n> +\n> /**\n> * \\fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc)\n> * \\brief Construct a V4L2PixelFormat from a FourCC value\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 2ca22f485d45..c3343ca7078b 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -629,18 +629,14 @@ int V4L2VideoDevice::open()\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n> \n> +\tret = initFormats();\n> +\tif (ret)\n> +\t\treturn ret;\n\nCould you move this below the debug message to avoid changing the\nbehaviour ? Same in the other open() function.\n\n> +\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> -\tret = getFormat(&format_);\n> -\tif (ret) {\n> -\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> -\t\treturn ret;\n> -\t}\n> -\n> -\tformatInfo_ = &PixelFormatInfo::info(format_.fourcc);\n> -\n> \treturn 0;\n> }\n> \n> @@ -722,11 +718,25 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n> \n> +\tret = initFormats();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> -\tret = getFormat(&format_);\n> +\treturn 0;\n> +}\n> +\n> +int V4L2VideoDevice::initFormats()\n> +{\n> +\tconst std::vector &deviceFormats = enumPixelformats(0);\n\nNo error checking ?\n\n> +\tpixelFormats_ = std::unordered_set +\t\t\t\t\t V4L2PixelFormat::Hasher>\n> +\t\t\t(deviceFormats.begin(), deviceFormats.end());\n\nThe constructor you use here is not explicit, so you can write\n\n\tpixelFormats_ = { deviceFormats.begin(), deviceFormats.end() };\n\nWith these small issues addressed, and the V4L2PixelFormat changes\ndropped if you want to use the \"[PATCH] libcamera: v4l2_pixelformat:\nImplement std::hash specialization\" patch I've sent (and the commit\nmessage then updated accordingly),\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\tint ret = getFormat(&format_);\n> \tif (ret) {\n> \t\tLOG(V4L2, Error) << \"Failed to get format\";\n> \t\treturn ret;\n> @@ -1990,17 +2000,37 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> }\n> \n> /**\n> - * \\brief Convert \\a PixelFormat to its corresponding V4L2 FourCC\n> + * \\brief Convert \\a PixelFormat to a V4L2PixelFormat supported by the device\n> * \\param[in] pixelFormat The PixelFormat to convert\n> *\n> - * The V4L2 format variant the function returns the contiguous version\n> - * unconditionally.\n> + * Convert \\a pixelformat to a V4L2 FourCC that is known to be supported by\n> + * the video device.\n> *\n> - * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> + * A V4L2VideoDevice may support different V4L2 pixel formats that map the same\n> + * PixelFormat. This is the case of the contiguous and non-contiguous variants\n> + * of multiplanar formats, and with the V4L2 MJPEG and JPEG pixel formats.\n> + * Converting a PixelFormat to a V4L2PixelFormat may thus have multiple answers.\n> + *\n> + * This function converts the \\a pixelFormat using the list of V4L2 pixel\n> + * formats that the V4L2VideoDevice supports. This guarantees that the returned\n> + * V4L2PixelFormat will be valid for the device. If multiple matches are still\n> + * possible, contiguous variants are preferred. If the \\a pixelFormat is not\n> + * supported by the device, the function returns an invalid V4L2PixelFormat.\n> + *\n> + * \\return The V4L2PixelFormat corresponding to \\a pixelFormat if supported by\n> + * the device, or an invalid V4L2PixelFormat otherwise\n> */\n> V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat) const\n> {\n> -\treturn V4L2PixelFormat::fromPixelFormat(pixelFormat)[0];\n> +\tconst std::vector &v4l2PixelFormats =\n> +\t\tV4L2PixelFormat::fromPixelFormat(pixelFormat);\n> +\n> +\tfor (const V4L2PixelFormat &v4l2Format : v4l2PixelFormats) {\n> +\t\tif (pixelFormats_.count(v4l2Format))\n> +\t\t\treturn v4l2Format;\n> +\t}\n> +\n> +\treturn {};\n> }\n> \n> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E260CBE173\n\tfor ;\n\tTue, 2 Aug 2022 19:47:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D39463317;\n\tTue, 2 Aug 2022 21:47:20 +0200 (CEST)","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 21176603E7\n\tfor ;\n\tTue, 2 Aug 2022 21:47:19 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B3CAC25B;\n\tTue, 2 Aug 2022 21:47:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659469640;\n\tbh=d3Dda98VHo+rm8D3fZ4QFHxPg3H6tbLZib4WXGQT5QY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=voA5lV0y5Hbzd5a8MesxsSYaBve0SdfbrkJO4HjsBT3yKlsEJnnVDJ9t2wpd5jRU5\n\tDwP++C3kjywKDfh4m84F3aQOh7ROHkHkDZot1MktU/GrzFxRE4XOZ/Smpc2uedBsmt\n\t8TUP25n+yHWQC2Wihn2Usz6HjW8B5a83RGCmSi/xlC6TjOV8qhg7VoOi77JSLOttxO\n\tEaAbSFviZtJdC9ZHOmKyFnbUDVA1EN1L4ezow7JJQfVYo0xnnd47YXTO2yN2YuMZlf\n\twNjxsrzpte/vtQUPUgdIdWQC7dbYSepBUZ9Z/71GnkAj64I3k0jZZ2gNlL3xQFO/k1\n\t3iTmUYvCzYSoQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659469638;\n\tbh=d3Dda98VHo+rm8D3fZ4QFHxPg3H6tbLZib4WXGQT5QY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ATexan2Jbgw4KQv8kvjrCXPue57BQI6wFVlT/2ikP5ACW8vYcerkFURcS/fHJ+iji\n\twd+U8VtlgreoHSjrRSu7Ros4hCi0G1kJD7nP6VJ++I5+yjt1lXZ4dL3ZT3QnAQA5tQ\n\t+argQp38brXrTy0zGy6Z/g0paKTjQ3M1NB63Zq6I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ATexan2J\"; dkim-atps=neutral","Date":"Tue, 2 Aug 2022 22:47:13 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220802160136.63075-1-jacopo@jmondi.org>\n\t<20220802160136.63075-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220802160136.63075-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 5/6] libcamera: v4l2_videodevice:\n\tMatch formats supported by the device","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, jozef@mlich.cz,\n\tPavel Machek ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]