[libcamera-devel] [PATCH v3 05/30] libcamera: formats: Move plane info structure to PixelFormatInfo

[Kieran Bingham]

On 06/09/2021 23:56, Laurent Pinchart wrote:
> Move the PixelFormatPlaneInfo structure within the PixelFormatInfo class
> definition and rename it to Plane, to align the naming scheme with other
> parts of libcamera, such as FrameBuffer::Plane or FrameMetadata::Plane.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/formats.h | 13 ++++++-------
>  src/libcamera/formats.cpp            | 12 ++++++------
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 51a8a6b8b0ae..a07de6bc6020 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -19,12 +19,6 @@
>  
>  namespace libcamera {
>  
> -struct PixelFormatPlaneInfo
> -{
> -	unsigned int bytesPerGroup;
> -	unsigned int verticalSubSampling;
> -};
> -
>  class PixelFormatInfo
>  {
>  public:
> @@ -34,6 +28,11 @@ public:
>  		ColourEncodingRAW,
>  	};
>  
> +	struct Plane {
> +		unsigned int bytesPerGroup;
> +		unsigned int verticalSubSampling;
> +	};
> +
>  	bool isValid() const { return format.isValid(); }
>  
>  	static const PixelFormatInfo &info(const PixelFormat &format);
> @@ -58,7 +57,7 @@ public:
>  
>  	unsigned int pixelsPerGroup;
>  
> -	std::array<PixelFormatPlaneInfo, 3> planes;
> +	std::array<Plane, 3> planes;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 603d88619fe0..c993960eb982 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -24,15 +24,15 @@ namespace libcamera {
>  LOG_DEFINE_CATEGORY(Formats)
>  
>  /**
> - * \class PixelFormatPlaneInfo
> + * \class PixelFormatInfo::Plane

Isn't this a struct ? But perhaps this is just a doxygenism because
structs and classes are very similar.

As long as there's no requirement to move this Plane documentation into
Class PixelformatInfo or 'after' it?

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

>   * \brief Information about a single plane of a pixel format
>   *
> - * \var PixelFormatPlaneInfo::bytesPerGroup
> + * \var PixelFormatInfo::Plane::bytesPerGroup
>   * \brief The number of bytes that a pixel group consumes
>   *
>   * \sa PixelFormatInfo::pixelsPerGroup
>   *
> - * \var PixelFormatPlaneInfo::verticalSubSampling
> + * \var PixelFormatInfo::Plane::verticalSubSampling
>   * \brief Vertical subsampling multiplier
>   *
>   * This value is the ratio between the number of rows of pixels in the frame
> @@ -87,7 +87,7 @@ LOG_DEFINE_CATEGORY(Formats)
>   *
>   * A pixel group is defined as the minimum number of pixels (including padding)
>   * necessary in a row when the image has only one column of effective pixels.
> - * pixelsPerGroup refers to this value. PixelFormatPlaneInfo::bytesPerGroup,
> + * pixelsPerGroup refers to this value. PixelFormatInfo::Plane::bytesPerGroup,
>   * then, refers to the number of bytes that a pixel group consumes. This
>   * definition of a pixel group allows simple calculation of stride, as
>   * ceil(width / pixelsPerGroup) * bytesPerGroup. These values are determined
> @@ -122,7 +122,7 @@ LOG_DEFINE_CATEGORY(Formats)
>   * \var PixelFormatInfo::planes
>   * \brief Information about pixels for each plane
>   *
> - * \sa PixelFormatPlaneInfo
> + * \sa PixelFormatInfo::Plane
>   */
>  
>  /**
> @@ -869,7 +869,7 @@ unsigned int PixelFormatInfo::numPlanes() const
>  {
>  	unsigned int count = 0;
>  
> -	for (const PixelFormatPlaneInfo &p : planes) {
> +	for (const Plane &p : planes) {
>  		if (p.bytesPerGroup == 0)
>  			break;
>  
>