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

[Philipp Zabel <p.zabel@pengutronix.de>]

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.

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