[libcamera-devel] [PATCH v2 1/3] android: generic_camera_buffer: Correct buffer mapping

[Jacopo Mondi]

Hi Hiro,

On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:
> buffer_hanlde_t doesn't provide sufficient info to map a buffer
> properly. cros::CameraBufferManager enables handling the buffer on
> ChromeOS, but no way is provided for other platforms.
>
> Therefore, we put the assumption that planes are in the same buffer
> and they are consecutive. This modifies the way of mapping in
> generic_camera_buffer with the assumption.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_buffer.h              |  14 ++-
>  src/android/camera_stream.cpp            |   4 +-
>  src/android/mm/cros_camera_buffer.cpp    |   7 +-
>  src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------
>  4 files changed, 99 insertions(+), 30 deletions(-)
>
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index c4e3a9e7..87df2570 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -11,13 +11,17 @@
>
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/span.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
>
>  class CameraBuffer final : public libcamera::Extensible
>  {
>  	LIBCAMERA_DECLARE_PRIVATE()
>
>  public:
> -	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> +	CameraBuffer(buffer_handle_t camera3Buffer,
> +		     libcamera::PixelFormat pixelFormat,
> +		     const libcamera::Size &size, int flags);
>  	~CameraBuffer();
>
>  	bool isValid() const;
> @@ -31,8 +35,12 @@ public:
>  };
>
>  #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION				\
> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
> -	: Extensible(std::make_unique<Private>(this, camera3Buffer, flags)) \
> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer,		\
> +			   libcamera::PixelFormat pixelFormat,		\
> +			   const libcamera::Size &size, int flags)	\
> +	: Extensible(std::make_unique<Private>(this, camera3Buffer,	\
> +					       pixelFormat, size,	\
> +					       flags))			\
>  {									\
>  }									\
>  CameraBuffer::~CameraBuffer()						\
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 61b44183..01909ec7 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>  	 * \todo Buffer mapping and processing should be moved to a
>  	 * separate thread.
>  	 */
> -	CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);
> +	const StreamConfiguration &output = configuration();
> +	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> +			  PROT_READ | PROT_WRITE);
>  	if (!dest.isValid()) {
>  		LOG(HAL, Error) << "Failed to map android blob buffer";
>  		return -EINVAL;
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index e8783ff8..50732637 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private
>  	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
>
>  public:
> -	Private(CameraBuffer *cameraBuffer,
> -		buffer_handle_t camera3Buffer, int flags);
> +	Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> +		libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> +		int flags);
>  	~Private();
>
>  	bool isValid() const { return valid_; }
> @@ -46,6 +47,8 @@ private:
>
>  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  			       buffer_handle_t camera3Buffer,
> +			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> +			       [[maybe_unused]] const libcamera::Size &size,
>  			       [[maybe_unused]] int flags)
>  	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
>  	  registered_(false)
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index b3af194c..0bfdc2ba 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -12,6 +12,7 @@
>
>  #include <libcamera/base/log.h>
>
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/mapped_framebuffer.h"
>
>  using namespace libcamera;
> @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,
>  	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
>
>  public:
> -	Private(CameraBuffer *cameraBuffer,
> -		buffer_handle_t camera3Buffer, int flags);
> +	Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> +		libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> +		int flags);
>  	~Private();
>
>  	unsigned int numPlanes() const;
> @@ -33,35 +35,89 @@ public:
>  	Span<uint8_t> plane(unsigned int plane);
>
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> +
> +private:
> +	/* \todo Remove planes_ when it will be added to MappedBuffer */
> +	std::vector<Span<uint8_t>> planes_;
>  };
>
>  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> -			       buffer_handle_t camera3Buffer, int flags)
> +			       buffer_handle_t camera3Buffer,
> +			       libcamera::PixelFormat pixelFormat,
> +			       const libcamera::Size &size, int flags)
>  {
> -	maps_.reserve(camera3Buffer->numFds);
>  	error_ = 0;
> -
> +	/*
> +	 * As Android doesn't offer an API to query buffer layouts, assume for
> +	 * now that the buffer is backed by a single dmabuf, with planes being
> +	 * stored contiguously.
> +	 */
> +	int fd = -1;
>  	for (int i = 0; i < camera3Buffer->numFds; i++) {

unsigned int i

> -		if (camera3Buffer->data[i] == -1)
> +		if (camera3Buffer->data[i] == -1 ||
> +		    camera3Buffer->data[i] == fd) {
>  			continue;
> -
> -		off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);
> -		if (length < 0) {
> -			error_ = -errno;
> -			LOG(HAL, Error) << "Failed to query plane length";
> -			break;
>  		}
>
> -		void *address = mmap(nullptr, length, flags, MAP_SHARED,
> -				     camera3Buffer->data[i], 0);
> -		if (address == MAP_FAILED) {
> -			error_ = -errno;
> -			LOG(HAL, Error) << "Failed to mmap plane";
> -			break;
> -		}
> +		if (fd != -1)
> +			LOG(HAL, Fatal) << "Discontiguous planes are not supported";

Do we just warn ? Or exit with error ? See the below question

> +		fd = camera3Buffer->data[i];

This assigns fd to the 'next' valid data[i].fd. I might have missed
what is the policy here if we have more than one valid dmabuf fds.

Currently you warn and use the last valid one. I would instead error out,
or warn but use the first available one (even if that would result in
undefined behaviour, or most probably an access to non valid memory when
creating planes below).

Do you have a use case for formats with planes allocated in different
buffers that you need to support ? Or can we error out without causing
regressions ?

> +	}

The loop is pretty convoluted. If I'm not mistaken what you want to
verify is that
1) Either all data[i].fd are the same
2) All but the data[0].fd are -1

