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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

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.

> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart