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

[Nicolas Dufresne]

Le lundi 16 août 2021 à 13:31 +0900, Hirokazu Honda a écrit :
> V4L2VideDevice::CreateBuffer() creates the same number of
> 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)
>  
>  		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;

There might be a issue here, please carefully review. In V4L2. buf.length (or
plane.length in mplane) includes data_offset but in your implementation you add
it again. We need to document the semantic we want in libcamera and make sure
its all proper.

>  
>  		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;
> +	}
> +
> +	const auto &info = PixelFormatInfo::info(format.fourcc);
> +	if (info.isValid() && info.numPlanes() != numPlanes) {
> +		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);
> +			offset += planes[i].length;
> +		}
> +	}
> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>