From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
Jernej Skrabec <jernej.skrabec@siol.net>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
Maxime Ripard <mripard@kernel.org>,
devel@driverdev.osuosl.org, Jonas Karlman <jonas@kwiboo.se>,
Greg KH <gregkh@linuxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Chen-Yu Tsai <wens@csie.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
linux-media <linux-media@vger.kernel.org>
Subject: Re: [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content
Date: Sun, 07 Jun 2020 16:29:40 -0400 [thread overview]
Message-ID: <3b1150a327aa1e6810b153a4ba1fe32cf6c98f45.camel@ndufresne.ca> (raw)
In-Reply-To: <572f23d1945a685bf899e96de147454f31674ae1.camel@ndufresne.ca>
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
prev parent reply other threads:[~2020-06-07 20:29 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-04 18:57 [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content Jernej Skrabec
2020-06-04 18:57 ` [PATCH 1/3] media: uapi: h264: update reference lists Jernej Skrabec
2020-06-05 17:08 ` Nicolas Dufresne
2020-06-05 17:13 ` Nicolas Dufresne
2020-06-05 17:26 ` Jernej Škrabec
2020-07-08 13:28 ` Ezequiel Garcia
2020-07-08 15:57 ` Jernej Škrabec
2020-07-09 0:59 ` Nicolas Dufresne
2020-06-04 18:57 ` [PATCH 2/3] media: cedrus: h264: Properly configure reference field Jernej Skrabec
2020-06-05 17:16 ` Nicolas Dufresne
2020-06-05 17:26 ` Jernej Škrabec
2020-06-04 18:57 ` [PATCH 3/3] media: cedrus: h264: Fix frame list construction Jernej Skrabec
2020-06-06 12:46 ` [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content Ezequiel Garcia
2020-06-07 20:21 ` Nicolas Dufresne
2020-06-07 20:29 ` Nicolas Dufresne [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b1150a327aa1e6810b153a4ba1fe32cf6c98f45.camel@ndufresne.ca \
--to=nicolas@ndufresne.ca \
--cc=devel@driverdev.osuosl.org \
--cc=ezequiel@vanguardiasur.com.ar \
--cc=gregkh@linuxfoundation.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=mripard@kernel.org \
--cc=paul.kocialkowski@bootlin.com \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).