Re: [PATCH 2/6 v5] V4L: Add a UVC Metadata format

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

Hi Guennadi,

On Wednesday, 8 November 2017 12:43:46 EET Guennadi Liakhovetski wrote:
> On Tue, 7 Nov 2017, Laurent Pinchart wrote:
> > On Monday, 6 November 2017 16:53:10 EET Guennadi Liakhovetski wrote:
> > > On Mon, 30 Oct 2017, Hans Verkuil wrote:
> > > > On 07/28/2017 02:46 PM, Hans Verkuil wrote:
> > > >> On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote:
> > > >>> Add a pixel format, used by the UVC driver to stream metadata.
> > > >>> 
> > > >>> Signed-off-by: Guennadi Liakhovetski
> > > >>> <guennadi.liakhovetski@intel.com>
> > > >>> ---
> > > >>> 
> > > >>>  Documentation/media/uapi/v4l/meta-formats.rst    |  1 +
> > > >>>  Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst | 39
> > > >>>  +++++++++++++++++
> > > >>>  include/uapi/linux/videodev2.h                   |  1 +
> > > >>>  3 files changed, 41 insertions(+)
> > > >>>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-uvc.rst
> > > 
> > > [snip]
> > > 
> > > >>> diff --git a/include/uapi/linux/videodev2.h
> > > >>> b/include/uapi/linux/videodev2.h index 45cf735..0aad91c 100644
> > > >>> --- a/include/uapi/linux/videodev2.h
> > > >>> +++ b/include/uapi/linux/videodev2.h
> > > >>> @@ -682,6 +682,7 @@ struct v4l2_pix_format {
> > > >>> 
> > > >>>  /* Meta-data formats */
> > > >>>  #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 */> >>
> > > >>> 
> > > >>> +#define V4L2_META_FMT_UVC         v4l2_fourcc('U', 'V', 'C', 'H')
> > > >>> /*
> > > >>> UVC Payload Header metadata */
> > > > 
> > > > I discussed this with Laurent last week and since the metadata for UVC
> > > > starts with a standard header followed by vendor-specific data it
> > > > makes
> > > > sense to use V4L2_META_FMT_UVC for just the standard header. Any
> > > > vendor
> > > > specific formats should have their own fourcc which starts with the
> > > > standard header followed by the custom data. The UVC driver would
> > > > enumerate both the standard and the vendor specific fourcc. This would
> > > > allow generic UVC applications to use the standard header.
> > > > Applications
> > > > that know about the vendor specific data can select the vendor
> > > > specific
> > > > format.
> > > > 
> > > > This change would make this much more convenient to use.
> > > 
> > > Then the driver should be able to decide, which private fourcc code to
> > > use
> > > for each of those devices. A natural way to do that seems to be to put
> > > that in the .driver_info field of struct usb_device_id. For that I'd
> > > replace the current use of that field for quirks with a pointer to a
> > > struct in a separate patch. Laurent, would that be acceptable? Then add
> > > a
> > > field to that struct for a private metadata fourcc code.
> > 
> > I've been thinking about doing so for some time now. If you can write a
> > patch it would be great ! What I've been wondering is how to keep the
> > code both readable and small. If we declared those structures separately
> > from the devices array we could use one instance for multiple devices,
> > but naming might become awkward. On the other hand, if we defined them
> > inline within the devices array, we'd get rid of the naming issue, but at
> > the expense of increased memory usage.
> 
> We have in the meantime agreed to always use structs and to create named
> structs for reoccurring quirk flags and in-place structs for
> single-occurrance flags.
> 
> While implementing this I came across yet one more compromise, that we'll
> have to accept with this approach:
> 
> To recall the metadata buffer layout should be
> 
> struct uvc_meta_buf {
> 	uint64_t ns;
> 	uint16_t sof;
> 	uint8_t length;
> 	uint8_t flags;
> 	uint8_t buf[];
> } __attribute__((packed));
> 
> where all the fields, beginning with "length" are a direct copy from the
> UVC payload header. If multiple such payload headers have arrived for a
> single frame, they will be appended and .bytesused will as usually have
> the total byte count, used up in this frame. An application would then
> calculate lengths of those individual metadata blocks as
> 
> sizeof(.ns) + sizeof(.sof) + .length
> 
> But this won't work with the "standard" UVC metadata format where any
> private data, following standard fields, are dropped. In that case
> applications would have to look at .flags and calculate the block length
> based on them. Another possibility would be to rewrite the .length field
> in the driver to only include standard fields, but I really don't think
> that would be a good idea.

For the standard header the length can indeed be easily computed from the 
flags. I wonder, however, why you think rewriting length would be a bad idea ?

> > One middle-ground option would be to allow storing either a structure
> > pointer or quirks flags in the field, relying on the fact that the low
> > order bit of a pointer will be NULL. We could repurpose flag BIT(0) to
> > indicate that the field contains flags instead of a pointer.
> > 
> > Maybe I'm over-engineering this and that the extra memory consumption
> > won't be too bad, or separately defined structures will be easy to name.
> > I'd appreciate your opinion on this matter.

-- 
Regards,

Laurent Pinchart