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

[Ming Qian(OSS)]

Hi Nicolas,

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