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

[Laurent Pinchart]

Hi Nicolas,

On Wed, Aug 18, 2021 at 09:47:36AM -0400, Nicolas Dufresne wrote:
> 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.

As mentioned in my review of this patch, V4L2 indeed uses a different
semantic, which doesn't match what we want to use. The V4L2 data_offset
field is really meant for codecs to indicate the header size. Reusing it
as-is would be an API abuse in my opinion. The DRM semantic is better,
and one day we may extend V4L2 to support it too, and finally be able to
support NV12M stored in a single dmabuf. Until then, I recommend
ignoring the V4L2 data_offset completely, and mapping the libcamera
formats::NV12 to V4L2_PIX_FMT_NV12, with a translation in this class
between the 2 colour planes in the FrameBuffer and the single buffer
plane in the V4L2 buffer.

> >  
> >  		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));
> >  }
> >  

-- 
Regards,

Laurent Pinchart