[{"id":24075,"web_url":"https://patchwork.libcamera.org/comment/24075/","msgid":"","date":"2022-07-23T17:45:36","subject":"Re: [libcamera-devel] [PATCH v2 5/8] 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 Sat, Jul 23, 2022 at 11:53:27AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Now that V4L2PixelFormat::fromPixelFormat() returns a list of formats\n> to chose from by checking which ones are actually supported by the video\n> device.\n> \n> The first format found to match one of the device supported ones is\n> returned.\n> \n> As V4L2VideoDevice::toV4L2PixelFormat() is a const function which uses\n> other functions of the class, those functions has to be made const as\n> well.\n> \n> In particular:\n> \n> - enumPixelformats() and enumSizes() do not modify the class state and\n> can safely be made const.\n> - formats() uses the above functions and does not itself modify the\n> class state and can be made const\n> - add a const version of V4L2Device::ioctl() to be used by the now const\n> functions\n\nCan we avoid that by enumerating all V4L2 pixel formats when the\nV4L2VideoDevice is opened and caching them internally ? The call to\nenumPixelformats() with a non-zero code needs to go to the device, but\nthe call in V4L2VideoDevice::toV4L2PixelFormat() can use cached data.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/v4l2_device.h | 1 +\n> include/libcamera/internal/v4l2_videodevice.h | 6 ++--\n> src/libcamera/v4l2_device.cpp | 15 +++++++++\n> src/libcamera/v4l2_videodevice.cpp | 31 ++++++++++++++-----\n> 4 files changed, 43 insertions(+), 10 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index a52a5f2c99f9..f7ec3c7004a6 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -55,6 +55,7 @@ protected:\n> \tint setFd(UniqueFD fd);\n> \n> \tint ioctl(unsigned long request, void *argp);\n> +\tint ioctl(unsigned long request, void *argp) const;\n> \n> \tint fd() const { return fd_.get(); }\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 29fa0bbaf670..6d8850c99afd 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -205,7 +205,7 @@ public:\n> \tint getFormat(V4L2DeviceFormat *format);\n> \tint tryFormat(V4L2DeviceFormat *format);\n> \tint setFormat(V4L2DeviceFormat *format);\n> -\tFormats formats(uint32_t code = 0);\n> +\tFormats formats(uint32_t code = 0) const;\n> \n> \tint setSelection(unsigned int target, Rectangle *rect);\n> \n> @@ -251,8 +251,8 @@ private:\n> \tint getFormatSingleplane(V4L2DeviceFormat *format);\n> \tint trySetFormatSingleplane(V4L2DeviceFormat *format, bool set);\n> \n> -\tstd::vector enumPixelformats(uint32_t code);\n> -\tstd::vector enumSizes(V4L2PixelFormat pixelFormat);\n> +\tstd::vector enumPixelformats(uint32_t code) const;\n> +\tstd::vector enumSizes(V4L2PixelFormat pixelFormat) const;\n> \n> \tint requestBuffers(unsigned int count, enum v4l2_memory memoryType);\n> \tint createBuffers(unsigned int count,\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 3fc8438f6579..59f92403db80 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -459,6 +459,21 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\copydoc ioctl()\n> + */\n> +int V4L2Device::ioctl(unsigned long request, void *argp) const\n> +{\n> +\t/*\n> +\t * Printing out an error message is usually better performed\n> +\t * in the caller, which can provide more context.\n> +\t */\n> +\tif (::ioctl(fd_.get(), request, argp) < 0)\n> +\t\treturn -errno;\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\fn V4L2Device::deviceNode()\n> * \\brief Retrieve the device node path\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 43c3d0f69266..a3242ba755c0 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1045,7 +1045,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> *\n> * \\return A list of the supported video device formats\n> */\n> -V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code)\n> +V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code) const\n> {\n> \tFormats formats;\n> \n> @@ -1067,7 +1067,7 @@ V4L2VideoDevice::Formats V4L2VideoDevice::formats(uint32_t code)\n> \treturn formats;\n> }\n> \n> -std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n> +std::vector V4L2VideoDevice::enumPixelformats(uint32_t code) const\n> {\n> \tstd::vector formats;\n> \tint ret;\n> @@ -1101,7 +1101,7 @@ std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n> \treturn formats;\n> }\n> \n> -std::vector V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)\n> +std::vector V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat) const\n> {\n> \tstd::vector sizes;\n> \tint ret;\n> @@ -1990,20 +1990,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 one of the device supported V4L2 FourCC\n> * \\param[in] pixelFormat The PixelFormat to convert\n> *\n> + * Convert a\\ pixelformat to a V4L2 FourCC that is known to be supported by\n> + * the video device.\n\nYou should also document what happens if the device supports multiple\nV4L2 pixel formats that match the given PixelFormat.\n\n> + *\n> * For multiplanar formats, the V4L2 format variant (contiguous or\n> * non-contiguous planes) is selected automatically based on the capabilities\n> * of the video device. If the video device supports the V4L2 multiplanar API,\n> * non-contiguous formats are preferred.\n> *\n> - * \\return The V4L2_PIX_FMT_* pixel format code corresponding to \\a pixelFormat\n> + * \\return The V4L2PixelFormat corresponding to \\a pixelFormat or an invalid\n> + * PixelFormat if \\a pixelFormat is not supported by the video device\n> */\n> V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat) const\n> {\n> -\treturn V4L2PixelFormat::fromPixelFormat(pixelFormat,\n> -\t\t\t\t\t\tcaps_.isMultiplanar())[0];\n> +\tstd::vector deviceFormats = enumPixelformats(0);\n> +\tstd::vector v4l2PixelFormats =\n> +\t\tV4L2PixelFormat::fromPixelFormat(pixelFormat, caps_.isMultiplanar());\n\nI think we should avoid reintroducing the automatic multiplanar\nselection, which is the reason why the\nV4L2VideoDevice::toV4L2PixelFormat() function got dropped in the first\nplace. Merging the single and multi vectors as recommended in the review\nof an earlier patch in this series will help.\n\nI'd also use const references here. It won't make a difference as the\ntwo functions return objects by value, but when calling functions that\nreturn a reference, it avoids copies. It's thus a good practice to use\nconst references everywhere possible, as there's no drawback using them\nwhen the function returns by value (a const reference extends the\nlifetime of the value returned by the function).\n\n> +\n> +\tfor (const V4L2PixelFormat &v4l2Format : v4l2PixelFormats) {\n> +\t\tauto it = std::find_if(deviceFormats.begin(), deviceFormats.end(),\n> +\t\t\t\t [&v4l2Format](const V4L2PixelFormat &deviceFormat) {\n> +\t\t\t\t\t return v4l2Format == deviceFormat;\n> +\t\t\t\t });\n> +\n> +\t\tif (it != deviceFormats.end())\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 27D74C3275\n\tfor ;\n\tSat, 23 Jul 2022 17:45:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 301496330E;\n\tSat, 23 Jul 2022 19:45:41 +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 B0853603F8\n\tfor ;\n\tSat, 23 Jul 2022 19:45:39 +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 2093E9F7;\n\tSat, 23 Jul 2022 19:45:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658598341;\n\tbh=S8ZfaxtbfGu2XSgjcVoaBCEbPFgfY/bN8sW83Ljv7cU=;\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=X8GyuV0Whr6ct2ZI4wMN0ReIQLG9en0Jy5+no3xt8NafQ18pQGXc1w0BKA16OyvYS\n\tf1w5T2GftvWP3dH8e06mJht7g5Uflomsepx+pjb4A1PI2IDwYjMrORzKH7MIs9Ej3P\n\tqC/NsaLHPky4pQlf9dTNoPc6xq7qtYNCv1mwO2QzRYEf9fwXf9BfF01xbPP9VLqIcv\n\t3O8PFmGOZWf8mS8AbTiP/Sih++zpGAcAW7xAryWpKP/ospkMXPWeIHCUw5k5ghp+D1\n\tgtpi1mJWiDmW2pE0eaioTOPhu3yMiFnZmmGIj5UjTLgBLAmnKS1DWsDEL4Ujs5cWei\n\tEoR4JAf1Ie6RQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658598339;\n\tbh=S8ZfaxtbfGu2XSgjcVoaBCEbPFgfY/bN8sW83Ljv7cU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FiPr3D+yWBpWK9S9azNWZ2gUkFviP7FyqIMkVq6y5Qxis8UvSFoHtqCdxdIm7jyi9\n\tL4R7XFYygTZoya41LGKoiKywuU8GV5nF41+syeyhLYKBaOuz7XV39RfsO0nwFdtxS8\n\tn+V3eR60ImXla4EZ9z1r0SFDMkSllxKMn2cNxe3Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FiPr3D+y\"; dkim-atps=neutral","Date":"Sat, 23 Jul 2022 20:45:36 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220723095330.43542-1-jacopo@jmondi.org>\n\t<20220723095330.43542-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220723095330.43542-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 5/8] 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\" "}}]