Hi Guennadi,
Thank you for the patch.
On Saturday, 23 December 2017 13:11:00 EEST Guennadi Liakhovetski wrote:
> From: Guennadi Liakhovetski
>
> 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/pixfmt-meta-d4xx.rst | 202 +++
> drivers/media/usb/uvc/uvc_driver.c| 11 ++
> include/uapi/linux/videodev2.h| 1 +
> 3 files changed, 214 insertions(+)
> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
>
> 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..950780d
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> @@ -0,0 +1,202 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-d4xx:
> +
> +***
> +V4L2_META_FMT_D4XX ('D4XX')
> +***
> +
> +D4XX Metadata
How about "Intel D4xx UVC Cameras Metadata" ?
> +
> +
> +Description
> +===
> +
> +D4XX (D435 and other) cameras include per-frame metadata in their UVC
> payload
Should this be "Intel D4XX" ?
> +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
> +include MetadataId_CaptureStats (ID 3), MetadataId_CameraExtrinsics (ID 4),
> and
> +MetadataId_CameraIntrinsics (ID 5). For their description see [1_].
Does "including" mean that the list isn't exhaustive and that other standard
types could be returned too ? If so, would it be possible to get an exhaustive
list ? And if the list is exhaustive, could you word this paragraph to make
that clear ?
> This
> +document describes proprietary metadata types, used by DS4XX cameras.
Is it D4XX or DS4XX ?
> +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
What is this field used for ?
> +* - __u32 Flags
> + - A bitmask of flags: see [2_] below
> +* - __u32 Gain
> + - Manual gain value
What is the gain unit ?
> +* - __u32 Exposure
> + - Manual exposure time in microseconds
When auto-exposure is enabled, does this reflect the actual exposure time used
to capture the image ? If so I'd name the field just "exposure time", and
expand the document to explain this. Maybe something like
"Exposure time (in microseconds) that was used to capture the frame."
It would also be useful to explain what happens when auto-exposure is
disabled.
This comment applies to the gain as well.
> +* - __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 frameerate
s/frameerate/frame rate/
No other value than 0 is valid ?
> +* - __u32 AE ROI left
> + - Left border of the AE Region of Interest
> +* - __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
What are the units and range for those fields ?
> +* - __u32 Preset
> + - Preset selector value
Could you elaborate a bit on what the preset selector value is ?
> +* - __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
That's interesting. Does it increase by exactly one for every frame ? I think
it would be useful to document that.
> +* - __u32 Optical time
> + -
