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

16873 diff mbox series

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.

Signed-off-by: Jacopo Mondi <jacopo@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(-)

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@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@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;