On Mon, Jul 27, 2020 at 4:39 PM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote: > > Hi Alexandre, > > Thanks a lot for the review. > > On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote: > > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote: > > > The H.264 specification requires in its "Slice header semantics" > > > section that the following values shall be the same in all slice headers: > > > > > > pic_parameter_set_id > > > frame_num > > > field_pic_flag > > > bottom_field_flag > > > idr_pic_id > > > pic_order_cnt_lsb > > > delta_pic_order_cnt_bottom > > > delta_pic_order_cnt[ 0 ] > > > delta_pic_order_cnt[ 1 ] > > > sp_for_switch_flag > > > slice_group_change_cycle > > > > > > and can therefore be moved to the per-frame decode parameters control. > > > > I am really not a H.264 expert, so this question may not be relevant, > > All questions are welcome. I'm more than happy to discuss this patchset. > > > but are these values specified for every slice header in the > > bitstream, or are they specified only once per frame? > > > > I am asking this because it would certainly make user-space code > > simpler if we could remain as close to the bitstream as possible. If > > these values are specified once per slice, then factorizing them would > > leave user-space with the burden of deciding what to do if they change > > across slices. > > > > Note that this is a double-edged sword, because it is not necessarily > > better to leave the firmware in charge of deciding what to do in such > > a case. :) So hopefully these are only specified once per frame in the > > bitstream, in which case your proposal makes complete sense. > > Frame-based hardwares accelerators such as Hantro and Rockchip VDEC > are doing the slice header parsing themselves. Therefore, the > driver is not really parsing these fields on each slice header. > > Currently, we are already using only the first slice in a frame, > as you can see from: > > if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) > reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E; > > Even if these fields are transported in the slice header, > I think it makes sense for us to split them into the decode params > (per-frame) control. > > They are really specified to be the same across all slices, > so even I'd say if a bitstream violates this, it's likely > either a corrupted bitstream or an encoder bug. > > OTOH, one thing this makes me realize is that the slice params control > is wrongly specified as an array. It is _not_. > Namely, this text > should be removed: > > This structure is expected to be passed as an array, with one > entry for each slice included in the bitstream buffer. > > As the API is really not defined that way. > > I'll remove that on next iteration. The v4l2_ctrl_h264_slice_params struct has more data than those that are deemed to be the same across all the slices. A remarkable example are the size and start_byte_offset fields.
