Re: [PATCH v4] uvcvideo: add a metadata device node

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

Hi Guennadi,

Thank you for the patch.

> 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.

The issue we still haven't addressed is how to ensure that vendors will
document their metadata format :-S

> This version of the patch only creates such metadata nodes for cameras,
> specifying a UVC_QUIRK_METADATA_NODE quirk flag.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
> 
> v4:
> - add support for isochronous cameras. Metadata is now collected from as 
> many payloads as they fit in the buffer
> - add a USB Start Of Frame and a system timestamp to each metadata block 
> for user-space clock synchronisation
> - use a default buffer size of 1024 bytes
> 
> Thanks to Laurent for patient long discussions and to everybody, who 
> helped me conduct all the investigation into various past, present and 
> future UVC cameras :-)
> 
>  drivers/media/usb/uvc/Makefile       |   2 +-
>  drivers/media/usb/uvc/uvc_driver.c   |   4 +
>  drivers/media/usb/uvc/uvc_isight.c   |   2 +-
>  drivers/media/usb/uvc/uvc_metadata.c | 158 +++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvc_queue.c    |  68 ++++++++++-----
>  drivers/media/usb/uvc/uvc_video.c    | 101 +++++++++++++++++++---
>  drivers/media/usb/uvc/uvcvideo.h     |  23 ++++-
>  drivers/media/v4l2-core/v4l2-ioctl.c |   1 +
>  include/uapi/linux/uvcvideo.h        |  19 +++++
>  include/uapi/linux/videodev2.h       |   3 +
>  10 files changed, 347 insertions(+), 34 deletions(-)
>  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c
> index 70842c5..9f23dcf 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1880,6 +1880,7 @@ static void uvc_unregister_video(struct uvc_device
> *dev)
>  			continue;
>  
>  		video_unregister_device(&stream->vdev);
> +		video_unregister_device(&stream->meta.vdev);
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
> @@ -1941,6 +1942,9 @@ static int uvc_register_video(struct uvc_device *dev,
>  		return ret;
>  	}
>  
> +	/* Register a metadata node. */
> +	uvc_meta_register(stream);

This can fail, you should handle errors.

>  	if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
>  	else

