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

[Jacopo Mondi]

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 :)

> + * \param[in] align The number of bytes to align to (0 for default alignment)

I would then make this a default parameter

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

> + * PixelFormatInfo instance or the \a plane is not valid
> + */
> +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,
> +				     unsigned int align) const
                                     ^ allignment
> +{
> +	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;

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

The rest of the function seems identical to me.

> +	}
> +
> +	return sum;
> +}
> +
>  } /* namespace libcamera */
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel