Message-ID: <eb5928ee-1524-4b85-88c4-035b140f3028@oss.nxp.com>
Date: Wed, 3 Sep 2025 09:58:03 +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,

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

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)

Powered by blists - more mailing lists