[libcamera-devel] [RFC PATCH 08/10] libcamera: v4l2_videodevice: Create color-format planes in CreateBuffer()

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote:
> V4L2VideDevice::CreateBuffer() creates the same number of

s/CreateBuffer/createBuffer/

> FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2
> format single is single-planar format, the created number of
> FrameBuffer::Planes is 1.
> It should rather create the same number of FrameBuffer::Planes as the
> color format planes.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  4 ++-
>  src/libcamera/v4l2_videodevice.cpp            | 28 +++++++++++++++++--
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index e767ec84..d0aeb614 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -136,11 +136,13 @@ private:
>  	private:
>  		struct Plane {
>  			Plane(const FrameBuffer::Plane &plane)
> -				: fd(plane.fd.fd()), length(plane.length)
> +				: fd(plane.fd.fd()), offset(plane.offset),
> +				  length(plane.length)
>  			{
>  			}
>  
>  			int fd;
> +			unsigned int offset;
>  			unsigned int length;
>  		};
>  
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ce60dff6..549418c8 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -25,6 +25,7 @@
>  
>  #include <libcamera/file_descriptor.h>
>  
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/media_object.h"
>  
> @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>  
>  	for (unsigned int i = 0; i < planes.size(); i++)
>  		if (planes_[i].fd != planes[i].fd.fd() ||
> +		    planes_[i].offset != planes_[i].offset ||
>  		    planes_[i].length != planes[i].length)
>  			return false;
>  	return true;
> @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)

It would be good to capture the fact that the FrameBuffer planes now
match the colour planes and not the V4L2 planes. This function is
private and isn't documented, so we can't handle it here, and should
instead document this in V4L2VideoDevice::allocateBuffers() and
V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that
state

 * The number of planes and the plane sizes for the allocation are determined
 * by the currently active format on the device as set by setFormat().

It could be extended to

 * The number of planes and their offsets and sizes are determined by the
 * currently active format on the device as set by setFormat(). They do not map
 * to the V4L2 buffer planes, but to colour planes of the pixel format. For
 * instance, if the active format is formats::NV12, the allocated FrameBuffer
 * instances will have two planes, for the luma and chroma components,
 * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
 * V4L2_PIX_FMT_NV12M.

>  
>  		FrameBuffer::Plane plane;
>  		plane.fd = std::move(fd);
> -		plane.length = multiPlanar ?
> -			buf.m.planes[nplane].length : buf.length;
> +		plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0;
> +		plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;

The data_offset field defined by V4L2 is, unfortunately, not the offset
in the dmabuf, but the offset of the data from the start of the plane.
It's used by codec drivers to indicate the size of the header stored at
the beginning of the plane. The data_offset is actually contained in the
length.

I think we can ignore the data offset reported by V4L2, until the V4L2
API is expanded with a "real" data offset. plane.offset can be hardcoded
to 0. The change to operator== could also probably be dropped.

>  
>  		planes.push_back(std::move(plane));
>  	}
>  
> +	V4L2DeviceFormat format{};
> +	ret = getFormat(&format);
> +	if (ret < 0) {
> +		LOG(V4L2, Error)
> +			<< "Failed to get format: " << strerror(-ret);
> +		return nullptr;
> +	}
> +

A comment to explain what's going on would be useful.

> +	const auto &info = PixelFormatInfo::info(format.fourcc);
> +	if (info.isValid() && info.numPlanes() != numPlanes) {

If info isn't valid, should we return nullptr ?

> +		ASSERT(numPlanes == 1u);
> +		planes.resize(info.numPlanes());
> +		const FileDescriptor &fd = planes[0].fd;
> +		size_t offset = 0;
> +		for (size_t i = 0; i < info.numPlanes(); ++i) {
> +			planes[i].fd = fd;
> +			planes[i].offset = offset;
> +			planes[i].length = info.frameSize(format.size);

Isn't it the plane size that we need here, not the full frame size ?
Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format,
will return the total size of the image, including both colour planes.

There's also one change missing. You're addressing the buffer allocation
side, but not the buffer queuing side. Now that an NV12 FrameBuffer has
two planes, we need to translate from two colour planes to one V4L2
plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used.

> +			offset += planes[i].length;
> +		}
> +	}
> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>  

-- 
Regards,

Laurent Pinchart