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

13671 diff mbox series

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@ideasonboard.com>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/formats.h | 13 ++++++-------
 src/libcamera/formats.cpp            | 12 ++++++------
 2 files changed, 12 insertions(+), 13 deletions(-)

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@ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh@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@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;
>  
>

Hi Kieran,

On Tue, Sep 07, 2021 at 12:32:44AM +0100, Kieran Bingham wrote:
> 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@ideasonboard.com>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Reviewed-by: Hirokazu Honda <hiroh@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?

Good points, I'll use \struct and move the documentation block to the
right place.

> Reviewed-by: Kieran Bingham <kieran.bingham@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;
> >