Re: [PATCH v5 2/4] media: Documentation: Add note about UVCH length field

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, May 26, 2025 at 04:04:03PM +0200, Ricardo Ribalda wrote:
> On Mon, 26 May 2025 at 15:49, Laurent Pinchart wrote:
> > On Fri, Apr 04, 2025 at 06:37:35AM +0000, Ricardo Ribalda wrote:
> > > The documentation currently describes the UVC length field as the "length
> > > of the rest of the block", which can be misleading. The driver limits the
> > > data copied to a maximum of 12 bytes.
> > >
> > > This change adds a clarifying sentence to the documentation to make this
> > > restriction explicit.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  Documentation/userspace-api/media/v4l/metafmt-uvc.rst | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> > > index 784346d14bbdbf28348262084d5b0646d30bd1da..42599875331c0066cf529153caccb731148023b9 100644
> > > --- a/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> > > +++ b/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> > > @@ -44,7 +44,9 @@ Each individual block contains the following fields:
> > >          them
> > >      * - :cspan:`1` *The rest is an exact copy of the UVC payload header:*
> > >      * - __u8 length;
> > > -      - length of the rest of the block, including this field
> > > +      - length of the rest of the block, including this field. Please note that
> > > +        regardless of the this value, for V4L2_META_FMT_UVC the kernel will
> >
> > "the this value" looks like a typo.
> 
> Thanks! Will fix in v2
> 
> >
> > > +        never copy more than 2-12 bytes.
> >
> > Are you saying here that length can be larger than 12, but only up to 12
> > bytes will be copied (when both SCR and PTS are present) ? If that's the
> > case, it would be better to fix the driver to clamp the length value to
> > the number of bytes actually present in the buffer.
> 
> As the documentation says, this is an exact copy of the UVC payload header.
> 
> Assuming SCR and PTS, for devices that have metadata length will be
> the real length provided by the hardware. but buf[] will only contain
> 12 bytes.
> 
> Replacing the value of length with the actual value will be a uAPI
> breakage. I do not think that is a very good idea to change it,
> considering that this number is used by parsers.

Do you think there could be userspace code that relies on the value
being larger than 12, even though the metadata after the standard UVC
block isn't present in the buffer ? Are you aware of any particular
implementation of such userspace code ?

> > >      * - __u8 flags;
> > >        - Flags, indicating presence of other standard UVC fields
> > >      * - __u8 buf[];

-- 
Regards,

Laurent Pinchart