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

[Laurent Pinchart]

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

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

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

-- 
Regards,

Laurent Pinchart