[libcamera-devel] [PATCH 7/7] libcamera: v4l2_device: Improve documentation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Feb 01, 2019 at 04:42:48PM +0100, Jacopo Mondi wrote:
> Improve the V4L2DeviceFormat documentation as suggested during patches
> review.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/v4l2_device.cpp | 42 +++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 4d1f76b..5ae208f 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -84,48 +84,52 @@ LOG_DEFINE_CATEGORY(V4L2)
>   * \class V4L2DeviceFormat
>   * \brief The V4L2 device image format and sizes
>   *
> - * Describes the image format and image sizes to be programmed on a V4L2
> - * video device. The image format is defined by fourcc code as defined by
> - * the V4L2 APIs with the V4L2_PIX_FMT_ macros, a visible width and height
> - * and a variable number of planes (1 to 3) with variable sizes and line
> - * strides.
> + * This class describes the image format and image sizes to be programmed
> + * on a V4L2 video device. The image format is defined by a fourcc code
> + * as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, a visible
> + * width and height and one to three planes with configurable line stride
> + * and a total per-plane size in bytes.
>   *
> - * Formats defined as 'single planar' by the V4L2 APIs are represented with
> - * V4L2DeviceFormat instances with a single plane
> - * (V4L2DeviceFormat::planesCount = 1). Semi-planar and multiplanar formats use
> - * 2 and 3 planes respectively.
> + * The V4L2 APIs defines packed, semi-planar and planar image formats.
> + * Packed formats are stored as a single, contiguous memory area, while
> + * semi-planar and multi-planar ones use two or three (possibly not contiguous)
> + * memory regions to store the image components as separate planes.
>   *
> - * V4L2DeviceFormat defines the exchange format between components that
> - * receive image configuration requests from applications and a V4L2Device.
> - * The V4L2Device validates and applies the requested size and format to
> - * the device driver.
> + * Packed formats are represented by V4L2DeviceFormat instances with a
> + * single valid \ref planes array entry (\ref planesCount = 1).
> + * Non-packed formats allows configuring per plane size and line stride length,

s/allows/allow/
s/stride length/stride/

> + * but only when applied to devices that support the V4L2 multi-planar APIs.

I don't think this is correct. bytesperline can be set for all formats
(packed and planar), both using the MPLANE and non-MPLANE APIs.

> + * When a non-packed image format gets applied to a device that only supports

I'd write "planar or semi-planar" instead of "non-packed".

> + * the V4L2 single-planar APIs, it is not possible to configure per-plane sizes

Did you mean strides instead of sizes ?

> + * as the only valid informations are contained in the first entry of the \ref

s/informations/information/

> + * planes array, and apply to the image as a whole.

I'd write "[...] per-plane sizes. Only the first entry of the \ref
planes array is taken into account in that case, and apply to all
planes, with appropriate subsampling."

>   */
>  
>  /**
>   * \var V4L2DeviceFormat::width
> - * \brief The image width
> + * \brief The image width in pixels
>   */
>  
>  /**
>   * \var V4L2DeviceFormat::height
> - * \brief The image height
> + * \brief The image height in pixels
>   */
>  
>  /**
>   * \var V4L2DeviceFormat::fourcc
>   * \brief The pixel encoding scheme
>   *
> - * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros,
> + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,
>   * that identifies the image format pixel encoding scheme.
>   */
>  
>  /**
>   * \var V4L2DeviceFormat::planes
> - * \brief The per-plane size information
> + * \brief The per-plane memory size information
>   *
>   * Images are stored in memory in one or more data planes. Each data plane
> - * has a specific size and line length, which could differ from the image
> - * visible sizes to accommodate line or plane padding data.
> + * has a specific memory size and line length, which could differ from the

s/memory size and line length/line stride and memory size/

> + * image visible sizes to accommodate line or plane padding data.

s/line or plane padding data/padding at the end of lines and end of the
plane/ ?

>   *
>   * Only the first \ref planesCount entries are considered valid.
>   */

-- 
Regards,

Laurent Pinchart