[PATCH] libcamera: v4l2_videodevice: Use bufferType_ in [get|try|set]Format()

[Jacopo Mondi]

Hi Hans

On Sat, Jul 13, 2024 at 08:43:27PM GMT, Hans de Goede wrote:
> V4L2VideoDevice is using the caps to determine which kind of buffers to
> use with the video-device in 2 different cases:
>
> 1. V4L2VideoDevice::open()
> 2. V4L2VideoDevice::[get|try|set]Format()
>
> And the order in which the caps are checked is different between
> these 2 cases. This is a problem for /dev/video# nodes which support
> both video-capture and metadata buffers. open() sets bufferType_ to
> V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] in this case, where as
> [get|try|set]Format() will call [get|set]FormatMeta() which does not
> work with V4L2_BUF_TYPE_VIDEO_CAPTURE[_MPLANE] buffers.
>
> Switch [get|try|set]Format() to use the bufferType_ to determine on what
> sort of buffers they should be operating, leaving the V4L2VideoDevice
> code with only a single place where the decision is made what sort
> of buffers it should operate on for a specific /dev/video# node.
>
> This will also allow to modify open() in the future to take a bufferType
> argument to allow overriding the default bufferType it selects for
> /dev/video# nodes which are capable of supporting more then 1 buffer type.

Does this mean that, at the moment, on a device that supports both
capture and metadata buffer types, video-capture is selected
unconditionally and the metadata interface cannot be used ?

If you plan to add an option to override the bufferType_ at open()
time, does it mean it is needed to go through a open-close-open cycle
to switch buffer type or:

1) The device can be opened multiple times and bufferType_ gets
overriden ? (I'm not sure this is a good ideas as the last caller of
open() selects the bufferType_)

2) Should we instead allow overriding the bufferType_ to use with an
optional parameter to [get|try|set]Format() instead ?

>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 56 ++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 4947aa3d..a8dc5355 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -803,12 +803,19 @@ std::string V4L2VideoDevice::logPrefix() const
>   */
>  int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)
>  {
> -	if (caps_.isMeta())
> -		return getFormatMeta(format);
> -	else if (caps_.isMultiplanar())
> -		return getFormatMultiplane(format);
> -	else
> +	switch (bufferType_) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		return getFormatSingleplane(format);
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		return getFormatMultiplane(format);
> +	case V4L2_BUF_TYPE_META_CAPTURE:
> +	case V4L2_BUF_TYPE_META_OUTPUT:
> +		return getFormatMeta(format);
> +	default:
> +		return -EINVAL;
> +	}
>  }
>
>  /**
> @@ -823,12 +830,19 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)
>   */
>  int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
>  {
> -	if (caps_.isMeta())
> -		return trySetFormatMeta(format, false);
> -	else if (caps_.isMultiplanar())
> -		return trySetFormatMultiplane(format, false);
> -	else
> +	switch (bufferType_) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		return trySetFormatSingleplane(format, false);
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		return trySetFormatMultiplane(format, false);
> +	case V4L2_BUF_TYPE_META_CAPTURE:
> +	case V4L2_BUF_TYPE_META_OUTPUT:
> +		return trySetFormatMeta(format, false);
> +	default:
> +		return -EINVAL;
> +	}
>  }
>
>  /**
> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
>  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
>  {
>  	int ret = 0;

I don't think it is necessary to initialize ret, but it wasn't already
so
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

> -	if (caps_.isMeta())
> -		ret = trySetFormatMeta(format, true);
> -	else if (caps_.isMultiplanar())
> -		ret = trySetFormatMultiplane(format, true);
> -	else
> +
> +	switch (bufferType_) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		ret = trySetFormatSingleplane(format, true);
> +		break;
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		ret = trySetFormatMultiplane(format, true);
> +		break;
> +	case V4L2_BUF_TYPE_META_CAPTURE:
> +	case V4L2_BUF_TYPE_META_OUTPUT:
> +		ret = trySetFormatMeta(format, true);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
>
>  	/* Cache the set format on success. */
>  	if (ret)
> --
> 2.45.2
>