Message-ID: <141a14f366abe431be65e3977c27d2b39180f38e.camel@ndufresne.ca>
Date: Fri, 07 Nov 2025 09:50:27 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: "Ming Qian(OSS)" <ming.qian@....nxp.com>, 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,

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


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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists