Re: [PATCH v4 04/11] media: uapi: h264: Add the concept of start code

[Hans Verkuil <hverkuil@xs4all.nl>]

On 8/12/19 1:56 PM, Ezequiel Garcia wrote:
> On Mon, 2019-08-12 at 12:11 +0200, Hans Verkuil wrote:
>> On 8/8/19 12:34 PM, Ezequiel Garcia wrote:
>>> Stateless decoders have different expectations about the
>>> start code that is prepended on H264 slices. Add a
>>> menu control to express the supported start code types
>>> (including no start code).
>>>
>>> Drivers are allowed to support only one start code type,
>>> but they can support both too.
>>>
>>> Note that this is independent of the H264 decoding mode,
>>> which specifies the granularity of the decoding operations.
>>> Either in frame-based or slice-based mode, this new control
>>> will allow to define the start code expected on H264 slices.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>> Changes in v4:
>>> * New patch.
>>> ---
>>>  .../media/uapi/v4l/ext-ctrls-codec.rst        | 31 +++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++++
>>>  include/media/h264-ctrls.h                    |  6 ++++
>>>  3 files changed, 46 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> index ea0455957149..94fd3a9b8b9e 100644
>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> @@ -2062,6 +2062,37 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>          The OUTPUT buffer should contain all slices needed to decode the
>>>          frame/field.
>>>  
>>> +``V4L2_CID_MPEG_VIDEO_H264_STARTCODE (enum)``
>>> +    Specifies the H264 slice start code expected for each slice.
>>> +    This control shall e used to complement V4L2_PIX_FMT_H264_SLICE
>>
>> e -> be
>>
>>> +    pixel format. Drivers may expose a single or multiple
>>> +    start codes, depending on what they can support.
>>> +
>>> +    .. note::
>>> +
>>> +       This menu control is not yet part of the public kernel API and
>>> +       it is expected to change.
>>> +
>>> +.. c:type:: v4l2_mpeg_video_h264_startcode
>>> +
>>> +.. cssclass:: longtable
>>> +
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +    :widths:       1 1 2
>>> +
>>> +    * - ``V4L2_MPEG_VIDEO_H264_NO_STARTCODE``
>>> +      - 0
>>> +      - Selecting this value specifies that H264 slices are passed
>>> +        to the driver without any start code.
>>> +        Bla.
>>
>> Bla?
>>
> 
> Well, despite how many times I reviewed this doc, it seems
> this slipped through :-(
> 
>>> +    * - ``V4L2_MPEG_VIDEO_H264_ANNEX_B_STARTCODE``
>>> +      - 1
>>> +      - Selecting this value specifies that H264 slices are expected
>>> +        to be prefixed by Annex B start codes. According to :ref:`h264`
>>> +        valid start codes can be 3-bytes 0x000001, or 4-bytes 0x00000001.
>>> +
>>
>> I had the impression that it is more common to require startcodes. If that's
>> indeed the case, shouldn't this have value 0 instead of 1?
>> 1?
>>
> 
> I'm not sure this is indeed the case.
> 
> For instance, VA-API accelerators work on H264 slices that are not
> prepended by the NALU start code.
> 
> I'm fine flipping the values, but at this point, with only cedrus and hantro,
> there's doesn't seem to be a "natural" choice.

OK, let's keep this as-is then.

Regards,

	Hans

> 
> Thanks,
> Ezequiel
>