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

[Tomasz Figa <tfiga@chromium.org>]

On Thu, Oct 17, 2019 at 12:08 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Wed, 2019-10-16 at 15:37 +0200, Paul Kocialkowski wrote:
> [...]
> > > > The bottomline is that we have use cases for each of the two set of fields
> > > > independently, so I feel like this is reason enough to avoid mixing them
> > > > together.
> > >
> > > What do you mean by mixing together? Hardware parsing the slices
> > > always uses num_ref_idx_l[01]_default_active_minus1 from the PPS.
> > > Hardware not parsing the slices always sets override to 1 and uses
> > > num_ref_idx_l[01]_active_minus1 from the slice header struct.
> >
> > To summarize, what I don't understand is why it's worth re-purposing
> > the slice header's num_ref_idx_l[01]_active_minus1 to contain
> > num_ref_idx_l[01]_default_active_minus1 when the flag is not set in the initial
> > bitstream instead of exposing the flag.
> >
> > There's hardware (like cedrus) which takes both fields and the flag directly
> > in-registers, so it's really not a simplification here. And even in cases where
> > the hardware only takes one field, I believe that the downside of re-purposing
> > the field of the control is much greater than the benefit of the supposed
> > simplification.
> >
> > I know this sounds quite futile, but I thought there was an implicit agreement
> > that controls must stick as close as possible to the bitstream. This is an
> > occurence where we are diverging for no particularly strong reason.
>
> FWIW, I agree with Paul on this. That drivers for codecs which do not
> parse the slice headers always completely ignore the
> num_ref_idx_l[01]_default_active_minus1 fields, but instead expect the
> num_ref_idx_l[01]_active_minus1 field to be repurposed to contain the
> default values if the corresponding fields do not exist in the slice
> header (that is, when the num_ref_idx_active_override_flag is not set),
> confused me at first [1].
>
> This seems to follow what libva does [2], and it does simplify drivers a
> tiny bit, but I'd still prefer to explicitly have the
> num_ref_idx_active_override_flag contained in the API, and to have the
> num_ref_idx_l[01]_active_minus1 fields only be used for
> num_ref_idx_l[01]_active_minus1, and not have them sometimes contain the
> values of another field.
>
> [1] https://patchwork.linuxtv.org/patch/58580/
> [2] https://github.com/intel/libva/blob/95eb8cf469367b532b391042fa0e77ca513ac94e/va/va.h#L3138
>
> > Expecting that userspace does this pre-processing of fields feels quite
> > counter-intuitive and confusing for people wishing to use the API, too.
> > One would certainly naively expect that the fields in the controls carry the
> > same meaning as in the bitstream when they have the same name.
>
> I certainly naively did.
>

Okay, I think you convinced me. :)

+Alexandre Courbot to be aware of the upcoming UAPI change.

Best regards,
Tomasz