[libcamera-devel] [PATCH 10/10] android: Introduce Chromium OS buffer manager

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 01, 2021 at 04:01:11PM +0100, Jacopo Mondi wrote:
> Introduce the CameraBuffer backend for the Chromium OS operating system
> and the associated meson option.
> 
> The Chromium OS CameraBuffer implementation uses the
> cros::CameraBufferManager class to perform mapping of 1 plane and multiplane
> buffers and to retrieve size information.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  meson_options.txt                     |   2 +-
>  src/android/mm/cros_camera_buffer.cpp | 141 ++++++++++++++++++++++++++
>  src/android/mm/meson.build            |   3 +
>  3 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 src/android/mm/cros_camera_buffer.cpp
> 
> diff --git a/meson_options.txt b/meson_options.txt
> index d840543b01f5..870914662f3e 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -7,7 +7,7 @@ option('android',
>  
>  option('android_platform',
>          type : 'combo',
> -        choices : ['generic'],
> +        choices : ['generic', 'cros'],

Alphabetical order ?

>          value : 'generic',
>          description : 'Select the Android platform to compile for')
>  
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> new file mode 100644
> index 000000000000..1103f4423c2f
> --- /dev/null
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * cros_camera_buffer.cpp - Chromium OS buffer backend using CameraBufferManager
> + */
> +
> +#include "../camera_buffer.h"
> +
> +#include "libcamera/internal/log.h"
> +#include <libcamera/span.h>

I think you can drop span.h, it's provided by camera_buffer.h as part of
the CameraBuffer interface..

> +
> +#include "cros-camera/camera_buffer_manager.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL)
> +
> +class CameraBuffer::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
> +
> +public:
> +	Private(CameraBuffer *cameraBuffer,
> +		buffer_handle_t camera3Buffer, int flags);
> +	~Private();
> +
> +	bool isValid() const { return valid_; }
> +
> +	unsigned int numPlanes() const;
> +
> +	Span<const uint8_t> plane(unsigned int plane) const;
> +	Span<uint8_t> plane(unsigned int plane);
> +
> +private:
> +	cros::CameraBufferManager *bufferManager_;
> +	buffer_handle_t handle_;
> +	unsigned int numPlanes_;
> +	bool valid_;
> +	union {
> +		void *addr;
> +		android_ycbcr ycbcr;
> +	} mem;
> +
> +	const uint8_t *planeAddr(unsigned int plane) const;
> +	uint8_t *planeAddr(unsigned int plane);
> +};
> +
> +CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> +			       buffer_handle_t camera3Buffer, int flags)
> +	: Extensible::Private(cameraBuffer), handle_(camera3Buffer),
> +	  numPlanes_(0), valid_(false)
> +{
> +	bufferManager_ = cros::CameraBufferManager::GetInstance();
> +
> +	bufferManager_->Register(camera3Buffer);
> +
> +	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> +	switch (numPlanes_) {
> +	case 1: {
> +		int ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> +		if (ret) {
> +			LOG(HAL, Error) << "Single plane buffer mapping failed";
> +			return;
> +		}
> +		break;
> +	}
> +	case 2:
> +	case 3: {
> +		int ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> +						    &mem.ycbcr);
> +		if (ret) {
> +			LOG(HAL, Error) << "YCbCr buffer mapping failed";
> +			return;
> +		}
> +		break;
> +	}
> +	default:
> +		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> +		return;
> +	}
> +
> +	valid_ = true;
> +}
> +
> +CameraBuffer::Private::~Private()
> +{
> +	bufferManager_->Unlock(handle_);
> +	bufferManager_->Deregister(handle_);
> +}
> +
> +unsigned int CameraBuffer::Private::numPlanes() const
> +{
> +	return bufferManager_->GetNumPlanes(handle_);
> +}
> +
> +Span<const uint8_t> CameraBuffer::Private::plane(unsigned int plane) const
> +{
> +	const uint8_t *addr = planeAddr(plane);
> +	return { addr, bufferManager_->GetPlaneSize(handle_, plane) };
> +}
> +
> +Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> +{
> +	uint8_t *addr = planeAddr(plane);
> +	return { addr, bufferManager_->GetPlaneSize(handle_, plane) };
> +}
> +
> +const uint8_t *CameraBuffer::Private::planeAddr(unsigned int plane) const
> +{
> +	const void *addr;
> +
> +	switch (numPlanes()) {
> +	case 1:
> +		addr = mem.addr;
> +		break;
> +	default:
> +		switch (plane) {
> +		case 1:
> +			addr = mem.ycbcr.y;
> +			break;
> +		case 2:
> +			addr = mem.ycbcr.cb;
> +			break;
> +		case 3:
> +			addr = mem.ycbcr.cr;
> +			break;
> +		}
> +	}
> +
> +	return static_cast<const uint8_t *>(addr);
> +}
> +
> +uint8_t *CameraBuffer::Private::planeAddr(unsigned int plane)
> +{
> +	const CameraBuffer::Private *self =
> +		const_cast<const CameraBuffer::Private *>(this);
> +	return const_cast<uint8_t *>(self->planeAddr(plane));
> +}

These functions are only called in plane(), I'd inline them there (and
following the comments on previous patches, you can get rid of the
non-const version).

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +PUBLIC_CAMERA_BUFFER
> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> index 97f83f2a7380..eeb5cc2e6a31 100644
> --- a/src/android/mm/meson.build
> +++ b/src/android/mm/meson.build
> @@ -3,4 +3,7 @@
>  platform = get_option('android_platform')
>  if platform == 'generic'
>      android_hal_sources += files(['generic_camera_buffer.cpp'])
> +elif platform == 'cros'
> +    android_hal_sources += files(['cros_camera_buffer.cpp'])
> +    android_deps += [dependency('libcros_camera')]
>  endif

-- 
Regards,

Laurent Pinchart