[libcamera-devel] [PATCH 1/2] libcamera: v4l2-format: Add V4L2DeviceFormat

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Jan 28, 2019 at 04:11:36PM +0100, Jacopo Mondi wrote:
> Add a V4L2DeviceFormat class aimed to be used to provide format configuration
> requests to a V4L2Device.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h | 14 ++++++++
>  src/libcamera/v4l2_device.cpp       | 56 +++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index c67ebbf..c70959e 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -53,6 +53,20 @@ struct V4L2Capability final : v4l2_capability {
>  	}
>  };
>  
> +class V4L2DeviceFormat

Should this be named V4L2Format ?

> +{
> +public:
> +	uint32_t width;
> +	uint32_t height;

I would have gone for unsigned int instead of uint32_t as the exact
integer type isn't really relevant, but uint32_t is fine too. You should
however include stdint.h to ensure that v4l2_device.h stays
self-contained.

> +	uint32_t fourcc;
> +
> +	struct {
> +		uint32_t size;
> +		uint32_t bpl;
> +	} planesFmt[3];
> +	unsigned int planes;

I would have named the first field planes and the second numPlanes or
planesCount.

> +};
> +
>  class MediaEntity;

Should we keep all forward declarations at the top of the file ?
Otherwise we'll likely end up with duplicated forward declarations at
some point.

>  class V4L2Device
>  {
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 408f9b9..d6143f2 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -80,6 +80,62 @@ LOG_DEFINE_CATEGORY(V4L2)
>   * \return True if the device provides Streaming I/O IOCTLs
>   */
>  
> +/**
> + * \class V4L2DeviceFormat
> + * \brief The V4L2 device image format and sizes
> + *
> + * Describes the image format and image sizes to be programmed on a V4L2

"This class describes ...3

It would be fine for the \brief text, but the documentation body should
be made of full sentences.

> + * video device. The image format is defined by fourcc code as defined by

s/fourcc code/a fourcc code/

Or maybe "a fourcc" (or "a 4CC" or "a FOURCC" ?), as the last C in
fourcc stands for code already.

> + * the V4L2 APIs with the V4L2_PIX_FMT_ macros, a visible width and height

s/V4L2_PIX_FMT_/V4L2_PIX_FMT_*/

> + * and a variable number of planes (1 to 3) with variable sizes and line
> + * strides.

I wouldn't say variable as they don't really vary. Maybe "and one to
three planes with configurable line stride and total size for each
plane" ?

> + *
> + * Formats defined as 'single planar' by the V4L2 APIs are represented with
> + * V4L2DeviceFormat instances with a single plane
> + * (V4L2DeviceFormat::planes = 1). Semi-planar and multiplanar formats use
> + * 2 and 3 planes respectively.

I would add one paragraph here to explain that line and plane padding
may not be separately configurable, depending on the device (devices
that don't support the MPLANE API have a fixed relationship between
padding for different planes), and how this works in that case (do we
ignore padding information for planes > 0 ? do we require them to be
specified an in sync, and return an error otherwise ?).

> + * 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.

Shouldn't we define the class as a way to pass format information to
V4L2Device, without caring about what's on the other side (and thus
without mentioning it's an exchange format) ?

> + */
> +
> +/**
> + * \var V4L2DeviceFormat::width
> + * \brief The image width

The image width in pixels

> + */
> +
> +/**
> + * \var V4L2DeviceFormat::height
> + * \brief The image height

In pixels here too.

> + */
> +
> +/**
> + * \var V4L2DeviceFormat::fourcc
> + * \brief The pixel encoding scheme
> + *
> + * The fourcc code, as defined by the V4L2 APIs with the V4L2_PIX_FMT_ macros,

s/APIs/API/
s/V4L2_PIX_FMT_/V4L2_PIX_FMT_*/

> + * that identifies the image format pixel encoding scheme.
> + */
> +
> +/**
> + * \var V4L2DeviceFormat::planesFmt
> + * \brief The per-plane size information

I'd mention "memory size" instead of just "size" to make it clear this
doesn't refer to the size in pixels.

> + *
> + * 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

s/size/memory size/ here too.

> + * visible sizes to accommodate line or plane padding data.
> + *
> + * Only the first V4L2DeviceFormat::planes entries are considered valid.

You could also write "\ref planes".

> + *

Extra blank line.

> + */
> +
> +/**
> + * \var V4L2DeviceFormat::planes
> + * \brief The number of valid data planes
> + */
> +
>  /**
>   * \class V4L2Device
>   * \brief V4L2Device object and API

-- 
Regards,

Laurent Pinchart