Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

On Thu, 8 Dec 2016, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Thursday 08 Dec 2016 14:34:46 Guennadi Liakhovetski wrote:
> > On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > > On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> > >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >>> On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> >  On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >  +  /*
> >  +   * Register a metadata node. TODO: shall this only be enabled
> >  for some
> >  +   * cameras?
> >  +   */
> >  +  if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >  +  uvc_meta_register(stream);
> >  +
> > >>> 
> > >>> I think so, only for the cameras that can produce metadata.
> > >> 
> > >> Every UVC camera produces metadata, but most cameras only have
> > >> standard fields there. Whether we should stream standard header
> > >> fields from the metadata node will be discussed later. If we do
> > >> decide to stream standard header fields, then every USB camera gets
> > >> metadata nodes. If we decide not to include standard fields, how do
> > >> we know whether the camera has any private fields in headers
> > >> without streaming from it? Do you want a quirk for such cameras?
> > > 
> > > Unless they can be detected in a standard way that's probably the
> > > best solution.
> > 
> > How about a module parameter with a list of VID:PID pairs?
> 
> I'd like something that works out of the box for end-users, at least in most 
> cases. There's already a way to set quirks through a module parameter, and I 
> think I'd accept a patch extending that it make it VID:PID dependent.

Ok, that helps already, sure.

> That's 
> an acceptable solution for testing, but should not be considered as the way 
> to 
> go for production.
> 
> > The problem with the quirk is, that as vendors produce multiple cameras with
> > different PIDs they will have to push patches for each such camera.
> 
> How many such devices do you expect ?

No idea, significantly more than 2, let's say :) But well, you already can 
count a few RealSense USB / UVC cameras on the market.

Concerning metadata for isochronous endpoints. I actually don't know what 
to do with it. I don't have any such cameras with non-standard metadata. 
For the standard metadata it would probably be enough to get either the 
first or the last or the middle payload. Collecting all of them seems 
redundant to me. Maybe I could for now only enable metadata nodes for bulk 
endpoints. Would that be acceptable?

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Thursday 08 Dec 2016 14:34:46 Guennadi Liakhovetski wrote:
> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >>> On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
>  On Tue, 6 Dec 2016, Laurent Pinchart wrote:
>  +/*
>  + * Register a metadata node. TODO: shall this only be enabled
>  for some
>  + * cameras?
>  + */
>  +if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
>  +uvc_meta_register(stream);
>  +
> >>> 
> >>> I think so, only for the cameras that can produce metadata.
> >> 
> >> Every UVC camera produces metadata, but most cameras only have
> >> standard fields there. Whether we should stream standard header
> >> fields from the metadata node will be discussed later. If we do
> >> decide to stream standard header fields, then every USB camera gets
> >> metadata nodes. If we decide not to include standard fields, how do
> >> we know whether the camera has any private fields in headers
> >> without streaming from it? Do you want a quirk for such cameras?
> > 
> > Unless they can be detected in a standard way that's probably the
> > best solution.
> 
> How about a module parameter with a list of VID:PID pairs?

I'd like something that works out of the box for end-users, at least in most 
cases. There's already a way to set quirks through a module parameter, and I 
think I'd accept a patch extending that it make it VID:PID dependent. That's 
an acceptable solution for testing, but should not be considered as the way to 
go for production.

> The problem with the quirk is, that as vendors produce multiple cameras with
> different PIDs they will have to push patches for each such camera.

How many such devices do you expect ?

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

One more question:

On Tue, 6 Dec 2016, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> > On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > > On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> > >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >> +/*
> > >> + * Register a metadata node. TODO: shall this only be enabled
> > >> for some
> > >> + * cameras?
> > >> + */
> > >> +if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > >> +uvc_meta_register(stream);
> > >> +
> > > 
> > > I think so, only for the cameras that can produce metadata.
> >  
> >  Every UVC camera produces metadata, but most cameras only have standard
> >  fields there. Whether we should stream standard header fields from the
> >  metadata node will be discussed later. If we do decide to stream
> >  standard header fields, then every USB camera gets metadata nodes. If
> >  we decide not to include standard fields, how do we know whether the
> >  camera has any private fields in headers without streaming from it? Do
> >  you want a quirk for such cameras?
> > >>> 
> > >>> Unless they can be detected in a standard way that's probably the best
> > >>> solution.

