Re: [PATCH v3 3/3] media: uvcvideo: Introduce V4L2_META_FMT_UVC_CUSTOM

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

Hi Ricardo,

On Fri, Mar 14, 2025 at 09:28:34AM +0100, Ricardo Ribalda wrote:
> On Fri, 14 Mar 2025 at 07:35, Mauro Carvalho Chehab wrote:
> > Em Thu, 13 Mar 2025 12:06:27 +0000 Ricardo Ribalda escreveu:
> >
> > > The UVC driver provides two metadata types V4L2_META_FMT_UVC, and
> > > V4L2_META_FMT_D4XX. The only difference between the two of them is that
> > > V4L2_META_FMT_UVC only copies PTS, SCR, size and flags, and
> > > V4L2_META_FMT_D4XX copies the whole metadata section.
> > >
> > > Now we only enable V4L2_META_FMT_D4XX for the Intel D4xx family of
> > > devices, but it is useful to have the whole metadata section for any
> > > device where vendors include other metadata, such as the one described by
> > > Microsoft:
> > > https://learn.microsoft.com/en-us/windows-hardware/drivers/stream/mf-capture-metadata
> > >
> > > This patch introduces a new format V4L2_META_FMT_UVC_CUSTOM, that is
> > > identical to V4L2_META_FMT_D4XX but it is available to all the UVC
> > > devices.
> > >
> > > Suggested-by: Hans de Goede <hdegoede@redhat.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  .../userspace-api/media/v4l/meta-formats.rst       |  1 +
> > >  .../userspace-api/media/v4l/metafmt-uvc-custom.rst | 31 +++++++++++++++++
> > >  MAINTAINERS                                        |  1 +
> > >  drivers/media/usb/uvc/uvc_metadata.c               | 40 ++++++++++++++++++----
> > >  drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
> > >  include/uapi/linux/videodev2.h                     |  1 +
> > >  6 files changed, 69 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > > index 86ffb3bc8ade2e0c563dd84441572ecea1a571a6..9fd83f4a3cc8509702a2a9f032fdc04bf6c6d1bc 100644
> > > --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
> > > +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > > @@ -19,6 +19,7 @@ These formats are used for the :ref:`metadata` interface only.
> > >      metafmt-pisp-fe
> > >      metafmt-rkisp1
> > >      metafmt-uvc
> > > +    metafmt-uvc-custom
> > >      metafmt-vivid
> > >      metafmt-vsp1-hgo
> > >      metafmt-vsp1-hgt
> > > diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc-custom.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc-custom.rst
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..9f150fc2b6f379cc4707ff45041dd014956ae11a
> > > --- /dev/null
> > > +++ b/Documentation/userspace-api/media/v4l/metafmt-uvc-custom.rst
> > > @@ -0,0 +1,31 @@
> > > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > > +
> > > +.. _v4l2-meta-fmt-uvc-custom:
> > > +
> > > +*********************************
> > > +V4L2_META_FMT_UVC_CUSTOM ('UVCC')
> > > +*********************************
> > > +
> > > +UVC Custom Payload Metadata.
> > > +
> > > +
> > > +Description
> > > +===========
> > > +
> > > +V4L2_META_FMT_UVC_CUSTOM buffers follow the metadata buffer layout of
> > > +V4L2_META_FMT_UVC with the only difference that it includes all the UVC
> > > +metadata, not just the first 2-12 bytes.
> > > +
> > > +The most common metadata format is the one proposed by Microsoft(R)'s UVC
> > > +extension [1_], but other vendors might have different formats.
> > > +
> > > +Applications might use information from the Hardware Database (hwdb)[2_] to
> > > +process the camera's metadata accordingly.
> >
> > Having something like that at the userspace API shouldn't be handled
> > lightly. This sounds to me that passing a blank check for vendors to stream
> > whatever they want without any requirements to provide and sort of
> > documentation for the usersace to decode it.
> 
> As HdG previously mentioned, all the processing is done in the camera
> so the metadata is not going to hide highly secret required for
> processing:
> https://lore.kernel.org/linux-media/67c1a857-7656-421f-994c-751709b6ae01@redhat.com/

Without judging whether or not such an undocumented format should be
supported by the driver, a correction is needed here: the issue is not
"secrets required for processing", but giving closed-source application
an unfair advantage.

> > Also, it would be hard for userspace to distinguish what metatata
> > is contained for a random UVC camera. Please let's not do that.
> 
> Userspace will use hwdb info to properly parse the metadata.

I don't have experience with that, so I would like to see the effort
being started on hwdb support to see how it will look like before we
merge this patch. A few cameras should be added as examples, and a
stategy to ensure the hwdb will be properly populated should be
proposed.

> > As the specific issue here is to support an already known extension,
> > which is already documented, Just add an specific format for it, e.g.
> > you could add something like that at the documentation:
> 
> The problem here is how do we know from the driver if a device
> supports V4L2_META_FMT_MSXU_UVC_1_5 or not.
> 
> In Windows it seems that vendors add that information to the device
> .inf file. That is equivalent to the hwdb proposal.
> In ChromeOS we are trying to push vendors to use an extension saying
> if there is metadata or not. But that will take some time to land and
> there are thousands of modules out there not ChromeOS compliant.
> 
> >
> >         V4L2_META_FMT_MSXU_UVC_1_5
> >            Microsoft extensions to USB Video Class 1.5 specification.
> >
> >            For more details, see: https://learn.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
> >
> > And then add the corresponding format to V4L2 API.

-- 
Regards,

Laurent Pinchart