[libcamera-devel] [RFC PATCH v1 10/12] libcamera: v4l2_videodevice: Coalesce planes when queuing buffer

[Kieran Bingham]

On 02/09/2021 05:23, Laurent Pinchart wrote:
> When queueing a buffer to 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 coalesce all FrameBuffer planes into a single V4L2 buffer plane.
> Do so, and add validity checks to reject frame buffers that can't be
> described using a single V4L2 buffer plane.


Yikes ;-) lets see...

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 68 +++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 2d9a94c3c974..79cb792117d5 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -22,10 +22,12 @@
>  
>  #include <libcamera/base/event_notifier.h>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/utils.h>
>  
>  #include <libcamera/file_descriptor.h>
>  
>  #include "libcamera/internal/formats.h"
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/media_object.h"
>  
> @@ -1468,10 +1470,21 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  
>  	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	const std::vector<FrameBuffer::Plane> &planes = buffer->planes();
> +	unsigned int numV4l2Planes = format_.fourcc == formatInfo_->v4l2Format[0]
> +				   ? 1 : planes.size();

Aha, ok so that's how we recover the planar/multiplanar information I
was concerned we had lost earlier.

I wonder if we should set/cache numV4l2Planes_ when we set formatInfo_
as they are so closely paired .. But it's a trivial operation here.



> +
> +	/*
> +	 * If the frame buffer has multiple planes and the V4L2 format requires
> +	 * contiguous planes, ensure that's the case.
> +	 */
> +	if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
> +		LOG(V4L2, Error) << "Device format requires contiguous buffer";
> +		return -EINVAL;
> +	}
>  
>  	if (buf.memory == V4L2_MEMORY_DMABUF) {
>  		if (multiPlanar) {
> -			for (unsigned int p = 0; p < planes.size(); ++p)
> +			for (unsigned int p = 0; p < numV4l2Planes; ++p)
>  				v4l2Planes[p].m.fd = planes[p].fd.fd();
>  		} else {
>  			buf.m.fd = planes[0].fd.fd();
> @@ -1479,23 +1492,58 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  	}
>  
>  	if (multiPlanar) {
> -		buf.length = planes.size();
> +		buf.length = numV4l2Planes;
>  		buf.m.planes = v4l2Planes;
>  	}
>  
>  	if (V4L2_TYPE_IS_OUTPUT(buf.type)) {
>  		const FrameMetadata &metadata = buffer->metadata();
>  
> -		if (multiPlanar) {
> -			unsigned int nplane = 0;
> -			for (const FrameMetadata::Plane &plane : metadata.planes()) {
> -				v4l2Planes[nplane].bytesused = plane.bytesused;
> -				v4l2Planes[nplane].length = buffer->planes()[nplane].length;
> -				nplane++;
> +		if (numV4l2Planes != planes.size()) {
> +			/*
> +			 * If we have a multi-planar buffer with a V4L2
> +			 * single-planar format, coalesce all planes. The length
> +			 * and number of bytes used may only differ in the last
> +			 * plane as any other situation can't be represented.
> +			 */
> +			unsigned int bytesused = 0;
> +			unsigned int length = 0;
> +
> +			for (auto [i, plane] : utils::enumerate(planes)) {
> +				bytesused += metadata.planes()[i].bytesused;
> +				length += plane.length;
> +
> +				if (i != planes.size() - 1 && bytesused != length) {

Ah, i almost thought that was wrong, but it's
   "If we're not the last plane, and the plane is not fully used..."

So it's fine.


> +					LOG(V4L2, Error)
> +						<< "Holes in multi-planar buffer not supported";
> +					return -EINVAL;
> +				}
> +			}
> +
> +			if (multiPlanar) {
> +				v4l2Planes[0].bytesused = bytesused;
> +				v4l2Planes[0].length = length;
> +			} else {
> +				buf.bytesused = bytesused;
> +				buf.length = length;
> +			}
> +		} 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 (auto [i, plane] : utils::enumerate(planes)) {
> +				v4l2Planes[i].bytesused = metadata.planes()[i].bytesused;
> +				v4l2Planes[i].length = plane.length;
>  			}
>  		} else {
> -			if (metadata.planes().size())

Guaranteeing the planes are there is much nicer here.

Phew, this patch 'looks' a lot more complex than it actually is,

So I think ... this is fine.

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

> -				buf.bytesused = metadata.planes()[0].bytesused;
> +			/*
> +			 * Single-planar API with a single plane in the buffer
> +			 * is trivial to handle.
> +			 */
> +			buf.bytesused = metadata.planes()[0].bytesused;
> +			buf.length = planes[0].length;
>  		}
>  
>  		buf.sequence = metadata.sequence;
>