[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..876badd
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -0,0 +1,158 @@
> +/*
> + *      uvc_metadata.c  --  USB Video Class driver - Metadata handling
> + *
> + *      Copyright (C) 2016
> + *          Guennadi Liakhovetski (guennadi.liakhovetski@intel.com)
> + *
> + *      This program is free software; you can redistribute it and/or
> modify
> + *      it under the terms of the GNU General Public License as published
> by
> + *      the Free Software Foundation; either version 2 of the License, or
> + *      (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/videobuf2-vmalloc.h>
> +
> +#include "uvcvideo.h"
> +
> +/* -----------------------------------------------------------------------
> + * videobuf2 Queue Operations
> + */
> +
> +static struct vb2_ops uvc_meta_queue_ops = {
> +	.queue_setup = uvc_queue_setup,
> +	.buf_prepare = uvc_buffer_prepare,
> +	.buf_queue = uvc_buffer_queue,
> +	.wait_prepare = vb2_ops_wait_prepare,
> +	.wait_finish = vb2_ops_wait_finish,
> +	.stop_streaming = uvc_stop_streaming,

This looks unbalanced without a start_streaming operation. I know that
you've modified the uvc_stop_streaming() function to not stop the video
stream for metadata video nodes, but I think the code would be clearer
if you implemented a uvc_meta_stop_streaming() function 
for 

> +};
> +
> +/* ------------------------------------------------------------------------
> + * V4L2 ioctls
> + */
> +
> +static int meta_v4l2_querycap(struct file *file, void *fh,
> +			      struct v4l2_capability *cap)

For consistency with the rest of the code, could you prefix this and the
next function with uvc_ ?

> +{
> +	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;
> +}

[snip]

> +int uvc_meta_register(struct uvc_streaming *stream)
> +{
> +	struct uvc_device *dev = stream->dev;
> +	struct uvc_meta_device *meta = &stream->meta;
> +	struct video_device *vdev = &meta->vdev;
> +	struct uvc_video_queue *quvc = &meta->queue;
> +	struct vb2_queue *queue = &quvc->queue;
> +	int ret;
> +
> +	mutex_init(&quvc->mutex);
> +	spin_lock_init(&quvc->irqlock);
> +	INIT_LIST_HEAD(&quvc->irqqueue);
> +
> +	/*
> +	 * We register metadata device nodes only if the METADATA_NODE quirk
> is
> +	 * set and only on interfaces with bulk endpoints. To meaningfully
> +	 * support interfaces with isochronous endpoints, we need to collect
> +	 * headers from all payloads, comprising a single frame. For that we
> +	 * need to know the maximum number of such payloads per frame to be
> able
> +	 * to calculate the buffer size. Currently this information is
> +	 * unavailable. A proposal should be made to the UVC committee to add
> +	 * this information to camera descriptors.
> +	 */
> +	if (!(dev->quirks & UVC_QUIRK_METADATA_NODE) ||
> +	    dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT)
> +		return 0;
> +
> +	vdev->v4l2_dev = &dev->vdev;
> +	vdev->fops = &uvc_meta_fops;
> +	vdev->ioctl_ops = &uvc_meta_ioctl_ops;
> +	vdev->release = video_device_release_empty;

Please, no. You'll end up crashing the kernel if an application has the meta
devnode open when the device is disconnected. Proper refcounting is needed.

> +	vdev->prio = &stream->chain->prio;
> +	vdev->vfl_dir = VFL_DIR_RX;
> +	vdev->queue = queue;
> +	vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> +	strlcpy(vdev->name, dev->name, sizeof(vdev->name));
> +
> +	video_set_drvdata(vdev, stream);
> +
> +	/* Initialize the video buffer queue. */
> +	queue->type = V4L2_BUF_TYPE_META_CAPTURE;
> +	queue->io_modes = VB2_MMAP | VB2_USERPTR;
> +	queue->drv_priv = quvc;
> +	queue->buf_struct_size = sizeof(struct uvc_buffer);
> +	queue->ops = &uvc_meta_queue_ops;
> +	queue->mem_ops = &vb2_vmalloc_memops;
> +	queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> +		| V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> +	queue->lock = &quvc->mutex;
> +	ret = vb2_queue_init(queue);
> +	if (ret < 0)
> +		return ret;

How about calling uvc_queue_init() instead of duplicating the code ? You would
just need to extend the function with a parameter for the queue ops. Or,
possibly even better, you could move the definition of the queue ops to
uvc_queue.c. That way you won't have to expose a bunch of currently static
queue functions.

Similarly, it looks like you're duplicating code from uvc_register_video(),
which I believe could be shared.

> +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret < 0)
> +		uvc_printk(KERN_ERR, "Failed to register metadata device (%d).
\n", ret);

This line is over 80 characters.

> +	return ret;
> +}
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c
> index aa21997..77dedbc 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -74,13 +74,26 @@ static void uvc_queue_return_buffers(struct
> uvc_video_queue *queue,
>   * videobuf2 queue operations
>   */
>  
> -static int uvc_queue_setup(struct vb2_queue *vq,
> -			   unsigned int *nbuffers, unsigned int *nplanes,
> -			   unsigned int sizes[], struct device *alloc_devs[])
> +int uvc_queue_setup(struct vb2_queue *vq,
> +		    unsigned int *nbuffers, unsigned int *nplanes,
> +		    unsigned int sizes[], struct device *alloc_devs[])
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> -	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> -	unsigned size = stream->ctrl.dwMaxVideoFrameSize;
> +	struct uvc_streaming *stream;
> +	unsigned int size;
> +
> +	switch (vq->type) {
> +	case V4L2_BUF_TYPE_META_CAPTURE:
> +		size = UVC_METATADA_BUF_SIZE;
> +
> +		if (*nplanes && *nplanes != 1)
> +			return -EINVAL;

As the driver doesn't support any multi-plane format, should this check be
moved out of the switch ? You should then add it in a separate patch before
this one.

> +		break;

Could you add a blank line between break statements and the next case to be
consistent with the driver's coding style ?

> +	default:
> +		stream = uvc_queue_to_stream(queue);
> +		size = stream->ctrl.dwMaxVideoFrameSize;

While not strictly needed, the driver uses break statements in the default
cases. Could you please do so here (and below) too ?

> +	}
>  
>  	/* Make sure the image size is large enough. */
>  	if (*nplanes)
> @@ -90,34 +103,47 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  	return 0;
>  }
>  
> -static int uvc_buffer_prepare(struct vb2_buffer *vb)
> +int uvc_buffer_prepare(struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
>  	struct uvc_buffer *buf = uvc_vbuf_to_buffer(vbuf);
>  
> -	if (vb->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> -	    vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) {
> -		uvc_trace(UVC_TRACE_CAPTURE, "[E] Bytes used out of bounds
> .\n");
> -		return -EINVAL;
> -	}
> -
>  	if (unlikely(queue->flags & UVC_QUEUE_DISCONNECTED))
>  		return -ENODEV;
>  
> +	switch (vb->type) {
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> +		if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) {
> +			uvc_trace(UVC_TRACE_CAPTURE,
> +				  "[E] Bytes used out of bounds.\n");
> +			return -EINVAL;
> +		}
> +
> +		buf->bytesused = vb2_get_plane_payload(vb, 0);
> +
> +		break;
> +	case V4L2_BUF_TYPE_META_CAPTURE:
> +		if (vb->num_planes != 1 ||
> +		    vb2_plane_size(vb, 0) < UVC_METATADA_BUF_SIZE) {

Can this happen, given that queue_setup rejects buffers that verify those
conditions ?

> +			uvc_trace(UVC_TRACE_CAPTURE,
> +				  "[E] Invalid buffer configuration.\n");
> +			return -EINVAL;
> +		}
> +		/* Fall through */
> +	default:
> +		buf->bytesused = 0;
> +	}
> +
>  	buf->state = UVC_BUF_STATE_QUEUED;
>  	buf->error = 0;
>  	buf->mem = vb2_plane_vaddr(vb, 0);
>  	buf->length = vb2_plane_size(vb, 0);
> -	if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)

Unless I was mistaken in my previous comment, I think you only need to change
the condition to vb->type != V4L2_BUF_TYPE_VIDEO_OUTPUT, no other change to
the function should be needed.

> -		buf->bytesused = 0;
> -	else
> -		buf->bytesused = vb2_get_plane_payload(vb, 0);
>  
>  	return 0;
>  }
>  
> -static void uvc_buffer_queue(struct vb2_buffer *vb)
> +void uvc_buffer_queue(struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> @@ -169,13 +195,15 @@ static int uvc_start_streaming(struct vb2_queue *vq,
> unsigned int count)
>  	return ret;
>  }
>  
> -static void uvc_stop_streaming(struct vb2_queue *vq)
> +void uvc_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> -	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>  	unsigned long flags;
>  
> -	uvc_video_enable(stream, 0);
> +	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
> +		struct uvc_streaming *stream = uvc_queue_to_stream(queue);

