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

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

Hi Nicolas,

On 2025/1/10 0:03, Nicolas Dufresne wrote:
> Le jeudi 09 janvier 2025 à 10:25 +0800, Ming Qian(OSS) a écrit :
>> Hi Nicolas,
>>
>> On 2025/1/9 3:34, Nicolas Dufresne wrote:
>>> Hi,
>>>
>>> Le mardi 07 janvier 2025 à 14:36 +0900, Ming Qian a écrit :
>>>> If colorspace changes, the client needs to renegotiate the pipeline,
>>>> otherwise the decoded frame may not be displayed correctly.
>>>>
>>>> If it can trigger an source change event, then client can switch to the
>>>> correct stream setting. And each frame can be displayed properly.
>>>>
>>>> So add colorspace as a trigger parameter for dynamic resolution change.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>> ---
>>>>    Documentation/userspace-api/media/v4l/dev-decoder.rst | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> index ef8e8cf31f90..49566569ad26 100644
>>>> --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>> @@ -932,7 +932,9 @@ 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.
>>>
>>> Did you really mean colorspace in the way this term is used in V4L2 ? What we
>>> want this event to be used for is when the capture storage size or amount
>>> changes, perhaps you mean when the capture pixelformat changes ? This will
>>> indeed happen if you change the bit-depth, subsampling (not mentioned here
>>> either) or change the way colors are repsented (RGB, YCbCr, etc.).
>>>
>>
>> I am referring to the following attributes in v4l2_pix_fmt:
>> 	__u32		colorspace;	/* enum v4l2_colorspace */
>> 	__u32		ycbcr_enc;	/* enum v4l2_ycbcr_encoding */
>> 	__u32		quantization;	/* enum v4l2_quantization */
>> 	__u32		xfer_func;	/* enum v4l2_xfer_func */
>>
>> For decoder, they are parsed from the sequence header.
>> Our issue is that when only these properties change in the middle of
>> some bitstream, but not the resolution or dpb amount, the decoder needs
>> to nofity the user.  As these properties are in v4l2_pix fmt, user need
>> to get/set them via VIDIOC_G_FMT/VIDIOC_S_FMT.
>> So in my opinion, it's reasonable to nitify user a source change event,
>> then user can call v4l_g_fmt() and renegotiate the pipeline.
>>
>> Apart from this, all I can think of is that user call v4l_g_fmt() before
>> dequeueing each frame. But I don't think this is a good idea.
> 
> I agree an event is better then this ...
> 
>>
>> As these properties are parts of the v4l2_format, I think it's
>> reasonable to handle their changes via the dynamic source change flow.
>>
>> We're currently facing some real cases on android.
>>
>> Or do you have any good suggestions? Then I can give a try.
> 
> But I think this is too much to put this under the changes
>    
>     #define V4L2_EVENT_SRC_CH_RESOLUTION            (1 << 0)
> 
> We include under "resolution" everything that affects the allocation. So perhaps
> for this one we can introduce
> 
>     #define V4L2_EVENT_SRC_CH_COLORSPACE            (2 << 0)
> 
> Of course, userspace implementation will be needed. Anyone one else have an
> opinion here ?
> 
> Nicolas
> 

I agree with you.
Defining a new colorspace changes does make things more clearer.
And it can avoid buffer reallocations. Then it will not affect the
existing flow.

Maybe I can prepare a v2 patch after doing some basic verification.

Thanks,
Ming


>>
>> Thanks,
>> Ming
>>
>>>>    
>>>>    Whenever that happens, the decoder must proceed as follows:
>>>>    
>>>
>