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

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

Note: I'm not 100% sure if flags for both, top and bottom fields are
needed. Any input here would be welcome.

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(-)
  

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?

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.

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
  

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
  

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

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