Re: [PATCH v3 2/4] media: docs: dev-decoder: Trigger dynamic source change for colorspace

["Ming Qian(OSS)" <ming.qian@oss.nxp.com>]

Hi Nicolas,

On 8/1/2025 11:23 PM, Nicolas Dufresne wrote:
> Hi Ming,
> 
> Le vendredi 18 avril 2025 à 16:54 +0800, ming.qian@oss.nxp.com a écrit :
>> From: Ming Qian <ming.qian@oss.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.

Regards,
Ming

>>
>> So add colorspace as a trigger parameter for dynamic resolution change.
>>
>> Signed-off-by: Ming Qian <ming.qian@oss.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.
>>