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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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@siol.net> 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.

> 
> 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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel