Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields
@ 2019-09-05 11:42 Philipp Zabel
  2019-09-09 12:09 ` Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Philipp Zabel @ 2019-09-05 11:42 UTC (permalink / raw)
  To: linux-media
  Cc: Tomasz Figa, Hans Verkuil, Paul Kocialkowski, Ezequiel Garcia,
	Boris Brezillon, kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields
  2019-09-05 11:42 [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields Philipp Zabel
@ 2019-09-09 12:09 ` Hans Verkuil
  2019-09-09 12:27   ` Philipp Zabel
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2019-09-09 12:09 UTC (permalink / raw)
  To: Philipp Zabel, linux-media
  Cc: Tomasz Figa, Paul Kocialkowski, Ezequiel Garcia, Boris Brezillon, kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields
  2019-09-09 12:09 ` Hans Verkuil
@ 2019-09-09 12:27   ` Philipp Zabel
  2019-09-09 12:43     ` Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Philipp Zabel @ 2019-09-09 12:27 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Tomasz Figa, Paul Kocialkowski, Ezequiel Garcia, Boris Brezillon, kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields
  2019-09-09 12:27   ` Philipp Zabel
@ 2019-09-09 12:43     ` Hans Verkuil
  2019-09-09 13:36       ` Philipp Zabel
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2019-09-09 12:43 UTC (permalink / raw)
  To: Philipp Zabel, linux-media
  Cc: Tomasz Figa, Paul Kocialkowski, Ezequiel Garcia, Boris Brezillon, kernel

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
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields
  2019-09-09 12:43     ` Hans Verkuil
@ 2019-09-09 13:36       ` Philipp Zabel
  2019-09-09 14:00         ` Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Philipp Zabel @ 2019-09-09 13:36 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Tomasz Figa, Paul Kocialkowski, Ezequiel Garcia, Boris Brezillon, kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields
  2019-09-09 13:36       ` Philipp Zabel
@ 2019-09-09 14:00         ` Hans Verkuil
  0 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2019-09-09 14:00 UTC (permalink / raw)
  To: Philipp Zabel, linux-media
  Cc: Tomasz Figa, Paul Kocialkowski, Ezequiel Garcia, Boris Brezillon, kernel

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
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 11:42 [PATCH] media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields Philipp Zabel
2019-09-09 12:09 ` Hans Verkuil
2019-09-09 12:27   ` Philipp Zabel
2019-09-09 12:43     ` Hans Verkuil
2019-09-09 13:36       ` Philipp Zabel
2019-09-09 14:00         ` Hans Verkuil

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox