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: 4247 bytes --]

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

Cheers,

Paul

> >
> > Cheers,
> >
> > Paul
> >
> > > 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.
> > >      * - __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.
> > >      * - __u8
> > >        - ``num_ref_idx_l1_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_l1_default_active_minus1.
> > >      * - __u32
> > >        - ``slice_group_change_cycle``
> > >        -
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com

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