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