Hasn't checkpatch complained about a missing blank line here ?

> +		uvc_video_enable(stream, 0);
> +	}
>  
>  	spin_lock_irqsave(&queue->irqlock, flags);
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index fb86d6a..3a54439 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1152,8 +1152,76 @@ static void uvc_video_validate_buffer(const struct
> uvc_streaming *stream,
>  /*
>   * Completion handler for video URBs.
>   */

I think this function would be better placed in a new "Metadata" section right
before the "URB handling" section.

> +
> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> +			struct uvc_buffer *buf, struct uvc_buffer *meta_buf,

You might want to call buf video_buf for clarity.

> +			u8 *mem, unsigned int length)
> +{
> +	struct uvc_meta_buf *meta;
> +	size_t len_std = 2;
> +	bool has_pts, has_scr;
> +	u8 *scr;
> +
> +	if (!meta_buf || length == 2 ||
> +	    meta_buf->length - meta_buf->bytesused <
> +	    length + sizeof(meta->ts) + sizeof(meta->sof) ||
> +	    length != mem[0])

I don't think the last condition can occur here, as it should be caught in
uvc_video_decode_start() and result in the payload being skipped.

> +		return;
> +
> +	has_pts = mem[1] & UVC_STREAM_PTS;
> +	has_scr = mem[1] & UVC_STREAM_SCR;
> +
> +	if (has_pts) {
> +		len_std += 4;
> +		scr = mem + 6;
> +	} else {
> +		scr = mem + 2;
> +	}
> +
> +	if (has_scr)
> +		len_std += 6;
> +
> +	if (length == len_std && (!has_scr ||
> +				  !memcmp(scr, stream->clock.last_scr, 6)))
> +		return;

I think a comment would be useful to explain the strategy. I understand it now
as we've discussed it recently, but I'm not sure I still will in 6 months :-)
You could add a comment block at the beginning of the function, similarly to
uvc_video_decode_start().

> +
> +	meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + meta_buf
> ->bytesused);
> +	uvc_video_get_ts(&meta->ts);
> +	meta->sof = usb_get_current_frame_number(stream->dev->udev);

You should disable local interrupts around those two calls to ensure that they
will be as close as possible.

> +
> +	if (!meta_buf->bytesused) {
> +		meta_buf->buf.sequence = buf->buf.sequence;
> +		meta_buf->buf.field = buf->buf.field;
> +		meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;

I wonder whether it wouldn't be clearer to do this in
uvc_video_step_buffers(). You wouldn't need to check bytesused then, and the
code would be more future-proof in case the driver gets changed to set one of
those fields later than uvc_video_decode_start().

> +	}
> +
> +	if (has_scr)
> +		memcpy(stream->clock.last_scr, scr, 6);
> +
> +	memcpy(&meta->length, mem, length);
> +	meta_buf->bytesused += length + sizeof(meta->ts) + sizeof(meta->sof);
> +
> +	uvc_trace(UVC_TRACE_FRAME,
> +		  "%s(): t-sys %lu.%09lus, SOF %u, len %u, flags 0x%x, PTS %u,
> STC %u frame SOF %u\n",
> +		  __func__, meta->ts.tv_sec, meta->ts.tv_nsec, meta->sof,
> +		  meta->length, meta->flags, has_pts ? *(u32 *)meta->buf : 0,
> +		  has_scr ? *(u32 *)scr : 0,
> +		  has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);
> +}
> +
> +static struct uvc_buffer *uvc_video_step_buffers(struct uvc_streaming
> *stream,
> +			struct uvc_buffer *buf, struct uvc_buffer **meta_buf)

