[libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer: Move planes check to constructor

[Kieran Bingham]

On 02/09/2021 05:22, Laurent Pinchart wrote:
> The FrameBuffer::planes() function checks that planes are correctly
> initialized with an offset. This can be done at construction time
> instead, as the planes are constant. The runtime overhead is reduced,
> and the backtrace generated by the assertion will show where the faulty
> frame buffer is created instead of where it is used, easing debugging.

That sounds very enticing...

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/framebuffer.h | 9 +--------
>  src/libcamera/framebuffer.cpp   | 3 +++
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index d5aeff00387b..fd68ed0a139d 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -51,14 +51,7 @@ public:
>  
>  	FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
>  
> -	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_;
> -	}
> -
> +	const std::vector<Plane> &planes() const { return planes_; }
>  	Request *request() const;
>  	const FrameMetadata &metadata() const { return metadata_; }
>  
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index c99f5b15e6ff..53ef89bf458f 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -199,6 +199,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>  	: Extensible(std::make_unique<Private>()), planes_(planes),
>  	  cookie_(cookie)
>  {
> +	/* \todo Remove the assertions after sufficient testing */

As a construction time validation event, which is compiled/optimised out
in release builds anyway, I'm tempted to remove the todo here and keep
the ASSERT ...

But either way,

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +	for (const auto &plane : planes_)
> +		ASSERT(plane.offset != Plane::kInvalidOffset);
>  }
>  
>  /**
>