[libcamera-devel] [PATCH v2 4/6] libcamera: PixelFormatInfo: Add functions bytesPerLine and imageSize

[Laurent Pinchart]

Hi again,

On Wed, Jul 01, 2020 at 01:48:28PM +0300, Laurent Pinchart wrote:
> On Tue, Jun 30, 2020 at 11:58:06PM +0900, Paul Elder wrote:
> > Add member functions to PixelFormatInfo for calculating bytes-per-line
> > and image size. This will simplify existing code that calculates these
> > things.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - make these functions const
> > - add documentation
> > - inline DIV_ROUND_UP
> > ---
> >  include/libcamera/internal/formats.h |  3 +++
> >  src/libcamera/formats.cpp            | 27 +++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > index 3c7440a..af0f1ac 100644
> > --- a/include/libcamera/internal/formats.h
> > +++ b/include/libcamera/internal/formats.h
> > @@ -46,6 +46,9 @@ public:
> >  	static const PixelFormatInfo &info(const PixelFormat &format);
> >  	static const PixelFormatInfo &info(const V4L2PixelFormat &format);
> >  
> > +	unsigned int bytesPerLine(unsigned int width) const;
> > +	unsigned int imageSize(const Size &size) const;
> > +
> >  	const char *name;
> >  	PixelFormat format;
> >  	V4L2PixelFormat v4l2Format;
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 4d18825..698c905 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -693,4 +693,31 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> >  	return info->second;
> >  }
> >  
> > +/**
> > + * \brief Compute the bytes per line
> > + * \param[in] width The width of the line, in pixels
> > + * \return The number of bytes necessary to store the line, or 0 if the
> > + * PixelFormatInfo instance not valid
> > + */
> > +unsigned int PixelFormatInfo::bytesPerLine(unsigned int width) const
> 
> I think the name (and documentation) should be updated based on the
> naming discussions in 2/6 v1 (stride or lineStride ?).
> 
> > +{
> > +	if (!isValid())
> > +		return 0;
> > +
> > +	/* ceil(width / pixelsPerGroup) * bytesPerGroup */
> > +	return ((width + pixelsPerGroup - 1) / pixelsPerGroup) * bytesPerGroup;
> 
> You could remove the outer parentheses.
> 
> > +
> 
> Extra blank line.
> 
> > +}
> > +
> > +/**
> > + * \brief Compute the bytes necessary to store the frame
> > + * \param[in] size The size of the frame, in pixels
> > + * \return The number of bytes necessary to store the frame, or 0 if the
> > + * PixelFormatInfo instance is not valid
> > + */
> > +unsigned int PixelFormatInfo::imageSize(const Size &size) const
> > +{
> > +	return size.height * bytesPerLine(size.width);
> > +}
> 
> This one is a bit of an issue. The calculation is fine assuming the
> device doesn't have particular requirements on the stride alignment.
> However, if you have a device that requires the stride to be aligned on
> a multiple of 32 bytes, the bytes per line value will be aligned by the
> pipeline handler manually after calling PixelFormatInfo::bytesPerLine(),
> but this won't be taken into account in PixelFormatInfo::imageSize(). I
> would either pass the stride and height instead of the size to
> PixelFormatInfo::imageSize(), or drop the function altogether as all it
> does is a multiplication that doesn't depend on the PixelFormatInfo and
> that can be easily done in the callers.

Actually I think we should keep the function, but we need to make it
more complicated. This won't work well for multi-planar formats, as
bytesPerLine() returns the stride for the first plane only. You need to
take the number of planes and the vertical subsampling into account.
Then there will be the question of whether we can assume that the
alignment restrictions of the hardware will be the same for all planes.
I wonder if the function should take an array of strides, with one value
per plane. We need to think a bit about how a good API would look like.

> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart