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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <dbd7ec6c-9837-4bf3-a363-e287d075b677@oss.nxp.com>
Date: Mon, 1 Sep 2025 17:41:40 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>, 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


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.

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)