[libcamera-devel] [RFC PATCH v2 10/10] libcamera: framebuffer: Add assertion to detect offset is unfilled

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 23, 2021 at 10:12:21PM +0900, Hirokazu Honda wrote:
> The offset variable is introduced to FrameBuffer::Plane. In order to
> detect that the plane is used while the offset is not set, this adds
> the assertion to FrameBuffer::planes(). It should be removed in the
> future.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/framebuffer.h | 13 +++++++++++--
>  src/libcamera/framebuffer.cpp   |  4 ++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index 5de3c744..d5aeff00 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -7,6 +7,8 @@
>  #ifndef __LIBCAMERA_FRAMEBUFFER_H__
>  #define __LIBCAMERA_FRAMEBUFFER_H__
>  
> +#include <assert.h>
> +#include <limits>
>  #include <stdint.h>
>  #include <vector>
>  
> @@ -41,14 +43,21 @@ class FrameBuffer final : public Extensible
>  
>  public:
>  	struct Plane {
> +		static constexpr unsigned int kInvalidOffset = std::numeric_limits<unsigned int>::max();
>  		FileDescriptor fd;
> -		unsigned int offset;
> +		unsigned int offset = kInvalidOffset;
>  		unsigned int length;
>  	};
>  
>  	FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
>  
> -	const std::vector<Plane> &planes() const { return planes_; }
> +	const std::vector<Plane> &planes() const
> +	{
> +		/* \todo Remove the assertions after sufficient testing */
> +		for (const auto &plane : planes_)
> +			assert(plane.offset != Plane::kInvalidOffset);
> +		return planes_;
> +	}
>  
>  	Request *request() const;
>  	const FrameMetadata &metadata() const { return metadata_; }
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index e9467aa0..5f612ecd 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -165,6 +165,10 @@ FrameBuffer::Private::Private()
>   * multiple dmabufs, based on the camera requirements.
>   */
>  
> +/**
> + * \var FrameBuffer::Plane::kInvalidOffset
> + * \brief The invalid offset value

"Invalid offset value, to identify uninitialized planes"

> + */

Blank line.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  /**
>   * \var FrameBuffer::Plane::fd
>   * \brief The dmabuf file descriptor

-- 
Regards,

Laurent Pinchart