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 >
