[PATCH] media: amphion: Drop the sequence header after seek for VC1L

[PATCH] media: amphion: Drop the sequence header after seek for VC1L

[ming.qian]

From: Ming Qian <ming.qian@oss.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@oss.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)))
-		scode->need_data = 0;
+	scode->need_data = 0;
 	if (scode->inst->total_input_count)
 		return 0;
-	scode->need_data = 0;
 
 	ret = vpu_malone_insert_scode_seq(scode, MALONE_CODEC_ID_VC1_SIMPLE, sizeof(rcv_seqhdr));
 	if (ret < 0)
-- 
2.34.1

Re: [PATCH] media: amphion: Drop the sequence header after seek for VC1L

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

Hi,

Le vendredi 25 juillet 2025 à 16:07 +0800, ming.qian@oss.nxp.com a écrit :
> From: Ming Qian <ming.qian@oss.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@oss.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.

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] media: amphion: Drop the sequence header after seek for VC1L

[Ming Qian(OSS)]

Hi Nicolas,

> Hi,
> 
> Le vendredi 25 juillet 2025 à 16:07 +0800, ming.qian@oss.nxp.com a écrit :
>> From: Ming Qian <ming.qian@oss.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@oss.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.

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.

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;
     }

Regards,
Ming


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

Re: [PATCH] media: amphion: Drop the sequence header after seek for VC1L

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 6935 bytes --]

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@oss.nxp.com a écrit :
> > > From: Ming Qian <ming.qian@oss.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@oss.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)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] media: amphion: Drop the sequence header after seek for VC1L

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4506 bytes --]

Hi,

just adding to my anwer,

Le mardi 02 septembre 2025 à 13:01 -0400, Nicolas Dufresne a écrit :
> Le lundi 01 septembre 2025 à 17:41 +0800, Ming Qian(OSS) a écrit :
> > 
> > 

[...}

> > > 
> > > 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 ?

So I checked Samsung implementation and your interpretation seems to be the
same. They MAP VC1_ANNEX_G to VC1 Advanced Profile Decoding in their
driver. Venus drivers does not care and just map both to VC1.

If I quote here Wikipedia's Window Media Video page:
   The Simple and Main profile levels in WMV 9 are compliant with the same
   profile levels in the VC-1 specification.[13] The Advanced Profile in VC-1 is
   implemented in a new WMV format called Windows Media Video 9 Advanced
   Profile. It improves compression efficiency for interlaced content and is
   made transport-independent, making it able to be encapsulated in an MPEG
   transport stream or RTP packet format. The format is not compatible with
   previous WMV 9 formats, however.[14]


It matches well with the fact Annex G introduce start codes and inline sequence
headers, since you absolutely need that to stream over MPEG TS. GStreamer uses
video/x-wmv as a family, and format=WVC1 for the advanced profiles, and WMV3 for
everything else it supports.

I think you should go ahead and upstream this mapping fix into GStreamer. V4L2
documentation should perhaps mention "Advanced Profile" to help devs.

Though, this gives me the impression that codec_data can be inline for ANNEX G.

Nicolas

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] media: amphion: Drop the sequence header after seek for VC1L

[Ming Qian(OSS)]

Hi Nicolas,

> 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@oss.nxp.com a écrit :
>>>> From: Ming Qian <ming.qian@oss.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@oss.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.
> 

OK, I'll prepare a patch that remove vpu_vb_is_codecconfig().

>>
>> 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 ?
> 

Understand.

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

Does this mean that the driver needs to parse the data in the buffer?
I think I can drop this patch first, then think about how to deal with 
this problem.

Thank you for your patience and guidance.

Regards,
Ming

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

Re: [PATCH] media: amphion: Drop the sequence header after seek for VC1L

[Ming Qian(OSS)]

Hi Nicolas,

On 9/3/2025 1:36 AM, Nicolas Dufresne wrote:
> Hi,
> 
> just adding to my anwer,
> 
> Le mardi 02 septembre 2025 à 13:01 -0400, Nicolas Dufresne a écrit :
>> Le lundi 01 septembre 2025 à 17:41 +0800, Ming Qian(OSS) a écrit :
>>>
>>>
> 
> [...}
> 
>>>>
>>>> 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 ?
> 
> So I checked Samsung implementation and your interpretation seems to be the
> same. They MAP VC1_ANNEX_G to VC1 Advanced Profile Decoding in their
> driver. Venus drivers does not care and just map both to VC1.
> 
> If I quote here Wikipedia's Window Media Video page:
>     The Simple and Main profile levels in WMV 9 are compliant with the same
>     profile levels in the VC-1 specification.[13] The Advanced Profile in VC-1 is
>     implemented in a new WMV format called Windows Media Video 9 Advanced
>     Profile. It improves compression efficiency for interlaced content and is
>     made transport-independent, making it able to be encapsulated in an MPEG
>     transport stream or RTP packet format. The format is not compatible with
>     previous WMV 9 formats, however.[14]
> 
> 
> It matches well with the fact Annex G introduce start codes and inline sequence
> headers, since you absolutely need that to stream over MPEG TS. GStreamer uses
> video/x-wmv as a family, and format=WVC1 for the advanced profiles, and WMV3 for
> everything else it supports.
> 
> I think you should go ahead and upstream this mapping fix into GStreamer. V4L2
> documentation should perhaps mention "Advanced Profile" to help devs.
> 
> Though, this gives me the impression that codec_data can be inline for ANNEX G.
> 
> Nicolas
> 

I also checked the ffmpeg, it also treats VC1 as the advanced profiles,
and WMV3 for Simple and Main Profiles.

And below is from the VC-1's wiki page:

	WMV3
	The Simple and Main Profiles of VC-1 remained completely
	faithful to the existing WMV3 implementation, making WMV3
	bitstreams fully VC-1 compliant. The WMV3 codec was designed to
	primarily support progressive encoding for computer displays. An
	interlaced encoding mode was implemented, but quickly became
	deprecated when Microsoft started implementing WMV Advanced
	Profile. Whereas WMV3 progressive encoding was implemented using
	the YUV 4:2:0 color sampling scheme, the deprecated interlaced
	mode was implemented using the less common YUV 4:1:1 sampling
	scheme.

	The Windows Media Video 9 (WMV3) codec implements the Simple and
	Main modes of the VC-1 codec standard, providing high-quality
	video for streaming and downloading. "It provides support for a
	wide range of bit rates, from high-definition content at
	one-half to one-third the bit rate of MPEG-2, to low-bit-rate
	Internet video delivered over a dial-up modem. This codec also
	supports professional-quality downloadable video with two-pass
	and variable bit rate (VBR) encoding."[7]


	WVC1
	WVC1, also known as Windows Media Video 9 Advanced Profile,
	implements a more recent and fully compliant Advanced Profile of
	the VC-1 codec standard. It offers support for interlaced
	content and is transport independent. With the previous version
	of the Windows Media Video 9 Series codec, users could deliver
	progressive content at data rates as low as one-third that of
	the MPEG-2 codec and still get equivalent or comparable quality
	to MPEG-2[citation needed]. The Windows Media Video 9 Advanced
	Profile codec also offers this same improvement in encoding
	efficiency with interlaced contents[citation needed]. A decoder
	for WVC1 is included in Windows Media Player 11, which is
	bundled with Windows Vista and is available as a download for
	Windows XP. This implementation is supported in Microsoft
	Silverlight.

And we will prepare a gstreamer patch first, and also try to add a
description of the advanced profile to V4L2 Documentation.

Thank you again for your patience.

Regards,
Ming

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