Re: [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[Nicolas Dufresne]

Le dimanche 07 juin 2020 Ã 16:21 -0400, Nicolas Dufresne a Ãcrit :
> Le samedi 06 juin 2020 Ã 09:46 -0300, Ezequiel Garcia a Ãcrit :
> > Hi Jernej,
> > 
> > On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@xxxxxxxx> wrote:
> > > Currently H264 interlaced content it's not properly decoded on Cedrus.
> > > There are two reasons for this:
> > > 1. slice parameters control doesn't provide enough information
> > > 2. bug in frame list construction in Cedrus driver
> > > 
> > > As described in commit message in patch 1, references stored in
> > > reference lists should tell if reference targets top or bottom field.
> > > However, this information is currently not provided. Patch 1 adds
> > > it in form of flags which are set for each reference. Patch 2 then
> > > uses those flags in Cedrus driver.
> > > 
> > > Frame list construction is fixed in patch 3.
> > > 
> > > This solution was extensively tested using Kodi on LibreELEC with A64,
> > > H3, H5 and H6 SoCs in slightly different form (flags were transmitted
> > > in MSB bits in index).
> > > 
> > 
> > So, if I understand correctly the field needs to be passed per-reference,
> > and the current per-DPB entry is not good?
> 
> For interlaced content we reference fields separately. That's the
> reason there is 32 entries in l0/l1. Though, as we decode both fields
> in the same buffer (interleaved), the DPB indice is not sufficient to
> inform the decoder what we are referencing. Understand that a top field
> can be used to decode the next bottom field. This even make sense as
> they are closer on the capture timeline. This covers slice based
> decoders.
> 
> The flags to indicate presence of top/bottom fields in DPB array seems
> only useful to frame base decoders. This is so that decoder can avoid
> using lost fields when constructing it's own l0/l1 internally.
> 
> > If you could point at the userspace code for this, it would be interesting
> > to take a look.
> > 
> > > Note: I'm not 100% sure if flags for both, top and bottom fields are
> > > needed. Any input here would be welcome.
> > > 
> > 
> > Given enum v4l2_field is already part of the uAPI, perhaps it makes
> > sense to just reuse that for the field type? Maybe it's an overkill,
> > but it would make sense to reuse the concepts and types that
> > already exist.
> > 
> > We can still add a reserved field to make this new reference type
> > extensive.
> 
> I think it's fine to create new flag for this, as your solution would
> require extending a reference to 24bit (this patch extend to 16bits).
> Though indeed, we could combine frame and TOP and reserve more bits for
> future use.

Sorry for the oversight, the flags is 16bits, so we already use 24bits.
But looking at "enum v4l2_field", which is not a flag, seems like a
miss-fit. It would create such a confusion, as V4L2_FIELD_SEQ_TB/BT can
still be used with this interface, even though we still need to say if
we reference TOP or BOTTOM. Only V4L2_FIELD_ALTERNATE is not supported.
But as you can see, "enum v4l2_fiel" is really meant to describe the
layout of the interleaved frame rather then signalling a field
directly.

> 
> > Thanks,
> > Ezequiel
> > 
> > 
> > > Please take a look.
> > > 
> > > Best regards,
> > > Jernej
> > > 
> > > Jernej Skrabec (3):
> > >   media: uapi: h264: update reference lists
> > >   media: cedrus: h264: Properly configure reference field
> > >   media: cedrus: h264: Fix frame list construction
> > > 
> > >  .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
> > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 27 +++++++------
> > >  include/media/h264-ctrls.h                    | 12 +++++-
> > >  3 files changed, 62 insertions(+), 17 deletions(-)
> > > 
> > > --
> > > 2.27.0
> > > 
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel