Message-ID: <98f5cd5c-cb9c-45ca-a7c7-a546f525c393@xs4all.nl>
Date: Fri, 19 Jul 2024 15:37:13 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, mchehab@...nel.org,
 ezequiel@...guardiasur.com.ar
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v4 0/2] Enumerate all pixels formats

On 19/07/2024 15:15, Benjamin Gaignard wrote:
> 
> Le 19/07/2024 à 14:57, Hans Verkuil a écrit :
>> On 17/07/2024 15:14, Benjamin Gaignard wrote:
>>> The goal of this series is to let userland applications enumerate
>>> all the supported pixels formats of a stateless decoder without
>>> setting all the possible codec-dependent control.
>>> That offer a simplest solution for applications to discover
>>> supported pixels formats and possibly let them doing smarter
>>> choice between stateless decoders.
>>>
>>> An example of how it can be used in GStreamer to discover the
>>> supported pixels formats for stateless decoder is available here:
>>> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/v4l2codecs_enum_all_supported_formats?ref_type=heads
>> So effectively specifying this flag makes ENUM_FMT also return
>> formats that do not match the bit depth.
>>
>> So the AV1 (for example) compressed video uses e.g. 8 bit depth, but instead of just
>> listing only 8 bit uncompressed pixelformats, you want to list them for any
>> bit depth.
>>
>> But what is the point of that if the decoder can't decode 8 bit compressed to,
>> say, 10 bit uncompressed video?
> 
> No decoder will do 8 bits to 10 bits (as far I knows).
> The point is to be able to say that decoder could produce 10 bit frames without
> setting a full sps/pps for each case (and for each supported codec).
> 
>>
>> I actually thought that this flag would just list all formats, independent
>> of the output format (e.g. AV1, H264, etc.), but that does not appear to be
>> the case? I.e., if capture pixelformat X is only available with AV1, will that still
>> be listed if the output pixel is set to H264?
>>
>> I think you need to describe a real use-case here, and I am not convinced about
>> the name of the flag either.
> 
> I may have miss something but yes the goal is to list all formats independently
> of the output format.
> When a SoC have multiple decoders for the same codec, knowing the supported formats
> is key to select the better one.
> Since I will have to do more iteration, feel free to provide a better name for the
> flag(s). I'm always bad for naming this kind of thing.

That really needs to be clarified, since in patch 1/2 it says:

+   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
+     all the supported formats without taking care of codec-dependent controls
+     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
+     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
+     enumerating.

Here it just talks about 'codec-dependent controls set on the ``OUTPUT`` queue', it
doesn't say anything about the compressed pixelformat set for the OUTPUT queue.

And patch 2/2 sets the ignore_depth_match boolean, suggesting also that it is
not listing all formats, but just ignoring a specific check.

But if you list all pixelformats without taking the OUTPUT pixelformat into
account, how do you know which pixelformat is valid for which codec?

Say that only MPEG support NV12 (just for the sake of argument), and that's
what you want to use, you have no way of knowing that NV12 is specific to MPEG,
you would have to try each codec and see if NV12 is supported for that codec.

I just don't see how this can be used in practice.

What exactly is the problem you want to solve? A real-life problem, not a theoretical
one :-)

Regards,

	Hans

> 
> 
>>
>>> changes in version 4:
>>> - Explicitly document that the new flags are targeting mem2mem devices.
>>>
>>> changes in version 3:
>>> - Add a flag to inform userspace application that driver
>>>    as take care of the flag.
>>>
>>> changes in version 2:
>>> - Clarify documentation.
>>> - Only keep V4L2_FMT_FLAG_ALL_FORMATS flag in ioctl.
>>>
>>> Benjamin
>>>
>>> Benjamin Gaignard (2):
>>>    media: videodev2: Add flags to unconditionnaly enumerate pixels
>>>      formats
>> I.e.: it is not unconditionally, it still depends on the chosen codec.
>>
>> Regards,
>>
>>     Hans
>>
>>>    media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag
>>>
>>>   .../media/v4l/dev-stateless-decoder.rst          |  6 ++++++
>>>   .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 11 +++++++++++
>>>   .../media/videodev2.h.rst.exceptions             |  2 ++
>>>   drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++---
>>>   drivers/media/v4l2-core/v4l2-ioctl.c             |  3 +++
>>>   include/uapi/linux/videodev2.h                   |  2 ++
>>>   6 files changed, 37 insertions(+), 3 deletions(-)
>>>
>>
> 

Powered by blists - more mailing lists