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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 5588 bytes --]

On Sat 05 Oct 19, 23:21, Tomasz Figa wrote:
> On Sat, Oct 5, 2019 at 11:12 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> >
> > On Sat 05 Oct 19, 22:54, Tomasz Figa wrote:
> > > On Sat, Oct 5, 2019 at 10:39 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Sat 05 Oct 19, 17:22, Tomasz Figa wrote:
> > > > > On Fri, Oct 4, 2019 at 6:12 AM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Thu 05 Sep 19, 13:42, 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.
> > > > > >
> > > > > > Is there any particular reason why this is preferable to exposing the flag?
> > > > > > It feels like having the flag around sticks closer to the bitstream,
> > > > > > so it's more straightforward for everyone.
> > > > > >
> > > > > > In case there's only one set of fields exposed by the hardware (and it doesn't
> > > > > > do slice parsing itself), we could always check the flag in the driver and use
> > > > > > either the default PPS values or the slice-specific values there.
> > > > > >
> > > > > > What do you think?
> > > > >
> > > > > IMHO it would only add further logic to the drivers, because they
> > > > > would need to have a conditional block that selects default or
> > > > > per-slice value based on the flag. The goal was to be able for the
> > > > > driver to just passively write num_ref_idx_l[01]_active_minus1 (or the
> > > > > default one for slice-parsing hardware) to corresponding hardware
> > > > > registers.
> > > >
> > > > Well, the Allwinner block has both set of fields and a field for the flag,
> > > > so in this case I find that it is cleaner to just copy the values and flag
> > > > rather than always setting the flag even when it's the default value used.
> > > >
> > > > More generally, the two sets + flag are closer to bitstream which feels less
> > > > confusing than re-purposing these fields from the slice header to fit the
> > > > result of flag ? per-slice : per-picture.
> > > >
> > > > > We're talking about kernel drivers here and for security reasons any
> > > > > logic should be reduced to the strictly necessary minimum.
> > > >
> > > > I definitely agree that bitstream parsing in the kernel is to be avoided for
> > > > security reasons (among other things), but don't see the harm in considering
> > > > the flags in-driver if needed. We do it for a number of other flags already
> > > > (and strongly need to).
> > >
> > > If the fields are well documented, does it really matter? I'd suggest
> > > just keeping it as is, rather than overpolishing things and preventing
> > > the interface from stabilizing.
> >
> > I just don't see the benefit of changing the purpose of a bitstream element.
> > Even with documentation, it adds some unnecessary confusion and I find this to
> > be a complicated enough topic without it ;)
> >
> > Especially for the case of hardware that does slice header parsing itself, it
> > feels particularly unsettling to have to take the default PPS values for the
> > fields from the slice header control rather than PPS.
> 
> num_ref_idx_l[01]_default_active_minus1 are members of v4l2_ctrl_h264_pps.

Oh right, they are currently still here at the moment. So I'm just advocating
for adding the flag then. I was under the impression that only the set from the
slice header was still around. Thanks!

> >
> > 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.

Well, just specifying that the per-slice set takes values from the PPS set if
the flag is not there in the original bitstream is mixing up both fields.

What I mean is that the per-slice value is not specified to take the PPS value
as a fallback in bitstream syntax: that's why there are two distinct sets of
elements. Adding the flag avoids mixing them up and sticks closer to bitstream.

> > We are still in the process of polishing the API anyway, so now feels like a
> > good time to change these things.
> 
> Hmm, it seemed to me like things already calmed down and we were just
> polishing the documentation. I was convinced we were actually about to
> destage things. Are you aware of some other changes coming?

I believe the next step is to go through some bitstream coverage testing before
we can have a clearer idea of whether the current controls are ready to be
stabilized or not.

I also feel like I haven't looking into existing and possible H.264 features
enough to have a clear idea of whether we're missing something or not.

I'm also under the impression that there wasn't strong feedback about the
control fields either so I feel like it would be best to be careful here.

What do you think?

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]