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

[Laurent Pinchart]

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.

> +
> +	/* 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.

> +{
> +	/* 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 */

-- 
Regards,

Laurent Pinchart