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

[Laurent Pinchart]

Hi Kieran,

On Thu, Sep 02, 2021 at 10:54:19AM +0100, Kieran Bingham wrote:
> 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?

It's not supposed to happen, I'd say it's a kernel bug in that case. But
it's a good point, I think we can return the buffer, and mark its status
as erroneous. I'll do so.

> > +				<< "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;
> > 

-- 
Regards,

Laurent Pinchart