[libcamera-devel] [RFC PATCH 4/5] WIP: libcamera: V4L2VideoDevice: Fix a bug in CreateBuffer()

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Wed, Aug 11, 2021 at 09:40:14PM +0900, Hirokazu Honda wrote:
> FrameBuffer created in V4L2VideoDevice::CreateBuffer() has the same
> number of planes as v4l2 buffer planes. However, if the format is
> a single planar format, the number of planes is one. FrameBuffer
> should have the same number of planes as color format planes.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ce60dff6..e70076f3 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1269,7 +1269,6 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  
>  	const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	const unsigned int numPlanes = multiPlanar ? buf.length : 1;
> -

Unrelated change ?

>  	if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
>  		LOG(V4L2, Error) << "Invalid number of planes";
>  		return nullptr;
> @@ -1289,6 +1288,20 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  		planes.push_back(std::move(plane));
>  	}
>  
> +

Extra blank line.

> +	V4L2DeviceFormat format{};

V4L2DeviceFormat initializes all members to 0 when constructed, you can
drop {}.

> +	ret = getFormat(&format);
> +	if (ret < 0) {
> +		LOG(V4L2, Error) << "Failed to get buffer format";
> +		return nullptr;
> +	}

I'd move the extra blank line from above here :-)

This being said, maybe we could cache the fourcc in V4L2VideoDevice
instead of querying it back from the kernel ?

> +	if (format.fourcc == V4L2_PIX_FMT_NV12) {
> +		planes.resize(2);
> +		planes[0].length = format.size.width * format.size.height;
> +		planes[1].fd = planes[0].fd;
> +		planes[1].length = format.size.width * ((format.size.height + 1) / 2);
> +	}

I don't think this is right. Well, it is, but at the same time, it isn't
:-)

At the moment, the number of planes in the FrameBuffer class matches the
number of "buffer planes" used by V4L2 (1 for NV12, as opposed to the
number of "format planes", which would be 2). I agree that we want to
move to "format planes" in the long term here, to avoid exposing the
V4L2 historical design mistake to applications ("long term" meaning "as
soon as possible" here). However, this requires adding an offset field
to the FrameBuffer::Plane structure, as otherwise we miss the
information to implement the calculation properly (or at least we miss
exposing the information explictly and rely on undocumented
implementation details).

There's no need, I think, to implement this as part of this series. We
can keep the calculation internal to MappedFrameBuffer as a first step,
and then address the issue in the FrameBuffer class. It may also be
possible to do it the other way around, and move to "format planes" in
FrameBuffer at the bottom of the series. I have a feeling that it may be
difficult to keep patches reasonable in size and avoid bisection
breakages in that case, but I may be wrong.

> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>  

-- 
Regards,

Laurent Pinchart