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

13230 diff mbox series

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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.

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

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@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@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',
>

Hi Laurent, thank you for the patch.

On Fri, Aug 6, 2021 at 6:48 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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@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
> > + *

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

If there is some extra data between planes. Does the size cover the
extra data size?
That is, plene[0].size is possibly not the offset of plane[1]?

-Hiro
> > + * 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@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',
> >

Hi Laurent

On Thu, Aug 05, 2021 at 09:22:03PM +0300, 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.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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.
>
> ---
>  include/libcamera/format.h    |  43 ++++++++++

We do have a include/libcamera/formats.h.in which generate
pixelformats. Do you think is there a naming conflict ? Once built
formats.h.in get generated into formats.h

>  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
> + */
> +#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;

Do we use the kConstant convention ?

> +
> +	struct Plane {
> +		std::size_t stride;
> +		std::size_t size;
> +	};

We do have so many items named Plane :(

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

Do we prefix the file name with libcamera/ ?

> + * \brief Frame Format

Quite brief :)

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

per plane ?

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

Very long (). Maybe as an example paragraph ?

> + *
> + * Planes cover the whole frame. Their number of lines and sample per lines

per line ?

> + * correspond to the frame width and height in pixels, possibly divided by

number -> correspond

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

MAybe it's me but = default is usually in the header file

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

Why can't it take an index ? I mean, it's either an accessor to each
single plane stride or application should go through the planes_
vector and find stride there. I understand planes_[0] is special, but
considering we don't have a variable to express how many planes a
format has, requesting application to poke into planes_ might not be
ideal ?

> +{
> +	return planes_[0].stride;
> +}
> +
> +/**
> + * \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 */

Nits apart, I am missing how this is meant to be used...
A frame format is a property of the pixel format. I would expect each
PixelFormat to have a FrameFormat associated to describe its layout in
memory.

How do expect this class to be used ?

Thanks
  j

> 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',
> --
> Regards,
>
> Laurent Pinchart
>