How about a module parameter with a list of VID:PID pairs? The problem 
with the quirk is, that as vendors produce multiple cameras with different 
PIDs they will have to push patches for each such camera.

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >> +  /*
> >> +   * Register a metadata node. TODO: shall this only be enabled
> >> for some
> >> +   * cameras?
> >> +   */
> >> +  if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >> +  uvc_meta_register(stream);
> >> +
> > 
> > I think so, only for the cameras that can produce metadata.
>  
>  Every UVC camera produces metadata, but most cameras only have standard
>  fields there. Whether we should stream standard header fields from the
>  metadata node will be discussed later. If we do decide to stream
>  standard header fields, then every USB camera gets metadata nodes. If
>  we decide not to include standard fields, how do we know whether the
>  camera has any private fields in headers without streaming from it? Do
>  you want a quirk for such cameras?
> >>> 
> >>> Unless they can be detected in a standard way that's probably the best
> >>> solution. Please remember that the UVC specification doesn't allow
> >>> vendors to extend headers in a vendor-specific way.
> >> 
> >> Does it not? Where is that specified? I didn't find that anywhere.
> > 
> > It's the other way around, it document a standard way to extend the
> > payload header. Any option you pick risks being incompatible with future
> > revisions of the specification. For instance the payload header's
> > bmHeaderInfo field can be extended through the use of the EOF bit, but a
> > future version of the specification could also extend it, in a way that
> > would make a vendor-specific extension be mistaken for the standard
> > extension.
> > 
> > Now, you could add data after the standard payload without touching the
> > bmHeaderInfo field, but I'm still worried it could be broken by future
> > versions of the specification.
> 
> Exactly - using "free" space in payload headers is not a part of the spec,
> but is also not prohibited by it. As for future versions - cameras specify
> which version of the spec they implement, and existing versions cannot
> change. If a camera decides to upgrade and in future UVC versions there
> won't be enough space left for the private data, only then the camera
> manufacturer will have a problem, that they will have to solve. The
> user-space software will have to know, that UVC 5.1 metadata has a
> different format, than UVC 1.5, that's true.

I agree that the specification doesn't explicitly forbid it, but it's a very 
grey area. An extension mechanism should be standardized by the USB-IF UVC 
working group. I'd propose it myself if they hadn't decided to kick me out 
years ago :-)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

On Tue, 6 Dec 2016, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> > Just one question:
> > 
> > On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >  +  /*
> >  +   * Register a metadata node. TODO: shall this only be enabled
> >  for some
> >  +   * cameras?
> >  +   */
> >  +  if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >  +  uvc_meta_register(stream);
> >  +
> > >>> 
> > >>> I think so, only for the cameras that can produce metadata.
> > >> 
> > >> Every UVC camera produces metadata, but most cameras only have standard
> > >> fields there. Whether we should stream standard header fields from the
> > >> metadata node will be discussed later. If we do decide to stream
> > >> standard header fields, then every USB camera gets metadata nodes. If we
> > >> decide not to include standard fields, how do we know whether the camera
> > >> has any private fields in headers without streaming from it? Do you want
> > >> a quirk for such cameras?
> > > 
> > > Unless they can be detected in a standard way that's probably the best
> > > solution. Please remember that the UVC specification doesn't allow vendors
> > > to extend headers in a vendor-specific way.
> > 
> > Does it not? Where is that specified? I didn't find that anywhere.
> 
> It's the other way around, it document a standard way to extend the payload 
> header. Any option you pick risks being incompatible with future revisions of 
> the specification. For instance the payload header's bmHeaderInfo field can 
> be 
> extended through the use of the EOF bit, but a future version of the 
> specification could also extend it, in a way that would make a 
> vendor-specific 
> extension be mistaken for the standard extension.
> 
> Now, you could add data after the standard payload without touching the 
> bmHeaderInfo field, but I'm still worried it could be broken by future 
> versions of the specification.

Exactly - using "free" space in payload headers is not a part of the spec, 
but is also not prohibited by it. As for future versions - cameras specify 
which version of the spec they implement, and existing versions cannot 
change. If a camera decides to upgrade and in future UVC versions there 
won't be enough space left for the private data, only then the camera 
manufacturer will have a problem, that they will have to solve. The 
user-space software will have to know, that UVC 5.1 metadata has a 
different format, than UVC 1.5, that's true.

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> Just one question:
> 
> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
>  +/*
>  + * Register a metadata node. TODO: shall this only be enabled
>  for some
>  + * cameras?
>  + */
>  +if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
>  +uvc_meta_register(stream);
>  +
> >>> 
> >>> I think so, only for the cameras that can produce metadata.
> >> 
> >> Every UVC camera produces metadata, but most cameras only have standard
> >> fields there. Whether we should stream standard header fields from the
> >> metadata node will be discussed later. If we do decide to stream
> >> standard header fields, then every USB camera gets metadata nodes. If we
> >> decide not to include standard fields, how do we know whether the camera
> >> has any private fields in headers without streaming from it? Do you want
> >> a quirk for such cameras?
> > 
> > Unless they can be detected in a standard way that's probably the best
> > solution. Please remember that the UVC specification doesn't allow vendors
> > to extend headers in a vendor-specific way.
> 
> Does it not? Where is that specified? I didn't find that anywhere.

It's the other way around, it document a standard way to extend the payload 
header. Any option you pick risks being incompatible with future revisions of 
the specification. For instance the payload header's bmHeaderInfo field can be 
extended through the use of the EOF bit, but a future version of the 
specification could also extend it, in a way that would make a vendor-specific 
extension be mistaken for the standard extension.

Now, you could add data after the standard payload without touching the 
bmHeaderInfo field, but I'm still worried it could be broken by future 
versions of the specification.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Just one question:

On Tue, 6 Dec 2016, Laurent Pinchart wrote:

> > >> +/*
> > >> + * Register a metadata node. TODO: shall this only be enabled 
> > >> for some
> > >> + * cameras?
> > >> + */
> > >> +if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > >> +uvc_meta_register(stream);
> > >> +
> > > 
> > > I think so, only for the cameras that can produce metadata.
> > 
> > Every UVC camera produces metadata, but most cameras only have standard
> > fields there. Whether we should stream standard header fields from the
> > metadata node will be discussed later. If we do decide to stream standard
> > header fields, then every USB camera gets metadata nodes. If we decide not
> > to include standard fields, how do we know whether the camera has any
> > private fields in headers without streaming from it? Do you want a quirk
> > for such cameras?
> 
> Unless they can be detected in a standard way that's probably the best 
> solution. Please remember that the UVC specification doesn't allow vendors to 
> extend headers in a vendor-specific way.

Does it not? Where is that specified? I didn't find that anywhere.

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Monday 05 Dec 2016 16:35:39 Guennadi Liakhovetski wrote:
> On Mon, 5 Dec 2016, Laurent Pinchart wrote:
> > On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> >> Some UVC video cameras contain metadata in their payload headers. This
> >> patch extracts that data, skipping the standard part of the header, 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.
> >> 
> >> Signed-off-by: Guennadi Liakhovetski 
> >> ---
> >> 
> >> v2:
> >> - updated to the current media/master
> >> - removed superfluous META capability from capture nodes
> >> - now the complete UVC payload header is copied to buffers, including
> >>   standard fields
> >> 
> >> Still open for discussion: is this really OK to always create an
> >> additional metadata node for each UVC camera or UVC video interface.
> 
> [snip]
> 
> >> +  /*
> >> +   * Register a metadata node. TODO: shall this only be enabled for some
> >> +   * cameras?
> >> +   */
> >> +  if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >> +  uvc_meta_register(stream);
> >> +
> > 
> > I think so, only for the cameras that can produce metadata.
> 
> Every UVC camera produces metadata, but most cameras only have standard
> fields there. Whether we should stream standard header fields from the
> metadata node will be discussed later. If we do decide to stream standard
> header fields, then every USB camera gets metadata nodes. If we decide not
> to include standard fields, how do we know whether the camera has any
> private fields in headers without streaming from it? Do you want a quirk
> for such cameras?

Unless they can be detected in a standard way that's probably the best 
solution. Please remember that the UVC specification doesn't allow vendors to 
extend headers in a vendor-specific way. This is an abuse of the 
specification, so a quirk is probably the best option.

> [snip]
> 
> > > +static struct vb2_ops uvc_meta_queue_ops = {
> > > + .queue_setup = meta_queue_setup,
> > > + .buf_prepare = meta_buffer_prepare,
> > > + .buf_queue = meta_buffer_queue,
> > > + .wait_prepare = vb2_ops_wait_prepare,
> > > + .wait_finish = vb2_ops_wait_finish,
> > > + .start_streaming = meta_start_streaming,
> > > + .stop_streaming = meta_stop_streaming,
> > > +};
> > 
> > How about reusing the uvc_queue.c implementation, with a few new checks
> > for metadata buffers where needed, instead of duplicating code ? At first
> > sight the changes don't seem to be too intrusive (but I might have
> > overlooked something).
> 
> I thought about that in the beginning and even started implementing it
> that way, but at some point it became too inconvenient, so, I switched
> over to a separate implementation. I'll think about it more and either
> explain, why I didn't want to do that, or unite them.
> 
> [snip]
> 
> > > +{
> > > + size_t nbytes;
> > > +
> > > + if (!meta_buf || !length)
> > > + return;
> > > +
> > > + nbytes = min_t(unsigned int, length, meta_buf->length);
> > > +
> > > + 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;
> > > +
> > > + memcpy(meta_buf->mem, mem, nbytes);
> > 
> > Do you need the whole header ? Shouldn't you strip the part already
> > handled by the driver ?
> 
> My original version did that, but we also need the timestamp from the
> header. The driver does parse it, but the implementation there has
> multiple times been reported as buggy and noone has been able to fix it so
> far :)

-ENOTIME I'm afraid, but feel free to give it a go :-) On the other hand 
supplying the PTS and SCR values to userspace would be useful to implement a 
high-accuracy clock translation algorithm that could make use of floating 
point operations.

> So, I ended up pulling the complete header to the user-space. Unless time-
> stamp processing is fixed in the kernel, I don't think we can frop this.

SCR and PTS should be provided to userspace in a standard way.

> >> +  meta_buf->bytesused = nbytes;
> >> +  meta_buf->state = UVC_BUF_STATE_READY;
> >> +
> >> +  uvc_queue_next_buffer(>meta.queue, meta_buf);
> > 
> > This essentially means that you'll need one buffer per isochronous packet.
> > Given the number of isochronous packets that make a complete image the
> > cost seem prohibitive to me. You should instead gather metadata from all
> > headers into a single buffer.
> 
> Hm, I only worked with cameras, using bulk transfers, so, didn't consider
> ISOC implications. Will have to think about this.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for the review! I'll work to address your comments. A couple of 
clarifications:

On Mon, 5 Dec 2016, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> > Some UVC video cameras contain metadata in their payload headers. This
> > patch extracts that data, skipping the standard part of the header, 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.
> > 
> > Signed-off-by: Guennadi Liakhovetski 
> > ---
> > 
> > v2:
> > - updated to the current media/master
> > - removed superfluous META capability from capture nodes
> > - now the complete UVC payload header is copied to buffers, including
> >   standard fields
> > 
> > Still open for discussion: is this really OK to always create an
> > additional metadata node for each UVC camera or UVC video interface.

[snip]

> > +   /*
> > +* Register a metadata node. TODO: shall this only be enabled for some
> > +* cameras?
> > +*/
> > +   if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > +   uvc_meta_register(stream);
> > +
> 
> I think so, only for the cameras that can produce metadata.

Every UVC camera produces metadata, but most cameras only have standard 
fields there. Whether we should stream standard header fields from the 
metadata node will be discussed later. If we do decide to stream standard 
header fields, then every USB camera gets metadata nodes. If we decide not 
to include standard fields, how do we know whether the camera has any 
private fields in headers without streaming from it? Do you want a quirk 
for such cameras?

[snip]

> > +static struct vb2_ops uvc_meta_queue_ops = {
> > +   .queue_setup = meta_queue_setup,
> > +   .buf_prepare = meta_buffer_prepare,
> > +   .buf_queue = meta_buffer_queue,
> > +   .wait_prepare = vb2_ops_wait_prepare,
> > +   .wait_finish = vb2_ops_wait_finish,
> > +   .start_streaming = meta_start_streaming,
> > +   .stop_streaming = meta_stop_streaming,
> > +};
> 
> How about reusing the uvc_queue.c implementation, with a few new checks for 
> metadata buffers where needed, instead of duplicating code ? At first sight 
> the changes don't seem to be too intrusive (but I might have overlooked 
> something).

I thought about that in the beginning and even started implementing it 
that way, but at some point it became too inconvenient, so, I switched 
over to a separate implementation. I'll think about it more and either 
explain, why I didn't want to do that, or unite them.

[snip]

> > +{
> > +   size_t nbytes;
> > +
> > +   if (!meta_buf || !length)
> > +   return;
> > +
> > +   nbytes = min_t(unsigned int, length, meta_buf->length);
> > +
> > +   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;
> > +
> > +   memcpy(meta_buf->mem, mem, nbytes);
> 
> Do you need the whole header ? Shouldn't you strip the part already handled 
> by 
> the driver ?

My original version did that, but we also need the timestamp from the 
header. The driver does parse it, but the implementation there has 
multiple times been reported as buggy and noone has been able to fix it so 
far :) So, I ended up pulling the complete header to the user-space. 
Unless time-stamp processing is fixed in the kernel, I don't think we can 
frop this.

> > +   meta_buf->bytesused = nbytes;
> > +   meta_buf->state = UVC_BUF_STATE_READY;
> > +
> > +   uvc_queue_next_buffer(>meta.queue, meta_buf);
> 
> This essentially means that you'll need one buffer per isochronous packet. 
> Given the number of isochronous packets that make a complete image the cost 
> seem prohibitive to me. You should instead gather metadata from all headers 
> into a single buffer.

Hm, I only worked with cameras, using bulk transfers, so, didn't consider 
ISOC implications. Will have to think about this.

[snip]

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> Some UVC video cameras contain metadata in their payload headers. This
> patch extracts that data, skipping the standard part of the header, 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.
> 
> Signed-off-by: Guennadi Liakhovetski 
> ---
> 
> v2:
> - updated to the current media/master
> - removed superfluous META capability from capture nodes
> - now the complete UVC payload header is copied to buffers, including
>   standard fields
> 
> Still open for discussion: is this really OK to always create an
> additional metadata node for each UVC camera or UVC video interface.
>
> IIUC, Laurent's metadata node patch
> https://patchwork.linuxtv.org/patch/36810/ has been acked by Hans and the
> only thing, that's preventing it from being merged it the lack of
> documentation. While waiting for documentation, I'd appreciate some
> discussion of this patch to beat it into shape soon enough and have it
> ready for merge soon after Laurent's patches are pulled in.
> 
> Thanks
> Guennadi
> 
>  drivers/media/usb/uvc/Makefile   |   2 +-
>  drivers/media/usb/uvc/uvc_driver.c   |  10 ++
>  drivers/media/usb/uvc/uvc_isight.c   |   2 +-
>  drivers/media/usb/uvc/uvc_metadata.c | 228 +++
>  drivers/media/usb/uvc/uvc_video.c|  47 ++--
>  drivers/media/usb/uvc/uvcvideo.h |  12 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c |   1 +
>  include/uapi/linux/uvcvideo.h|  10 ++
>  include/uapi/linux/videodev2.h   |   3 +
>  9 files changed, 304 insertions(+), 11 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 04bf350..edb67ac 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1866,6 +1866,9 @@ static void uvc_unregister_video(struct uvc_device
> *dev)
> 
>   video_unregister_device(>vdev);
> 
> + if (video_is_registered(>meta.vdev))
> + video_unregister_device(>meta.vdev);

video_unregister_device() contains a video_is_registered(), no need to 
duplicate it.

> +
>   uvc_debugfs_cleanup_stream(stream);
>   }
> 
> @@ -1926,6 +1929,13 @@ static int uvc_register_video(struct uvc_device *dev,
> return ret;
>   }
> 
> + /*
> +  * Register a metadata node. TODO: shall this only be enabled for some
> +  * cameras?
> +  */
> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> + uvc_meta_register(stream);
> +

I think so, only for the cameras that can produce metadata.

>   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 000..ddf77d9
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -0,0 +1,228 @@
> +/*
> + *  uvc_metadata.c  --  USB Video Class driver - Metadata handling
> + *
> + *  Copyright (C) 2016
> + *  Guennadi Liakhovetski (guennadi.liakhovet...@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 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "uvcvideo.h"
> +
> +static inline struct uvc_buffer *to_uvc_buffer(struct vb2_v4l2_buffer
> *vbuf)
> +{
> + return container_of(vbuf, struct uvc_buffer, buf);
> +}

You can move this function to uvcvideo.h and use it in uvc_queue.c (a separate 
patch would be best).

> +/* 
> + * videobuf2 Queue Operations
> + */
> +
> +static int meta_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> + unsigned int *nplanes, unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + if (*nplanes) {
> + if (*nplanes != 1)
> + return -EINVAL;
> +
> + if (sizes[0] < UVC_PAYLOAD_HEADER_MAX_SIZE)
> + return -EINVAL;
> +
> + return 0;
> + }
> +
> + *nplanes = 1;
> + sizes[0] =