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

[Hans de Goede]

Hi,

On 7/21/24 6:33 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Fri, Jul 19, 2024 at 03:07:26PM +0200, Hans de Goede wrote:
>> On 7/19/24 9:37 AM, Laurent Pinchart wrote:
>>> On Sat, Jul 13, 2024 at 08:43:27PM +0200, 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.
>>>>
>>>> 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;
>>>
>>> This can never happen, so you could fold the default vase with any of
>>> the above. I suppose returning -EINVAL could help catching future errors
>>> when we'll update open() to support more types.
>>
>> Right I'm not a fan of just adding a default: to some existing case
>> when there is no clear default existing case.
>>
>>> but forget to update the
>>> code here. Is that a real risk ?
>>
>> Humans make mistakes so yes IMHO this is real risk and I would
>> prefer to keep this bit as is.
> 
> OK.
> 
>>>> +	}
>>>>  }
>>>>  
>>>>  /**
>>>> @@ -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;
>>>
>>> Same here.
>>>
>>>> +	}
>>>>  }
>>>>  
>>>>  /**
>>>> @@ -843,12 +857,24 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
>>>>  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
>>>>  {
>>>>  	int ret = 0;
>>>
>>> While at it you can drop the initialization of the ret variable.
>>
>> Ack.
>>
>>>> -	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;
>>>
>>> And here.
>>>
>>> If you think we should drop the default case, please send a new version.
>>> Otherwise I can drop the initialization of ret when applying. Either
>>> way,
>>
>> As mendtioned above I prefer to keep the default case. If you can
>> drop the initialization of ret while applying this that would be
>> great.
>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> Thanks & Regsrds,
> 
> Pushed.

Thank you.

> Unless I'm mistaken, initial IPU6 support is now fully merged.

Correct \o/

Regards,

Hans