[libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo: Add functions stride and frameSize

[paul.elder at ideasonboard.com]

Hi Laurent,

Thank you for the review.

On Wed, Jul 08, 2020 at 05:52:10PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Wed, Jul 08, 2020 at 10:44:02PM +0900, Paul Elder wrote:
> > Add member functions to PixelFormatInfo for calculating stride and frame
> > size. This will simplify existing code that calculates these things.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > ---
> > Changes in v4:
> > - add overloaded frameSize() that takes array of strides
> > - add optional parameter to stride() for byte alignment
> > 
> > Changes in v3:
> > - rename functions to stride and frameSize, from bytesPerLine and
> >   imageSize, respectively
> > 
> > Changes in v2:
> > - make these functions const
> > - add documentation
> > - inline DIV_ROUND_UP
> > ---
> >  include/libcamera/internal/formats.h |  6 +++
> >  src/libcamera/formats.cpp            | 73 ++++++++++++++++++++++++++++
> >  2 files changed, 79 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > index 054be37..e347a46 100644
> > --- a/include/libcamera/internal/formats.h
> > +++ b/include/libcamera/internal/formats.h
> > @@ -52,6 +52,12 @@ public:
> >  	static const PixelFormatInfo &info(const PixelFormat &format);
> >  	static const PixelFormatInfo &info(const V4L2PixelFormat &format);
> >  
> > +	unsigned int stride(unsigned int width, unsigned int plane,
> > +			    unsigned int align = 0) const;
> > +	unsigned int frameSize(const Size &size) const;
> > +	unsigned int frameSize(const Size &size,
> > +			       const std::array<unsigned int, 3> &strides) const;
> > +
> >  	/* \todo Add support for non-contiguous memory planes */
> >  	const char *name;
> >  	PixelFormat format;
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 6d96055..c355e57 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -732,4 +732,77 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> >  	return info->second;
> >  }
> >  
> > +/**
> > + * \brief Compute the stride
> > + * \param[in] width The width of the line, in pixels
> > + * \param[in] plane The index of the plane whose stride is to be computed
> > + * \param[in] align The number of bytes to align to (0 for default alignment)
> > + * \return The number of bytes necessary to store a line, or 0 if the
> > + * PixelFormatInfo instance or the \a plane is not valid
> > + */
> > +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,
> > +				     unsigned int align) const
> > +{
> > +	if (!isValid())
> > +		return 0;
> > +
> > +	if (plane > planes.size() || !planes[plane].bytesPerGroup)
> > +		return 0;
> > +
> > +	/* ceil(width / pixelsPerGroup) * bytesPerGroup */
> > +	unsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup
> > +			      * planes[plane].bytesPerGroup;
> 
> * should be under =
> 
> > +
> > +	if (!align)
> > +		return stride;
> 
> I wonder if we should default align to 1 instead of 0 and drop this.

Well at first I thought that align to 1 bytes doesn't make sense, until
I realized that stride must be a multiple of align, so 1 does actually
make sense, both semantically and mathematically. So yeah, I'll change
it to 1.

> > +
> > +	/* ceil(stride / align) * align */
> > +	return (stride + align - 1) / align * align;
> > +}
> > +
> > +/**
> > + * \brief Compute the bytes necessary to store the frame
> 
> s/the bytes/the number of bytes/
> s/the frame/a frame/
> 
> Same for the next function
> 
> > + * \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::frameSize(const Size &size) const
> 
> Would it be useful to add an align parameter to this function, that
> would default to 0 (or 1 depending on the outcome of the discussion
> above) ? That should cover most pipeline handlers, with the next
> function being for really odd cases.

Yes, I think it would be useful.

> > +{
> > +	/* stride * ceil(height / verticalSubSampling) */
> > +	unsigned int sum = 0;
> > +	for (unsigned int i = 0; i < 3; i++) {
> > +		unsigned int vertSubSample = planes[i].verticalSubSampling;
> > +		if (!vertSubSample)
> > +			continue;
> > +		sum += stride(size.width, i)
> > +		     * ((size.height + vertSubSample - 1) / vertSubSample);
> > +	}
> > +
> > +	return sum;
> > +}
> > +
> > +/**
> > + * \brief Compute the bytes necessary to store the frame
> > + * \param[in] size The size of the frame, in pixels
> > + * \param[in] strides The strides to use for each plane
> 
>  *
>  * This function is an overloaded version that takes custom strides for each
>  * plane, to be used when the device has custom alignment constraints that
>  * can't be described by just an alignment value.
>  *
> 
> ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > + * \return The number of bytes necessary to store the frame, or 0 if the
> > + * PixelFormatInfo instance is not valid
> > + */
> > +unsigned int
> > +PixelFormatInfo::frameSize(const Size &size,
> > +			   const std::array<unsigned int, 3> &strides) const
> > +{
> > +	/* stride * ceil(height / verticalSubSampling) */
> > +	unsigned int sum = 0;
> > +	for (unsigned int i = 0; i < 3; i++) {
> > +		unsigned int vertSubSample = planes[i].verticalSubSampling;
> > +		if (!vertSubSample)
> > +			continue;
> > +		sum += strides[i]
> > +		     * ((size.height + vertSubSample - 1) / vertSubSample);
> > +	}
> > +
> > +	return sum;
> > +}
> > +
> >  } /* namespace libcamera */


Thanks,

Paul