Re: [RFC 7/7] media: v4l: Support line-based metadata capture

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Fri, Jun 02, 2023 at 01:50:54PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Sat, May 06, 2023 at 12:52:57AM +0300, Sakari Ailus wrote:
> > many camera sensors, among other devices, transmit embedded data and image
> 
> s/many/Many/
> 
> > data for each CSI-2 frame. This embedded data typically contains register
> > configuration of the sensor that has been used to capture the image data
> > of the same frame.
> > 
> > The embedded data is received by the CSI-2 receiver and has the same
> > properties as the image data, including that it is line based: it has
> > width, height and bytesperline (stride).
> > 
> > Add these fields to struct v4l2_meta_format and document them.
> > 
> > Also add V4L2_FMT_FLAG_META_LINE_BASED to tell a given format is
> > line-based i.e. these fields of struct v4l2_meta_format are valid for it.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  .../userspace-api/media/v4l/dev-meta.rst          | 15 +++++++++++++++
> >  .../userspace-api/media/v4l/vidioc-enum-fmt.rst   |  7 +++++++
> >  include/uapi/linux/videodev2.h                    | 10 ++++++++++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/dev-meta.rst b/Documentation/userspace-api/media/v4l/dev-meta.rst
> > index 0e7e1ee1471a..7d3a64514db0 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-meta.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-meta.rst
> > @@ -65,3 +65,18 @@ to 0.
> >        - ``buffersize``
> >        - Maximum buffer size in bytes required for data. The value is set by the
> >          driver.
> > +    * - __u32
> > +      - ``width``
> > +      - Width of a line of metadata in bytes. Valid when :c:type`v4l2_fmtdesc`
> 
> This departs from pixel formats, where the width is defined in pixels. I
> wonder what the implications will be for userspace. Seeing one
> implementation, both in a kernel driver and in libcamera, will help
> validating the API.

I thought of bytes when writing this but pixels (or samples) are probably a
better term for this.

> 
> > +	flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is set, otherwise zero. See
> > +	:c:func:`VIDIOC_ENUM_FMT`.
> > +    * - __u32
> > +      - ``height``
> > +      - Height of a line of metadata in bytes. Valid when :c:type`v4l2_fmtdesc`
> 
> The "height of a line" seems like a weird concept, especially if the
> height is expressed in bytes. I assume this is a bad copy&paste.

Yes. I'll address these for v2.

> 
> > +	flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is set, otherwise zero. See
> > +	:c:func:`VIDIOC_ENUM_FMT`.
> > +    * - __u32
> > +      - ``bytesperlines``
> > +      - Offset in bytes between the beginning of two consecutive lines. Valid
> > +	when :c:type`v4l2_fmtdesc` flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is
> > +	set, otherwise zero. See :c:func:`VIDIOC_ENUM_FMT`.
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > index 000c154b0f98..6d7664345a4e 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > @@ -227,6 +227,13 @@ the ``mbus_code`` field is handled differently:
> >  	The application can ask to configure the quantization of the capture
> >  	device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
> >  	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> > +    * - ``V4L2_FMT_FLAG_META_LINE_BASED``
> > +      - 0x0200
> > +      - The metadata format is line-based. In this case the ``width``,
> > +	``height`` and ``bytesperline`` fields of :c:type:`v4l2_meta_format` are
> > +	valid. The buffer consists of ``height`` lines, each having ``width``
> > +	bytes of data and offset between the beginning of each two consecutive
> > +	lines is ``bytesperline``.
> 
> If we add width and height for metadata formats, does it mean that
> drivers have to (or can) implement VIDIOC_ENUM_FRAMESIZES ? This should
> be documented.

Good point.

I don't think it's meaningful to implement that as the pipeline
configuration determines this in any case.

But I wonder whether this would better be documented for V4L2_CAP_IO_MC
rather than for metadata formats. There's currently only a reference to
this in ENUM_FMT documentation.

> 
> >  
> >  Return Value
> >  ============
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index adcbdc15dcdb..3681b2c15901 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -873,6 +873,7 @@ struct v4l2_fmtdesc {
> >  #define V4L2_FMT_FLAG_CSC_YCBCR_ENC		0x0080
> >  #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> >  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> > +#define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> >  
> >  	/* Frame Size and frame rate enumeration */
> >  /*
> > @@ -2407,10 +2408,19 @@ struct v4l2_sdr_format {
> >   * struct v4l2_meta_format - metadata format definition
> >   * @dataformat:		little endian four character code (fourcc)
> >   * @buffersize:		maximum size in bytes required for data
> > + * @width:		number of bytes of data per line (valid for line based
> > + *			formats only, see format documentation)
> > + * @height:		number of lines of data per buffer (valid for line based
> > + *			formats only)
> > + * @bytesperline:	offset between the beginnings of two adjacent lines in
> > + *			bytes (valid for line based formats only)
> >   */
> >  struct v4l2_meta_format {
> >  	__u32				dataformat;
> >  	__u32				buffersize;
> > +	__u32				width;
> > +	__u32				height;
> > +	__u32				bytesperline;
> >  } __attribute__ ((packed));
> >  
> >  /**

-- 
Regards,

Sakari Ailus