Message-ID: <CANiDSCsM4mj5kE_+4cD59kznqtpg=T=3FY8QcayLHe0pSrmzow@mail.gmail.com>
Date: Thu, 3 Apr 2025 23:40:02 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Hans de Goede <hdegoede@...hat.com>, 
	Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] media: uvcvideo: Introduce V4L2_META_FMT_UVC_CUSTOM

Hi Laurent

On Fri, 14 Mar 2025 at 11:17, Ricardo Ribalda <ribalda@...omium.org> wrote:
>
> On Fri, 14 Mar 2025 at 10:09, Laurent Pinchart
> <laurent.pinchart@...asonboard.com> wrote:
> >
> > 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@...hat.com>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.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.
>
> We could argue that vendors already have the possibility to pass
> secrets to userspace:
> - A camera could add proprietary information inside the frame, only
> parseable by a closed-source application
> - They can use undocumented UVC controls
> - They can exploit documented controls to do something else that they
> are designed for.
> Given these existing possibilities, I question whether "evil metadata"
>  offers any fundamentally new capabilities that cannot be achieved
> through these established methods.
>
> If we have to talk about unfair advantage, Linux is at an unfair
> advantage right now: there is no way to use the *documented*
> information provided by the metadata. Other OSs can use it.
> The way I see it, with this artificial limitation we are not blocking
> evil vendors but punishing good users.
>
> if it makes you feel more comfortable we can start enabling only
> V4L2_META_FMT_UVC_CUSTOM (or V4L2_META_FMT_MSXU_UVC_1_5 as proposed by
> Mauro) to devices that support MSXU_CONTROL_METADATA, the future
> ChromeOS XU, or quirks. But that artificial limitation will hurt a lot
> of current cameras for no real reason.

I guess this mail has fallen through the cracks....

I have just sent a new revision that limits the new metadata format to
devices that support MSXU_CONTROL_METADATA, so it can bump the
discussion.


>
> >
> > > > 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.
>
> We can start by mapping the D4XX cameras. D4XX format follows
> Microsoft standard.
>
> Would that work for you?
>
> >
> > > > 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
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Powered by blists - more mailing lists