[libcamera-devel] [RFC PATCH v1 11/12] libcamera: v4l2_videodevice: Split planes when dequeuing buffer

[Laurent Pinchart]

Hi Kieran,

On Thu, Sep 02, 2021 at 12:17:31PM +0100, Kieran Bingham wrote:
> On 02/09/2021 05:23, Laurent Pinchart wrote:
> > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in
> > the FrameBuffer may not match the number of V4L2 buffer planes if the
> > PixelFormat is multi-planar (has multiple colour planes) and the V4L2
> > format is single-planar (has a single buffer plane). In this case, we
> > need to split the single V4L2 buffer plane into FrameBuffer planes. Do
> > so, and add checks to reject invalid V4L2 buffers in case of a driver
> > issue.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 54 ++++++++++++++++++++++++------
> >  1 file changed, 43 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 79cb792117d5..b80b9038a914 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -7,6 +7,7 @@
> >  
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >  
> > +#include <algorithm>
> >  #include <array>
> >  #include <fcntl.h>
> >  #include <iomanip>
> > @@ -1637,18 +1638,49 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> >  	buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> >  				    + buf.timestamp.tv_usec * 1000ULL;
> >  
> > -	if (multiPlanar) {
> > -		if (buf.length > buffer->metadata_.planes().size()) {
> > -			LOG(V4L2, Error)
> > -				<< "Invalid number of planes (" << buf.length
> > -				<< " != " << buffer->metadata_.planes().size() << ")";
> > -			return nullptr;
> > +	if (V4L2_TYPE_IS_CAPTURE(buf.type)) {
> 
> Should we swap this around and return the buffer early to lower the
> indents below?
> 
>  if (!V4L2_TYPE_IS_CAPTURE(buf.type)) {
> 	/* Output buffer is completed */
> 	return buffer;
>  }

Yes, good point.

> > +		unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> > +		FrameMetadata &metadata = buffer->metadata_;
> > +
> > +		if (numV4l2Planes != metadata.planes().size()) {
> > +			/*
> > +			 * If we have a multi-planar buffer with a V4L2
> > +			 * single-planar format, split the V4L2 buffer across
> > +			 * the buffer planes. Only the last plane may have less
> > +			 * bytes used than its length.
> > +			 */
> > +			if (numV4l2Planes != 1) {
> > +				LOG(V4L2, Error)
> > +					<< "Invalid number of planes (" << numV4l2Planes
> > +					<< " != " << metadata.planes().size() << ")";
> > +				return nullptr;
> > +			}
> > +
> > +			unsigned int bytesused = multiPlanar ? planes[0].bytesused
> > +					       : buf.bytesused;
> > +
> > +			for (auto [i, plane] : utils::enumerate(buffer->planes())) {
> > +				if (!bytesused) {
> > +					LOG(V4L2, Error)
> > +						<< "Dequeued buffer is too small";
> 
> Presumably if we got here - it's likely something (hardware) already
> overwrote memory it likely didn't own?

It likely means something bad happened, yes. It's a bit of defensive
programming, to output a clear error message if the kernel does
something bad, but also to possibly catch bugs in the implementation of
this function.

> > +					return nullptr;
> 
> I'm still very weary of returning nullptrs on dequeue when a buffer is
> removed from queuedBuffers_
> 
> 
> Wouldn't it be better to return the buffer, but mark it as corrupted or
> invalid or such ?

Yes, I'll do that.

> > +				}
> > +
> > +				metadata.planes()[i].bytesused =
> > +					std::min(plane.length, bytesused);
> > +				bytesused -= metadata.planes()[i].bytesused;
> > +			}
> > +		} else if (multiPlanar) {
> > +			/*
> > +			 * If we use the multi-planar API, fill in the planes.
> > +			 * The number of planes in the frame buffer and in the
> > +			 * V4L2 buffer is guaranteed to be equal at this point.
> > +			 */
> > +			for (unsigned int i = 0; i < numV4l2Planes; ++i)
> > +				metadata.planes()[i].bytesused = planes[i].bytesused;
> > +		} else {
> > +			metadata.planes()[0].bytesused = buf.bytesused;
> >  		}
> > -
> > -		for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> > -			buffer->metadata_.planes()[nplane].bytesused = planes[nplane].bytesused;
> > -	} else {
> > -		buffer->metadata_.planes()[0].bytesused = buf.bytesused;
> >  	}
> >  
> >  	return buffer;
> > 

-- 
Regards,

Laurent Pinchart