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: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.zabel@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?

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