Re: [PATCH 02/14] videodev2.h: add V4L2_FMT_FLAG_HAS_BITSTREAM_PARSER

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 7/29/19 3:18 PM, Tomasz Figa wrote:
> On Mon, Jul 29, 2019 at 10:12 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
>>
>> Hi,
>>
>> On Sun 28 Jul 19, 23:05, Tomasz Figa wrote:
>>> On Sat, Jul 27, 2019 at 6:37 PM Paul Kocialkowski
>>> <paul.kocialkowski@bootlin.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed 24 Jul 19, 13:05, Hans Verkuil wrote:
>>>>> Add an enum_fmt format flag to specifically tag coded formats where
>>>>> full bitstream parsing is supported by the device.
>>>>>
>>>>> Some stateful decoders are capable of fully parsing a bitstream,
>>>>> but others require that userspace pre-parses the bitstream into
>>>>> frames or fields (see the corresponding pixelformat descriptions
>>>>> for details).
>>>>>
>>>>> If this flag is set, then this pre-parsing step is not required
>>>>> (but still possible, of course).
>>>>>
>>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>> ---
>>>>>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst | 8 ++++++++
>>>>>  Documentation/media/videodev2.h.rst.exceptions   | 1 +
>>>>>  include/uapi/linux/videodev2.h                   | 5 +++--
>>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
>>>>> index 822d6730e7d2..4e24e671f32e 100644
>>>>> --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
>>>>> +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
>>>>> @@ -127,6 +127,14 @@ one until ``EINVAL`` is returned.
>>>>>        - This format is not native to the device but emulated through
>>>>>       software (usually libv4l2), where possible try to use a native
>>>>>       format instead for better performance.
>>>>> +    * - ``V4L2_FMT_FLAG_HAS_BITSTREAM_PARSER``
>>>>> +      - 0x0004
>>>>> +      - The hardware decoder for this compressed bitstream format (aka coded
>>>>> +     format) is capable of parsing the bitstream. Applications do not
>>>>> +     need to parse the bitstream themselves to find the boundaries between
>>>>> +     frames/fields. This flag can only be used in combination with the
>>>>> +     ``V4L2_FMT_FLAG_COMPRESSED`` flag, since this applies to compressed
>>>>> +     formats only.
>>>>
>>>> Should this flag be set for stateless codecs as well? It seems a bit over-kill
>>>> for this case. I am not sure whether "compressed bitstream format" clearly only
>>>> covers the formats used by stateful decoders and not the ones for stateless
>>>> decoders.
>>>
>>> I'd suggest using a different name for the flag, because "bitstream
>>> parser" is actually one of the core differences between stateful and
>>> stateless. All stateful decoders have bitstream parsers, the only
>>> difference between the implementations is the unit on which the parser
>>> operates, i.e. full stream, frame, NALU.
>>>
>>> Perhaps V4L2_FMT_FLAG_CONTINUOUS_BITSTREAM (as opposed to discrete,
>>> framed/sliced chunks)?
>>
>> Sure, that seems like a more explicit name regarding what it's supposed to
>> describe in my opinion.

I like that name. And this flag is valid for stateful decoders only.

>>
>>> Regardless of that, it doesn't make sense for a stateless decoder to
>>> set this flag anyway, because the userspace needs to parse the whole
>>> stream anyway and the whole stateless API is based on the assumption
>>> that the userspace splits the bitstream into frames (or slices).
>>
>> Indeed, I agree that it doesn't make sense, but I thought that the name of the
>> flag could be confusing. Since there is no direct equivalency between
>> "stateless" and "doesn't parse the bitstream" (as we've seen with the rockchip
>> decoder needing to parse the slice header on its own), that could have been
>> ambiguous. I think the name you're suggesting mostly solves this concern.
>>
>> I'm still a bit unsure about what "compressed formats" entails or not, so it
>> could be good to explicitly mention that this applies to stateful decoders only
>> (but it's just a suggestion, advanced users of the API will probably find it
>> straightforward).
> 
> My understanding is that a compressed format is any format that
> doesn't have a directly accessible 2D pixel matrix in its memory
> representation, so all the bitstream formats should have it set.

Correct.

Regards,

	Hans

> 
> Best regards,
> Tomasz
>