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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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

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