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
