[libcamera-devel] [PATCH v3 5/6] libcamera: v4l2_videodevice: Match formats supported by the device

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 29, 2022 at 06:00:13PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Now that V4L2PixelFormat::fromPixelFormat() returns a list of formats
> to chose from, select the one supported by the video device by matching
> against the list of supported pixel formats.
> 
> The first format found to match one of the device supported ones is
> returned.
> 
> As the list of pixel formats supported by the video device does not
> change at run-time, cache it at device open() time.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  1 +
>  src/libcamera/v4l2_videodevice.cpp            | 28 +++++++++++++++----
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 29fa0bbaf670..6fe81d5e4cf0 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -268,6 +268,7 @@ private:
>  	V4L2Capability caps_;
>  	V4L2DeviceFormat format_;
>  	const PixelFormatInfo *formatInfo_;
> +	std::vector<V4L2PixelFormat> pixelFormats_;
>  
>  	enum v4l2_buf_type bufferType_;
>  	enum v4l2_memory memoryType_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 2ca22f485d45..0aeaae6ad573 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -633,6 +633,8 @@ int V4L2VideoDevice::open()
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
>  
> +	pixelFormats_ = enumPixelformats(0);
> +
>  	ret = getFormat(&format_);
>  	if (ret) {
>  		LOG(V4L2, Error) << "Failed to get format";
> @@ -726,6 +728,8 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
>  
> +	pixelFormats_ = enumPixelformats(0);
> +
>  	ret = getFormat(&format_);
>  	if (ret) {
>  		LOG(V4L2, Error) << "Failed to get format";
> @@ -1990,17 +1994,31 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>  }
>  
>  /**
> - * \brief Convert \a PixelFormat to its corresponding V4L2 FourCC
> + * \brief Convert \a PixelFormat to one of the device supported V4L2 FourCC

 * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device

>   * \param[in] pixelFormat The PixelFormat to convert
>   *
> - * The V4L2 format variant the function returns the contiguous version
> - * unconditionally.
> + * Convert a\ pixelformat to a V4L2 FourCC that is known to be supported by

s/a\/\a/

> + * the video device.
>   *
> - * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat
> + * \return The V4L2PixelFormat corresponding to \a pixelFormat or an invalid
> + * PixelFormat if \a pixelFormat is not supported by the video device

s/PixelFormat/V4L2PixelFormat/

I'd expand this as it's also meant to cover the JPEG vs. MJPEG case:

 * A V4L2VideoDevice may support different V4L2 pixel formats that map the same
 * PixelFormat. This is the case of the contiguous and non-contiguous variants
 * of multiplanar formats, and with the V4L2 MJPEG and JPEG pixel formats.
 * Converting a PixelFormat to a V4L2PixelFormat may thus have multiple answers.
 *
 * This function converts the \a pixelFormat using the list of V4L2 pixel
 * formats that the V4L2VideoDevice supports. This guarantees that the returned
 * V4L2PixelFormat will be valid for the device. If multiple matches are still
 * possible, contiguous variants are preferred. If the \a pixelFormat is not
 * supported by the device, the function returns an invalid V4L2PixelFormat.
 *
 * \return The V4L2PixelFormat corresponding to \a pixelFormat if supported by
 * the device, or an invalid V4L2PixelFormat otherwise

>   */
>  V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat) const
>  {
> -	return V4L2PixelFormat::fromPixelFormat(pixelFormat)[0];
> +	std::vector<V4L2PixelFormat> v4l2PixelFormats =

	const std::vector<V4L2PixelFormat> &v4l2PixelFormats =

> +		V4L2PixelFormat::fromPixelFormat(pixelFormat);
> +
> +	for (const V4L2PixelFormat &v4l2Format : v4l2PixelFormats) {
> +		auto it = std::find_if(pixelFormats_.begin(), pixelFormats_.end(),
> +				       [&v4l2Format](const V4L2PixelFormat &devFormat) {
> +					       return v4l2Format == devFormat;
> +				       });
> +
> +		if (it != pixelFormats_.end())
> +			return v4l2Format;

Should we convert pixelFormats_ to a std::unordered_set to make the
lookup more efficient ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	}
> +
> +	return {};
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart