[libcamera-devel] [RFC PATCH] libcamera: Add FrameFormat class

[Kieran Bingham]

Hi Laurent,

On 05/08/2021 19:22, Laurent Pinchart wrote:
> The FrameFormat class describes how a frame is stored in memory. It
> groups a pixel format, size in pixels, and per-plane stride and size.
> 
> This new class will be used in the configuration API rework, but could
> already be put in use if desired to replace the various data structures
> used to carry the same information.

I think that second paragraph can go under the ---

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> 
> I've developed this as part of the configuration API rework, and
> realized it could already be useful, in particular if we want to start
> cleaning up support for multi-planar formats. I'm thus sending it as an
> RFC.

This looks like a good idea. I was considering what information such as
pixel-format we might have to add to FrameBuffer to convey the
information required, but we could equally keep it separate.

> 
> ---
>  include/libcamera/format.h    |  43 ++++++++++
>  include/libcamera/meson.build |   1 +
>  src/libcamera/format.cpp      | 142 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  4 files changed, 187 insertions(+)
>  create mode 100644 include/libcamera/format.h
>  create mode 100644 src/libcamera/format.cpp
> 
> diff --git a/include/libcamera/format.h b/include/libcamera/format.h
> new file mode 100644
> index 000000000000..4982ab8230dc
> --- /dev/null
> +++ b/include/libcamera/format.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * format.h - Frame Format

Should this be named frame_format.h?

Otherwise it might be confused the Stream format or others?

