On 9/9/19 2:27 PM, Philipp Zabel wrote:
> On Mon, 2019-09-09 at 14:09 +0200, Hans Verkuil wrote:
>> On 9/5/19 1:42 PM, Philipp Zabel wrote:
>>> To explain why num_ref_idx_active_override_flag is not part of the API,
>>> describe how the num_ref_idx_l[01]_active_minus1 fields and the
>>> num_ref_idx_l[01]_default_active_minus1 fields are used, depending on
>>> whether the decoder parses slice headers itself or not.
>>>
>>> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
>>> ---
>>>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst 
>>> b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> index bc5dd8e76567..b9834625a939 100644
>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> @@ -1630,10 +1630,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>        -
>>>      * - __u8
>>>        - ``num_ref_idx_l0_default_active_minus1``
>>> -      -
>>> +      - This field is only used by decoders that parse slices themselves.
>>
>> How do you know that the decoder does this?
> 
> We don't, so usespace has to provide it unconditionally.
> 
> This was meant as an explanation why. As you can see from the "media:
> hantro: h264: per-slice num_ref_idx_l[01]_active override" thread I
> found this a bit unintuitive.
> 
>>>      * - __u8
>>>        - ``num_ref_idx_l1_default_active_minus1``
>>> -      -
>>> +      - This field is only used by decoders that parse slices themselves.
>>>      * - __u8
>>>        - ``weighted_bipred_idc``
>>>        -
>>> @@ -1820,10 +1820,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>        -
>>>      * - __u8
>>>        - ``num_ref_idx_l0_active_minus1``
>>> -      -
>>> +      - This field is used by decoders that do not parse slices themselves.
>>> +        If num_ref_idx_active_override_flag is not set, this field must be
>>> +        set to the value of num_ref_idx_l0_default_active_minus1.
>>
>> I don't think you can know if the decoder parses the slices.
> 
> That is correct.
> 
>> Wouldn't it be better to just delete the 'This field is only used by decoders
>> that parse slices themselves.' sentence? Drivers for HW that handle this can
>> just ignore these fields.
> 
> If this has no place in the API documentation, or if it just might
> confuse the user in a different way, it's indeed better drop these.
> Is there another place where this could be clarified instead, perhaps
> the kerneldoc comments?

A code comment in those drivers where the HW parses this would make
sense since that explains why that driver ignores these fields.

But I would not mention this at all in the userspace API.

The 'If num_ref_idx_active_override_flag is not set, this field must be
set to the value of num_ref_idx_l0_default_active_minus1.' addition is
of course fine.

I'm a bit confused, though: you say some HW can parse this, but how?
It's part of the slice_header, so it ends up in struct 
v4l2_ctrl_h264_slice_params,
right? So how can the HW parse this without also providing the
num_ref_idx_active_override_flag value?

Regards,

        Hans

> 
> Basically I was confused about why both the default and the override
> have to be provided, and how the kernel driver determines which one to
> choose, since the override flag is not part of the kernel API. As it
> turns out, it doesn't have to choose; depending on whether the hardware
> parses slices, the driver can pick either one or the other, and always
> use that.
> 
> regards
> Philipp
> 

Reply via email to