On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Tuesday, 5 December 2017 15:44:34 EET Guennadi Liakhovetski wrote:
> > On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > > On Tuesday, 5 December 2017 12:56:53 EET Guennadi Liakhovetski wrote:
> > >> On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > >>> On Wednesday, 8 November 2017 18:00:14 EET Guennadi Liakhovetski wrote:
> > >>>> Some UVC video cameras contain metadata in their payload headers.
> > >>>> This patch extracts that data, adding more clock synchronisation
> > >>>> information, on both bulk and isochronous endpoints and makes it
> > >>>> available to the user space on a separate video node, using the
> > >>>> V4L2_CAP_META_CAPTURE capability and the V4L2_BUF_TYPE_META_CAPTURE
> > >>>> buffer queue type. Even though different cameras will have different
> > >>>> metadata formats, we use the same V4L2_META_FMT_UVC pixel format for
> > >>>> all of them. Users have to parse data, based on the specific camera
> > >>>> model information. This version of the patch only creates such metadata
> > >>>> nodes for cameras, specifying a UVC_QUIRK_METADATA_NODE quirk flag.
> > >>>
> > >>> I don't think this is correct anymore, as we'll use different 4CCs for
> > >>> different vendor metadata. How would you like to rephrase the commit
> > >>> message ?
> > >>
> > >> Something like
> > >>
> > >> "
> > >> Some UVC video cameras contain metadata in their payload headers. This
> > >> patch extracts that data, adding more clock synchronisation information,
> > >> on both bulk and isochronous endpoints and makes it available to the
> > >> user space on a separate video node, using the V4L2_CAP_META_CAPTURE
> > >> capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. By
> > >> default, only the V4L2_META_FMT_UVC pixel format is available from those
> > >> nodes. However, cameras can be added to the device ID table to
> > >> additionally specify their own metadata format, in which case that
> > >> format will also become available from the metadata node.
> > >> "
> > >
> > > Sounds good to me.
> > >
> > >>>> Signed-off-by: Guennadi Liakhovetski
> > >>>> <guennadi.liakhovetski@xxxxxxxxx>
> > >>>> ---
> > >>>>
> > >>>> v7: support up to two metadata formats per camera - the standard one
> > >>>> and an optional private one, if specified in device information
> > >>>>
> > >>>> drivers/media/usb/uvc/Makefile | 2 +-
> > >>>> drivers/media/usb/uvc/uvc_driver.c | 15 +++
> > >>>> drivers/media/usb/uvc/uvc_isight.c | 2 +-
> > >>>> drivers/media/usb/uvc/uvc_metadata.c | 204 +++++++++++++++++++++++++++
> > >>>> drivers/media/usb/uvc/uvc_queue.c | 41 +++++--
> > >>>> drivers/media/usb/uvc/uvc_video.c | 127 ++++++++++++++++++++--
> > >>>> drivers/media/usb/uvc/uvcvideo.h | 19 +++-
> > >>>> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > >>>> include/uapi/linux/uvcvideo.h | 26 +++++
> > >>>> 9 files changed, 416 insertions(+), 21 deletions(-)
> > >>>> create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> > >
> > > [snip]
> > >
> > >>>> diff --git a/drivers/media/usb/uvc/uvc_metadata.c
> > >>>> b/drivers/media/usb/uvc/uvc_metadata.c new file mode 100644
> > >>>> index 0000000..21eeee9
> > >>>> --- /dev/null
> > >>>> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> > >
> > > [snip]
> > >
> > >>>> +static int uvc_meta_v4l2_querycap(struct file *file, void *fh,
> > >>>> + struct v4l2_capability *cap)
> > >>>> +{
> > >>>> + struct v4l2_fh *vfh = file->private_data;
> > >>>> + struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
> > >>>> +
> > >>>> + strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> > >>>> + strlcpy(cap->card, vfh->vdev->name, sizeof(cap->card));
> > >>>> + usb_make_path(stream->dev->udev, cap->bus_info,
> > >>>> sizeof(cap->bus_info));
> > >>>> +
> > >>>> + return 0;
> > >>>> +}
> > >>>
> > >>> Do you think we could reuse uvc_ioctl_querycap() as-is ?
> > >>
> > >> AFAICS it still has
> > >>
> > >> cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> > > | chain->caps;
> > >>
> > >> in it, which doesn't suit the metadata node?
> > >
> > > I'd say this is debatable, isn't the capabilities field supposed to
> > > include all capabilities from all video nodes for the device ? chain->caps
> > > would need to include metadata capability in that case.
> > >
> > > Code reuse is not a big deal as the function is small, but getting the
> > > capabilities value right is important regardless.
> >
> > Hm, but that's how applications would naturally work - open a node, query
> > its capabilities and handle it accordingly. What good would be having
> > equal capabilities on all nodes? Why do you think that should be the case?
>
> Quoting https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-querycap.html,
>
> __u32 capabilities
>
> Available capabilities of the physical device as a whole, see Device
> Capabilities Flags. The same physical device can export multiple devices in /
> dev (e.g. /dev/videoX, /dev/vbiY and /dev/radioZ). The capabilities field
> should contain a union of all capabilities available around the several V4L2
> devices exported to userspace. For all those devices the capabilities field
> returns the same set of capabilities. This allows applications to open just
> one of the devices (typically the video device) and discover whether video,
> vbi and/or radio are also supported.
>
> __u32 device_caps
>
> Device capabilities of the opened device, see Device Capabilities Flags.
> Should contain the available capabilities of that specific device node. So,
> for example, device_caps of a radio device will only contain radio related
> capabilities and no video or vbi capabilities. This field is only set if the
> capabilities field contains the V4L2_CAP_DEVICE_CAPS capability. Only the
> capabilities field can have the V4L2_CAP_DEVICE_CAPS capability, device_caps
> will never set V4L2_CAP_DEVICE_CAPS.
Ah, ok, thanks, will try to do that then.
> By the way, on a side note, how have you tested this patch series ? I'd like
> to perform some tests on my side too, and avoid reinventing the wheel if
> possible.
I modified the capture-example tool to dequeue metadata buffers, I've also
added some primitive buffer parsing. If you want I can send you that
modified version off-list.
Thanks
Guennadi
