Re: [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields

[Hans Verkuil <hverkuil@xs4all.nl>]

On 9/9/19 3:36 PM, Philipp Zabel wrote:
> On Mon, 2019-09-09 at 14:43 +0200, Hans Verkuil wrote:
>> 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:
> [...]
>>>>> @@ -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.
> 
> Ok. I'll revise the patch accordingly.
> 
>> 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?
> 
> The complete slice queued via VIDIOC_QBUF still contains all these
> fields (and more). Presumably that's where the Hantro G1 reads the
> num_ref_idx_active_override_flag from, as well as other fields that it
> doesn't use from v4l2_ctrl_h264_slice_params.

Right. Can you check if the current description for V4L2_PIX_FMT_H264_SLICE_RAW
in our spec is sufficiently detailed to make it clear what is in the buffer?

In particular I would like to see a reference to the H.264 spec that
describes the slice data format.

Regards,

	Hans

> 
> G1 can not parse the slice header completely by itself though,
> it needs to be told the total size of the (pic_order_cnt_lsb /
> delta_pic_order_cnt_bottom / delta_pic_order_cnt0 /
> delta_pic_order_cnt1) syntax elements and the size of the
> dec_ref_pic_marking() syntax element, as well as the values of
> pic_parameter_set_id, frame_num, and idr_pic_id, and some flags.
> The num_ref_idx_l[01]_active_minus1 fields are among those parsed from
> the vb2 buffer directly.
> 
> That's why the hantro-vpu driver ignores the header_bit_size field,
> whereas cedrus has to use it to tell the hardware how to skip the
> header.
> 
> Cedrus completely ignores the num_ref_idx_l[01]_default_active_minus1
> fields, and always uses the values passed via
> num_ref_idx_l[01]_active_minus1, see cedrus_h264.c +343:
>         /*
>          * FIXME: the kernel headers are allowing the default value to
>          * be passed, but the libva doesn't give us that.
>          */
>         reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10;
>         reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5;
>         cedrus_write(dev, VE_H264_PPS, reg);
> 
> and +388:
>         reg |= VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD;
>         reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 24;
>         reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 16;
>         cedrus_write(dev, VE_H264_SHS2, reg);
> 
> ^ that's the override flag being set unconditionally, to select the
> values from SHS2 over those from PPS.
> 
> regards
> Philipp
>