[libcamera-devel] [PATCH v3 4/6] libcamera: v4l2_pixelformat: Return the list of V4L2 formats

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 29, 2022 at 06:00:12PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Multiple V4L2 formats can be associated with a single PixelFormat.
> Now that users of V4L2PixelFormat::fromPixelFormat() have been converted
> to use V4L2VideoDevice::toV4L2PixelFormat(), return the full list of
> V4L2 formats in order to prepare to match them against the ones
> supported by the video device.
> 
> The V4L2 compatibility layer, not having any video device to interact
> with, are converted to use the first returned format by default.

s/are converted/is converted/
s/by default/unconditionally/

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/v4l2_pixelformat.h |  4 +++-
>  src/libcamera/v4l2_pixelformat.cpp            | 14 +++++++-------
>  src/libcamera/v4l2_videodevice.cpp            |  2 +-
>  src/v4l2/v4l2_camera_proxy.cpp                |  6 +++---
>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_pixelformat.h b/include/libcamera/internal/v4l2_pixelformat.h
> index ed94baf92795..a8aec29552e4 100644
> --- a/include/libcamera/internal/v4l2_pixelformat.h
> +++ b/include/libcamera/internal/v4l2_pixelformat.h
> @@ -11,6 +11,7 @@
>  #include <ostream>
>  #include <stdint.h>
>  #include <string>
> +#include <vector>
>  
>  #include <linux/videodev2.h>
>  
> @@ -44,7 +45,8 @@ public:
>  	const char *description() const;
>  
>  	PixelFormat toPixelFormat() const;
> -	static V4L2PixelFormat fromPixelFormat(const PixelFormat &pixelFormat);
> +	static const std::vector<V4L2PixelFormat> &
> +		fromPixelFormat(const PixelFormat &pixelFormat);

I think we usually wrap lines as

	static const std::vector<V4L2PixelFormat> &
	fromPixelFormat(const PixelFormat &pixelFormat);

The function name isn't great anymore, as static from*() functions are
expected to act as constructors and return an instance of the class. I
can't think of a much better name right now though.

>  
>  private:
>  	uint32_t fourcc_;
> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> index 91a882baa75b..bca97d1e3b4f 100644
> --- a/src/libcamera/v4l2_pixelformat.cpp
> +++ b/src/libcamera/v4l2_pixelformat.cpp
> @@ -302,23 +302,23 @@ PixelFormat V4L2PixelFormat::toPixelFormat() const
>  }
>  
>  /**
> - * \brief Convert \a pixelFormat to its corresponding V4L2PixelFormat
> + * \brief Retrieve the list of V4L2PixelFormat associated with \a pixelFormat
>   * \param[in] pixelFormat The PixelFormat to convert
>   *
>   * Multiple V4L2 formats may exist for one PixelFormat as V4L2 defines separate
>   * 4CCs for contiguous and non-contiguous versions of the same image format.
>   *
> - * Return the contiguous planes format version by default.
> - *
> - * \return The V4L2PixelFormat corresponding to \a pixelFormat
> + * \return The list of V4L2PixelFormat corresponding to \a pixelFormat
>   */
> -V4L2PixelFormat V4L2PixelFormat::fromPixelFormat(const PixelFormat &pixelFormat)
> +const std::vector<V4L2PixelFormat> &V4L2PixelFormat::fromPixelFormat(const PixelFormat &pixelFormat)

You could wrap this line too.

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

>  {
> +	static const std::vector<V4L2PixelFormat> empty;
> +
>  	const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
>  	if (!info.isValid())
> -		return V4L2PixelFormat();
> +		return empty;
>  
> -	return info.v4l2Formats[0];
> +	return info.v4l2Formats;
>  }
>  
>  /**
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index f41afa06f460..2ca22f485d45 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -2000,7 +2000,7 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>   */
>  V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat) const
>  {
> -	return V4L2PixelFormat::fromPixelFormat(pixelFormat);
> +	return V4L2PixelFormat::fromPixelFormat(pixelFormat)[0];
>  }
>  
>  /**
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 26a227da6db2..55ff62cdb430 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -182,7 +182,7 @@ void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
>  
>  	v4l2PixFormat_.width        = size.width;
>  	v4l2PixFormat_.height       = size.height;
> -	v4l2PixFormat_.pixelformat  = V4L2PixelFormat::fromPixelFormat(streamConfig.pixelFormat);
> +	v4l2PixFormat_.pixelformat  = V4L2PixelFormat::fromPixelFormat(streamConfig.pixelFormat)[0];
>  	v4l2PixFormat_.field        = V4L2_FIELD_NONE;
>  	v4l2PixFormat_.bytesperline = streamConfig.stride;
>  	v4l2PixFormat_.sizeimage    = streamConfig.frameSize;
> @@ -290,7 +290,7 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *file, struct v4l2_fmtdesc *
>  		return -EINVAL;
>  
>  	PixelFormat format = streamConfig_.formats().pixelformats()[arg->index];
> -	V4L2PixelFormat v4l2Format = V4L2PixelFormat::fromPixelFormat(format);
> +	V4L2PixelFormat v4l2Format = V4L2PixelFormat::fromPixelFormat(format)[0];
>  
>  	arg->flags = format == formats::MJPEG ? V4L2_FMT_FLAG_COMPRESSED : 0;
>  	utils::strlcpy(reinterpret_cast<char *>(arg->description),
> @@ -333,7 +333,7 @@ int V4L2CameraProxy::tryFormat(struct v4l2_format *arg)
>  
>  	arg->fmt.pix.width        = config.size.width;
>  	arg->fmt.pix.height       = config.size.height;
> -	arg->fmt.pix.pixelformat  = V4L2PixelFormat::fromPixelFormat(config.pixelFormat);
> +	arg->fmt.pix.pixelformat  = V4L2PixelFormat::fromPixelFormat(config.pixelFormat)[0];
>  	arg->fmt.pix.field        = V4L2_FIELD_NONE;
>  	arg->fmt.pix.bytesperline = config.stride;
>  	arg->fmt.pix.sizeimage    = config.frameSize;

-- 
Regards,

Laurent Pinchart