Message-ID: <d0102df15412fc827dca3b330b10904f97a1a240.camel@ndufresne.ca>
Date: Tue, 02 Sep 2025 13:01:32 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: "Ming Qian(OSS)" <ming.qian@....nxp.com>, mchehab@...nel.org, 
	hverkuil-cisco@...all.nl
Cc: sebastian.fricke@...labora.com, shawnguo@...nel.org,
 s.hauer@...gutronix.de, 	kernel@...gutronix.de, festevam@...il.com,
 linux-imx@....com, xiahong.bao@....com, 	eagle.zhou@....com,
 imx@...ts.linux.dev, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] media: amphion: Drop the sequence header after seek for
 VC1L

Le lundi 01 septembre 2025 à 17:41 +0800, Ming Qian(OSS) a écrit :
> 
> Hi Nicolas,
> 
> > Hi,
> > 
> > Le vendredi 25 juillet 2025 à 16:07 +0800, ming.qian@....nxp.com a écrit :
> > > From: Ming Qian <ming.qian@....nxp.com>
> > > 
> > > For Simple and Main Profiles of VC-1 format stream, the amphion vpu
> > > requires driver to discard the sequence header, but insert a custom
> > > sequence start code at the beginning.
> > > The first buffer after a seek always contains only the sequence header.
> > > But vpu_vb_is_codecconfig() always return false as there is currently no
> > > flag indicating that the buffer contains only sequence header data and
> > > not frame data.
> > > So driver needs to drop the first buffer after seek, otherwise the driver
> > > will treat the sequence header as a frame, which will cause the image to
> > > be corrupted after the vpu decodes.
> > > 
> > > Signed-off-by: Ming Qian <ming.qian@....nxp.com>
> > > ---
> > >   drivers/media/platform/amphion/vpu_malone.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> > > index ba688566dffd..a4c423600d70 100644
> > > --- a/drivers/media/platform/amphion/vpu_malone.c
> > > +++ b/drivers/media/platform/amphion/vpu_malone.c
> > > @@ -1373,11 +1373,9 @@ static int vpu_malone_insert_scode_vc1_l_seq(struct malone_scode_t *scode)
> > >   	int size = 0;
> > >   	u8 rcv_seqhdr[MALONE_VC1_RCV_SEQ_HEADER_LEN];
> > >   
> > > -	if (vpu_vb_is_codecconfig(to_vb2_v4l2_buffer(scode->vb)))
> > 
> > Please remove vpu_vb_is_codecconfig() entirely, it always returns false, so its
> > miss-leading.
> > 
> 
> We have tried to define V4L2_BUF_FLAG_HEADERS_ONLY to to distinguish
> whether it only contains codec data.
> 
> https://lore.kernel.org/lkml/20221125020741.28239-1-ming.qian@nxp.com/
> 
> Although it was not accepted, we applied this flag in our Android
> project. Then in the Android platform, vpu_vb_is_codecconfig() doesn't
> always return false.
> 
> I know that's not enough reason to keep it. I just want to say that this
> vpu need to know if the buffer only contains codec header in some cases.
> And if we remove this, I need to add some comments to remind users that
> they need to pay attention here.

In all cases, this dead code have to go away, if we had noticed earlier it would
have been rejected.

Either the format document strictly requires codec data as first buffer (alone),
or you create a new format for you IP. As said, legacy codecs are ill-defined
and we need to gather information from other maintainers of IP that supports it
to fill in the doc. Perhaps this is the behavior that should have been
documented, and if this is the case, the fix is simply to put that in the
documentation.

> 
> So I tend to keep it.
> 
> 
> > > -		scode->need_data = 0;
> > > +	scode->need_data = 0;
> > >   	if (scode->inst->total_input_count)
> > >   		return 0;
> > > -	scode->need_data = 0;
> > 
> > I only remember testing this once quickly on Exynos 4 and I had no clue what
> > Annex G vs J was and most likley the MFC firmware was detecting it. Checking
> > quickly, I'm not sure GStreamer actually support both, despite the v4l2 wrapper
> > pretending. I would expect one to be used in ASF/ISOMP4/AVI, and the other used
> > in MPEG Transport Stream. GStreamer does not support VC1 in MPEG TS.
> > 
> > Have you tested this with FFMPEG ? It only maps annex G.
> > 
> > In general, I don't mind the the change if this is correct userspace behavior.
> > If ffmpeg and gstreamer don't agree though, we'll have to rethink. GStreamer
> > code back in the days was design in a way that it should not matter if the
> > header is split or not. This limitation came with lower latency decoder later,
> > but none had VC1.
> > 
> > Please test both userspace, and lets see if this solution is acceptable. ffmpeg
> > have ffplay and you can seek with your keyboard arrows.
> > 
> > Nicolas
> 
> I tested this with gstreaer, not FFMPEG,
> And I checked the gstreamer code in our repository, Then I found the
> following related code:
> 
>    } else if (g_str_equal (mimetype, "video/x-wmv")) {
>      const gchar *format = gst_structure_get_string (structure, "format");
>      if (format) {
>        if (!g_ascii_strcasecmp (format, "WVC1"))
>          fourcc = V4L2_PIX_FMT_VC1_ANNEX_G;
>        else if (!g_ascii_strcasecmp (format, "WMV3"))
>          fourcc = V4L2_PIX_FMT_VC1_ANNEX_L;
>      }
> 
> Basically it processes WMV3 into VC1_ANNEX_L, and WVC1 to VC1_ANNEX_G.
> I didn't found them in the upstream gstreamer repository.
> Now I'm not sure if it is correct userspace behavior.

Its a little concerning, since we are in the largely untested territory. Without
proper documentation and with all the downstream changes done to userspace, we
can easily endup with NXP considering this is the right mapping and let's say
Qualcomm or Samsung thinking differently. Since this is for upstream, we need to
ensure this is concistant. Have you reached to other driver maintainers already
to discuss and resolve the subject in a way it works for everyone ?

> 
> And the reason of this issue is the below code in gstreamer, that the
> v4l2decoder may only send codec data after seek.
> 
>      codec_data = self->input_state->codec_data;
> 
>      /* We are running in byte-stream mode, so we don't know the 
> headers, but
>       * we need to send something, otherwise the decoder will refuse to
>       * initialize.
>       */
>      if (codec_data) {
>        gst_buffer_ref (codec_data);
>      } else {
>        codec_data = gst_buffer_ref (frame->input_buffer);
>        processed = TRUE;
>      }

That is truncating a bit too much of the context. The "processed" boolean is set
when the codec data and frame is combined. In the case the codec data is only to
be found in caps, it will queue the codec data and immediately queue the frame
data. This is perfectly valid with the way the stateful decoder specification is
written.

In practice, GStreamer stateful decoder is multi-threaded, so it will fill the
OUTPUT queue with following frames too. What you can do to make your driver more
flexible is whenever you didn't find a frame in a buffer, merge this buffer with
the next one, and do that until there is no more space in one buffer. This way
you don't copy all the time like ring buffers do, but you can survive abitrary
splits of byte-stream.

Nicolas

> 
> Regards,
> Ming
> 
> 
> > 
> > >   
> > >   	ret = vpu_malone_insert_scode_seq(scode, MALONE_CODEC_ID_VC1_SIMPLE, sizeof(rcv_seqhdr));
> > >   	if (ret < 0)

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists