Re: [PATCH 08/10] media: uapi: h264: Clean slice invariants syntax elements

[Maxime Ripard <maxime@cerno.tech>]

Hi,

On Mon, Jul 27, 2020 at 11:39:12AM -0300, Ezequiel Garcia wrote:
> On Sat, 2020-07-25 at 23:34 +0900, Alexandre Courbot wrote:
> > On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> 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.

I don't really get it though. The initial plan that was asked was to
mimic as much as possible the bitstream and that's what we did.

But that requirement seems to have changed now?

Even if it did change though, if this is defined as a slice parameter in
the spec, why would we want to move it to some other control entirely if
we are to keep the slice parameters control?

Especially if the reason is to make the life easier on a couple of
drivers, that's really not the point of a userspace API, and we can
always add an helper if it really shows up as a pattern.

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

That doesn't look like something we should worry about though. Or at
least, this is true for pretty much any other field in the bitstream and
we won't change those.

Maxime