Wouldn't it better to make the function more symmetrical by passing **buf the
same way we pass **meta_buf ? And as commented above, I think I would rename
buf to video_buf.

> +{
> +	if (*meta_buf) {
> +		(*meta_buf)->state = UVC_BUF_STATE_READY;
> +		*meta_buf = uvc_queue_next_buffer(&stream->meta.queue,
> +						  *meta_buf);
> +	}
> +	return uvc_queue_next_buffer(&stream->queue, buf);
> +}
> +
>  static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
>  *stream,
> -	struct uvc_buffer *buf)
> +			struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
>  {
>  	u8 *mem;
>  	int ret, i;

[snip]

> diff --git a/drivers/media/usb/uvc/uvcvideo.h 
b/drivers/media/usb/uvc/uvcvideo.h
> index 15e415e..d06e2d6 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -185,6 +185,7 @@
>  #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
>  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  #define UVC_QUIRK_FORCE_Y8		0x00000800
> +#define UVC_QUIRK_METADATA_NODE		0x00001000

I notice that you don't set the quirk for any device. What's your plan there,
will there be more patches ?

>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001

[snip]

> @@ -546,6 +556,8 @@ struct uvc_streaming {
>  		u16 last_sof;
>  		u16 sof_offset;
>  
> +		u8 last_scr[6];
> +
>  		spinlock_t lock;
>  	} clock;
>  };
> @@ -684,6 +696,12 @@ extern unsigned long uvc_queue_get_unmapped_area(struct 
uvc_video_queue *queue,
>  		unsigned long pgoff);
>  #endif
>  extern int uvc_queue_allocated(struct uvc_video_queue *queue);
> +extern int uvc_queue_setup(struct vb2_queue *vq,
> +			   unsigned int *nbuffers, unsigned int *nplanes,
> +			   unsigned int sizes[], struct device *alloc_devs[]);
> +extern int uvc_buffer_prepare(struct vb2_buffer *vb);
> +extern void uvc_buffer_queue(struct vb2_buffer *vb);
> +extern void uvc_stop_streaming(struct vb2_queue *vq);

You can drop the extern keyword, it's not needed.

>  static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
>  {
>  	return vb2_is_streaming(&queue->queue);

[snip]

> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 3b08186..c503dcc 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -67,4 +67,23 @@ struct uvc_xu_control_query {
>  #define UVCIOC_CTRL_MAP		_IOWR('u', 0x20, struct 
uvc_xu_control_mapping)
>  #define UVCIOC_CTRL_QUERY	_IOWR('u', 0x21, struct uvc_xu_control_query)
>  
> +/*
> + * Metadata node
> + */
> +
> +/*

Could you please use kerneldoc comments to document this structure ?

> + * UVC metadata nodes fill buffers with possibly multiple instances of the
> + * following struct. The first two fields are added by the driver, they can
> be
> + * used for clock synchronisation. The rest is an exact copy of a UVC
> payload
> + * header. Only complete objects with complete buffers are included.
> Therefore
> + * it's always sizeof(meta->ts) + sizeof(meta->sof) + meta->length bytes
> large.
> + */
> +struct uvc_meta_buf {
> +	struct timespec ts;

timespec has a different size on 32-bit and 64-bit architectures, so there
could be issues on 32-bit userspace running on a 64-bit kernel.

Additionally, on 32-bit platforms, timespec is not year 2038-safe. I thought
that timespec64 was exposed to userspace nowadays, but it doesn't seem to be
the case. I'm not sure how to handle this.

> +	__u16 sof;
> +	__u8 length;
> +	__u8 flags;
> +	__u8 buf[];

I had imagined inserting the ts + sof after the standard header, but this
probably makes more sense.

> +};
> +
>  #endif
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 45cf735..9b0cc7e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -683,6 +683,9 @@ struct v4l2_pix_format {
>  #define V4L2_META_FMT_VSP1_HGO    v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car
>  VSP1 1-D Histogram */
>  #define V4L2_META_FMT_VSP1_HGT    v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car
>  VSP1 2-D Histogram */
>  
> +/* Meta-data formats */
> +#define V4L2_META_FMT_UVC         v4l2_fourcc('U', 'V', 'C', 'H') /* UVC 
Payload Header metadata */

This should be documented in Documentation/media/, and probably split to a
separate patch.

> +
>  /* priv field value to indicates that subsequent fields are valid. */
>  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
>  

-- 
Regards,

Laurent Pinchart