Wouldn't it be nicer as

        int fd = camera3Buffer->data[0].fd;
        for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {
                if (camera3Buffer->data[i].fd != -1)
                        continue;

                if (camera3Buffer->data[i].fd != fd)
                        /* Here I would error out */
        }
        if (fd == -1) {
                /* No valid fds, error out */
        }

> +
> +	if (fd != -1) {

Shouldn't this be fd == -1 ? As in "no valid fd found ?"
If I'm not mistaken checking for fd != -1 should fail all the times as
at least one valid fd should be found. On which platform have you
tested this patch ?

> +		error_ = -EINVAL;
> +		LOG(HAL, Error) << "No valid file descriptor";
> +		return;
> +	}
>
> -		maps_.emplace_back(static_cast<uint8_t *>(address),
> -				   static_cast<size_t>(length));
> +	const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> +	if (!info.isValid()) {
> +		error_ = -EINVAL;
> +		LOG(HAL, Error) << "Invalid pixel format: "
> +				<< pixelFormat.toString();
> +		return;
> +	}

We could check this at the very beginning of the function and avoid
looping on fds if the format is invalid.

> +
> +	off_t bufferLength = lseek(fd, 0, SEEK_END);
> +	if (bufferLength < 0) {
> +		error_ = -errno;
> +		LOG(HAL, Error) << "Failed to get buffer length";
> +		return;
> +	}
> +
> +	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> +	if (address == MAP_FAILED) {
> +		error_ = -errno;
> +		LOG(HAL, Error) << "Failed to mmap plane";
> +		return;
> +	}
> +	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> +
> +	const unsigned int numPlanes = info.numPlanes();
> +	planes_.resize(numPlanes);
> +	unsigned int offset = 0;
> +	for (unsigned int i = 0; i < numPlanes; ++i) {
> +		/*
> +		 * \todo Remove if this plane size computation function is
> +		 * added to PixelFormatInfo.
> +		 */
> +		const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
> +		const unsigned int stride = info.stride(size.width, i, 1u);
> +		const unsigned int planeSize =
> +			stride * ((size.height + vertSubSample - 1) / vertSubSample);
> +
> +		planes_[i] = libcamera::Span<uint8_t>(
> +			static_cast<uint8_t *>(address) + offset, planeSize);
> +
> +		if (bufferLength < offset + planeSize) {
> +			error_ = -EINVAL;
> +			LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> +					<< ", buffer length=" << bufferLength
> +					<< ", offset=" << offset
> +					<< ", size=" << planeSize;
> +			return;
> +		}
> +		offset += planeSize;
>  	}
>  }
>
> @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()
>
>  unsigned int CameraBuffer::Private::numPlanes() const
>  {
> -	return maps_.size();
> +	return planes_.size();
>  }
>
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  {
> -	if (plane >= maps_.size())
> +	if (plane >= planes_.size())
>  		return {};
>
> -	return maps_[plane];
> +	return planes_[plane];
>  }
>
>  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>