[libcamera-devel] [PATCH v2 16/27] libcamera: framebuffer: Allocate metadata planes at construction time

[Jean-Michel Hautbois]

Hi Laurent,

On 06/09/2021 04:00, 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>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> Changes since v1:
> 
> - Return buffer with state set to FrameError on error
> ---
>  src/libcamera/framebuffer.cpp      |  2 ++
>  src/libcamera/v4l2_videodevice.cpp | 10 +++++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index e71c2ffae034..e4f8419a9063 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;
>  	bool isContiguous = true;
>  	ino_t inode = 0;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 59aa53c7c27e..e729e608448c 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1670,7 +1670,6 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
>  
>  	unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
>  	FrameMetadata &metadata = buffer->metadata_;
> -	metadata.planes.clear();
>  
>  	if (numV4l2Planes != buffer->planes().size()) {
>  		/*
> @@ -1700,8 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
>  				return buffer;
>  			}
>  
> -			metadata.planes.push_back({ std::min(plane.length, bytesused) });
> -			bytesused -= metadata.planes.back().bytesused;
> +			metadata.planes[i].bytesused =
> +				std::min(plane.length, bytesused);
> +			bytesused -= metadata.planes[i].bytesused;
>  		}
>  	} else if (multiPlanar) {
>  		/*
> @@ -1710,9 +1710,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
>  		 * V4L2 buffer is guaranteed to be equal at this point.
>  		 */
>  		for (unsigned int i = 0; i < numV4l2Planes; ++i)
> -			metadata.planes.push_back({ planes[i].bytesused });
> +			metadata.planes[i].bytesused = planes[i].bytesused;
>  	} else {
> -		metadata.planes.push_back({ buf.bytesused });
> +		metadata.planes[0].bytesused = buf.bytesused;
>  	}
>  
>  	return buffer;
>