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

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

[Philipp Zabel]

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

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

[Hans Verkuil]

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?

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

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.

Regards,

	Hans

>      * - __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``
>        -
> 

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

[Philipp Zabel]

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

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

[Hans Verkuil]

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:
>>> 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?

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.

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?

Regards,

	Hans

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

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

[Philipp Zabel]

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

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

[Hans Verkuil]

On 9/9/19 3:36 PM, Philipp Zabel wrote:
> 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.

Right. Can you check if the current description for V4L2_PIX_FMT_H264_SLICE_RAW
in our spec is sufficiently detailed to make it clear what is in the buffer?

In particular I would like to see a reference to the H.264 spec that
describes the slice data format.

Regards,

	Hans

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

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

[Paul Kocialkowski]

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

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?

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

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

[Tomasz Figa]

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.

We're talking about kernel drivers here and for security reasons any
logic should be reduced to the strictly necessary minimum.

Best regards,
Tomasz

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

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

[Paul Kocialkowski]

[-- 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 --]

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

[Tomasz Figa]

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.

Best regards,
Tomasz

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

[Paul Kocialkowski]

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

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.

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.

We are still in the process of polishing the API anyway, so now feels like a
good time to change these things.

Cheers,

Paul

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

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

[Tomasz Figa]

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.

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

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

Best regards,
Tomasz

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

[Paul Kocialkowski]

[-- 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 --]

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

[Paul Kocialkowski]

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

Hi,

I thought I had answered here already, but looks I never sent the email.

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.

Sorry, I got confused here and lost sight of the fact that the two members
are already part of the structures. So my point here is to introduce the flag
and have drivers use it to select between the two values.

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

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.

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

From my perspective, we still lack a few things:
- Coverage testing of the current controls against a large number of bitstreams;
- Making sure that the current controls cover all the bitstream features we want
  to support in this API and take a decision about what we explicitly decide
  to exclude if needed;
- We need a stateless (i.e. not firmware-backed) encoder supported;
- Maybe having formal rules regarding how to adapt codec bitstream elements to
  controls would also be nice.

So tl;dr, I don't think we're at a point where things are sufficiently
well-defined to consider destaging. So that's why I feel like we should take the
chance to keep polish things (especially small details like the one discussed
here, which will set a precedent for how to do things in the future).

What do you think?

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

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

[Philipp Zabel]

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.

regards
Philipp

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

[Tomasz Figa]

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