[libcamera-devel] [RFC PATCH v1 07/12] libcamera: framebuffer: Allocate metadata planes at construction time

[Kieran Bingham]

On 02/09/2021 05:22, Laurent Pinchart wrote:
> The metadata planes are allocated by V4L2VideoDevice when dequeuing a
> buffer. This causes the metadata planes to only be allocated after a
> buffer gets dequeued, and doesn't provide any strong guarantee that
> their number matches the number of FrameBuffer planes. The lack of this
> invariant makes the FrameBuffer class fragile.
> 
> As a first step towards fixing this, allocate the metadata planes when
> the FrameBuffer is constructed. The FrameMetadata API should be further
> improved by preventing a change in the number of planes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/framebuffer.cpp      |  2 ++
>  src/libcamera/v4l2_videodevice.cpp | 12 +++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 99265b44da43..c1529dfb0ad1 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -210,6 +210,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>  	: Extensible(std::make_unique<Private>()), planes_(planes),
>  	  cookie_(cookie)
>  {
> +	metadata_.planes.resize(planes_.size());
> +
>  	unsigned int offset = 0;
>  
>  	/* \todo Remove the assertions after sufficient testing */
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index adabd4720668..a51971879e75 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1586,12 +1586,18 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
>  	buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
>  				    + buf.timestamp.tv_usec * 1000ULL;
>  
> -	buffer->metadata_.planes.clear();
>  	if (multiPlanar) {
> +		if (buf.length > buffer->metadata_.planes.size()) {
> +			LOG(V4L2, Error)

Are we going to 'leak' queued FrameBuffers at this point?
Is there a way to prevent this occurring at queue time rather than dequeue?

In fact, Can it even happen? or should it be an ASSERT/Fatal?


> +				<< "Invalid number of planes (" << buf.length
> +				<< " != " << buffer->metadata_.planes.size() << ")";
> +			return nullptr;
> +		}
> +
>  		for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> -			buffer->metadata_.planes.push_back({ planes[nplane].bytesused });
> +			buffer->metadata_.planes[nplane].bytesused = planes[nplane].bytesused;
>  	} else {
> -		buffer->metadata_.planes.push_back({ buf.bytesused });
> +		buffer->metadata_.planes[0].bytesused = buf.bytesused;
>  	}
>  
>  	return buffer;
>