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

[paul.elder at ideasonboard.com]

Hi Jacopo,

Thank you for the review.

On Wed, Jul 08, 2020 at 11:10:45PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> 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
> 
> s/is/has ? (if this wasn't intentional, your English is slightly
> better than mine so it might be correct :)

This is fine as-is :) We /at the moment/ want to compute the stride of a
plane, so we can refer to the stride in present tense.

> > + * \param[in] align The number of bytes to align to (0 for default alignment)
> 
> I would then make this a default parameter

It is, in the header.

> > + * \return The number of bytes necessary to store a line, or 0 if the
> 
> How is likely be used stride by applications ? Could it be used as
> divisor ? silent divisios by 0 are nasty to debug, is it worth an Info
> message ? Alternatively you could return an int with an error code,
> but I wonder how many would actually check that, so an error message
> might be better.

Yeah, that's true. I'll add an Info message at least, then. We return
unsigned int for stride (as does struct v4l2_format.fmt.pix.bytesperline),
so we can't really return negative error code.

> > + * PixelFormatInfo instance or the \a plane is not valid
> > + */
> > +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,
> > +				     unsigned int align) const
>                                      ^ allignment

This, and the other alignments that you point out, are fine. Maybe
something happened in the mail client :/

> > +{
> > +	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;
> > +
> > +	if (!align)
> > +		return stride;
> 
> yes, maybe a default parameter is better, and maybe
> > +
> 	/* ceil(stride / align) * align */
>         return !align ? stride
>                       : (stride + align - 1) / align * align;

After making the default alignment 1 instead of 0, the branch won't be
necessary anymore.

> > +	return (stride + align - 1) / align * align;
> > +}
> > +
> > +/**
> > + * \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::frameSize(const Size &size) 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 += 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
> > + * \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
>                            ^ alignment (doesn't checkstyle complains?)
> 
> > +{
> > +	/* 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);
> 
> If you make of the stride stride a parameter defaulted to and empty
> vector you could merge the two functions here. Just
> 
>                 unsigned int s = ((size.height + vertSubSample - 1)/ vertSubSample);
>                 if (!strides.empty())
>                         s *= stride(size.width, i);
>                 else
>                         s *= strides[i];
> 
>                 sum += s;
> 
> or something similar

Oh yeah, that's better.

> The rest of the function seems identical to me.

It is :)

> > +	}
> > +
> > +	return sum;
> > +}
> > +
> >  } /* namespace libcamera */
> > --
> > 2.27.0


Thanks,

Paul