Message-ID: <b0f41153-00b5-43ca-ad7a-bfa874f9043c@oss.nxp.com>
Date: Mon, 10 Nov 2025 09:28:56 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>, mchehab@...nel.org,
 hverkuil-cisco@...all.nl
Cc: 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 v3 2/4] media: docs: dev-decoder: Trigger dynamic source
 change for colorspace


Hi Nicolas，

On 11/7/2025 10:50 PM, Nicolas Dufresne wrote:
> Hi,
> 
> I'm reviewing my backlog and this one has been stalled.
> 
> Le lundi 03 novembre 2025 à 09:45 +0800, Ming Qian(OSS) a écrit :
>> Hi Nicolas,
>>
>> On 8/1/2025 11:23 PM, Nicolas Dufresne wrote:
>>> Hi Ming,
>>>
>>> Le vendredi 18 avril 2025 à 16:54 +0800, ming.qian@....nxp.com a écrit :
>>>> From: Ming Qian <ming.qian@....nxp.com>
>>>>
>>>> If colorspace changes, the client needs to renegotiate the pipeline,
>>>> otherwise the decoded frame may not be displayed correctly.
>>>>
>>>> When a colorspace change in the stream, the decoder sends a
>>>> V4L2_EVENT_SOURCE_CHANGE event with changes set to
>>>> V4L2_EVENT_SRC_CH_COLORSPACE. After client receive this source change
>>>> event, then client can switch to the correct stream setting. And each
>>>> frame can be displayed properly.
>>>
>>> sorry for the long delay. While reading this, in any case userspace have to read
>>> the new format. Why can't userspace compare the old and new v4l2_format and
>>> decide to avoid re-allocation that way ?
>>>
>>> There is also backward compatbility issues for driver that was sending
>>> V4L2_EVENT_SRC_CH_RESOLUTION for colorspace change before. Despite the costly
>>> re-allocation, userspace only watching for V4L2_EVENT_SRC_CH_RESOLUTION would
>>> endup not updating the colorspace anymore.
>>>
>>> Combining both would be an option, but then V4L2_EVENT_SRC_CH_RESOLUTION means
>>> any v4l2_format changes, which is awkward. What do you think of leaving to
>>> userspace the task of comparing the old and new v4l2_format ?
>>>
>>> Nicolas
>>>
>>
>> Sorry for the delay response (I don't understand why I received this
>> email several months late.)
>>
>> I agree that comparing the old and new v4l2_format is feasible. And
>> there are currently four conditions that can trigger source change.
>> - coded resolution (OUTPUT width and height),
>> - visible resolution (selection rectangles),
>> - the minimum number of buffers needed for decoding,
>> - bit-depth of the bitstream has been changed.
>>
>> Therefore, comparing only v4l2_format is insufficient. This is much more
>> complicated than checking a flag. I'm not sure if existing applications
>> have already done this.
>>
>> I also think it's OK for driver to send V4L2_EVENT_SRC_CH_RESOLUTION for
>> colorspace change, This will only incur additional allocation overhead,
>> without causing any functional issues.
>>
>> Therefore, from a driver perspective:
>> - V4L2_EVENT_SRC_CH_RESOLUTION for format change      OK
>> - V4L2_EVENT_SRC_CH_RESOLUTION for colorspace chagne  OK, but
>> re-allocation cost
>> - V4L2_EVENT_SOURCE_CHANGE for colorspace change      OK
>>
>> from a client perspective:
>> - V4L2_EVENT_SRC_CH_RESOLUTION without compareing format    OK,
>> re-allocation
>> - V4L2_EVENT_SRC_CH_RESOLUTION with comparing format        OK,
>> additional support required
>> - V4L2_EVENT_SOURCE_CHANGE                                  OK,
>> additional support required
>>
>>
>> I believe that adding a V4L2_EVENT_SOURCE_CHANGE flag will help simplify
>> the process and will not cause too many backward compatibility issues.
> 
> So stepping back a little, the point is the V4L2_EVENT_SRC_CH_RESOLUTION is not
> clear enough. For backward compatibility reason we have to keep it though. So
> that gives two choices:
> 
> 1. We don't add new API, and its up to clients to check what actually changes.
> Documentation to help this would be nice.
> 
> This is easy driver side, you just emit more SOURCE_CHANGE event, and get a
> performance hit on client that have not been updated and just naively re-
> allocate.
> 
> 2. We keep V4L2_EVENT_SRC_CH_RESOLUTION, but introduce SRC_CH_REALLOC,
> SRC_CH_COLORSPACE, ...
> 
> We still endup seding more events. If the flags are exactly
> V4L2_EVENT_SRC_CH_RESOLUTION, we have a legacy driver and can't assume anything.
> Just realloc or check in the format details. Then if one or more flags of the
> other are present, use this in order to minimize the work.
> 
> In all cases, a drain operation must happen, otherwise you would not know at
> which boundary this change takes place. I will mark this as "Change Requested"
> so it is removed from my queue. If you want to continue this work, I think this
> is best plan I can think of with consideration for backward compatibility. If
> you go for 1, no new API is needed, but perhaps some better doc would help.
> 
> Nicolas
> 
> 

Thank you for your explanation. I agree that method 1 is more reasonable
and easier.
I'll prepare the V4 driver patch based on the method 1.

Regards,
Ming

>>
>> Regards,
>> Ming
>>
>>>>
>>>> So add colorspace as a trigger parameter for dynamic resolution change.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@....nxp.com>
>>>> ---
>>>> v2
>>>> - Add V4L2_EVENT_SRC_CH_COLORSPACE for colorspace source change event
>>>>
>>>>    .../userspace-api/media/v4l/dev-decoder.rst     | 17 +++++++++++++----
>>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> index ef8e8cf31f90..51d6da3eea4a 100644
>>>> --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> @@ -784,8 +784,8 @@ before the sequence started. Last of the buffers will have
>>>> the
>>>>    must check if there is any pending event and:
>>>>    
>>>>    * if a ``V4L2_EVENT_SOURCE_CHANGE`` event with ``changes`` set to
>>>> -  ``V4L2_EVENT_SRC_CH_RESOLUTION`` is pending, the `Dynamic Resolution
>>>> -  Change` sequence needs to be followed,
>>>> +  ``V4L2_EVENT_SRC_CH_RESOLUTION`` or ``V4L2_EVENT_SRC_CH_COLORSPACE`` is
>>>> pending,
>>>> +  the `Dynamic Resolution Change` sequence needs to be followed,
>>>>    
>>>>    * if a ``V4L2_EVENT_EOS`` event is pending, the `End of Stream` sequence
>>>> needs
>>>>      to be followed.
>>>> @@ -932,13 +932,17 @@ reflected by corresponding queries):
>>>>    
>>>>    * the minimum number of buffers needed for decoding,
>>>>    
>>>> -* bit-depth of the bitstream has been changed.
>>>> +* bit-depth of the bitstream has been changed,
>>>> +
>>>> +* colorspace of the bitstream has been changed.
>>>>    
>>>>    Whenever that happens, the decoder must proceed as follows:
>>>>    
>>>>    1.  After encountering a resolution change in the stream, the decoder sends a
>>>>        ``V4L2_EVENT_SOURCE_CHANGE`` event with ``changes`` set to
>>>> -    ``V4L2_EVENT_SRC_CH_RESOLUTION``.
>>>> +    ``V4L2_EVENT_SRC_CH_RESOLUTION``, or a colorspace change in the stream,
>>>> the
>>>> +    decoder sends a ``V4L2_EVENT_SOURCE_CHANGE`` event with ``changes`` set
>>>> to
>>>> +    ``V4L2_EVENT_SRC_CH_COLORSPACE``.
>>>>    
>>>>        .. important::
>>>>    
>>>> @@ -946,6 +950,11 @@ Whenever that happens, the decoder must proceed as
>>>> follows:
>>>>           values applying to the stream after the resolution change, including
>>>>           queue formats, selection rectangles and controls.
>>>>    
>>>> +.. note::
>>>> +        A ``V4L2_EVENT_SOURCE_CHANGE`` event with ``changes`` set to
>>>> +        ``V4L2_EVENT_SRC_CH_RESOLUTION`` will affect the allocation, but
>>>> +        ``V4L2_EVENT_SRC_CH_COLORSPACE`` won't.
>>>> +
>>>>    2.  The decoder will then process and decode all remaining buffers from
>>>> before
>>>>        the resolution change point.
>>>>    

Powered by blists - more mailing lists