Re: [PATCH v4 1/3] media: v4l2-dev.c: Add Meta: to the name of metadata devices

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Ricardo Ribalda <ribalda@chromium.org>,
	Yunke Cao <yunkec@chromium.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	linux-media@vger.kernel.org,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/3] media: v4l2-dev.c: Add Meta: to the name of metadata devices
Date: Thu, 22 Dec 2022 00:21:16 +0200	[thread overview]
Message-ID: <Y6OG3Oar7NmReCGn@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20221221221601.1636c763@sal.lan>

Hi Mauro,

On Wed, Dec 21, 2022 at 10:16:01PM +0000, Mauro Carvalho Chehab wrote:
> Em Fri, 02 Dec 2022 18:08:17 +0100 Ricardo Ribalda escreveu:
> 
> > Devices with Metadata output (like uvc), create two video devices, one
> > for the data itself and another one for the metadata.
> > 
> > Add a "Meta: " to the beginning of the device name, as suggested by Mauro,
> > to avoid having multiple devices with the same name.
> 
> Hmm... I don't remember when I suggested it, but I suspect that such
> change could cause regressions on userspace.
> 
> I double-checked: the two apps I know that have code to detect meta
> devices (Camorama and ZBar) won't be affected by name changes, as they
> always open v4l2 devnodes before start seeing them as capture devices,
> and run VIDIOC_QUERYCAP to check for V4L2_CAP_VIDEO_CAPTURE:
> 
>    if (first_device < 0) {
>         fd = open(file, O_RDWR);
>         if (fd < 0) {
>             devices[n_devices].is_valid = FALSE;
>         } else {
>             if (ioctl(fd, VIDIOC_QUERYCAP, &vid_cap) == -1) {
>                 devices[n_devices].is_valid = FALSE;
>             } else if (!(vid_cap.device_caps & V4L2_CAP_VIDEO_CAPTURE)) {
>                 devices[n_devices].is_valid = FALSE;
>             } else {
>                 n_valid_devices++;
>                 devices[n_devices].is_valid = TRUE;
>             }
>         }
> 
>         close(fd);
>     } else {
>         devices[n_devices].is_valid = devices[first_device].is_valid;
>     }
> 
> You need to double-check if this won't cause userspace regressions, specially
> on browsers, as they have more complex logic, and seem to have already some
> logic to detect and remove meta files.

As commented separately in the same mail thread, this change would break
libcamera. For drivers that have well-known names for entities, we look
up some entities by name.

For the uvcvideo driver, which is affected by the v4l2-compliance
failure, a prefix (or suffix, or anything else) would be fine, but we
can't do that unconditionally for all devices.

> > Fixes v4l2-compliance:
> > Media Controller ioctls:
> >      fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
> >    test MEDIA_IOC_G_TOPOLOGY: FAIL
> >      fail: v4l2-test-media.cpp(394): num_data_links != num_links
> >    test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> > 
> > Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 397d553177fa..5c2c9ebb6b96 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -901,6 +901,15 @@ int __video_register_device(struct video_device *vdev,
> >  	if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
> >  		return -EINVAL;
> >  
> > +	/* Add Meta: to metadata device names */
> > +	if (vdev->device_caps &
> > +	    (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT)) {
> > +		char aux[sizeof(vdev->name)];
> > +
> > +		snprintf(aux, sizeof(aux), "Meta: %s", vdev->name);
> > +		strscpy(vdev->name, aux, sizeof(aux));
> > +	}
> > +
> >  	/* v4l2_fh support */
> >  	spin_lock_init(&vdev->fh_lock);
> >  	INIT_LIST_HEAD(&vdev->fh_list);

-- 
Regards,

Laurent Pinchart