Re: [PATCH v2 2/2] uvcvideo: add a D4M camera description
Hi Laurent,
Thanks for the review.
On Sat, 25 Aug 2018, Laurent Pinchart wrote:
> Hi Guennadi,
>
> Thank you for the patch.
>
> Overall this looks good to me, I only have small comments. Please see below,
> with a summary at the end.
>
> On Friday, 3 August 2018 14:37:08 EEST Guennadi Liakhovetski wrote:
> > D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> > This patch adds a descriptor for it, which enables reading per-frame
> > metadata from it.
> >
> > Signed-off-by: Guennadi Liakhovetski
> > ---
> > Documentation/media/uapi/v4l/meta-formats.rst | 1 +
> > Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 +++
> > drivers/media/usb/uvc/uvc_driver.c| 11 ++
> > include/uapi/linux/videodev2.h| 1 +
> > 4 files changed, 217 insertions(+)
> > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
>
> [snip]
>
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> > index 000..57ecfd9
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> > @@ -0,0 +1,204 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _v4l2-meta-fmt-d4xx:
> > +
> > +***
> > +V4L2_META_FMT_D4XX ('D4XX')
> > +***
> > +
> > +Intel D4xx UVC Cameras Metadata
> > +
> > +
> > +Description
> > +===
> > +
> > +Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
> > +payload headers, following the Microsoft(R) UVC extension proposal [1_].
> > That +means, that the private D4XX metadata, following the standard UVC
> > header, is +organised in blocks. D4XX cameras implement several standard
> > block types, +proposed by Microsoft, and several proprietary ones.
> > Supported standard metadata +types are MetadataId_CaptureStats (ID 3),
> > MetadataId_CameraExtrinsics (ID 4), +and MetadataId_CameraIntrinsics (ID
> > 5). For their description see [1_]. This +document describes proprietary
> > metadata types, used by D4xx cameras.
> > +
> > +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> > +V4L2_META_FMT_UVC with the only difference, that it also includes
> > proprietary +payload header data. D4xx cameras use bulk transfers and only
> > send one payload +per frame, therefore their headers cannot be larger than
> > 255 bytes.
> > +
> > +Below are proprietary Microsoft style metadata types, used by D4xx cameras,
> > +where all fields are in little endian order:
> > +
> > +.. flat-table:: D4xx metadata
> > +:widths: 1 4
> > +:header-rows: 1
> > +:stub-columns: 0
> > +
> > +* - Field
> > + - Description
> > +* - :cspan:`1` *Depth Control*
>
> Does this mean that all fields in this structure apply to the depth image
> only
> ? If so, do you mind if I mention that explicitly ?
I don't think that's the case. E.g. only this struct has laser parameters
and the laser can be used without the depth node being active. In fact I
just tried streaming from the other node and reading metadata from its
metadata node - it also included ID 0x8000 with flags set to 0xff,
i.e. all fields valid.
> > +* - __u32 ID
> > + - 0x8000
> > +* - __u32 Size
> > + - Size in bytes (currently 56)
> > +* - __u32 Version
> > + - Version of the struct
>
> I would elaborate a bit here, how about the following ?
>
> "Version of this structure. The documentation herein corresponds to version
> xxx. The version number will be incremented when new fields are added."
>
> If you can provide me with the current version number I can update this when
> applying the patch (I would get it myself, but I'm about to board a plane and
> haven't taken the camera with me :-/).
Sure, sounds good, the "depth control" is version 2, the rest is version
1.
> > +* - __u32 Flags
> > + - A bitmask of flags: see [2_] below
> > +* - __u32 Gain
> > + - Gain value in internal units, same as the V4L2_CID_GAIN control,
> > used to
> > +capture the frame
> > +* - __u32 Exposure
> > + - Exposure time (in microseconds) used to capture the frame
> > +* - __u32 Laser power
> > + - Power of the laser LED 0-360, used for depth measurement
> > +* - __u32 AE mode
> > + - 0: manual; 1: automatic exposure
> > +* - __u32 Exposure priority
> > + - Exposure priority value: 0 - constant frame rate
> > +* - __u32 AE ROI left
> > + - Left border of the AE Region of Interest (all ROI values are in
> > pixels
> > +and lie between 0 and maximum width or height respectively)
> > +* - __u32 AE ROI right
> > + - Right border of the AE Region of Interest
> > +* - __u32 AE ROI top
> > + - Top border of the AE Region of Interest
> > +* - __u32 AE ROI bottom
> > + - Bottom border of the AE Region of Interest
> > +* -
Re: [PATCH v2 2/2] uvcvideo: add a D4M camera description
Hi Guennadi,
Thank you for the patch.
Overall this looks good to me, I only have small comments. Please see below,
with a summary at the end.
On Friday, 3 August 2018 14:37:08 EEST Guennadi Liakhovetski wrote:
> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> This patch adds a descriptor for it, which enables reading per-frame
> metadata from it.
>
> Signed-off-by: Guennadi Liakhovetski
> ---
> Documentation/media/uapi/v4l/meta-formats.rst | 1 +
> Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 +++
> drivers/media/usb/uvc/uvc_driver.c| 11 ++
> include/uapi/linux/videodev2.h| 1 +
> 4 files changed, 217 insertions(+)
> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
[snip]
> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> index 000..57ecfd9
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> @@ -0,0 +1,204 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-d4xx:
> +
> +***
> +V4L2_META_FMT_D4XX ('D4XX')
> +***
> +
> +Intel D4xx UVC Cameras Metadata
> +
> +
> +Description
> +===
> +
> +Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
> +payload headers, following the Microsoft(R) UVC extension proposal [1_].
> That +means, that the private D4XX metadata, following the standard UVC
> header, is +organised in blocks. D4XX cameras implement several standard
> block types, +proposed by Microsoft, and several proprietary ones.
> Supported standard metadata +types are MetadataId_CaptureStats (ID 3),
> MetadataId_CameraExtrinsics (ID 4), +and MetadataId_CameraIntrinsics (ID
> 5). For their description see [1_]. This +document describes proprietary
> metadata types, used by D4xx cameras.
> +
> +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> +V4L2_META_FMT_UVC with the only difference, that it also includes
> proprietary +payload header data. D4xx cameras use bulk transfers and only
> send one payload +per frame, therefore their headers cannot be larger than
> 255 bytes.
> +
> +Below are proprietary Microsoft style metadata types, used by D4xx cameras,
> +where all fields are in little endian order:
> +
> +.. flat-table:: D4xx metadata
> +:widths: 1 4
> +:header-rows: 1
> +:stub-columns: 0
> +
> +* - Field
> + - Description
> +* - :cspan:`1` *Depth Control*
Does this mean that all fields in this structure apply to the depth image only
? If so, do you mind if I mention that explicitly ?
> +* - __u32 ID
> + - 0x8000
> +* - __u32 Size
> + - Size in bytes (currently 56)
> +* - __u32 Version
> + - Version of the struct
I would elaborate a bit here, how about the following ?
"Version of this structure. The documentation herein corresponds to version
xxx. The version number will be incremented when new fields are added."
If you can provide me with the current version number I can update this when
applying the patch (I would get it myself, but I'm about to board a plane and
haven't taken the camera with me :-/).
> +* - __u32 Flags
> + - A bitmask of flags: see [2_] below
> +* - __u32 Gain
> + - Gain value in internal units, same as the V4L2_CID_GAIN control,
> used to
> +capture the frame
> +* - __u32 Exposure
> + - Exposure time (in microseconds) used to capture the frame
> +* - __u32 Laser power
> + - Power of the laser LED 0-360, used for depth measurement
> +* - __u32 AE mode
> + - 0: manual; 1: automatic exposure
> +* - __u32 Exposure priority
> + - Exposure priority value: 0 - constant frame rate
> +* - __u32 AE ROI left
> + - Left border of the AE Region of Interest (all ROI values are in
> pixels
> +and lie between 0 and maximum width or height respectively)
> +* - __u32 AE ROI right
> + - Right border of the AE Region of Interest
> +* - __u32 AE ROI top
> + - Top border of the AE Region of Interest
> +* - __u32 AE ROI bottom
> + - Bottom border of the AE Region of Interest
> +* - __u32 Preset
> + - Preset selector value, default: 0, unless changed by the user
I won't block this patch for this, but could we get the documentation of the
corresponding XU control ? Even better, if Intel could publish documentation
of the full XUs, that would be great :-)
> +* - __u32 Laser mode
> + - 0: off, 1: on
> +* - :cspan:`1` *Capture Timing*
Similarly, what does this apply to ? The left sensor, the fish eye camera, or
both (or something else) ?
> +* - __u32 ID
> + - 0x8001
> +* - __u32 Size
> + - Size in bytes (currently 40)
> +* - __u32 Version
> + - Version of the struct
> +* - __u32 Flags
> + - A bitmask of flags: see
[PATCH v2 2/2] uvcvideo: add a D4M camera description
D4M is a mobile model from the D4XX family of Intel RealSense cameras.
This patch adds a descriptor for it, which enables reading per-frame
metadata from it.
Signed-off-by: Guennadi Liakhovetski
---
Documentation/media/uapi/v4l/meta-formats.rst | 1 +
Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 ++
drivers/media/usb/uvc/uvc_driver.c| 11 ++
include/uapi/linux/videodev2.h| 1 +
4 files changed, 217 insertions(+)
create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
b/Documentation/media/uapi/v4l/meta-formats.rst
index 0c4e1ec..cf971d5 100644
--- a/Documentation/media/uapi/v4l/meta-formats.rst
+++ b/Documentation/media/uapi/v4l/meta-formats.rst
@@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` interface only.
.. toctree::
:maxdepth: 1
+pixfmt-meta-d4xx
pixfmt-meta-uvc
pixfmt-meta-vsp1-hgo
pixfmt-meta-vsp1-hgt
diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
new file mode 100644
index 000..57ecfd9
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
@@ -0,0 +1,204 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _v4l2-meta-fmt-d4xx:
+
+***
+V4L2_META_FMT_D4XX ('D4XX')
+***
+
+Intel D4xx UVC Cameras Metadata
+
+
+Description
+===
+
+Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
+payload headers, following the Microsoft(R) UVC extension proposal [1_]. That
+means, that the private D4XX metadata, following the standard UVC header, is
+organised in blocks. D4XX cameras implement several standard block types,
+proposed by Microsoft, and several proprietary ones. Supported standard
metadata
+types are MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4),
+and MetadataId_CameraIntrinsics (ID 5). For their description see [1_]. This
+document describes proprietary metadata types, used by D4xx cameras.
+
+V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
+V4L2_META_FMT_UVC with the only difference, that it also includes proprietary
+payload header data. D4xx cameras use bulk transfers and only send one payload
+per frame, therefore their headers cannot be larger than 255 bytes.
+
+Below are proprietary Microsoft style metadata types, used by D4xx cameras,
+where all fields are in little endian order:
+
+.. flat-table:: D4xx metadata
+:widths: 1 4
+:header-rows: 1
+:stub-columns: 0
+
+* - Field
+ - Description
+* - :cspan:`1` *Depth Control*
+* - __u32 ID
+ - 0x8000
+* - __u32 Size
+ - Size in bytes (currently 56)
+* - __u32 Version
+ - Version of the struct
+* - __u32 Flags
+ - A bitmask of flags: see [2_] below
+* - __u32 Gain
+ - Gain value in internal units, same as the V4L2_CID_GAIN control, used
to
+ capture the frame
+* - __u32 Exposure
+ - Exposure time (in microseconds) used to capture the frame
+* - __u32 Laser power
+ - Power of the laser LED 0-360, used for depth measurement
+* - __u32 AE mode
+ - 0: manual; 1: automatic exposure
+* - __u32 Exposure priority
+ - Exposure priority value: 0 - constant frame rate
+* - __u32 AE ROI left
+ - Left border of the AE Region of Interest (all ROI values are in pixels
+ and lie between 0 and maximum width or height respectively)
+* - __u32 AE ROI right
+ - Right border of the AE Region of Interest
+* - __u32 AE ROI top
+ - Top border of the AE Region of Interest
+* - __u32 AE ROI bottom
+ - Bottom border of the AE Region of Interest
+* - __u32 Preset
+ - Preset selector value, default: 0, unless changed by the user
+* - __u32 Laser mode
+ - 0: off, 1: on
+* - :cspan:`1` *Capture Timing*
+* - __u32 ID
+ - 0x8001
+* - __u32 Size
+ - Size in bytes (currently 40)
+* - __u32 Version
+ - Version of the struct
+* - __u32 Flags
+ - A bitmask of flags: see [3_] below
+* - __u32 Frame counter
+ - Monotonically increasing counter
+* - __u32 Optical time
+ - Time in microseconds from the beginning of a frame till its middle
+* - __u32 Readout time
+ - Time, used to read out a frame in microseconds
+* - __u32 Exposure time
+ - Frame exposure time in microseconds
+* - __u32 Frame interval
+ - In microseconds = 100 / framerate
+* - __u32 Pipe latency
+ - Time in microseconds from start of frame to data in USB buffer
+* - :cspan:`1` *Configuration*
+* - __u32 ID
+ - 0x8002
+* - __u32 Size
+ - Size in bytes (currently 40)
+* - __u32 Version
+ - Version of the struct
+* - __u32 Flags
+ - A bitmask of flags: see [4_] below
+* - __u8 Hardware type
+ - Camera