> + */
> +#ifndef __LIBCAMERA_FORMAT_H__
> +#define __LIBCAMERA_FORMAT_H__
> +
> +#include <array>
> +#include <cstddef>
> +#include <string>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +namespace libcamera {
> +
> +class FrameFormat
> +{
> +public:
> +	static constexpr unsigned int kMaxPlanes = 4;
> +
> +	struct Plane {
> +		std::size_t stride;
> +		std::size_t size;
> +	};
> +
> +	FrameFormat();
> +	FrameFormat(PixelFormat format, const Size &size);
> +
> +	std::size_t stride() const;
> +	std::size_t frameSize() const;
> +	std::string toString() const;
> +
> +	PixelFormat format_;
> +	Size size_;
> +	std::array<Plane, kMaxPlanes> planes_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_FORMAT_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 5b25ef847ed4..dcfadb381874 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -6,6 +6,7 @@ libcamera_public_headers = files([
>      'compiler.h',
>      'controls.h',
>      'file_descriptor.h',
> +    'format.h',
>      'framebuffer.h',
>      'framebuffer_allocator.h',
>      'geometry.h',
> diff --git a/src/libcamera/format.cpp b/src/libcamera/format.cpp
> new file mode 100644
> index 000000000000..f77413781de1
> --- /dev/null
> +++ b/src/libcamera/format.cpp
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * format.cpp - Frame Format
> + */
> +
> +#include <libcamera/format.h>
> +
> +#include <numeric>
> +
> +/**
> + * \file libcamera/format.h
> + * \brief Frame Format
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class FrameFormat
> + * \brief Format of a frame as stored in memory
> + *
> + * The FrameFormat class fully describes how a frame is stored in memory. It
> + * combines a pixel format, a size in pixels, and memory layout information for
> + * planes.
> + *
> + * Frames are stored in memory in one or multiple planes, as specified by the
> + * pixel format. A plane is a two-dimensional memory area organized as lines of
> + * samples. A sample typically stores one or multiple components of a pixel (for
> + * instance, the NV12 format uses two planes, with the first plane storing only
> + * the luminance Y component of the pixels, and the second plane storing both
> + * the chroma Cb and Cr components).
> + *
> + * Planes cover the whole frame. Their number of lines and sample per lines
> + * correspond to the frame width and height in pixels, possibly divided by
> + * horizontal and vertical subsampling factors as specified by the pixel
> + * format.
> + *
> + * A plane may contain padding at the end of line, to support requirements of
> + * applications and cameras, such as alignment constraints. This is specified
> + * as a line stride value, equal to the distance in bytes between the first
> + * sample of successive lines in memory, including pixel data and padding. All
> + * lines in a plane have the same stride value, including the last line.
> + * Additionally, padding may also occur at the end of the plane, specified as
> + * the total plane size in bytes.
> + *
> + * \var FrameFormat::kMaxPlanes
> + * \brief Maximum number of data planes for a frame
> + *
> + * \var FrameFormat::format_
> + * \brief Frame pixel format, defines how pixel data is packed and stored in
> + * memory
> + *
> + * \var FrameFormat::size_
> + * \brief Frame size, defines the horizontal and vertical dimensions of the
> + * frame in pixels (excluding any padding)
> + *
> + * \var FrameFormat::planes_
> + * \brief Frame data planes, defines the memory layout for each data plane of
> + * the frame
> + *
> + * Unused entries in the planes array shall be zero-initialized.
> + */
> +
> +/**
> + * \class FrameFormat::Plane
> + * \brief Memory layout configuration for one data plane of a frame
> + *
> + * \var FrameFormat::Plane::stride
> + * \brief Distance in bytes between the beginning of two successive lines
> + *
> + * If the strides of different planes can't be set separately, the stride of the
> + * first plane takes precedence and is used by the camera.
> + *
> + * \var FrameFormat::Plane::size
> + * \brief Maximum number of bytes required to store the plane
> + *
> + * The \a size reports the number of bytes required to store data for plane.
> + * For uncompressed formats, it takes into account the plane's stride, the
> + * frame height, the plane's vertical subsampling and the overall alignment
> + * constraints of the camera (for instance many cameras require buffer sizes
> + * to be a multiple of the page size). For compressed formats, it indicates the
> + * maximum size of the compressed data.
> + */
> +
> +/**
> + * \brief Construct a zero-initialized FrameFormat
> + */
> +FrameFormat::FrameFormat() = default;
> +
> +/**
> + * \brief Construct a FrameFormat
> + * \param[in] format The pixel format
> + * \param[in] size The frame size in pixels
> + */
> +FrameFormat::FrameFormat(PixelFormat format, const Size &size)
> +	: format_(format), size_(size)
> +{
> +}
> +
> +/**
> + * \brief Retrieve the frame stride in bytes
> + *
> + * The stride() function is a convenience helper to retrieve the stride of the
> + * first plane. For multi-planar formats that have different stride values for
> + * each plane, applications shall access the stride from the \ref planes_
> + * instead.
> + *
> + * \return The frame stride in bytes
> + */
> +std::size_t FrameFormat::stride() const
> +{
> +	return planes_[0].stride;
> +}
> +

What about having the plane as a parameter, defaulting to 0.

Then the access to reading the strides can be done through a const
accessor rather than digging into the planes_ directly.

With a similar accessor for size - could the Plane could be made private
and populated during the constructor only?

With those considered:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +/**
> + * \brief Retrieve the full frame size
> + *
> + * The frameSize() function is a convenience helper to retrieve the full frame
> + * size, defined as the sum of the sizes of all planes.
> + *
> + * \return The full frame size, in bytes
> + */
> +std::size_t FrameFormat::frameSize() const
> +{
> +	return std::accumulate(planes_.begin(), planes_.end(), 0,
> +			       [](std::size_t a, const Plane &b) {
> +				       return a + b.size;
> +			       });
> +}
> +
> +/**
> + * \brief Assemble and return a string describing the frame format
> + *
> + * \return A string describing the FrameFormat
> + */
> +std::string FrameFormat::toString() const
> +{
> +	return size_.toString() + "-" + format_.toString();
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 4f08580157f9..7cf17ec2c628 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -15,6 +15,7 @@ libcamera_sources = files([
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
>      'file_descriptor.cpp',
> +    'format.cpp',
>      'formats.cpp',
>      'framebuffer.cpp',
>      'framebuffer_allocator.cpp',
>