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 <guennadi.liakhovet...@intel.com>
> ---
> 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 0000000..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
> + - 0x80000000
> + * - __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
> + - 0x80000001
> + * - __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
I assume this increases by exactly one for every frame. I'll test it when I
get back home, and will update the documentation accordingly.
> + * - __u32 Optical time
> + - Time in microseconds from the beginning of a frame till its middle
I'm still puzzled by this value, as I expect it to be exactly half the
exposure time, which is reported separately. If that's not the case I'd like
to know what this represents.
> + * - __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 = 1000000 / framerate
> + * - __u32 Pipe latency
> + - Time in microseconds from start of frame to data in USB buffer
> + * - :cspan:`1` *Configuration*
> + * - __u32 ID
> + - 0x80000002
> + * - __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 hardware version [5_]
> + * - __u8 SKU ID
> + - Camera hardware configuration [6_]
> + * - __u32 Cookie
> + - Internal synchronisation
> + * - __u16 Format
> + - Image format code [7_]
> + * - __u16 Width
> + - Width in pixels
> + * - __u16 Height
> + - Height in pixels
> + * - __u16 Framerate
> + - Requested frame rate per second
> + * - __u16 Trigger
> + - Byte 0: bit 0: depth and RGB are synchronised, bit 1: external
> trigger
> +
> +.. _1:
> +
> +[1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extens
> ions-1-5
> +
> +.. _2:
> +
> +[2] Depth Control flags specify which fields are valid: ::
> +
> + 0x00000001 Gain
> + 0x00000002 Exposure
> + 0x00000004 Laser power
> + 0x00000008 AE mode
> + 0x00000010 Exposure priority
> + 0x00000020 AE ROI
> + 0x00000040 Preset
> +
> +.. _3:
> +
> +[3] Capture Timing flags specify which fields are valid: ::
> +
> + 0x00000001 Frame counter
> + 0x00000002 Optical time
> + 0x00000004 Readout time
> + 0x00000008 Exposure time
> + 0x00000010 Frame interval
> + 0x00000020 Pipe latency
> +
> +.. _4:
> +
> +[4] Configuration flags specify which fields are valid: ::
> +
> + 0x00000001 Hardware type
> + 0x00000002 SKU ID
> + 0x00000004 Cookie
> + 0x00000008 Format
> + 0x00000010 Width
> + 0x00000020 Height
> + 0x00000040 Framerate
> + 0x00000080 Trigger
> + 0x00000100 Cal count
> +
> +.. _5:
> +
> +[5] Camera model: ::
> +
> + 0 DS5
> + 1 IVCAM2
> +
> +.. _6:
> +
> +[6] 8-bit camera hardware configuration bitfield: ::
> +
> + [1:0] depthCamera
> + 00: no depth
> + 01: standard depth
> + 10: wide depth
> + 11: reserved
> + [2] depthIsActive - has a laser projector
> + [3] RGB presence
> + [4] IMU presence
Do you mind if I write this "Inertial Measurement Unit (IMU) presense" ? Or is
it just me who's not familiar enough with depth cameras ? :-)
On a side note, does the camera expose IMU data to the host over USB ?
> + [5] projectorType
> + 0: HPTG
> + 1: Princeton
> + [6] 0: a projector, 1: an LED
> + [7] reserved
> +
> +.. _7:
> +
> +[7] Image format codes per camera interface:
I was initially puzzled by this until I read your explanation. How about
wording it as "Image format codes per video streaming interface" to use the
UVC vocabulary ?
> +
> +Depth: ::
> +
> + 1 Z16
> + 2 Z
> +
> +Left sensor: ::
> +
> + 1 Y8
> + 2 UYVY
> + 3 R8L8
> + 4 Calibration
> + 5 W10
> +
> +Fish Eye sensor: ::
> +
> + 1 RAW8
If you agree with the comments above, I'll update the patch when applying it
to my tree. I would however still like to get the following information
- What are the version of the three structures documented in this patch ? I
can check that when I get back home, but if you have the information it would
be faster.
- Do the fields in the Depth Control structure apply to the depth video stream
only ? (I assume they do)
- What do the fields in the Capture Control structure apply to ? (I assume the
left sensor and/or fish eye video streams)
- Does the optical time differ from half the exposure time ?
If you think it will take time to get this information and we risk missing the
next kernel version, I'm OK applying the patch already, but please then submit
a follow-up patch (or just drop a mail in reply to this one with the
information and I can turn that into a patch).
[snip]
--
Regards,
Laurent Pinchart
