[libcamera-devel,v3,6/6] android: hal: Add Camera3 HAL

1761 diff mbox series

Add libcamera Android Camera HALv3 implementation.

The initial camera HAL implementation supports the LIMITED hardware
level and uses statically defined metadata and camera characteristics.

Add a build option named 'android' and adjust the build system to
selectively compile the Android camera HAL and link it against the
required Android libraries.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 meson_options.txt                  |   5 +
 src/android/camera3_hal.cpp        | 111 ++++
 src/android/camera_device.cpp      | 817 +++++++++++++++++++++++++++++
 src/android/camera_device.h        |  63 +++
 src/android/camera_hal_manager.cpp | 142 +++++
 src/android/camera_hal_manager.h   |  47 ++
 src/android/camera_proxy.cpp       | 199 +++++++
 src/android/camera_proxy.h         |  45 ++
 src/android/meson.build            |   8 +
 src/android/thread_rpc.cpp         |  42 ++
 src/android/thread_rpc.h           |  54 ++
 src/libcamera/meson.build          |   9 +
 src/meson.build                    |   5 +-
 13 files changed, 1546 insertions(+), 1 deletion(-)
 create mode 100644 src/android/camera3_hal.cpp
 create mode 100644 src/android/camera_device.cpp
 create mode 100644 src/android/camera_device.h
 create mode 100644 src/android/camera_hal_manager.cpp
 create mode 100644 src/android/camera_hal_manager.h
 create mode 100644 src/android/camera_proxy.cpp
 create mode 100644 src/android/camera_proxy.h
 create mode 100644 src/android/thread_rpc.cpp
 create mode 100644 src/android/thread_rpc.h

Hi Jacopo,

Thank you for the patch.

On Fri, Aug 09, 2019 at 12:04:05PM +0200, Jacopo Mondi wrote:
> Add libcamera Android Camera HALv3 implementation.
> 
> The initial camera HAL implementation supports the LIMITED hardware
> level and uses statically defined metadata and camera characteristics.
> 
> Add a build option named 'android' and adjust the build system to
> selectively compile the Android camera HAL and link it against the
> required Android libraries.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  meson_options.txt                  |   5 +
>  src/android/camera3_hal.cpp        | 111 ++++
>  src/android/camera_device.cpp      | 817 +++++++++++++++++++++++++++++
>  src/android/camera_device.h        |  63 +++
>  src/android/camera_hal_manager.cpp | 142 +++++
>  src/android/camera_hal_manager.h   |  47 ++
>  src/android/camera_proxy.cpp       | 199 +++++++
>  src/android/camera_proxy.h         |  45 ++
>  src/android/meson.build            |   8 +
>  src/android/thread_rpc.cpp         |  42 ++
>  src/android/thread_rpc.h           |  54 ++
>  src/libcamera/meson.build          |   9 +
>  src/meson.build                    |   5 +-
>  13 files changed, 1546 insertions(+), 1 deletion(-)
>  create mode 100644 src/android/camera3_hal.cpp
>  create mode 100644 src/android/camera_device.cpp
>  create mode 100644 src/android/camera_device.h
>  create mode 100644 src/android/camera_hal_manager.cpp
>  create mode 100644 src/android/camera_hal_manager.h
>  create mode 100644 src/android/camera_proxy.cpp
>  create mode 100644 src/android/camera_proxy.h
>  create mode 100644 src/android/thread_rpc.cpp
>  create mode 100644 src/android/thread_rpc.h
> 
> diff --git a/meson_options.txt b/meson_options.txt
> index 97efc85b4412..2d78b8d91f9c 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -1,3 +1,8 @@
> +option('android',
> +        type : 'boolean',
> +        value : false,
> +        description : 'Compile libcamera with Android Camera3 HAL interface')
> +
>  option('documentation',
>          type : 'boolean',
>          description : 'Generate the project documentation')
> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> new file mode 100644
> index 000000000000..8d2629ca356c
> --- /dev/null
> +++ b/src/android/camera3_hal.cpp
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera3_hal.cpp - Android Camera HALv3 module
> + */
> +
> +#include <hardware/camera_common.h>
> +
> +#include "camera_hal_manager.h"
> +#include "camera_proxy.h"
> +#include "log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(HAL)
> +
> +static CameraHalManager cameraManager;
> +
> +/*------------------------------------------------------------------------------
> + * Android Camera HAL callbacks
> + */
> +
> +static int hal_get_number_of_cameras(void)
> +{
> +	return cameraManager.numCameras();
> +}
> +
> +static int hal_get_camera_info(int id, struct camera_info *info)
> +{
> +	return cameraManager.getCameraInfo(id, info);
> +}
> +
> +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
> +{
> +	return 0;
> +}
> +
> +static int hal_open_legacy(const struct hw_module_t *module, const char *id,
> +			   uint32_t halVersion, struct hw_device_t **device)
> +{
> +	return -ENOSYS;
> +}
> +
> +static int hal_set_torch_mode(const char *camera_id, bool enabled)
> +{
> +	return -ENOSYS;
> +}
> +
> +/*
> + * First entry point of the camera HAL module.
> + *
> + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)
> + */
> +static int hal_init()
> +{
> +	LOG(HAL, Info) << "Initialising Android camera HAL";
> +
> +	cameraManager.init();
> +
> +	return 0;
> +}
> +
> +/*------------------------------------------------------------------------------
> + * Android Camera Device
> + */
> +
> +static int hal_dev_open(const hw_module_t *module, const char *name,
> +			hw_device_t **device)
> +{
> +	LOG(HAL, Debug) << "Open camera " << name;
> +
> +	int id = atoi(name);
> +	CameraProxy *proxy = cameraManager.open(id, module);
> +	if (!proxy) {
> +		LOG(HAL, Error)
> +			<< "Failed to open camera module '" << id << "'";
> +		return -ENODEV;
> +	}
> +
> +	*device = &proxy->camera3Device()->common;
> +
> +	return 0;
> +}
> +
> +static struct hw_module_methods_t hal_module_methods = {
> +	.open = hal_dev_open,
> +};
> +
> +camera_module_t HAL_MODULE_INFO_SYM = {
> +	.common = {
> +		.tag = HARDWARE_MODULE_TAG,
> +		.module_api_version = CAMERA_MODULE_API_VERSION_2_4,
> +		.hal_api_version = HARDWARE_HAL_API_VERSION,
> +		.id = CAMERA_HARDWARE_MODULE_ID,
> +		.name = "libcamera camera HALv3 module",
> +		.author = "libcamera",
> +		.methods = &hal_module_methods,
> +		.dso = nullptr,
> +		.reserved = {},
> +	},
> +
> +	.get_number_of_cameras = hal_get_number_of_cameras,
> +	.get_camera_info = hal_get_camera_info,
> +	.set_callbacks = hal_set_callbacks,
> +	.get_vendor_tag_ops = nullptr,
> +	.open_legacy = hal_open_legacy,
> +	.set_torch_mode = hal_set_torch_mode,
> +	.init = hal_init,
> +	.reserved = {},
> +};
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> new file mode 100644
> index 000000000000..18bf459a37e1
> --- /dev/null
> +++ b/src/android/camera_device.cpp
> @@ -0,0 +1,817 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_device.cpp - libcamera Android Camera Device
> + */
> +
> +#include "camera_device.h"
> +
> +#include <memory>
> +
> +#include <hardware/camera3.h>
> +#include <system/camera_metadata.h>
> +
> +#include <libcamera/camera.h>
> +
> +#include "log.h"
> +#include "message.h"
> +
> +#include "thread_rpc.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL);
> +
> +/*
> + * \class CameraDevice
> + *
> + * The CameraDevice class wraps a libcamera::Camera instance, and implements
> + * the camera_device_t interface by handling RPC requests received from its
> + * associated CameraProxy.
> + *
> + * It translate parameters and operations from Camera HALv3 API to the libcamera
> + * ones to provide static informations on a Camera, create request templates

s/informations on/information for/

> + * for it and process capture requests by then delivering capture results back

s/and process/, process/
s/by then delevering/, and then deliver/

> + * to the framework using the designated callbacks.
> + */
> +
> +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> +	: running_(false), camera_(camera), staticMetadata_(nullptr),
> +	  requestTemplate_(nullptr)
> +{
> +	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> +}
> +
> +CameraDevice::~CameraDevice()
> +{
> +	if (staticMetadata_)
> +		free_camera_metadata(staticMetadata_);
> +	staticMetadata_ = nullptr;
> +
> +	if (requestTemplate_)
> +		free_camera_metadata(requestTemplate_);
> +	requestTemplate_ = nullptr;
> +}
> +
> +/*
> + * Handle RPC request received from the associated proxy.
> + */
> +void CameraDevice::message(Message *message)
> +{
> +	if (message->type() != ThreadRpcMessage::type())
> +		return Object::message(message);
> +
> +	ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message);
> +	ThreadRpc *rpc = rpcMessage->rpc;
> +
> +	switch (rpc->tag) {
> +	case ThreadRpc::ProcessCaptureRequest:
> +		processCaptureRequest(rpc->request);
> +		break;
> +	case ThreadRpc::Close:
> +		close();
> +		break;
> +	default:
> +		LOG(HAL, Error) << "Unknown RPC operation: " << rpc->tag;
> +	}
> +
> +	rpc->notifyReception();
> +}
> +
> +int CameraDevice::open()
> +{
> +	int ret = camera_->acquire();
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to acquire the camera";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void CameraDevice::close()
> +{
> +	camera_->stop();
> +
> +	camera_->freeBuffers();
> +	camera_->release();
> +
> +	running_ = false;
> +}
> +
> +void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> +{
> +	callbacks_ = callbacks;
> +}
> +
> +/*
> + * Return static informations on the camera.

s/informations on/information for/

> + */
> +camera_metadata_t *CameraDevice::getStaticMetadata()
> +{
> +	int ret;
> +
> +	if (staticMetadata_)
> +		return staticMetadata_;
> +
> +	/*
> +	 * The here reported metadata are enough to implement a basic capture
> +	 * example application, but a real camera implementation will require
> +	 * more.
> +	 */
> +
> +	/* \todo Use correct sizes */
> +	#define STATIC_ENTRY_CAP 256
> +	#define STATIC_DATA_CAP 6688
> +	camera_metadata_t *staticMetadata =
> +		allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> +
> +	/* Sensor static metadata. */
> +	int32_t pixelArraySize[] = {
> +		2592, 1944,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> +				&pixelArraySize, 2);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t sensorSizes[] = {
> +		0, 0, 2560, 1920,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> +				&sensorSizes, 4);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t sensitivityRange[] = {
> +		32, 2400,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> +				&sensitivityRange, 2);
> +	METADATA_ASSERT(ret);
> +
> +	uint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> +				&filterArr, 1);
> +	METADATA_ASSERT(ret);
> +
> +	int64_t exposureTimeRange[] = {
> +		100000, 200000000,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> +				&exposureTimeRange, 2);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t orientation = 0;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_SENSOR_ORIENTATION,
> +				&orientation, 1);
> +	METADATA_ASSERT(ret);
> +
> +	/* Flash static metadata. */
> +	char flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_FLASH_INFO_AVAILABLE,
> +			&flashAvailable, 1);
> +	METADATA_ASSERT(ret);
> +
> +	/* Lens static metadata. */
> +	float fn = 2.53 / 100;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +				ANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);
> +	METADATA_ASSERT(ret);
> +
> +	/* Control metadata. */
> +	char controlMetadata = ANDROID_CONTROL_MODE_AUTO;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_CONTROL_AVAILABLE_MODES,
> +			&controlMetadata, 1);
> +	METADATA_ASSERT(ret);
> +
> +	char availableAntiBandingModes[] = {
> +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,
> +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,
> +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,
> +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> +			availableAntiBandingModes, 4);
> +	METADATA_ASSERT(ret);
> +
> +	char aeAvailableModes[] = {
> +		ANDROID_CONTROL_AE_MODE_ON,
> +		ANDROID_CONTROL_AE_MODE_OFF,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_CONTROL_AE_AVAILABLE_MODES,
> +			aeAvailableModes, 2);
> +	METADATA_ASSERT(ret);
> +
> +	controlMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> +			&controlMetadata, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> +			&awbLockAvailable, 1);
> +
> +	/* Scaler static metadata. */
> +	std::vector<uint32_t> availableStreamFormats = {
> +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB,
> +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,
> +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_SCALER_AVAILABLE_FORMATS,
> +			availableStreamFormats.data(),
> +			availableStreamFormats.size());
> +	METADATA_ASSERT(ret);
> +
> +	std::vector<uint32_t> availableStreamConfigurations = {
> +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,
> +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,
> +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,
> +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> +			availableStreamConfigurations.data(),
> +			availableStreamConfigurations.size());
> +	METADATA_ASSERT(ret);
> +
> +	std::vector<int64_t> availableStallDurations = {
> +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> +			availableStallDurations.data(),
> +			availableStallDurations.size());
> +	METADATA_ASSERT(ret);
> +
> +	std::vector<int64_t> minFrameDurations = {
> +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,
> +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,
> +	};
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> +			minFrameDurations.data(), minFrameDurations.size());
> +	METADATA_ASSERT(ret);
> +
> +	/* Info static metadata. */
> +	uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	ret = add_camera_metadata_entry(staticMetadata,
> +			ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> +			&supportedHWLevel, 1);
> +
> +	return staticMetadata;
> +}
> +
> +/*
> + * Produce a metadata pack to be used as template for a capture request.
> + */
> +const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> +{
> +	int ret;
> +
> +	/*
> +	 * \todo Inspect type and pick the right metadata pack.
> +	 * As of now just use a single one for all templates.
> +	 */
> +	uint8_t captureIntent;
> +	switch (type) {
> +	case CAMERA3_TEMPLATE_PREVIEW:
> +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> +		break;
> +	case CAMERA3_TEMPLATE_STILL_CAPTURE:
> +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> +		break;
> +	case CAMERA3_TEMPLATE_VIDEO_RECORD:
> +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> +		break;
> +	case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> +		break;
> +	case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> +		break;
> +	case CAMERA3_TEMPLATE_MANUAL:
> +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> +		break;
> +	default:
> +		LOG(HAL, Error) << "Invalid template request type: " << type;
> +		return nullptr;
> +	}
> +
> +	if (requestTemplate_)
> +		return requestTemplate_;
> +
> +	/* \todo Use correct sizes */
> +	#define REQUEST_TEMPLATE_ENTRIES	  30
> +	#define REQUEST_TEMPLATE_DATA		2048
> +	requestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,
> +						    REQUEST_TEMPLATE_DATA);
> +	if (!requestTemplate_) {
> +		LOG(HAL, Error) << "Failed to allocate template metadata";
> +		return nullptr;
> +	}
> +
> +	/* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */
> +	int32_t maxOutStream[] = { 0, 2, 0 };
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
> +			maxOutStream, 3);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t maxPipelineDepth = 5;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> +			&maxPipelineDepth, 1);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t inputStreams = 0;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,
> +			&inputStreams, 1);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t partialResultCount = 1;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> +			&partialResultCount, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t availableCapabilities[] = {
> +		ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> +	};
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> +			availableCapabilities, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AE_MODE,
> +			&aeMode, 1);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t aeExposureCompensation = 0;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> +			&aeExposureCompensation, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> +			&aePrecaptureTrigger, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AE_LOCK,
> +			&aeLock, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AF_TRIGGER,
> +			&afTrigger, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AWB_MODE,
> +			&awbMode, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AWB_LOCK,
> +			&awbLock, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> +			&awbLockAvailable, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t flashMode = ANDROID_FLASH_MODE_OFF;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_FLASH_MODE,
> +			&flashMode, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_STATISTICS_FACE_DETECT_MODE,
> +			&faceDetectMode, 1);
> +	METADATA_ASSERT(ret);
> +
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_CONTROL_CAPTURE_INTENT,
> +			&captureIntent, 1);
> +	METADATA_ASSERT(ret);
> +
> +	/*
> +	 * This is quite hard to list at the moment wihtout knowing what
> +	 * we could control.
> +	 *
> +	 * For now, just list in the available Request keys and in the available
> +	 * result keys the control and reporting of the AE algorithm.
> +	 */
> +	std::vector<int32_t> availableRequestKeys = {
> +		ANDROID_CONTROL_AE_MODE,
> +		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> +		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> +		ANDROID_CONTROL_AE_LOCK,
> +		ANDROID_CONTROL_AF_TRIGGER,
> +		ANDROID_CONTROL_AWB_MODE,
> +		ANDROID_CONTROL_AWB_LOCK,
> +		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> +		ANDROID_CONTROL_CAPTURE_INTENT,
> +		ANDROID_FLASH_MODE,
> +		ANDROID_STATISTICS_FACE_DETECT_MODE,
> +	};
> +
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
> +			availableRequestKeys.data(),
> +			availableRequestKeys.size());
> +	METADATA_ASSERT(ret);
> +
> +	std::vector<int32_t> availableResultKeys = {
> +		ANDROID_CONTROL_AE_MODE,
> +		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> +		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> +		ANDROID_CONTROL_AE_LOCK,
> +		ANDROID_CONTROL_AF_TRIGGER,
> +		ANDROID_CONTROL_AWB_MODE,
> +		ANDROID_CONTROL_AWB_LOCK,
> +		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> +		ANDROID_CONTROL_CAPTURE_INTENT,
> +		ANDROID_FLASH_MODE,
> +		ANDROID_STATISTICS_FACE_DETECT_MODE,
> +	};
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,
> +			availableResultKeys.data(),
> +			availableResultKeys.size());
> +	METADATA_ASSERT(ret);
> +
> +	/*
> +	 * \todo The available characteristics are be the tags reported
> +	 * as part of the static metadata reported at hal_get_camera_info()
> +	 * time. As of now, report an empty list.
> +	 */
> +	std::vector<int32_t> availableCharacteristicsKeys = {};
> +	ret = add_camera_metadata_entry(requestTemplate_,
> +			ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> +			availableCharacteristicsKeys.data(),
> +			availableCharacteristicsKeys.size());
> +	METADATA_ASSERT(ret);
> +
> +	return requestTemplate_;
> +}
> +
> +/*
> + * Inspect the stream_list to produce a list of StreamConfiguration to
> + * be use to configure the Camera.
> + */
> +int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> +{
> +

Extra blank line.

> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> +		camera3_stream_t *stream = stream_list->streams[i];
> +
> +		LOG(HAL, Info) << "Stream #" << i
> +			       << ", direction: " << stream->stream_type
> +			       << ", width: " << stream->width
> +			       << ", height: " << stream->height
> +			       << ", format: " << std::hex << stream->format;
> +	}
> +
> +	/* Hardcode viewfinder role, collecting sizes from the stream config. */
> +	if (stream_list->num_streams != 1) {
> +		LOG(HAL, Error) << "Only one stream supported";
> +		return -EINVAL;
> +	}
> +
> +	StreamRoles roles = { StreamRole::Viewfinder };
> +	config_ = camera_->generateConfiguration(roles);
> +	if (!config_ || config_->empty()) {
> +		LOG(HAL, Error) << "Failed to generate camera configuration";
> +		return -EINVAL;
> +	}
> +
> +	/* Only one stream is supported. */
> +	camera3_stream_t *camera3Stream = stream_list->streams[0];
> +	StreamConfiguration *streamConfiguration = &config_->at(0);
> +	streamConfiguration->size.width = camera3Stream->width;
> +	streamConfiguration->size.height = camera3Stream->height;
> +	streamConfiguration->memoryType = ExternalMemory;
> +
> +	/*
> +	 * \todo We'll need to translate from Android defined pixel format codes
> +	 * to the libcamera image format codes. As of now, do not change the

s/As of/For/

> +	 * format returned from Camera::generateConfiguration().
> +	 */
> +
> +	switch (config_->validate()) {
> +	case CameraConfiguration::Valid:
> +		break;
> +	case CameraConfiguration::Adjusted:
> +		LOG(HAL, Info) << "Camera configuration adjusted";
> +		config_.reset();
> +		return -EINVAL;
> +	case CameraConfiguration::Invalid:
> +		LOG(HAL, Info) << "Camera configuration invalid";
> +		config_.reset();
> +		return -EINVAL;
> +	}
> +
> +	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> +
> +	/*
> +	 * Once the CameraConfiguration has been adjusted/validated
> +	 * it can be applied to the camera.
> +	 */
> +	int ret = camera_->configure(config_.get());
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to configure camera '"
> +				<< camera_->name() << "'";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> +{
> +	StreamConfiguration *streamConfiguration = &config_->at(0);
> +	Stream *stream = streamConfiguration->stream();
> +
> +	if (camera3Request->num_output_buffers != 1) {
> +		LOG(HAL, Error) << "Invalid number of output buffers: "
> +				<< camera3Request->num_output_buffers;
> +		return -EINVAL;
> +	}
> +
> +	/* Start the camera if that's the first request we handle. */
> +	if (!running_) {
> +		int ret = camera_->allocateBuffers();
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to allocate buffers";
> +			return ret;
> +		}
> +
> +		ret = camera_->start();
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to start camera";
> +			camera_->freeBuffers();
> +			return ret;
> +		}
> +
> +		running_ = true;
> +	}
> +
> +	/*
> +	 * Queue a request for the Camera with the provided dmabuf file
> +	 * descriptors.
> +	 */
> +	const camera3_stream_buffer_t *camera3Buffers =
> +					camera3Request->output_buffers;
> +
> +	/*
> +	 * Save the request descriptors for use at completion time.
> +	 * The descriptor and the associated memory reserved here are freed
> +	 * at request complete time.
> +	 */
> +	Camera3RequestDescriptor *descriptor = new Camera3RequestDescriptor();
> +	descriptor->frameNumber = camera3Request->frame_number;
> +	descriptor->numBuffers = camera3Request->num_output_buffers;
> +	descriptor->buffers = new camera3_stream_buffer_t[descriptor->numBuffers];
> +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> +		/*
> +		 * Keep track of which stream the request belong and of the
> +		 * native buffer handles.
> +		 *
> +		 * \todo Currently we only support one capture buffer. Copy
> +		 * all of them to be ready once we'll support more.
> +		 */
> +		descriptor->buffers[i].stream = camera3Buffers[i].stream;
> +		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> +	}
> +
> +	/*
> +	 * Create a libcamera buffer using the dmabuf descriptors of the first
> +	 * and (currently) only supported request buffer.
> +	 */
> +	const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer;
> +	std::array<int, 3> fds = {
> +		camera3Handle->data[0],
> +		camera3Handle->data[1],
> +		camera3Handle->data[2],
> +	};
> +
> +	std::unique_ptr<Buffer> buffer = stream->createBuffer(fds);
> +	if (!buffer) {
> +		LOG(HAL, Error) << "Failed to create buffer";
> +		return -EINVAL;

You're leaking descriptor and descriptor->buffers.

I would implement a constructor for Camera3RequestDescriptor that takes
the number of buffers and allocate the buffers member, and a destructor
that deletes the buffers member. Then you will only need to care about
deleting descriptor here.

> +	}
> +
> +	Request *request =
> +		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> +	request->addBuffer(std::move(buffer));
> +
> +	int ret = camera_->queueRequest(request);
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to queue request";
> +		return ret;

You're leaking request, descriptor and descriptor->buffers.

I wonder if Camera::createRequest() shouldn't return an std::unique_ptr
(this doesn't need to be addressed in this patch series).

> +	}
> +
> +	return 0;
> +}
> +
> +void CameraDevice::requestComplete(Request *request,
> +				   const std::map<Stream *, Buffer *> &buffers)
> +{
> +	Buffer *libcameraBuffer = buffers.begin()->second;
> +	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> +	camera_metadata_t *resultMetadata = nullptr;
> +
> +	if (request->status() != Request::RequestComplete) {
> +		LOG(HAL, Error) << "Request not succesfully completed: "
> +				<< request->status();
> +		status = CAMERA3_BUFFER_STATUS_ERROR;
> +	}
> +
> +	/* Prepare to call back the Android camera stack. */
> +	Camera3RequestDescriptor *descriptor =
> +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> +
> +	camera3_capture_result_t captureResult = {};
> +	captureResult.frame_number = descriptor->frameNumber;
> +	captureResult.num_output_buffers = descriptor->numBuffers;
> +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> +		/*
> +		 * The buffers in the descriptor have been set up at queue
> +		 * request time.

I think you can drop this sentence.

> +		 *
> +		 * \todo Currently we only support one capture buffer. Copy
> +		 * all of them to be ready once we'll support more.
> +		 */
> +		descriptor->buffers[i].acquire_fence = -1;
> +		descriptor->buffers[i].release_fence = -1;
> +		descriptor->buffers[i].status = status;
> +	}
> +	captureResult.output_buffers =
> +		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
> +
> +	if (status == CAMERA3_BUFFER_STATUS_ERROR) {
> +		/* \todo Improve error handling. */
> +		notifyError(descriptor->frameNumber,
> +			    descriptor->buffers[0].stream);
> +	} else {
> +		notifyShutter(descriptor->frameNumber,
> +			      libcameraBuffer->timestamp());
> +
> +		captureResult.partial_result = 1;
> +		resultMetadata = getResultMetadata(descriptor->frameNumber,
> +						   libcameraBuffer->timestamp());
> +		captureResult.result = resultMetadata;
> +	}
> +
> +	callbacks_->process_capture_result(callbacks_, &captureResult);
> +
> +	delete[] descriptor->buffers;
> +	delete descriptor;

This will also be simplified with a destructor for the
Camera3RequestDescriptor class.

> +	if (resultMetadata)
> +		free_camera_metadata(resultMetadata);
> +
> +	return;
> +}
> +
> +void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> +{
> +	camera3_notify_msg_t notify = {};
> +
> +	notify.type = CAMERA3_MSG_SHUTTER;
> +	notify.message.shutter.frame_number = frameNumber;
> +	notify.message.shutter.timestamp = timestamp;
> +
> +	callbacks_->notify(callbacks_, &notify);
> +}
> +
> +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> +{
> +	camera3_notify_msg_t notify = {};
> +
> +	notify.type = CAMERA3_MSG_ERROR;
> +	notify.message.error.error_stream = stream;
> +	notify.message.error.frame_number = frameNumber;
> +	notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> +
> +	callbacks_->notify(callbacks_, &notify);
> +}
> +
> +/*
> + * Produce a set of fixed result metadata.
> + */
> +camera_metadata_t *CameraDevice::getResultMetadata(int frame_number,
> +						   int64_t timestamp)
> +{
> +	int ret;
> +
> +	/* \todo Use correct sizes */
> +	#define RESULT_ENTRY_CAP 256
> +	#define RESULT_DATA_CAP 6688
> +	camera_metadata_t *resultMetadata =
> +		allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> +
> +	const uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;
> +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,
> +					&ae_state, 1);
> +	METADATA_ASSERT(ret);
> +
> +	const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
> +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,
> +					&ae_lock, 1);
> +	METADATA_ASSERT(ret);
> +
> +	uint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;
> +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,
> +					&af_state, 1);
> +	METADATA_ASSERT(ret);
> +
> +	const uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_CONTROL_AWB_STATE,
> +					&awb_state, 1);
> +	METADATA_ASSERT(ret);
> +
> +	const uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_CONTROL_AWB_LOCK,
> +					&awb_lock, 1);
> +	METADATA_ASSERT(ret);
> +
> +	const uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_LENS_STATE,
> +					&lens_state, 1);
> +	METADATA_ASSERT(ret);
> +
> +	int32_t sensorSizes[] = {
> +		0, 0, 2560, 1920,
> +	};
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_SCALER_CROP_REGION,
> +					sensorSizes, 4);
> +	METADATA_ASSERT(ret);
> +
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_SENSOR_TIMESTAMP,
> +					&timestamp, 1);
> +	METADATA_ASSERT(ret);
> +
> +	/* 33.3 msec */
> +	const int64_t rolling_shutter_skew = 33300000;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
> +					&rolling_shutter_skew, 1);
> +	METADATA_ASSERT(ret);
> +
> +	/* 16.6 msec */
> +	const int64_t exposure_time = 16600000;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_SENSOR_EXPOSURE_TIME,
> +					&exposure_time, 1);
> +	METADATA_ASSERT(ret);
> +
> +	const uint8_t lens_shading_map_mode =
> +				ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,
> +					&lens_shading_map_mode, 1);
> +	METADATA_ASSERT(ret);
> +
> +	const uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;
> +	ret = add_camera_metadata_entry(resultMetadata,
> +					ANDROID_STATISTICS_SCENE_FLICKER,
> +					&scene_flicker, 1);
> +	METADATA_ASSERT(ret);
> +
> +	return resultMetadata;
> +}
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> new file mode 100644
> index 000000000000..402c3dca69da
> --- /dev/null
> +++ b/src/android/camera_device.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_device.h - libcamera Android Camera Device
> + */
> +#ifndef __ANDROID_CAMERA_DEVICE_H__
> +#define __ANDROID_CAMERA_DEVICE_H__
> +
> +#include <memory>
> +
> +#include <hardware/camera3.h>
> +
> +#include <libcamera/libcamera.h>

Just the required headers please :-)

> +
> +#include "message.h"
> +
> +#define METADATA_ASSERT(_r)		\
> +	do {				\
> +		if (!(_r)) break;	\
> +		LOG(HAL, Error) << "Error: " << __func__ << ":" << __LINE__; \
> +		return nullptr;		\
> +	} while(0);
> +
> +class CameraDevice : public libcamera::Object
> +{
> +public:
> +	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);

You can make the second argument a reference to avoid an unnecessary
copy.

> +	~CameraDevice();
> +
> +	void message(libcamera::Message *message);
> +
> +	int open();
> +	void close();
> +	void setCallbacks(const camera3_callback_ops_t *callbacks);
> +	camera_metadata_t *getStaticMetadata();
> +	const camera_metadata_t *constructDefaultRequestSettings(int type);
> +	int configureStreams(camera3_stream_configuration_t *stream_list);
> +	int processCaptureRequest(camera3_capture_request_t *request);
> +	void requestComplete(libcamera::Request *request,
> +			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
> +
> +private:
> +	struct Camera3RequestDescriptor {
> +		uint32_t frameNumber;
> +		uint32_t numBuffers;
> +		camera3_stream_buffer_t *buffers;
> +	};
> +
> +	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> +	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> +	camera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);
> +
> +	bool running_;
> +	std::shared_ptr<libcamera::Camera> camera_;
> +	std::unique_ptr<libcamera::CameraConfiguration> config_;
> +
> +	camera_metadata_t *staticMetadata_;
> +	camera_metadata_t *requestTemplate_;
> +	const camera3_callback_ops_t *callbacks_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_DEVICE_H__ */
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> new file mode 100644
> index 000000000000..0b4d50368ff8
> --- /dev/null
> +++ b/src/android/camera_hal_manager.cpp
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_hal_manager.cpp - libcamera Android Camera Manager
> + */
> +
> +#include "camera_hal_manager.h"
> +
> +#include <hardware/hardware.h>
> +#include <system/camera_metadata.h>
> +
> +#include <libcamera/camera.h>
> +
> +#include "log.h"
> +
> +#include "camera_device.h"
> +#include "camera_proxy.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL);
> +
> +/*
> + * \class CameraHalManager
> + *
> + * The HAL camera manager is initializated at camera_module_t 'hal_init()' time
> + * and spawns its own thread where libcamera related events are dispatched to.
> + * It wraps the libcamera CameraManager operations and provides helpers for

s/for/for the/

> + * camera_module_t operations, to retrieve the number of cameras in the system

s/system/system, to retrieve /

> + * their static informations and to open and close camera devices.

s/informations/information/

> + */
> +
> +int CameraHalManager::init()
> +{
> +	/*
> +	 * Start the camera HAL manager thread and wait until its
> +	 * initialisation to complete to be fully operational before

s/to complete/completes/

> +	 * receiving calls from the camera stack.
> +	 */
> +	start();
> +
> +	MutexLocker locker(mutex_);
> +	cv_.wait(locker);
> +
> +	return 0;
> +}
> +
> +void CameraHalManager::run()
> +{
> +	/*
> +	 * All the libcamera components must be initialised here, in
> +	 * order to bind them to the camera HAL manager thread that
> +	 * executes the event dispatcher.
> +	 */
> +	cameraManager_ = libcamera::CameraManager::instance();
> +
> +	int ret = cameraManager_->start();
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to start camera manager: "
> +				<< strerror(-ret);
> +		return;
> +	}
> +
> +	/*
> +	 * For each Camera registered in the system, a CameraProxy
> +	 * gets created here wich wraps a camera device.

s/wich wraps/to wrap/

> +	 *
> +	 * \todo Support camera hotplug.
> +	 */
> +	unsigned int index = 0;
> +	for (auto &camera : cameraManager_->cameras()) {
> +		CameraProxy *proxy = new CameraProxy(index, camera);
> +		proxies_.emplace_back(proxy);
> +
> +		++index;
> +	}
> +
> +	/*
> +	 * libcamera has been initialized. Unlock the init() caller
> +	 * as we're now ready to handle calls from the framework.
> +	 */
> +	cv_.notify_one();
> +
> +	/* Now start processing events and messages. */
> +	exec();
> +}
> +
> +CameraProxy *CameraHalManager::open(unsigned int id,
> +				    const hw_module_t *hardwareModule)
> +{
> +	if (id < 0 || id >= numCameras()) {
> +		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> +		return nullptr;
> +	}
> +
> +	CameraProxy *proxy = proxies_[id].get();
> +	if (proxy->open(hardwareModule))
> +		return nullptr;
> +
> +	LOG(HAL, Info) << "Open camera '" << id << "'";
> +
> +	return proxy;
> +}
> +
> +int CameraHalManager::close(CameraProxy *proxy)
> +{
> +	proxy->close();
> +	LOG(HAL, Info) << "Close camera '" << proxy->id() << "'";
> +
> +	return 0;
> +}
> +
> +unsigned int CameraHalManager::numCameras() const
> +{
> +	return cameraManager_->cameras().size();
> +}
> +
> +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)
> +{
> +	if (!info)
> +		return -EINVAL;
> +
> +	int cameras = numCameras();
> +	if (id >= cameras || id < 0) {

s/cameras/numCameras()/

and drop the local variable.

> +		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> +		return -EINVAL;
> +	}
> +
> +	CameraProxy *proxy = proxies_[id].get();
> +
> +	/* \todo Get these info dynamically inspecting the camera module. */
> +	info->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;
> +	info->orientation = 0;
> +	info->device_version = 0;
> +	info->resource_cost = 0;
> +	info->static_camera_characteristics = proxy->getStaticMetadata();
> +	info->conflicting_devices = nullptr;
> +	info->conflicting_devices_length = 0;
> +
> +	return 0;
> +}
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> new file mode 100644
> index 000000000000..8004aaf660f5
> --- /dev/null
> +++ b/src/android/camera_hal_manager.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_hal_manager.h - libcamera Android Camera Manager
> + */
> +#ifndef __ANDROID_CAMERA_MANAGER_H__
> +#define __ANDROID_CAMERA_MANAGER_H__
> +
> +#include <condition_variable>
> +#include <mutex>
> +#include <vector>
> +
> +#include <hardware/hardware.h>
> +#include <system/camera_metadata.h>
> +
> +#include <libcamera/camera_manager.h>
> +
> +#include "thread.h"
> +
> +class CameraDevice;
> +class CameraProxy;
> +
> +class CameraHalManager : public libcamera::Thread
> +{
> +public:
> +	int init();
> +
> +	CameraProxy *open(unsigned int id, const hw_module_t *module);
> +	int close(CameraProxy *proxy);
> +
> +	unsigned int numCameras() const;
> +	int getCameraInfo(int id, struct camera_info *info);
> +
> +private:
> +	void run() override;
> +	camera_metadata_t *getStaticMetadata(unsigned int id);
> +
> +	libcamera::CameraManager *cameraManager_;
> +
> +	std::mutex mutex_;
> +	std::condition_variable cv_;
> +
> +	std::vector<std::unique_ptr<CameraProxy>> proxies_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_MANAGER_H__ */
> diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp
> new file mode 100644
> index 000000000000..23c33c8f6c9c
> --- /dev/null
> +++ b/src/android/camera_proxy.cpp
> @@ -0,0 +1,199 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_proxy.cpp - Proxy to camera devices
> + */
> +
> +#include "camera_proxy.h"
> +
> +#include <memory>
> +
> +#include <hardware/camera3.h>
> +#include <system/camera_metadata.h>
> +
> +#include <libcamera/camera.h>
> +
> +#include "log.h"
> +#include "message.h"
> +#include "utils.h"
> +
> +#include "camera_device.h"
> +#include "thread_rpc.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL);
> +
> +/*
> + * \class CameraProxy
> + *
> + * The CameraProxy wraps a CameraDevice and implements the camera3_device_t
> + * API, bridging calls received from the camera framework to the CameraDevice.
> + *
> + * Bridging operation calls between the framework and the CameraDevice is
> + * required as the two runs in two different threads and certain operations,

s/runs/run/

> + * such as queueing a new capture request to the camera, require to interrupt
> + * and restart the even dispatches associated with the thread where the

s/even dispatches/event dispatcher/

It's not about interrupting the event dispatcher, but about being called
in the same thread than the one that handles events.

"such as queueing a new capture request to the camera, shall be called
in the thread that dispatches events."

> + * CameraDevice is running. Other operations do not require any bridging and
> + * resolve to direct function calls on the CameraDevice instance instead.
> + */
> +
> +static int hal_dev_initialize(const struct camera3_device *dev,
> +			      const camera3_callback_ops_t *callback_ops)
> +{
> +	if (!dev)
> +		return -EINVAL;
> +
> +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> +	proxy->initialize(callback_ops);
> +
> +	return 0;
> +}
> +
> +static int hal_dev_configure_streams(const struct camera3_device *dev,
> +				     camera3_stream_configuration_t *stream_list)
> +{
> +	if (!dev)
> +		return -EINVAL;
> +
> +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> +	return proxy->configureStreams(stream_list);
> +}
> +
> +static const camera_metadata_t *
> +hal_dev_construct_default_request_settings(const struct camera3_device *dev,
> +					   int type)
> +{
> +	if (!dev)
> +		return nullptr;
> +
> +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> +	return proxy->constructDefaultRequestSettings(type);
> +}
> +
> +static int hal_dev_process_capture_request(const struct camera3_device *dev,
> +					   camera3_capture_request_t *request)
> +{
> +	if (!dev)
> +		return -EINVAL;
> +
> +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> +	return proxy->processCaptureRequest(request);
> +}
> +
> +static void hal_dev_dump(const struct camera3_device *dev, int fd)
> +{
> +}
> +
> +static int hal_dev_flush(const struct camera3_device *dev)
> +{
> +	return 0;
> +}
> +
> +static int hal_dev_close(hw_device_t *hw_device)
> +{
> +	if (!hw_device)
> +		return -EINVAL;
> +
> +	camera3_device_t *dev = reinterpret_cast<camera3_device_t *>(hw_device);
> +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> +
> +	proxy->close();
> +
> +	return 0;
> +}
> +
> +static camera3_device_ops hal_dev_ops = {
> +	.initialize = hal_dev_initialize,
> +	.configure_streams = hal_dev_configure_streams,
> +	.register_stream_buffers = nullptr,
> +	.construct_default_request_settings = hal_dev_construct_default_request_settings,
> +	.process_capture_request = hal_dev_process_capture_request,
> +	.get_metadata_vendor_tag_ops = nullptr,
> +	.dump = hal_dev_dump,
> +	.flush = hal_dev_flush,
> +	.reserved = { nullptr },
> +};
> +
> +CameraProxy::CameraProxy(unsigned int id, std::shared_ptr<Camera> camera)
> +	: id_(id)
> +{
> +	cameraDevice_ = new CameraDevice(id, camera);
> +}
> +
> +CameraProxy::~CameraProxy()
> +{
> +	delete cameraDevice_;
> +}
> +
> +int CameraProxy::open(const hw_module_t *hardwareModule)
> +{
> +	int ret = cameraDevice_->open();
> +	if (ret)
> +		return ret;
> +
> +	/* Initialize the hw_device_t in the instance camera3_module_t. */
> +	camera3Device_.common.tag = HARDWARE_DEVICE_TAG;
> +	camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3;
> +	camera3Device_.common.module = (hw_module_t *)hardwareModule;
> +	camera3Device_.common.close = hal_dev_close;
> +
> +	/*
> +	 * The camera device operations. These actually implement
> +	 * the Android Camera HALv3 interface.
> +	 */
> +	camera3Device_.ops = &hal_dev_ops;
> +	camera3Device_.priv = this;
> +
> +	return 0;
> +}
> +
> +void CameraProxy::close()
> +{
> +	ThreadRpc rpcRequest;
> +	rpcRequest.tag = ThreadRpc::Close;
> +
> +	threadRpcCall(rpcRequest);
> +}
> +
> +void CameraProxy::initialize(const camera3_callback_ops_t *callbacks)
> +{
> +	cameraDevice_->setCallbacks(callbacks);
> +}
> +
> +const camera_metadata_t *CameraProxy::getStaticMetadata()
> +{
> +	return cameraDevice_->getStaticMetadata();
> +}
> +
> +const camera_metadata_t *CameraProxy::constructDefaultRequestSettings(int type)
> +{
> +	return cameraDevice_->constructDefaultRequestSettings(type);
> +}
> +
> +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)
> +{
> +	return cameraDevice_->configureStreams(stream_list);
> +}
> +
> +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
> +{
> +	ThreadRpc rpcRequest;
> +	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> +	rpcRequest.request = request;
> +
> +	threadRpcCall(rpcRequest);
> +
> +	return 0;
> +}

Repeating my question for the previous version, does this method need to
be synchronous ? We can keep it as-is for now, but I wonder if it
wouldn't make sense to later move (part of) the validation code here and
make the call asynchronous.

> +
> +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)
> +{
> +	std::unique_ptr<ThreadRpcMessage> message =
> +				utils::make_unique<ThreadRpcMessage>();
> +	message->rpc = &rpcRequest;
> +
> +	cameraDevice_->postMessage(std::move(message));
> +	rpcRequest.waitDelivery();
> +}
> diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h
> new file mode 100644
> index 000000000000..da63bfa79fc9
> --- /dev/null
> +++ b/src/android/camera_proxy.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * camera_proxy.h - Proxy to camera devices
> + */
> +#ifndef __ANDROID_CAMERA_PROXY_H__
> +#define __ANDROID_CAMERA_PROXY_H__
> +
> +#include <memory>
> +
> +#include <hardware/camera3.h>
> +
> +#include <libcamera/camera.h>
> +
> +class CameraDevice;
> +class ThreadRpc;
> +
> +class CameraProxy
> +{
> +public:
> +	CameraProxy(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> +	~CameraProxy();
> +
> +	int open(const hw_module_t *hardwareModule);
> +	void close();
> +
> +	void initialize(const camera3_callback_ops_t *callbacks);
> +	const camera_metadata_t *getStaticMetadata();
> +	const camera_metadata_t *constructDefaultRequestSettings(int type);
> +	int configureStreams(camera3_stream_configuration_t *stream_list);
> +	int processCaptureRequest(camera3_capture_request_t *request);
> +
> +	unsigned int id() const { return id_; }
> +	camera3_device_t *camera3Device() { return &camera3Device_; }
> +
> +private:
> +	void threadRpcCall(ThreadRpc &rpcRequest);
> +
> +	unsigned int id_;
> +	CameraDevice *cameraDevice_;
> +	camera3_device_t camera3Device_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_PROXY_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 1f242953db37..26537794bc29 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -1,3 +1,11 @@
> +android_hal_sources = files([
> +    'camera3_hal.cpp',
> +    'camera_hal_manager.cpp',
> +    'camera_device.cpp',
> +    'camera_proxy.cpp',
> +    'thread_rpc.cpp'
> +])
> +
>  android_camera_metadata_sources = files([
>      'metadata/camera_metadata.c',
>  ])
> diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp
> new file mode 100644
> index 000000000000..295a05d7c676
> --- /dev/null
> +++ b/src/android/thread_rpc.cpp
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * thread_rpc.cpp - Inter-thread procedure call
> + */
> +
> +#include "thread_rpc.h"
> +
> +#include "message.h"
> +
> +using namespace libcamera;
> +
> +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;
> +
> +ThreadRpcMessage::ThreadRpcMessage()
> +	: Message(type())
> +{
> +}
> +
> +void ThreadRpc::notifyReception()
> +{
> +	{
> +		libcamera::MutexLocker locker(mutex_);
> +		delivered_ = true;
> +	}
> +	cv_.notify_one();
> +}
> +
> +void ThreadRpc::waitDelivery()
> +{
> +	libcamera::MutexLocker locker(mutex_);
> +	cv_.wait(locker, [&] { return delivered_; });
> +}
> +
> +Message::Type ThreadRpcMessage::type()
> +{
> +	if (ThreadRpcMessage::rpcType_ == Message::Type::None)
> +		rpcType_ = Message::registerMessageType();
> +
> +	return rpcType_;
> +}
> diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h
> new file mode 100644
> index 000000000000..6d8992839d0b
> --- /dev/null
> +++ b/src/android/thread_rpc.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * thread_rpc.h - Inter-thread procedure call
> + */
> +#ifndef __ANDROID_THREAD_RPC_H__
> +#define __ANDROID_THREAD_RPC_H__
> +
> +#include <condition_variable>
> +#include <mutex>
> +
> +#include <hardware/camera3.h>
> +
> +#include "message.h"
> +#include "thread.h"
> +
> +class ThreadRpc
> +{
> +public:
> +	enum RpcTag {
> +		ProcessCaptureRequest,
> +		Close,
> +	};
> +
> +	ThreadRpc()
> +		: delivered_(false) {}
> +
> +	void notifyReception();
> +	void waitDelivery();
> +
> +	RpcTag tag;
> +
> +	camera3_capture_request_t *request;
> +
> +private:
> +	bool delivered_;
> +	std::mutex mutex_;
> +	std::condition_variable cv_;
> +};
> +
> +class ThreadRpcMessage : public libcamera::Message
> +{
> +public:
> +	ThreadRpcMessage();
> +	ThreadRpc *rpc;
> +
> +	static Message::Type type();
> +
> +private:
> +	static libcamera::Message::Type rpcType_;
> +};
> +
> +#endif /* __ANDROID_THREAD_RPC_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index a09b23d60022..7d5d3c04fba0 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -103,9 +103,18 @@ libcamera_deps = [
>      dependency('threads'),
>  ]
>  
> +libcamera_link_with = []
> +
> +if get_option('android')
> +    libcamera_sources += android_hal_sources
> +    includes += android_includes
> +    libcamera_link_with += android_camera_metadata
> +endif
> +
>  libcamera = shared_library('camera',
>                             libcamera_sources,
>                             install : true,
> +                           link_with : libcamera_link_with,
>                             include_directories : includes,
>                             dependencies : libcamera_deps)
>  
> diff --git a/src/meson.build b/src/meson.build
> index 7148baee3eda..67ad20aab86b 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -1,4 +1,7 @@
> -subdir('android')
> +if get_option('android')
> +    subdir('android')
> +endif
> +
>  subdir('libcamera')
>  subdir('ipa')
>  subdir('cam')

Hi Laurent,

On Sat, Aug 10, 2019 at 07:51:51PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Aug 09, 2019 at 12:04:05PM +0200, Jacopo Mondi wrote:
> > Add libcamera Android Camera HALv3 implementation.
> >
> > The initial camera HAL implementation supports the LIMITED hardware
> > level and uses statically defined metadata and camera characteristics.
> >
> > Add a build option named 'android' and adjust the build system to
> > selectively compile the Android camera HAL and link it against the
> > required Android libraries.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  meson_options.txt                  |   5 +
> >  src/android/camera3_hal.cpp        | 111 ++++
> >  src/android/camera_device.cpp      | 817 +++++++++++++++++++++++++++++
> >  src/android/camera_device.h        |  63 +++
> >  src/android/camera_hal_manager.cpp | 142 +++++
> >  src/android/camera_hal_manager.h   |  47 ++
> >  src/android/camera_proxy.cpp       | 199 +++++++
> >  src/android/camera_proxy.h         |  45 ++
> >  src/android/meson.build            |   8 +
> >  src/android/thread_rpc.cpp         |  42 ++
> >  src/android/thread_rpc.h           |  54 ++
> >  src/libcamera/meson.build          |   9 +
> >  src/meson.build                    |   5 +-
> >  13 files changed, 1546 insertions(+), 1 deletion(-)
> >  create mode 100644 src/android/camera3_hal.cpp
> >  create mode 100644 src/android/camera_device.cpp
> >  create mode 100644 src/android/camera_device.h
> >  create mode 100644 src/android/camera_hal_manager.cpp
> >  create mode 100644 src/android/camera_hal_manager.h
> >  create mode 100644 src/android/camera_proxy.cpp
> >  create mode 100644 src/android/camera_proxy.h
> >  create mode 100644 src/android/thread_rpc.cpp
> >  create mode 100644 src/android/thread_rpc.h
> >
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 97efc85b4412..2d78b8d91f9c 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -1,3 +1,8 @@
> > +option('android',
> > +        type : 'boolean',
> > +        value : false,
> > +        description : 'Compile libcamera with Android Camera3 HAL interface')
> > +
> >  option('documentation',
> >          type : 'boolean',
> >          description : 'Generate the project documentation')
> > diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> > new file mode 100644
> > index 000000000000..8d2629ca356c
> > --- /dev/null
> > +++ b/src/android/camera3_hal.cpp
> > @@ -0,0 +1,111 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera3_hal.cpp - Android Camera HALv3 module
> > + */
> > +
> > +#include <hardware/camera_common.h>
> > +
> > +#include "camera_hal_manager.h"
> > +#include "camera_proxy.h"
> > +#include "log.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY(HAL)
> > +
> > +static CameraHalManager cameraManager;
> > +
> > +/*------------------------------------------------------------------------------
> > + * Android Camera HAL callbacks
> > + */
> > +
> > +static int hal_get_number_of_cameras(void)
> > +{
> > +	return cameraManager.numCameras();
> > +}
> > +
> > +static int hal_get_camera_info(int id, struct camera_info *info)
> > +{
> > +	return cameraManager.getCameraInfo(id, info);
> > +}
> > +
> > +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int hal_open_legacy(const struct hw_module_t *module, const char *id,
> > +			   uint32_t halVersion, struct hw_device_t **device)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> > +static int hal_set_torch_mode(const char *camera_id, bool enabled)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> > +/*
> > + * First entry point of the camera HAL module.
> > + *
> > + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)
> > + */
> > +static int hal_init()
> > +{
> > +	LOG(HAL, Info) << "Initialising Android camera HAL";
> > +
> > +	cameraManager.init();
> > +
> > +	return 0;
> > +}
> > +
> > +/*------------------------------------------------------------------------------
> > + * Android Camera Device
> > + */
> > +
> > +static int hal_dev_open(const hw_module_t *module, const char *name,
> > +			hw_device_t **device)
> > +{
> > +	LOG(HAL, Debug) << "Open camera " << name;
> > +
> > +	int id = atoi(name);
> > +	CameraProxy *proxy = cameraManager.open(id, module);
> > +	if (!proxy) {
> > +		LOG(HAL, Error)
> > +			<< "Failed to open camera module '" << id << "'";
> > +		return -ENODEV;
> > +	}
> > +
> > +	*device = &proxy->camera3Device()->common;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct hw_module_methods_t hal_module_methods = {
> > +	.open = hal_dev_open,
> > +};
> > +
> > +camera_module_t HAL_MODULE_INFO_SYM = {
> > +	.common = {
> > +		.tag = HARDWARE_MODULE_TAG,
> > +		.module_api_version = CAMERA_MODULE_API_VERSION_2_4,
> > +		.hal_api_version = HARDWARE_HAL_API_VERSION,
> > +		.id = CAMERA_HARDWARE_MODULE_ID,
> > +		.name = "libcamera camera HALv3 module",
> > +		.author = "libcamera",
> > +		.methods = &hal_module_methods,
> > +		.dso = nullptr,
> > +		.reserved = {},
> > +	},
> > +
> > +	.get_number_of_cameras = hal_get_number_of_cameras,
> > +	.get_camera_info = hal_get_camera_info,
> > +	.set_callbacks = hal_set_callbacks,
> > +	.get_vendor_tag_ops = nullptr,
> > +	.open_legacy = hal_open_legacy,
> > +	.set_torch_mode = hal_set_torch_mode,
> > +	.init = hal_init,
> > +	.reserved = {},
> > +};
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > new file mode 100644
> > index 000000000000..18bf459a37e1
> > --- /dev/null
> > +++ b/src/android/camera_device.cpp
> > @@ -0,0 +1,817 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_device.cpp - libcamera Android Camera Device
> > + */
> > +
> > +#include "camera_device.h"
> > +
> > +#include <memory>
> > +
> > +#include <hardware/camera3.h>
> > +#include <system/camera_metadata.h>
> > +
> > +#include <libcamera/camera.h>
> > +
> > +#include "log.h"
> > +#include "message.h"
> > +
> > +#include "thread_rpc.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL);
> > +
> > +/*
> > + * \class CameraDevice
> > + *
> > + * The CameraDevice class wraps a libcamera::Camera instance, and implements
> > + * the camera_device_t interface by handling RPC requests received from its
> > + * associated CameraProxy.
> > + *
> > + * It translate parameters and operations from Camera HALv3 API to the libcamera
> > + * ones to provide static informations on a Camera, create request templates
>
> s/informations on/information for/
>
> > + * for it and process capture requests by then delivering capture results back
>
> s/and process/, process/
> s/by then delevering/, and then deliver/
>
> > + * to the framework using the designated callbacks.
> > + */
> > +
> > +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> > +	: running_(false), camera_(camera), staticMetadata_(nullptr),
> > +	  requestTemplate_(nullptr)
> > +{
> > +	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > +}
> > +
> > +CameraDevice::~CameraDevice()
> > +{
> > +	if (staticMetadata_)
> > +		free_camera_metadata(staticMetadata_);
> > +	staticMetadata_ = nullptr;
> > +
> > +	if (requestTemplate_)
> > +		free_camera_metadata(requestTemplate_);
> > +	requestTemplate_ = nullptr;
> > +}
> > +
> > +/*
> > + * Handle RPC request received from the associated proxy.
> > + */
> > +void CameraDevice::message(Message *message)
> > +{
> > +	if (message->type() != ThreadRpcMessage::type())
> > +		return Object::message(message);
> > +
> > +	ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message);
> > +	ThreadRpc *rpc = rpcMessage->rpc;
> > +
> > +	switch (rpc->tag) {
> > +	case ThreadRpc::ProcessCaptureRequest:
> > +		processCaptureRequest(rpc->request);
> > +		break;
> > +	case ThreadRpc::Close:
> > +		close();
> > +		break;
> > +	default:
> > +		LOG(HAL, Error) << "Unknown RPC operation: " << rpc->tag;
> > +	}
> > +
> > +	rpc->notifyReception();
> > +}
> > +
> > +int CameraDevice::open()
> > +{
> > +	int ret = camera_->acquire();
> > +	if (ret) {
> > +		LOG(HAL, Error) << "Failed to acquire the camera";
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void CameraDevice::close()
> > +{
> > +	camera_->stop();
> > +
> > +	camera_->freeBuffers();
> > +	camera_->release();
> > +
> > +	running_ = false;
> > +}
> > +
> > +void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> > +{
> > +	callbacks_ = callbacks;
> > +}
> > +
> > +/*
> > + * Return static informations on the camera.
>
> s/informations on/information for/
>
> > + */
> > +camera_metadata_t *CameraDevice::getStaticMetadata()
> > +{
> > +	int ret;
> > +
> > +	if (staticMetadata_)
> > +		return staticMetadata_;
> > +
> > +	/*
> > +	 * The here reported metadata are enough to implement a basic capture
> > +	 * example application, but a real camera implementation will require
> > +	 * more.
> > +	 */
> > +
> > +	/* \todo Use correct sizes */
> > +	#define STATIC_ENTRY_CAP 256
> > +	#define STATIC_DATA_CAP 6688
> > +	camera_metadata_t *staticMetadata =
> > +		allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> > +
> > +	/* Sensor static metadata. */
> > +	int32_t pixelArraySize[] = {
> > +		2592, 1944,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > +				&pixelArraySize, 2);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t sensorSizes[] = {
> > +		0, 0, 2560, 1920,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > +				&sensorSizes, 4);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t sensitivityRange[] = {
> > +		32, 2400,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> > +				&sensitivityRange, 2);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> > +				&filterArr, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int64_t exposureTimeRange[] = {
> > +		100000, 200000000,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> > +				&exposureTimeRange, 2);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t orientation = 0;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_SENSOR_ORIENTATION,
> > +				&orientation, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	/* Flash static metadata. */
> > +	char flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_FLASH_INFO_AVAILABLE,
> > +			&flashAvailable, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	/* Lens static metadata. */
> > +	float fn = 2.53 / 100;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +				ANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	/* Control metadata. */
> > +	char controlMetadata = ANDROID_CONTROL_MODE_AUTO;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_CONTROL_AVAILABLE_MODES,
> > +			&controlMetadata, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	char availableAntiBandingModes[] = {
> > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,
> > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,
> > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,
> > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> > +			availableAntiBandingModes, 4);
> > +	METADATA_ASSERT(ret);
> > +
> > +	char aeAvailableModes[] = {
> > +		ANDROID_CONTROL_AE_MODE_ON,
> > +		ANDROID_CONTROL_AE_MODE_OFF,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > +			aeAvailableModes, 2);
> > +	METADATA_ASSERT(ret);
> > +
> > +	controlMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > +			&controlMetadata, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > +			&awbLockAvailable, 1);
> > +
> > +	/* Scaler static metadata. */
> > +	std::vector<uint32_t> availableStreamFormats = {
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB,
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_SCALER_AVAILABLE_FORMATS,
> > +			availableStreamFormats.data(),
> > +			availableStreamFormats.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	std::vector<uint32_t> availableStreamConfigurations = {
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,
> > +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,
> > +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,
> > +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> > +			availableStreamConfigurations.data(),
> > +			availableStreamConfigurations.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	std::vector<int64_t> availableStallDurations = {
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> > +			availableStallDurations.data(),
> > +			availableStallDurations.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	std::vector<int64_t> minFrameDurations = {
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,
> > +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,
> > +	};
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> > +			minFrameDurations.data(), minFrameDurations.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	/* Info static metadata. */
> > +	uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +	ret = add_camera_metadata_entry(staticMetadata,
> > +			ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > +			&supportedHWLevel, 1);
> > +
> > +	return staticMetadata;
> > +}
> > +
> > +/*
> > + * Produce a metadata pack to be used as template for a capture request.
> > + */
> > +const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * \todo Inspect type and pick the right metadata pack.
> > +	 * As of now just use a single one for all templates.
> > +	 */
> > +	uint8_t captureIntent;
> > +	switch (type) {
> > +	case CAMERA3_TEMPLATE_PREVIEW:
> > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> > +		break;
> > +	case CAMERA3_TEMPLATE_STILL_CAPTURE:
> > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> > +		break;
> > +	case CAMERA3_TEMPLATE_VIDEO_RECORD:
> > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> > +		break;
> > +	case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> > +		break;
> > +	case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> > +		break;
> > +	case CAMERA3_TEMPLATE_MANUAL:
> > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> > +		break;
> > +	default:
> > +		LOG(HAL, Error) << "Invalid template request type: " << type;
> > +		return nullptr;
> > +	}
> > +
> > +	if (requestTemplate_)
> > +		return requestTemplate_;
> > +
> > +	/* \todo Use correct sizes */
> > +	#define REQUEST_TEMPLATE_ENTRIES	  30
> > +	#define REQUEST_TEMPLATE_DATA		2048
> > +	requestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,
> > +						    REQUEST_TEMPLATE_DATA);
> > +	if (!requestTemplate_) {
> > +		LOG(HAL, Error) << "Failed to allocate template metadata";
> > +		return nullptr;
> > +	}
> > +
> > +	/* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */
> > +	int32_t maxOutStream[] = { 0, 2, 0 };
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
> > +			maxOutStream, 3);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t maxPipelineDepth = 5;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > +			&maxPipelineDepth, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t inputStreams = 0;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,
> > +			&inputStreams, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t partialResultCount = 1;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> > +			&partialResultCount, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t availableCapabilities[] = {
> > +		ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> > +	};
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> > +			availableCapabilities, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AE_MODE,
> > +			&aeMode, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t aeExposureCompensation = 0;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > +			&aeExposureCompensation, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > +			&aePrecaptureTrigger, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AE_LOCK,
> > +			&aeLock, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AF_TRIGGER,
> > +			&afTrigger, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AWB_MODE,
> > +			&awbMode, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AWB_LOCK,
> > +			&awbLock, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > +			&awbLockAvailable, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t flashMode = ANDROID_FLASH_MODE_OFF;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_FLASH_MODE,
> > +			&flashMode, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_STATISTICS_FACE_DETECT_MODE,
> > +			&faceDetectMode, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_CONTROL_CAPTURE_INTENT,
> > +			&captureIntent, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	/*
> > +	 * This is quite hard to list at the moment wihtout knowing what
> > +	 * we could control.
> > +	 *
> > +	 * For now, just list in the available Request keys and in the available
> > +	 * result keys the control and reporting of the AE algorithm.
> > +	 */
> > +	std::vector<int32_t> availableRequestKeys = {
> > +		ANDROID_CONTROL_AE_MODE,
> > +		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > +		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > +		ANDROID_CONTROL_AE_LOCK,
> > +		ANDROID_CONTROL_AF_TRIGGER,
> > +		ANDROID_CONTROL_AWB_MODE,
> > +		ANDROID_CONTROL_AWB_LOCK,
> > +		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > +		ANDROID_CONTROL_CAPTURE_INTENT,
> > +		ANDROID_FLASH_MODE,
> > +		ANDROID_STATISTICS_FACE_DETECT_MODE,
> > +	};
> > +
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
> > +			availableRequestKeys.data(),
> > +			availableRequestKeys.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	std::vector<int32_t> availableResultKeys = {
> > +		ANDROID_CONTROL_AE_MODE,
> > +		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > +		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > +		ANDROID_CONTROL_AE_LOCK,
> > +		ANDROID_CONTROL_AF_TRIGGER,
> > +		ANDROID_CONTROL_AWB_MODE,
> > +		ANDROID_CONTROL_AWB_LOCK,
> > +		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > +		ANDROID_CONTROL_CAPTURE_INTENT,
> > +		ANDROID_FLASH_MODE,
> > +		ANDROID_STATISTICS_FACE_DETECT_MODE,
> > +	};
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,
> > +			availableResultKeys.data(),
> > +			availableResultKeys.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	/*
> > +	 * \todo The available characteristics are be the tags reported
> > +	 * as part of the static metadata reported at hal_get_camera_info()
> > +	 * time. As of now, report an empty list.
> > +	 */
> > +	std::vector<int32_t> availableCharacteristicsKeys = {};
> > +	ret = add_camera_metadata_entry(requestTemplate_,
> > +			ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> > +			availableCharacteristicsKeys.data(),
> > +			availableCharacteristicsKeys.size());
> > +	METADATA_ASSERT(ret);
> > +
> > +	return requestTemplate_;
> > +}
> > +
> > +/*
> > + * Inspect the stream_list to produce a list of StreamConfiguration to
> > + * be use to configure the Camera.
> > + */
> > +int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > +{
> > +
>
> Extra blank line.
>
> > +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > +		camera3_stream_t *stream = stream_list->streams[i];
> > +
> > +		LOG(HAL, Info) << "Stream #" << i
> > +			       << ", direction: " << stream->stream_type
> > +			       << ", width: " << stream->width
> > +			       << ", height: " << stream->height
> > +			       << ", format: " << std::hex << stream->format;
> > +	}
> > +
> > +	/* Hardcode viewfinder role, collecting sizes from the stream config. */
> > +	if (stream_list->num_streams != 1) {
> > +		LOG(HAL, Error) << "Only one stream supported";
> > +		return -EINVAL;
> > +	}
> > +
> > +	StreamRoles roles = { StreamRole::Viewfinder };
> > +	config_ = camera_->generateConfiguration(roles);
> > +	if (!config_ || config_->empty()) {
> > +		LOG(HAL, Error) << "Failed to generate camera configuration";
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Only one stream is supported. */
> > +	camera3_stream_t *camera3Stream = stream_list->streams[0];
> > +	StreamConfiguration *streamConfiguration = &config_->at(0);
> > +	streamConfiguration->size.width = camera3Stream->width;
> > +	streamConfiguration->size.height = camera3Stream->height;
> > +	streamConfiguration->memoryType = ExternalMemory;
> > +
> > +	/*
> > +	 * \todo We'll need to translate from Android defined pixel format codes
> > +	 * to the libcamera image format codes. As of now, do not change the
>
> s/As of/For/
>
> > +	 * format returned from Camera::generateConfiguration().
> > +	 */
> > +
> > +	switch (config_->validate()) {
> > +	case CameraConfiguration::Valid:
> > +		break;
> > +	case CameraConfiguration::Adjusted:
> > +		LOG(HAL, Info) << "Camera configuration adjusted";
> > +		config_.reset();
> > +		return -EINVAL;
> > +	case CameraConfiguration::Invalid:
> > +		LOG(HAL, Info) << "Camera configuration invalid";
> > +		config_.reset();
> > +		return -EINVAL;
> > +	}
> > +
> > +	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> > +
> > +	/*
> > +	 * Once the CameraConfiguration has been adjusted/validated
> > +	 * it can be applied to the camera.
> > +	 */
> > +	int ret = camera_->configure(config_.get());
> > +	if (ret) {
> > +		LOG(HAL, Error) << "Failed to configure camera '"
> > +				<< camera_->name() << "'";
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > +{
> > +	StreamConfiguration *streamConfiguration = &config_->at(0);
> > +	Stream *stream = streamConfiguration->stream();
> > +
> > +	if (camera3Request->num_output_buffers != 1) {
> > +		LOG(HAL, Error) << "Invalid number of output buffers: "
> > +				<< camera3Request->num_output_buffers;
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Start the camera if that's the first request we handle. */
> > +	if (!running_) {
> > +		int ret = camera_->allocateBuffers();
> > +		if (ret) {
> > +			LOG(HAL, Error) << "Failed to allocate buffers";
> > +			return ret;
> > +		}
> > +
> > +		ret = camera_->start();
> > +		if (ret) {
> > +			LOG(HAL, Error) << "Failed to start camera";
> > +			camera_->freeBuffers();
> > +			return ret;
> > +		}
> > +
> > +		running_ = true;
> > +	}
> > +
> > +	/*
> > +	 * Queue a request for the Camera with the provided dmabuf file
> > +	 * descriptors.
> > +	 */
> > +	const camera3_stream_buffer_t *camera3Buffers =
> > +					camera3Request->output_buffers;
> > +
> > +	/*
> > +	 * Save the request descriptors for use at completion time.
> > +	 * The descriptor and the associated memory reserved here are freed
> > +	 * at request complete time.
> > +	 */
> > +	Camera3RequestDescriptor *descriptor = new Camera3RequestDescriptor();
> > +	descriptor->frameNumber = camera3Request->frame_number;
> > +	descriptor->numBuffers = camera3Request->num_output_buffers;
> > +	descriptor->buffers = new camera3_stream_buffer_t[descriptor->numBuffers];
> > +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > +		/*
> > +		 * Keep track of which stream the request belong and of the
> > +		 * native buffer handles.
> > +		 *
> > +		 * \todo Currently we only support one capture buffer. Copy
> > +		 * all of them to be ready once we'll support more.
> > +		 */
> > +		descriptor->buffers[i].stream = camera3Buffers[i].stream;
> > +		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> > +	}
> > +
> > +	/*
> > +	 * Create a libcamera buffer using the dmabuf descriptors of the first
> > +	 * and (currently) only supported request buffer.
> > +	 */
> > +	const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer;
> > +	std::array<int, 3> fds = {
> > +		camera3Handle->data[0],
> > +		camera3Handle->data[1],
> > +		camera3Handle->data[2],
> > +	};
> > +
> > +	std::unique_ptr<Buffer> buffer = stream->createBuffer(fds);
> > +	if (!buffer) {
> > +		LOG(HAL, Error) << "Failed to create buffer";
> > +		return -EINVAL;
>
> You're leaking descriptor and descriptor->buffers.
>

Are you sure? I delete them at the time a request completes. See
below, you have commented on it.

But now I get you meant I'm leaking them in the error path! I
re-ordered this sequence for cosmetic reasons and forgot to handle
this! thanks

> I would implement a constructor for Camera3RequestDescriptor that takes
> the number of buffers and allocate the buffers member, and a destructor
> that deletes the buffers member. Then you will only need to care about
> deleting descriptor here.

Good idea

>
> > +	}
> > +
> > +	Request *request =
> > +		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > +	request->addBuffer(std::move(buffer));
> > +
> > +	int ret = camera_->queueRequest(request);
> > +	if (ret) {
> > +		LOG(HAL, Error) << "Failed to queue request";
> > +		return ret;
>
> You're leaking request, descriptor and descriptor->buffers.
>
> I wonder if Camera::createRequest() shouldn't return an std::unique_ptr
> (this doesn't need to be addressed in this patch series).
>
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void CameraDevice::requestComplete(Request *request,
> > +				   const std::map<Stream *, Buffer *> &buffers)
> > +{
> > +	Buffer *libcameraBuffer = buffers.begin()->second;
> > +	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> > +	camera_metadata_t *resultMetadata = nullptr;
> > +
> > +	if (request->status() != Request::RequestComplete) {
> > +		LOG(HAL, Error) << "Request not succesfully completed: "
> > +				<< request->status();
> > +		status = CAMERA3_BUFFER_STATUS_ERROR;
> > +	}
> > +
> > +	/* Prepare to call back the Android camera stack. */
> > +	Camera3RequestDescriptor *descriptor =
> > +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> > +
> > +	camera3_capture_result_t captureResult = {};
> > +	captureResult.frame_number = descriptor->frameNumber;
> > +	captureResult.num_output_buffers = descriptor->numBuffers;
> > +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > +		/*
> > +		 * The buffers in the descriptor have been set up at queue
> > +		 * request time.
>
> I think you can drop this sentence.
>
> > +		 *
> > +		 * \todo Currently we only support one capture buffer. Copy
> > +		 * all of them to be ready once we'll support more.
> > +		 */
> > +		descriptor->buffers[i].acquire_fence = -1;
> > +		descriptor->buffers[i].release_fence = -1;
> > +		descriptor->buffers[i].status = status;
> > +	}
> > +	captureResult.output_buffers =
> > +		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
> > +
> > +	if (status == CAMERA3_BUFFER_STATUS_ERROR) {
> > +		/* \todo Improve error handling. */
> > +		notifyError(descriptor->frameNumber,
> > +			    descriptor->buffers[0].stream);
> > +	} else {
> > +		notifyShutter(descriptor->frameNumber,
> > +			      libcameraBuffer->timestamp());
> > +
> > +		captureResult.partial_result = 1;
> > +		resultMetadata = getResultMetadata(descriptor->frameNumber,
> > +						   libcameraBuffer->timestamp());
> > +		captureResult.result = resultMetadata;
> > +	}
> > +
> > +	callbacks_->process_capture_result(callbacks_, &captureResult);
> > +
> > +	delete[] descriptor->buffers;
> > +	delete descriptor;
>
> This will also be simplified with a destructor for the
> Camera3RequestDescriptor class.
>
> > +	if (resultMetadata)
> > +		free_camera_metadata(resultMetadata);
> > +
> > +	return;
> > +}
> > +
> > +void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> > +{
> > +	camera3_notify_msg_t notify = {};
> > +
> > +	notify.type = CAMERA3_MSG_SHUTTER;
> > +	notify.message.shutter.frame_number = frameNumber;
> > +	notify.message.shutter.timestamp = timestamp;
> > +
> > +	callbacks_->notify(callbacks_, &notify);
> > +}
> > +
> > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> > +{
> > +	camera3_notify_msg_t notify = {};
> > +
> > +	notify.type = CAMERA3_MSG_ERROR;
> > +	notify.message.error.error_stream = stream;
> > +	notify.message.error.frame_number = frameNumber;
> > +	notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> > +
> > +	callbacks_->notify(callbacks_, &notify);
> > +}
> > +
> > +/*
> > + * Produce a set of fixed result metadata.
> > + */
> > +camera_metadata_t *CameraDevice::getResultMetadata(int frame_number,
> > +						   int64_t timestamp)
> > +{
> > +	int ret;
> > +
> > +	/* \todo Use correct sizes */
> > +	#define RESULT_ENTRY_CAP 256
> > +	#define RESULT_DATA_CAP 6688
> > +	camera_metadata_t *resultMetadata =
> > +		allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> > +
> > +	const uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,
> > +					&ae_state, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
> > +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,
> > +					&ae_lock, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	uint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;
> > +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,
> > +					&af_state, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	const uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_CONTROL_AWB_STATE,
> > +					&awb_state, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	const uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_CONTROL_AWB_LOCK,
> > +					&awb_lock, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	const uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_LENS_STATE,
> > +					&lens_state, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	int32_t sensorSizes[] = {
> > +		0, 0, 2560, 1920,
> > +	};
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_SCALER_CROP_REGION,
> > +					sensorSizes, 4);
> > +	METADATA_ASSERT(ret);
> > +
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_SENSOR_TIMESTAMP,
> > +					&timestamp, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	/* 33.3 msec */
> > +	const int64_t rolling_shutter_skew = 33300000;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
> > +					&rolling_shutter_skew, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	/* 16.6 msec */
> > +	const int64_t exposure_time = 16600000;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_SENSOR_EXPOSURE_TIME,
> > +					&exposure_time, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	const uint8_t lens_shading_map_mode =
> > +				ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,
> > +					&lens_shading_map_mode, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	const uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;
> > +	ret = add_camera_metadata_entry(resultMetadata,
> > +					ANDROID_STATISTICS_SCENE_FLICKER,
> > +					&scene_flicker, 1);
> > +	METADATA_ASSERT(ret);
> > +
> > +	return resultMetadata;
> > +}
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > new file mode 100644
> > index 000000000000..402c3dca69da
> > --- /dev/null
> > +++ b/src/android/camera_device.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_device.h - libcamera Android Camera Device
> > + */
> > +#ifndef __ANDROID_CAMERA_DEVICE_H__
> > +#define __ANDROID_CAMERA_DEVICE_H__
> > +
> > +#include <memory>
> > +
> > +#include <hardware/camera3.h>
> > +
> > +#include <libcamera/libcamera.h>
>
> Just the required headers please :-)
>
> > +
> > +#include "message.h"
> > +
> > +#define METADATA_ASSERT(_r)		\
> > +	do {				\
> > +		if (!(_r)) break;	\
> > +		LOG(HAL, Error) << "Error: " << __func__ << ":" << __LINE__; \
> > +		return nullptr;		\
> > +	} while(0);
> > +
> > +class CameraDevice : public libcamera::Object
> > +{
> > +public:
> > +	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
>
> You can make the second argument a reference to avoid an unnecessary
> copy.

Isn't there some copy elision magic that helps here? The temporary
argument instance is created and destroyed immediately... I can indeed
use a reference though

>
> > +	~CameraDevice();
> > +
> > +	void message(libcamera::Message *message);
> > +
> > +	int open();
> > +	void close();
> > +	void setCallbacks(const camera3_callback_ops_t *callbacks);
> > +	camera_metadata_t *getStaticMetadata();
> > +	const camera_metadata_t *constructDefaultRequestSettings(int type);
> > +	int configureStreams(camera3_stream_configuration_t *stream_list);
> > +	int processCaptureRequest(camera3_capture_request_t *request);
> > +	void requestComplete(libcamera::Request *request,
> > +			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
> > +
> > +private:
> > +	struct Camera3RequestDescriptor {
> > +		uint32_t frameNumber;
> > +		uint32_t numBuffers;
> > +		camera3_stream_buffer_t *buffers;
> > +	};
> > +
> > +	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > +	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > +	camera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);
> > +
> > +	bool running_;
> > +	std::shared_ptr<libcamera::Camera> camera_;
> > +	std::unique_ptr<libcamera::CameraConfiguration> config_;
> > +
> > +	camera_metadata_t *staticMetadata_;
> > +	camera_metadata_t *requestTemplate_;
> > +	const camera3_callback_ops_t *callbacks_;
> > +};
> > +
> > +#endif /* __ANDROID_CAMERA_DEVICE_H__ */
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > new file mode 100644
> > index 000000000000..0b4d50368ff8
> > --- /dev/null
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -0,0 +1,142 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_hal_manager.cpp - libcamera Android Camera Manager
> > + */
> > +
> > +#include "camera_hal_manager.h"
> > +
> > +#include <hardware/hardware.h>
> > +#include <system/camera_metadata.h>
> > +
> > +#include <libcamera/camera.h>
> > +
> > +#include "log.h"
> > +
> > +#include "camera_device.h"
> > +#include "camera_proxy.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL);
> > +
> > +/*
> > + * \class CameraHalManager
> > + *
> > + * The HAL camera manager is initializated at camera_module_t 'hal_init()' time
> > + * and spawns its own thread where libcamera related events are dispatched to.
> > + * It wraps the libcamera CameraManager operations and provides helpers for
>
> s/for/for the/
>
> > + * camera_module_t operations, to retrieve the number of cameras in the system
>
> s/system/system, to retrieve /
>
> > + * their static informations and to open and close camera devices.
>
> s/informations/information/
>
> > + */
> > +
> > +int CameraHalManager::init()
> > +{
> > +	/*
> > +	 * Start the camera HAL manager thread and wait until its
> > +	 * initialisation to complete to be fully operational before
>
> s/to complete/completes/
>
> > +	 * receiving calls from the camera stack.
> > +	 */
> > +	start();
> > +
> > +	MutexLocker locker(mutex_);
> > +	cv_.wait(locker);
> > +
> > +	return 0;
> > +}
> > +
> > +void CameraHalManager::run()
> > +{
> > +	/*
> > +	 * All the libcamera components must be initialised here, in
> > +	 * order to bind them to the camera HAL manager thread that
> > +	 * executes the event dispatcher.
> > +	 */
> > +	cameraManager_ = libcamera::CameraManager::instance();
> > +
> > +	int ret = cameraManager_->start();
> > +	if (ret) {
> > +		LOG(HAL, Error) << "Failed to start camera manager: "
> > +				<< strerror(-ret);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * For each Camera registered in the system, a CameraProxy
> > +	 * gets created here wich wraps a camera device.
>
> s/wich wraps/to wrap/
>
> > +	 *
> > +	 * \todo Support camera hotplug.
> > +	 */
> > +	unsigned int index = 0;
> > +	for (auto &camera : cameraManager_->cameras()) {
> > +		CameraProxy *proxy = new CameraProxy(index, camera);
> > +		proxies_.emplace_back(proxy);
> > +
> > +		++index;
> > +	}
> > +
> > +	/*
> > +	 * libcamera has been initialized. Unlock the init() caller
> > +	 * as we're now ready to handle calls from the framework.
> > +	 */
> > +	cv_.notify_one();
> > +
> > +	/* Now start processing events and messages. */
> > +	exec();
> > +}
> > +
> > +CameraProxy *CameraHalManager::open(unsigned int id,
> > +				    const hw_module_t *hardwareModule)
> > +{
> > +	if (id < 0 || id >= numCameras()) {
> > +		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> > +		return nullptr;
> > +	}
> > +
> > +	CameraProxy *proxy = proxies_[id].get();
> > +	if (proxy->open(hardwareModule))
> > +		return nullptr;
> > +
> > +	LOG(HAL, Info) << "Open camera '" << id << "'";
> > +
> > +	return proxy;
> > +}
> > +
> > +int CameraHalManager::close(CameraProxy *proxy)
> > +{
> > +	proxy->close();
> > +	LOG(HAL, Info) << "Close camera '" << proxy->id() << "'";
> > +
> > +	return 0;
> > +}
> > +
> > +unsigned int CameraHalManager::numCameras() const
> > +{
> > +	return cameraManager_->cameras().size();
> > +}
> > +
> > +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)
> > +{
> > +	if (!info)
> > +		return -EINVAL;
> > +
> > +	int cameras = numCameras();
> > +	if (id >= cameras || id < 0) {
>
> s/cameras/numCameras()/
>
> and drop the local variable.
>
> > +		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> > +		return -EINVAL;
> > +	}
> > +
> > +	CameraProxy *proxy = proxies_[id].get();
> > +
> > +	/* \todo Get these info dynamically inspecting the camera module. */
> > +	info->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;
> > +	info->orientation = 0;
> > +	info->device_version = 0;
> > +	info->resource_cost = 0;
> > +	info->static_camera_characteristics = proxy->getStaticMetadata();
> > +	info->conflicting_devices = nullptr;
> > +	info->conflicting_devices_length = 0;
> > +
> > +	return 0;
> > +}
> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > new file mode 100644
> > index 000000000000..8004aaf660f5
> > --- /dev/null
> > +++ b/src/android/camera_hal_manager.h
> > @@ -0,0 +1,47 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_hal_manager.h - libcamera Android Camera Manager
> > + */
> > +#ifndef __ANDROID_CAMERA_MANAGER_H__
> > +#define __ANDROID_CAMERA_MANAGER_H__
> > +
> > +#include <condition_variable>
> > +#include <mutex>
> > +#include <vector>
> > +
> > +#include <hardware/hardware.h>
> > +#include <system/camera_metadata.h>
> > +
> > +#include <libcamera/camera_manager.h>
> > +
> > +#include "thread.h"
> > +
> > +class CameraDevice;
> > +class CameraProxy;
> > +
> > +class CameraHalManager : public libcamera::Thread
> > +{
> > +public:
> > +	int init();
> > +
> > +	CameraProxy *open(unsigned int id, const hw_module_t *module);
> > +	int close(CameraProxy *proxy);
> > +
> > +	unsigned int numCameras() const;
> > +	int getCameraInfo(int id, struct camera_info *info);
> > +
> > +private:
> > +	void run() override;
> > +	camera_metadata_t *getStaticMetadata(unsigned int id);
> > +
> > +	libcamera::CameraManager *cameraManager_;
> > +
> > +	std::mutex mutex_;
> > +	std::condition_variable cv_;
> > +
> > +	std::vector<std::unique_ptr<CameraProxy>> proxies_;
> > +};
> > +
> > +#endif /* __ANDROID_CAMERA_MANAGER_H__ */
> > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp
> > new file mode 100644
> > index 000000000000..23c33c8f6c9c
> > --- /dev/null
> > +++ b/src/android/camera_proxy.cpp
> > @@ -0,0 +1,199 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_proxy.cpp - Proxy to camera devices
> > + */
> > +
> > +#include "camera_proxy.h"
> > +
> > +#include <memory>
> > +
> > +#include <hardware/camera3.h>
> > +#include <system/camera_metadata.h>
> > +
> > +#include <libcamera/camera.h>
> > +
> > +#include "log.h"
> > +#include "message.h"
> > +#include "utils.h"
> > +
> > +#include "camera_device.h"
> > +#include "thread_rpc.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(HAL);
> > +
> > +/*
> > + * \class CameraProxy
> > + *
> > + * The CameraProxy wraps a CameraDevice and implements the camera3_device_t
> > + * API, bridging calls received from the camera framework to the CameraDevice.
> > + *
> > + * Bridging operation calls between the framework and the CameraDevice is
> > + * required as the two runs in two different threads and certain operations,
>
> s/runs/run/
>
> > + * such as queueing a new capture request to the camera, require to interrupt
> > + * and restart the even dispatches associated with the thread where the
>
> s/even dispatches/event dispatcher/
>
> It's not about interrupting the event dispatcher, but about being called
> in the same thread than the one that handles events.

I was actually thinking about the -first- capture request, that
by starting the video device, updates the list of file descriptors to
monitor and requires the even notifier to be interrupted and
restarted. In general, you're correct and I'll change this.

>
> "such as queueing a new capture request to the camera, shall be called
> in the thread that dispatches events."
>
> > + * CameraDevice is running. Other operations do not require any bridging and
> > + * resolve to direct function calls on the CameraDevice instance instead.
> > + */
> > +
> > +static int hal_dev_initialize(const struct camera3_device *dev,
> > +			      const camera3_callback_ops_t *callback_ops)
> > +{
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > +	proxy->initialize(callback_ops);
> > +
> > +	return 0;
> > +}
> > +
> > +static int hal_dev_configure_streams(const struct camera3_device *dev,
> > +				     camera3_stream_configuration_t *stream_list)
> > +{
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > +	return proxy->configureStreams(stream_list);
> > +}
> > +
> > +static const camera_metadata_t *
> > +hal_dev_construct_default_request_settings(const struct camera3_device *dev,
> > +					   int type)
> > +{
> > +	if (!dev)
> > +		return nullptr;
> > +
> > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > +	return proxy->constructDefaultRequestSettings(type);
> > +}
> > +
> > +static int hal_dev_process_capture_request(const struct camera3_device *dev,
> > +					   camera3_capture_request_t *request)
> > +{
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > +	return proxy->processCaptureRequest(request);
> > +}
> > +
> > +static void hal_dev_dump(const struct camera3_device *dev, int fd)
> > +{
> > +}
> > +
> > +static int hal_dev_flush(const struct camera3_device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int hal_dev_close(hw_device_t *hw_device)
> > +{
> > +	if (!hw_device)
> > +		return -EINVAL;
> > +
> > +	camera3_device_t *dev = reinterpret_cast<camera3_device_t *>(hw_device);
> > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > +
> > +	proxy->close();
> > +
> > +	return 0;
> > +}
> > +
> > +static camera3_device_ops hal_dev_ops = {
> > +	.initialize = hal_dev_initialize,
> > +	.configure_streams = hal_dev_configure_streams,
> > +	.register_stream_buffers = nullptr,
> > +	.construct_default_request_settings = hal_dev_construct_default_request_settings,
> > +	.process_capture_request = hal_dev_process_capture_request,
> > +	.get_metadata_vendor_tag_ops = nullptr,
> > +	.dump = hal_dev_dump,
> > +	.flush = hal_dev_flush,
> > +	.reserved = { nullptr },
> > +};
> > +
> > +CameraProxy::CameraProxy(unsigned int id, std::shared_ptr<Camera> camera)
> > +	: id_(id)
> > +{
> > +	cameraDevice_ = new CameraDevice(id, camera);
> > +}
> > +
> > +CameraProxy::~CameraProxy()
> > +{
> > +	delete cameraDevice_;
> > +}
> > +
> > +int CameraProxy::open(const hw_module_t *hardwareModule)
> > +{
> > +	int ret = cameraDevice_->open();
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Initialize the hw_device_t in the instance camera3_module_t. */
> > +	camera3Device_.common.tag = HARDWARE_DEVICE_TAG;
> > +	camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3;
> > +	camera3Device_.common.module = (hw_module_t *)hardwareModule;
> > +	camera3Device_.common.close = hal_dev_close;
> > +
> > +	/*
> > +	 * The camera device operations. These actually implement
> > +	 * the Android Camera HALv3 interface.
> > +	 */
> > +	camera3Device_.ops = &hal_dev_ops;
> > +	camera3Device_.priv = this;
> > +
> > +	return 0;
> > +}
> > +
> > +void CameraProxy::close()
> > +{
> > +	ThreadRpc rpcRequest;
> > +	rpcRequest.tag = ThreadRpc::Close;
> > +
> > +	threadRpcCall(rpcRequest);
> > +}
> > +
> > +void CameraProxy::initialize(const camera3_callback_ops_t *callbacks)
> > +{
> > +	cameraDevice_->setCallbacks(callbacks);
> > +}
> > +
> > +const camera_metadata_t *CameraProxy::getStaticMetadata()
> > +{
> > +	return cameraDevice_->getStaticMetadata();
> > +}
> > +
> > +const camera_metadata_t *CameraProxy::constructDefaultRequestSettings(int type)
> > +{
> > +	return cameraDevice_->constructDefaultRequestSettings(type);
> > +}
> > +
> > +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)
> > +{
> > +	return cameraDevice_->configureStreams(stream_list);
> > +}
> > +
> > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
> > +{
> > +	ThreadRpc rpcRequest;
> > +	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> > +	rpcRequest.request = request;
> > +
> > +	threadRpcCall(rpcRequest);
> > +
> > +	return 0;
> > +}
>
> Repeating my question for the previous version, does this method need to
> be synchronous ? We can keep it as-is for now, but I wonder if it
> wouldn't make sense to later move (part of) the validation code here and
> make the call asynchronous.

Are you suggesting to drop the wait on the delivery condition in the
rpc call? How else would you make it asynchronous?

Thanks
   j

>
> > +
> > +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)
> > +{
> > +	std::unique_ptr<ThreadRpcMessage> message =
> > +				utils::make_unique<ThreadRpcMessage>();
> > +	message->rpc = &rpcRequest;
> > +
> > +	cameraDevice_->postMessage(std::move(message));
> > +	rpcRequest.waitDelivery();
> > +}
> > diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h
> > new file mode 100644
> > index 000000000000..da63bfa79fc9
> > --- /dev/null
> > +++ b/src/android/camera_proxy.h
> > @@ -0,0 +1,45 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * camera_proxy.h - Proxy to camera devices
> > + */
> > +#ifndef __ANDROID_CAMERA_PROXY_H__
> > +#define __ANDROID_CAMERA_PROXY_H__
> > +
> > +#include <memory>
> > +
> > +#include <hardware/camera3.h>
> > +
> > +#include <libcamera/camera.h>
> > +
> > +class CameraDevice;
> > +class ThreadRpc;
> > +
> > +class CameraProxy
> > +{
> > +public:
> > +	CameraProxy(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> > +	~CameraProxy();
> > +
> > +	int open(const hw_module_t *hardwareModule);
> > +	void close();
> > +
> > +	void initialize(const camera3_callback_ops_t *callbacks);
> > +	const camera_metadata_t *getStaticMetadata();
> > +	const camera_metadata_t *constructDefaultRequestSettings(int type);
> > +	int configureStreams(camera3_stream_configuration_t *stream_list);
> > +	int processCaptureRequest(camera3_capture_request_t *request);
> > +
> > +	unsigned int id() const { return id_; }
> > +	camera3_device_t *camera3Device() { return &camera3Device_; }
> > +
> > +private:
> > +	void threadRpcCall(ThreadRpc &rpcRequest);
> > +
> > +	unsigned int id_;
> > +	CameraDevice *cameraDevice_;
> > +	camera3_device_t camera3Device_;
> > +};
> > +
> > +#endif /* __ANDROID_CAMERA_PROXY_H__ */
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 1f242953db37..26537794bc29 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -1,3 +1,11 @@
> > +android_hal_sources = files([
> > +    'camera3_hal.cpp',
> > +    'camera_hal_manager.cpp',
> > +    'camera_device.cpp',
> > +    'camera_proxy.cpp',
> > +    'thread_rpc.cpp'
> > +])
> > +
> >  android_camera_metadata_sources = files([
> >      'metadata/camera_metadata.c',
> >  ])
> > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp
> > new file mode 100644
> > index 000000000000..295a05d7c676
> > --- /dev/null
> > +++ b/src/android/thread_rpc.cpp
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * thread_rpc.cpp - Inter-thread procedure call
> > + */
> > +
> > +#include "thread_rpc.h"
> > +
> > +#include "message.h"
> > +
> > +using namespace libcamera;
> > +
> > +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;
> > +
> > +ThreadRpcMessage::ThreadRpcMessage()
> > +	: Message(type())
> > +{
> > +}
> > +
> > +void ThreadRpc::notifyReception()
> > +{
> > +	{
> > +		libcamera::MutexLocker locker(mutex_);
> > +		delivered_ = true;
> > +	}
> > +	cv_.notify_one();
> > +}
> > +
> > +void ThreadRpc::waitDelivery()
> > +{
> > +	libcamera::MutexLocker locker(mutex_);
> > +	cv_.wait(locker, [&] { return delivered_; });
> > +}
> > +
> > +Message::Type ThreadRpcMessage::type()
> > +{
> > +	if (ThreadRpcMessage::rpcType_ == Message::Type::None)
> > +		rpcType_ = Message::registerMessageType();
> > +
> > +	return rpcType_;
> > +}
> > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h
> > new file mode 100644
> > index 000000000000..6d8992839d0b
> > --- /dev/null
> > +++ b/src/android/thread_rpc.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * thread_rpc.h - Inter-thread procedure call
> > + */
> > +#ifndef __ANDROID_THREAD_RPC_H__
> > +#define __ANDROID_THREAD_RPC_H__
> > +
> > +#include <condition_variable>
> > +#include <mutex>
> > +
> > +#include <hardware/camera3.h>
> > +
> > +#include "message.h"
> > +#include "thread.h"
> > +
> > +class ThreadRpc
> > +{
> > +public:
> > +	enum RpcTag {
> > +		ProcessCaptureRequest,
> > +		Close,
> > +	};
> > +
> > +	ThreadRpc()
> > +		: delivered_(false) {}
> > +
> > +	void notifyReception();
> > +	void waitDelivery();
> > +
> > +	RpcTag tag;
> > +
> > +	camera3_capture_request_t *request;
> > +
> > +private:
> > +	bool delivered_;
> > +	std::mutex mutex_;
> > +	std::condition_variable cv_;
> > +};
> > +
> > +class ThreadRpcMessage : public libcamera::Message
> > +{
> > +public:
> > +	ThreadRpcMessage();
> > +	ThreadRpc *rpc;
> > +
> > +	static Message::Type type();
> > +
> > +private:
> > +	static libcamera::Message::Type rpcType_;
> > +};
> > +
> > +#endif /* __ANDROID_THREAD_RPC_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index a09b23d60022..7d5d3c04fba0 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -103,9 +103,18 @@ libcamera_deps = [
> >      dependency('threads'),
> >  ]
> >
> > +libcamera_link_with = []
> > +
> > +if get_option('android')
> > +    libcamera_sources += android_hal_sources
> > +    includes += android_includes
> > +    libcamera_link_with += android_camera_metadata
> > +endif
> > +
> >  libcamera = shared_library('camera',
> >                             libcamera_sources,
> >                             install : true,
> > +                           link_with : libcamera_link_with,
> >                             include_directories : includes,
> >                             dependencies : libcamera_deps)
> >
> > diff --git a/src/meson.build b/src/meson.build
> > index 7148baee3eda..67ad20aab86b 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -1,4 +1,7 @@
> > -subdir('android')
> > +if get_option('android')
> > +    subdir('android')
> > +endif
> > +
> >  subdir('libcamera')
> >  subdir('ipa')
> >  subdir('cam')
>
> --
> Regards,
>
> Laurent Pinchart

Hi Laurent, one additiona note

On Sat, Aug 10, 2019 at 07:51:51PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>

[snip]

> > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
> > +{
> > +	ThreadRpc rpcRequest;
> > +	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> > +	rpcRequest.request = request;
> > +
> > +	threadRpcCall(rpcRequest);
> > +
> > +	return 0;
> > +}
>
> Repeating my question for the previous version, does this method need to
> be synchronous ? We can keep it as-is for now, but I wonder if it
> wouldn't make sense to later move (part of) the validation code here and
> make the call asynchronous.
>

Quoting the camera3.h header description of the expected operation
sequence:

 * 7. The framework constructs and sends the first capture request to the HAL,
 *    with settings based on one of the sets of default settings, and with at
 *    least one output stream, which has been registered earlier by the
 *    framework. This is sent to the HAL with
 *    camera3_device_t->ops->process_capture_request(). The HAL must block the
 *    return of this call until it is ready for the next request to be sent.

The last statement makes me wonder if making the method asynchronous
would not then require serializin the requests on our side, as if we
return earlier, there might be a chance (no idea how large the
windonw could be) we receive a new capture request while the previous
one still has to be handled.

Consider this, I'm not sure it's a good idea to make this call
asynchronous.

Thanks
   j

Hi Jacopo,

On Mon, Aug 12, 2019 at 09:09:07AM +0200, Jacopo Mondi wrote:
> On Sat, Aug 10, 2019 at 07:51:51PM +0300, Laurent Pinchart wrote:
> > On Fri, Aug 09, 2019 at 12:04:05PM +0200, Jacopo Mondi wrote:
> > > Add libcamera Android Camera HALv3 implementation.
> > >
> > > The initial camera HAL implementation supports the LIMITED hardware
> > > level and uses statically defined metadata and camera characteristics.
> > >
> > > Add a build option named 'android' and adjust the build system to
> > > selectively compile the Android camera HAL and link it against the
> > > required Android libraries.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  meson_options.txt                  |   5 +
> > >  src/android/camera3_hal.cpp        | 111 ++++
> > >  src/android/camera_device.cpp      | 817 +++++++++++++++++++++++++++++
> > >  src/android/camera_device.h        |  63 +++
> > >  src/android/camera_hal_manager.cpp | 142 +++++
> > >  src/android/camera_hal_manager.h   |  47 ++
> > >  src/android/camera_proxy.cpp       | 199 +++++++
> > >  src/android/camera_proxy.h         |  45 ++
> > >  src/android/meson.build            |   8 +
> > >  src/android/thread_rpc.cpp         |  42 ++
> > >  src/android/thread_rpc.h           |  54 ++
> > >  src/libcamera/meson.build          |   9 +
> > >  src/meson.build                    |   5 +-
> > >  13 files changed, 1546 insertions(+), 1 deletion(-)
> > >  create mode 100644 src/android/camera3_hal.cpp
> > >  create mode 100644 src/android/camera_device.cpp
> > >  create mode 100644 src/android/camera_device.h
> > >  create mode 100644 src/android/camera_hal_manager.cpp
> > >  create mode 100644 src/android/camera_hal_manager.h
> > >  create mode 100644 src/android/camera_proxy.cpp
> > >  create mode 100644 src/android/camera_proxy.h
> > >  create mode 100644 src/android/thread_rpc.cpp
> > >  create mode 100644 src/android/thread_rpc.h
> > >
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 97efc85b4412..2d78b8d91f9c 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -1,3 +1,8 @@
> > > +option('android',
> > > +        type : 'boolean',
> > > +        value : false,
> > > +        description : 'Compile libcamera with Android Camera3 HAL interface')
> > > +
> > >  option('documentation',
> > >          type : 'boolean',
> > >          description : 'Generate the project documentation')
> > > diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> > > new file mode 100644
> > > index 000000000000..8d2629ca356c
> > > --- /dev/null
> > > +++ b/src/android/camera3_hal.cpp
> > > @@ -0,0 +1,111 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera3_hal.cpp - Android Camera HALv3 module
> > > + */
> > > +
> > > +#include <hardware/camera_common.h>
> > > +
> > > +#include "camera_hal_manager.h"
> > > +#include "camera_proxy.h"
> > > +#include "log.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DEFINE_CATEGORY(HAL)
> > > +
> > > +static CameraHalManager cameraManager;
> > > +
> > > +/*------------------------------------------------------------------------------
> > > + * Android Camera HAL callbacks
> > > + */
> > > +
> > > +static int hal_get_number_of_cameras(void)
> > > +{
> > > +	return cameraManager.numCameras();
> > > +}
> > > +
> > > +static int hal_get_camera_info(int id, struct camera_info *info)
> > > +{
> > > +	return cameraManager.getCameraInfo(id, info);
> > > +}
> > > +
> > > +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static int hal_open_legacy(const struct hw_module_t *module, const char *id,
> > > +			   uint32_t halVersion, struct hw_device_t **device)
> > > +{
> > > +	return -ENOSYS;
> > > +}
> > > +
> > > +static int hal_set_torch_mode(const char *camera_id, bool enabled)
> > > +{
> > > +	return -ENOSYS;
> > > +}
> > > +
> > > +/*
> > > + * First entry point of the camera HAL module.
> > > + *
> > > + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)
> > > + */
> > > +static int hal_init()
> > > +{
> > > +	LOG(HAL, Info) << "Initialising Android camera HAL";
> > > +
> > > +	cameraManager.init();
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*------------------------------------------------------------------------------
> > > + * Android Camera Device
> > > + */
> > > +
> > > +static int hal_dev_open(const hw_module_t *module, const char *name,
> > > +			hw_device_t **device)
> > > +{
> > > +	LOG(HAL, Debug) << "Open camera " << name;
> > > +
> > > +	int id = atoi(name);
> > > +	CameraProxy *proxy = cameraManager.open(id, module);
> > > +	if (!proxy) {
> > > +		LOG(HAL, Error)
> > > +			<< "Failed to open camera module '" << id << "'";
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	*device = &proxy->camera3Device()->common;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct hw_module_methods_t hal_module_methods = {
> > > +	.open = hal_dev_open,
> > > +};
> > > +
> > > +camera_module_t HAL_MODULE_INFO_SYM = {
> > > +	.common = {
> > > +		.tag = HARDWARE_MODULE_TAG,
> > > +		.module_api_version = CAMERA_MODULE_API_VERSION_2_4,
> > > +		.hal_api_version = HARDWARE_HAL_API_VERSION,
> > > +		.id = CAMERA_HARDWARE_MODULE_ID,
> > > +		.name = "libcamera camera HALv3 module",
> > > +		.author = "libcamera",
> > > +		.methods = &hal_module_methods,
> > > +		.dso = nullptr,
> > > +		.reserved = {},
> > > +	},
> > > +
> > > +	.get_number_of_cameras = hal_get_number_of_cameras,
> > > +	.get_camera_info = hal_get_camera_info,
> > > +	.set_callbacks = hal_set_callbacks,
> > > +	.get_vendor_tag_ops = nullptr,
> > > +	.open_legacy = hal_open_legacy,
> > > +	.set_torch_mode = hal_set_torch_mode,
> > > +	.init = hal_init,
> > > +	.reserved = {},
> > > +};
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > new file mode 100644
> > > index 000000000000..18bf459a37e1
> > > --- /dev/null
> > > +++ b/src/android/camera_device.cpp
> > > @@ -0,0 +1,817 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_device.cpp - libcamera Android Camera Device
> > > + */
> > > +
> > > +#include "camera_device.h"
> > > +
> > > +#include <memory>
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +
> > > +#include "log.h"
> > > +#include "message.h"
> > > +
> > > +#include "thread_rpc.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL);
> > > +
> > > +/*
> > > + * \class CameraDevice
> > > + *
> > > + * The CameraDevice class wraps a libcamera::Camera instance, and implements
> > > + * the camera_device_t interface by handling RPC requests received from its
> > > + * associated CameraProxy.
> > > + *
> > > + * It translate parameters and operations from Camera HALv3 API to the libcamera
> > > + * ones to provide static informations on a Camera, create request templates
> >
> > s/informations on/information for/
> >
> > > + * for it and process capture requests by then delivering capture results back
> >
> > s/and process/, process/
> > s/by then delevering/, and then deliver/
> >
> > > + * to the framework using the designated callbacks.
> > > + */
> > > +
> > > +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> > > +	: running_(false), camera_(camera), staticMetadata_(nullptr),
> > > +	  requestTemplate_(nullptr)
> > > +{
> > > +	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > > +}
> > > +
> > > +CameraDevice::~CameraDevice()
> > > +{
> > > +	if (staticMetadata_)
> > > +		free_camera_metadata(staticMetadata_);
> > > +	staticMetadata_ = nullptr;
> > > +
> > > +	if (requestTemplate_)
> > > +		free_camera_metadata(requestTemplate_);
> > > +	requestTemplate_ = nullptr;
> > > +}
> > > +
> > > +/*
> > > + * Handle RPC request received from the associated proxy.
> > > + */
> > > +void CameraDevice::message(Message *message)
> > > +{
> > > +	if (message->type() != ThreadRpcMessage::type())
> > > +		return Object::message(message);
> > > +
> > > +	ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message);
> > > +	ThreadRpc *rpc = rpcMessage->rpc;
> > > +
> > > +	switch (rpc->tag) {
> > > +	case ThreadRpc::ProcessCaptureRequest:
> > > +		processCaptureRequest(rpc->request);
> > > +		break;
> > > +	case ThreadRpc::Close:
> > > +		close();
> > > +		break;
> > > +	default:
> > > +		LOG(HAL, Error) << "Unknown RPC operation: " << rpc->tag;
> > > +	}
> > > +
> > > +	rpc->notifyReception();
> > > +}
> > > +
> > > +int CameraDevice::open()
> > > +{
> > > +	int ret = camera_->acquire();
> > > +	if (ret) {
> > > +		LOG(HAL, Error) << "Failed to acquire the camera";
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void CameraDevice::close()
> > > +{
> > > +	camera_->stop();
> > > +
> > > +	camera_->freeBuffers();
> > > +	camera_->release();
> > > +
> > > +	running_ = false;
> > > +}
> > > +
> > > +void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> > > +{
> > > +	callbacks_ = callbacks;
> > > +}
> > > +
> > > +/*
> > > + * Return static informations on the camera.
> >
> > s/informations on/information for/
> >
> > > + */
> > > +camera_metadata_t *CameraDevice::getStaticMetadata()
> > > +{
> > > +	int ret;
> > > +
> > > +	if (staticMetadata_)
> > > +		return staticMetadata_;
> > > +
> > > +	/*
> > > +	 * The here reported metadata are enough to implement a basic capture
> > > +	 * example application, but a real camera implementation will require
> > > +	 * more.
> > > +	 */
> > > +
> > > +	/* \todo Use correct sizes */
> > > +	#define STATIC_ENTRY_CAP 256
> > > +	#define STATIC_DATA_CAP 6688
> > > +	camera_metadata_t *staticMetadata =
> > > +		allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> > > +
> > > +	/* Sensor static metadata. */
> > > +	int32_t pixelArraySize[] = {
> > > +		2592, 1944,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > > +				&pixelArraySize, 2);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t sensorSizes[] = {
> > > +		0, 0, 2560, 1920,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > +				&sensorSizes, 4);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t sensitivityRange[] = {
> > > +		32, 2400,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> > > +				&sensitivityRange, 2);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> > > +				&filterArr, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int64_t exposureTimeRange[] = {
> > > +		100000, 200000000,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> > > +				&exposureTimeRange, 2);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t orientation = 0;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_SENSOR_ORIENTATION,
> > > +				&orientation, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/* Flash static metadata. */
> > > +	char flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_FLASH_INFO_AVAILABLE,
> > > +			&flashAvailable, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/* Lens static metadata. */
> > > +	float fn = 2.53 / 100;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +				ANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/* Control metadata. */
> > > +	char controlMetadata = ANDROID_CONTROL_MODE_AUTO;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_CONTROL_AVAILABLE_MODES,
> > > +			&controlMetadata, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	char availableAntiBandingModes[] = {
> > > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,
> > > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,
> > > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,
> > > +		ANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> > > +			availableAntiBandingModes, 4);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	char aeAvailableModes[] = {
> > > +		ANDROID_CONTROL_AE_MODE_ON,
> > > +		ANDROID_CONTROL_AE_MODE_OFF,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > > +			aeAvailableModes, 2);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	controlMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > +			&controlMetadata, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > +			&awbLockAvailable, 1);
> > > +
> > > +	/* Scaler static metadata. */
> > > +	std::vector<uint32_t> availableStreamFormats = {
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB,
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_SCALER_AVAILABLE_FORMATS,
> > > +			availableStreamFormats.data(),
> > > +			availableStreamFormats.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	std::vector<uint32_t> availableStreamConfigurations = {
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,
> > > +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,
> > > +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,
> > > +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> > > +			availableStreamConfigurations.data(),
> > > +			availableStreamConfigurations.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	std::vector<int64_t> availableStallDurations = {
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> > > +			availableStallDurations.data(),
> > > +			availableStallDurations.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	std::vector<int64_t> minFrameDurations = {
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,
> > > +		ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,
> > > +	};
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> > > +			minFrameDurations.data(), minFrameDurations.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/* Info static metadata. */
> > > +	uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > +	ret = add_camera_metadata_entry(staticMetadata,
> > > +			ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > > +			&supportedHWLevel, 1);
> > > +
> > > +	return staticMetadata;
> > > +}
> > > +
> > > +/*
> > > + * Produce a metadata pack to be used as template for a capture request.
> > > + */
> > > +const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > > +{
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * \todo Inspect type and pick the right metadata pack.
> > > +	 * As of now just use a single one for all templates.
> > > +	 */
> > > +	uint8_t captureIntent;
> > > +	switch (type) {
> > > +	case CAMERA3_TEMPLATE_PREVIEW:
> > > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> > > +		break;
> > > +	case CAMERA3_TEMPLATE_STILL_CAPTURE:
> > > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> > > +		break;
> > > +	case CAMERA3_TEMPLATE_VIDEO_RECORD:
> > > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> > > +		break;
> > > +	case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> > > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> > > +		break;
> > > +	case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> > > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> > > +		break;
> > > +	case CAMERA3_TEMPLATE_MANUAL:
> > > +		captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> > > +		break;
> > > +	default:
> > > +		LOG(HAL, Error) << "Invalid template request type: " << type;
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	if (requestTemplate_)
> > > +		return requestTemplate_;
> > > +
> > > +	/* \todo Use correct sizes */
> > > +	#define REQUEST_TEMPLATE_ENTRIES	  30
> > > +	#define REQUEST_TEMPLATE_DATA		2048
> > > +	requestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,
> > > +						    REQUEST_TEMPLATE_DATA);
> > > +	if (!requestTemplate_) {
> > > +		LOG(HAL, Error) << "Failed to allocate template metadata";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	/* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */
> > > +	int32_t maxOutStream[] = { 0, 2, 0 };
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
> > > +			maxOutStream, 3);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t maxPipelineDepth = 5;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > > +			&maxPipelineDepth, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t inputStreams = 0;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,
> > > +			&inputStreams, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t partialResultCount = 1;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> > > +			&partialResultCount, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t availableCapabilities[] = {
> > > +		ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> > > +	};
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> > > +			availableCapabilities, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AE_MODE,
> > > +			&aeMode, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t aeExposureCompensation = 0;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > > +			&aeExposureCompensation, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > +			&aePrecaptureTrigger, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AE_LOCK,
> > > +			&aeLock, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AF_TRIGGER,
> > > +			&afTrigger, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AWB_MODE,
> > > +			&awbMode, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AWB_LOCK,
> > > +			&awbLock, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > +			&awbLockAvailable, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t flashMode = ANDROID_FLASH_MODE_OFF;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_FLASH_MODE,
> > > +			&flashMode, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > +			&faceDetectMode, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_CONTROL_CAPTURE_INTENT,
> > > +			&captureIntent, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/*
> > > +	 * This is quite hard to list at the moment wihtout knowing what
> > > +	 * we could control.
> > > +	 *
> > > +	 * For now, just list in the available Request keys and in the available
> > > +	 * result keys the control and reporting of the AE algorithm.
> > > +	 */
> > > +	std::vector<int32_t> availableRequestKeys = {
> > > +		ANDROID_CONTROL_AE_MODE,
> > > +		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > > +		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > +		ANDROID_CONTROL_AE_LOCK,
> > > +		ANDROID_CONTROL_AF_TRIGGER,
> > > +		ANDROID_CONTROL_AWB_MODE,
> > > +		ANDROID_CONTROL_AWB_LOCK,
> > > +		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > +		ANDROID_CONTROL_CAPTURE_INTENT,
> > > +		ANDROID_FLASH_MODE,
> > > +		ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > +	};
> > > +
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
> > > +			availableRequestKeys.data(),
> > > +			availableRequestKeys.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	std::vector<int32_t> availableResultKeys = {
> > > +		ANDROID_CONTROL_AE_MODE,
> > > +		ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > > +		ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > +		ANDROID_CONTROL_AE_LOCK,
> > > +		ANDROID_CONTROL_AF_TRIGGER,
> > > +		ANDROID_CONTROL_AWB_MODE,
> > > +		ANDROID_CONTROL_AWB_LOCK,
> > > +		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > +		ANDROID_CONTROL_CAPTURE_INTENT,
> > > +		ANDROID_FLASH_MODE,
> > > +		ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > +	};
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,
> > > +			availableResultKeys.data(),
> > > +			availableResultKeys.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/*
> > > +	 * \todo The available characteristics are be the tags reported
> > > +	 * as part of the static metadata reported at hal_get_camera_info()
> > > +	 * time. As of now, report an empty list.
> > > +	 */
> > > +	std::vector<int32_t> availableCharacteristicsKeys = {};
> > > +	ret = add_camera_metadata_entry(requestTemplate_,
> > > +			ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> > > +			availableCharacteristicsKeys.data(),
> > > +			availableCharacteristicsKeys.size());
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	return requestTemplate_;
> > > +}
> > > +
> > > +/*
> > > + * Inspect the stream_list to produce a list of StreamConfiguration to
> > > + * be use to configure the Camera.
> > > + */
> > > +int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > +{
> > > +
> >
> > Extra blank line.
> >
> > > +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > > +		camera3_stream_t *stream = stream_list->streams[i];
> > > +
> > > +		LOG(HAL, Info) << "Stream #" << i
> > > +			       << ", direction: " << stream->stream_type
> > > +			       << ", width: " << stream->width
> > > +			       << ", height: " << stream->height
> > > +			       << ", format: " << std::hex << stream->format;
> > > +	}
> > > +
> > > +	/* Hardcode viewfinder role, collecting sizes from the stream config. */
> > > +	if (stream_list->num_streams != 1) {
> > > +		LOG(HAL, Error) << "Only one stream supported";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	StreamRoles roles = { StreamRole::Viewfinder };
> > > +	config_ = camera_->generateConfiguration(roles);
> > > +	if (!config_ || config_->empty()) {
> > > +		LOG(HAL, Error) << "Failed to generate camera configuration";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Only one stream is supported. */
> > > +	camera3_stream_t *camera3Stream = stream_list->streams[0];
> > > +	StreamConfiguration *streamConfiguration = &config_->at(0);
> > > +	streamConfiguration->size.width = camera3Stream->width;
> > > +	streamConfiguration->size.height = camera3Stream->height;
> > > +	streamConfiguration->memoryType = ExternalMemory;
> > > +
> > > +	/*
> > > +	 * \todo We'll need to translate from Android defined pixel format codes
> > > +	 * to the libcamera image format codes. As of now, do not change the
> >
> > s/As of/For/
> >
> > > +	 * format returned from Camera::generateConfiguration().
> > > +	 */
> > > +
> > > +	switch (config_->validate()) {
> > > +	case CameraConfiguration::Valid:
> > > +		break;
> > > +	case CameraConfiguration::Adjusted:
> > > +		LOG(HAL, Info) << "Camera configuration adjusted";
> > > +		config_.reset();
> > > +		return -EINVAL;
> > > +	case CameraConfiguration::Invalid:
> > > +		LOG(HAL, Info) << "Camera configuration invalid";
> > > +		config_.reset();
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> > > +
> > > +	/*
> > > +	 * Once the CameraConfiguration has been adjusted/validated
> > > +	 * it can be applied to the camera.
> > > +	 */
> > > +	int ret = camera_->configure(config_.get());
> > > +	if (ret) {
> > > +		LOG(HAL, Error) << "Failed to configure camera '"
> > > +				<< camera_->name() << "'";
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > > +{
> > > +	StreamConfiguration *streamConfiguration = &config_->at(0);
> > > +	Stream *stream = streamConfiguration->stream();
> > > +
> > > +	if (camera3Request->num_output_buffers != 1) {
> > > +		LOG(HAL, Error) << "Invalid number of output buffers: "
> > > +				<< camera3Request->num_output_buffers;
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Start the camera if that's the first request we handle. */
> > > +	if (!running_) {
> > > +		int ret = camera_->allocateBuffers();
> > > +		if (ret) {
> > > +			LOG(HAL, Error) << "Failed to allocate buffers";
> > > +			return ret;
> > > +		}
> > > +
> > > +		ret = camera_->start();
> > > +		if (ret) {
> > > +			LOG(HAL, Error) << "Failed to start camera";
> > > +			camera_->freeBuffers();
> > > +			return ret;
> > > +		}
> > > +
> > > +		running_ = true;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Queue a request for the Camera with the provided dmabuf file
> > > +	 * descriptors.
> > > +	 */
> > > +	const camera3_stream_buffer_t *camera3Buffers =
> > > +					camera3Request->output_buffers;
> > > +
> > > +	/*
> > > +	 * Save the request descriptors for use at completion time.
> > > +	 * The descriptor and the associated memory reserved here are freed
> > > +	 * at request complete time.
> > > +	 */
> > > +	Camera3RequestDescriptor *descriptor = new Camera3RequestDescriptor();
> > > +	descriptor->frameNumber = camera3Request->frame_number;
> > > +	descriptor->numBuffers = camera3Request->num_output_buffers;
> > > +	descriptor->buffers = new camera3_stream_buffer_t[descriptor->numBuffers];
> > > +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > > +		/*
> > > +		 * Keep track of which stream the request belong and of the
> > > +		 * native buffer handles.
> > > +		 *
> > > +		 * \todo Currently we only support one capture buffer. Copy
> > > +		 * all of them to be ready once we'll support more.
> > > +		 */
> > > +		descriptor->buffers[i].stream = camera3Buffers[i].stream;
> > > +		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Create a libcamera buffer using the dmabuf descriptors of the first
> > > +	 * and (currently) only supported request buffer.
> > > +	 */
> > > +	const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer;
> > > +	std::array<int, 3> fds = {
> > > +		camera3Handle->data[0],
> > > +		camera3Handle->data[1],
> > > +		camera3Handle->data[2],
> > > +	};
> > > +
> > > +	std::unique_ptr<Buffer> buffer = stream->createBuffer(fds);
> > > +	if (!buffer) {
> > > +		LOG(HAL, Error) << "Failed to create buffer";
> > > +		return -EINVAL;
> >
> > You're leaking descriptor and descriptor->buffers.
> 
> Are you sure? I delete them at the time a request completes. See
> below, you have commented on it.
> 
> But now I get you meant I'm leaking them in the error path! I
> re-ordered this sequence for cosmetic reasons and forgot to handle
> this! thanks

Yes that's what I meant :-)

> > I would implement a constructor for Camera3RequestDescriptor that takes
> > the number of buffers and allocate the buffers member, and a destructor
> > that deletes the buffers member. Then you will only need to care about
> > deleting descriptor here.
> 
> Good idea
> 
> > > +	}
> > > +
> > > +	Request *request =
> > > +		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > > +	request->addBuffer(std::move(buffer));
> > > +
> > > +	int ret = camera_->queueRequest(request);
> > > +	if (ret) {
> > > +		LOG(HAL, Error) << "Failed to queue request";
> > > +		return ret;
> >
> > You're leaking request, descriptor and descriptor->buffers.
> >
> > I wonder if Camera::createRequest() shouldn't return an std::unique_ptr
> > (this doesn't need to be addressed in this patch series).
> >
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void CameraDevice::requestComplete(Request *request,
> > > +				   const std::map<Stream *, Buffer *> &buffers)
> > > +{
> > > +	Buffer *libcameraBuffer = buffers.begin()->second;
> > > +	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> > > +	camera_metadata_t *resultMetadata = nullptr;
> > > +
> > > +	if (request->status() != Request::RequestComplete) {
> > > +		LOG(HAL, Error) << "Request not succesfully completed: "
> > > +				<< request->status();
> > > +		status = CAMERA3_BUFFER_STATUS_ERROR;
> > > +	}
> > > +
> > > +	/* Prepare to call back the Android camera stack. */
> > > +	Camera3RequestDescriptor *descriptor =
> > > +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> > > +
> > > +	camera3_capture_result_t captureResult = {};
> > > +	captureResult.frame_number = descriptor->frameNumber;
> > > +	captureResult.num_output_buffers = descriptor->numBuffers;
> > > +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > > +		/*
> > > +		 * The buffers in the descriptor have been set up at queue
> > > +		 * request time.
> >
> > I think you can drop this sentence.
> >
> > > +		 *
> > > +		 * \todo Currently we only support one capture buffer. Copy
> > > +		 * all of them to be ready once we'll support more.
> > > +		 */
> > > +		descriptor->buffers[i].acquire_fence = -1;
> > > +		descriptor->buffers[i].release_fence = -1;
> > > +		descriptor->buffers[i].status = status;
> > > +	}
> > > +	captureResult.output_buffers =
> > > +		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
> > > +
> > > +	if (status == CAMERA3_BUFFER_STATUS_ERROR) {
> > > +		/* \todo Improve error handling. */
> > > +		notifyError(descriptor->frameNumber,
> > > +			    descriptor->buffers[0].stream);
> > > +	} else {
> > > +		notifyShutter(descriptor->frameNumber,
> > > +			      libcameraBuffer->timestamp());
> > > +
> > > +		captureResult.partial_result = 1;
> > > +		resultMetadata = getResultMetadata(descriptor->frameNumber,
> > > +						   libcameraBuffer->timestamp());
> > > +		captureResult.result = resultMetadata;
> > > +	}
> > > +
> > > +	callbacks_->process_capture_result(callbacks_, &captureResult);
> > > +
> > > +	delete[] descriptor->buffers;
> > > +	delete descriptor;
> >
> > This will also be simplified with a destructor for the
> > Camera3RequestDescriptor class.
> >
> > > +	if (resultMetadata)
> > > +		free_camera_metadata(resultMetadata);
> > > +
> > > +	return;
> > > +}
> > > +
> > > +void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> > > +{
> > > +	camera3_notify_msg_t notify = {};
> > > +
> > > +	notify.type = CAMERA3_MSG_SHUTTER;
> > > +	notify.message.shutter.frame_number = frameNumber;
> > > +	notify.message.shutter.timestamp = timestamp;
> > > +
> > > +	callbacks_->notify(callbacks_, &notify);
> > > +}
> > > +
> > > +void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> > > +{
> > > +	camera3_notify_msg_t notify = {};
> > > +
> > > +	notify.type = CAMERA3_MSG_ERROR;
> > > +	notify.message.error.error_stream = stream;
> > > +	notify.message.error.frame_number = frameNumber;
> > > +	notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> > > +
> > > +	callbacks_->notify(callbacks_, &notify);
> > > +}
> > > +
> > > +/*
> > > + * Produce a set of fixed result metadata.
> > > + */
> > > +camera_metadata_t *CameraDevice::getResultMetadata(int frame_number,
> > > +						   int64_t timestamp)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* \todo Use correct sizes */
> > > +	#define RESULT_ENTRY_CAP 256
> > > +	#define RESULT_DATA_CAP 6688
> > > +	camera_metadata_t *resultMetadata =
> > > +		allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> > > +
> > > +	const uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > > +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,
> > > +					&ae_state, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
> > > +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,
> > > +					&ae_lock, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	uint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;
> > > +	ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,
> > > +					&af_state, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	const uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_CONTROL_AWB_STATE,
> > > +					&awb_state, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	const uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_CONTROL_AWB_LOCK,
> > > +					&awb_lock, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	const uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_LENS_STATE,
> > > +					&lens_state, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	int32_t sensorSizes[] = {
> > > +		0, 0, 2560, 1920,
> > > +	};
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_SCALER_CROP_REGION,
> > > +					sensorSizes, 4);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_SENSOR_TIMESTAMP,
> > > +					&timestamp, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/* 33.3 msec */
> > > +	const int64_t rolling_shutter_skew = 33300000;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
> > > +					&rolling_shutter_skew, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	/* 16.6 msec */
> > > +	const int64_t exposure_time = 16600000;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_SENSOR_EXPOSURE_TIME,
> > > +					&exposure_time, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	const uint8_t lens_shading_map_mode =
> > > +				ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,
> > > +					&lens_shading_map_mode, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	const uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;
> > > +	ret = add_camera_metadata_entry(resultMetadata,
> > > +					ANDROID_STATISTICS_SCENE_FLICKER,
> > > +					&scene_flicker, 1);
> > > +	METADATA_ASSERT(ret);
> > > +
> > > +	return resultMetadata;
> > > +}
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > new file mode 100644
> > > index 000000000000..402c3dca69da
> > > --- /dev/null
> > > +++ b/src/android/camera_device.h
> > > @@ -0,0 +1,63 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_device.h - libcamera Android Camera Device
> > > + */
> > > +#ifndef __ANDROID_CAMERA_DEVICE_H__
> > > +#define __ANDROID_CAMERA_DEVICE_H__
> > > +
> > > +#include <memory>
> > > +
> > > +#include <hardware/camera3.h>
> > > +
> > > +#include <libcamera/libcamera.h>
> >
> > Just the required headers please :-)
> >
> > > +
> > > +#include "message.h"
> > > +
> > > +#define METADATA_ASSERT(_r)		\
> > > +	do {				\
> > > +		if (!(_r)) break;	\
> > > +		LOG(HAL, Error) << "Error: " << __func__ << ":" << __LINE__; \
> > > +		return nullptr;		\
> > > +	} while(0);
> > > +
> > > +class CameraDevice : public libcamera::Object
> > > +{
> > > +public:
> > > +	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> >
> > You can make the second argument a reference to avoid an unnecessary
> > copy.
> 
> Isn't there some copy elision magic that helps here? The temporary
> argument instance is created and destroyed immediately... I can indeed
> use a reference though

As far as I can tell, copy elision only applies to return values.

> > > +	~CameraDevice();
> > > +
> > > +	void message(libcamera::Message *message);
> > > +
> > > +	int open();
> > > +	void close();
> > > +	void setCallbacks(const camera3_callback_ops_t *callbacks);
> > > +	camera_metadata_t *getStaticMetadata();
> > > +	const camera_metadata_t *constructDefaultRequestSettings(int type);
> > > +	int configureStreams(camera3_stream_configuration_t *stream_list);
> > > +	int processCaptureRequest(camera3_capture_request_t *request);
> > > +	void requestComplete(libcamera::Request *request,
> > > +			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
> > > +
> > > +private:
> > > +	struct Camera3RequestDescriptor {
> > > +		uint32_t frameNumber;
> > > +		uint32_t numBuffers;
> > > +		camera3_stream_buffer_t *buffers;
> > > +	};
> > > +
> > > +	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > > +	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > > +	camera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);
> > > +
> > > +	bool running_;
> > > +	std::shared_ptr<libcamera::Camera> camera_;
> > > +	std::unique_ptr<libcamera::CameraConfiguration> config_;
> > > +
> > > +	camera_metadata_t *staticMetadata_;
> > > +	camera_metadata_t *requestTemplate_;
> > > +	const camera3_callback_ops_t *callbacks_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_DEVICE_H__ */
> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > > new file mode 100644
> > > index 000000000000..0b4d50368ff8
> > > --- /dev/null
> > > +++ b/src/android/camera_hal_manager.cpp
> > > @@ -0,0 +1,142 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_hal_manager.cpp - libcamera Android Camera Manager
> > > + */
> > > +
> > > +#include "camera_hal_manager.h"
> > > +
> > > +#include <hardware/hardware.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +#include "camera_device.h"
> > > +#include "camera_proxy.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL);
> > > +
> > > +/*
> > > + * \class CameraHalManager
> > > + *
> > > + * The HAL camera manager is initializated at camera_module_t 'hal_init()' time
> > > + * and spawns its own thread where libcamera related events are dispatched to.
> > > + * It wraps the libcamera CameraManager operations and provides helpers for
> >
> > s/for/for the/
> >
> > > + * camera_module_t operations, to retrieve the number of cameras in the system
> >
> > s/system/system, to retrieve /
> >
> > > + * their static informations and to open and close camera devices.
> >
> > s/informations/information/
> >
> > > + */
> > > +
> > > +int CameraHalManager::init()
> > > +{
> > > +	/*
> > > +	 * Start the camera HAL manager thread and wait until its
> > > +	 * initialisation to complete to be fully operational before
> >
> > s/to complete/completes/
> >
> > > +	 * receiving calls from the camera stack.
> > > +	 */
> > > +	start();
> > > +
> > > +	MutexLocker locker(mutex_);
> > > +	cv_.wait(locker);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void CameraHalManager::run()
> > > +{
> > > +	/*
> > > +	 * All the libcamera components must be initialised here, in
> > > +	 * order to bind them to the camera HAL manager thread that
> > > +	 * executes the event dispatcher.
> > > +	 */
> > > +	cameraManager_ = libcamera::CameraManager::instance();
> > > +
> > > +	int ret = cameraManager_->start();
> > > +	if (ret) {
> > > +		LOG(HAL, Error) << "Failed to start camera manager: "
> > > +				<< strerror(-ret);
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * For each Camera registered in the system, a CameraProxy
> > > +	 * gets created here wich wraps a camera device.
> >
> > s/wich wraps/to wrap/
> >
> > > +	 *
> > > +	 * \todo Support camera hotplug.
> > > +	 */
> > > +	unsigned int index = 0;
> > > +	for (auto &camera : cameraManager_->cameras()) {
> > > +		CameraProxy *proxy = new CameraProxy(index, camera);
> > > +		proxies_.emplace_back(proxy);
> > > +
> > > +		++index;
> > > +	}
> > > +
> > > +	/*
> > > +	 * libcamera has been initialized. Unlock the init() caller
> > > +	 * as we're now ready to handle calls from the framework.
> > > +	 */
> > > +	cv_.notify_one();
> > > +
> > > +	/* Now start processing events and messages. */
> > > +	exec();
> > > +}
> > > +
> > > +CameraProxy *CameraHalManager::open(unsigned int id,
> > > +				    const hw_module_t *hardwareModule)
> > > +{
> > > +	if (id < 0 || id >= numCameras()) {
> > > +		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	CameraProxy *proxy = proxies_[id].get();
> > > +	if (proxy->open(hardwareModule))
> > > +		return nullptr;
> > > +
> > > +	LOG(HAL, Info) << "Open camera '" << id << "'";
> > > +
> > > +	return proxy;
> > > +}
> > > +
> > > +int CameraHalManager::close(CameraProxy *proxy)
> > > +{
> > > +	proxy->close();
> > > +	LOG(HAL, Info) << "Close camera '" << proxy->id() << "'";
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +unsigned int CameraHalManager::numCameras() const
> > > +{
> > > +	return cameraManager_->cameras().size();
> > > +}
> > > +
> > > +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)
> > > +{
> > > +	if (!info)
> > > +		return -EINVAL;
> > > +
> > > +	int cameras = numCameras();
> > > +	if (id >= cameras || id < 0) {
> >
> > s/cameras/numCameras()/
> >
> > and drop the local variable.
> >
> > > +		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	CameraProxy *proxy = proxies_[id].get();
> > > +
> > > +	/* \todo Get these info dynamically inspecting the camera module. */
> > > +	info->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;
> > > +	info->orientation = 0;
> > > +	info->device_version = 0;
> > > +	info->resource_cost = 0;
> > > +	info->static_camera_characteristics = proxy->getStaticMetadata();
> > > +	info->conflicting_devices = nullptr;
> > > +	info->conflicting_devices_length = 0;
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > > new file mode 100644
> > > index 000000000000..8004aaf660f5
> > > --- /dev/null
> > > +++ b/src/android/camera_hal_manager.h
> > > @@ -0,0 +1,47 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_hal_manager.h - libcamera Android Camera Manager
> > > + */
> > > +#ifndef __ANDROID_CAMERA_MANAGER_H__
> > > +#define __ANDROID_CAMERA_MANAGER_H__
> > > +
> > > +#include <condition_variable>
> > > +#include <mutex>
> > > +#include <vector>
> > > +
> > > +#include <hardware/hardware.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <libcamera/camera_manager.h>
> > > +
> > > +#include "thread.h"
> > > +
> > > +class CameraDevice;
> > > +class CameraProxy;
> > > +
> > > +class CameraHalManager : public libcamera::Thread
> > > +{
> > > +public:
> > > +	int init();
> > > +
> > > +	CameraProxy *open(unsigned int id, const hw_module_t *module);
> > > +	int close(CameraProxy *proxy);
> > > +
> > > +	unsigned int numCameras() const;
> > > +	int getCameraInfo(int id, struct camera_info *info);
> > > +
> > > +private:
> > > +	void run() override;
> > > +	camera_metadata_t *getStaticMetadata(unsigned int id);
> > > +
> > > +	libcamera::CameraManager *cameraManager_;
> > > +
> > > +	std::mutex mutex_;
> > > +	std::condition_variable cv_;
> > > +
> > > +	std::vector<std::unique_ptr<CameraProxy>> proxies_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_MANAGER_H__ */
> > > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp
> > > new file mode 100644
> > > index 000000000000..23c33c8f6c9c
> > > --- /dev/null
> > > +++ b/src/android/camera_proxy.cpp
> > > @@ -0,0 +1,199 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_proxy.cpp - Proxy to camera devices
> > > + */
> > > +
> > > +#include "camera_proxy.h"
> > > +
> > > +#include <memory>
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +
> > > +#include "log.h"
> > > +#include "message.h"
> > > +#include "utils.h"
> > > +
> > > +#include "camera_device.h"
> > > +#include "thread_rpc.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL);
> > > +
> > > +/*
> > > + * \class CameraProxy
> > > + *
> > > + * The CameraProxy wraps a CameraDevice and implements the camera3_device_t
> > > + * API, bridging calls received from the camera framework to the CameraDevice.
> > > + *
> > > + * Bridging operation calls between the framework and the CameraDevice is
> > > + * required as the two runs in two different threads and certain operations,
> >
> > s/runs/run/
> >
> > > + * such as queueing a new capture request to the camera, require to interrupt
> > > + * and restart the even dispatches associated with the thread where the
> >
> > s/even dispatches/event dispatcher/
> >
> > It's not about interrupting the event dispatcher, but about being called
> > in the same thread than the one that handles events.
> 
> I was actually thinking about the -first- capture request, that
> by starting the video device, updates the list of file descriptors to
> monitor and requires the even notifier to be interrupted and
> restarted. In general, you're correct and I'll change this.
> 
> > "such as queueing a new capture request to the camera, shall be called
> > in the thread that dispatches events."
> >
> > > + * CameraDevice is running. Other operations do not require any bridging and
> > > + * resolve to direct function calls on the CameraDevice instance instead.
> > > + */
> > > +
> > > +static int hal_dev_initialize(const struct camera3_device *dev,
> > > +			      const camera3_callback_ops_t *callback_ops)
> > > +{
> > > +	if (!dev)
> > > +		return -EINVAL;
> > > +
> > > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > +	proxy->initialize(callback_ops);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int hal_dev_configure_streams(const struct camera3_device *dev,
> > > +				     camera3_stream_configuration_t *stream_list)
> > > +{
> > > +	if (!dev)
> > > +		return -EINVAL;
> > > +
> > > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > +	return proxy->configureStreams(stream_list);
> > > +}
> > > +
> > > +static const camera_metadata_t *
> > > +hal_dev_construct_default_request_settings(const struct camera3_device *dev,
> > > +					   int type)
> > > +{
> > > +	if (!dev)
> > > +		return nullptr;
> > > +
> > > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > +	return proxy->constructDefaultRequestSettings(type);
> > > +}
> > > +
> > > +static int hal_dev_process_capture_request(const struct camera3_device *dev,
> > > +					   camera3_capture_request_t *request)
> > > +{
> > > +	if (!dev)
> > > +		return -EINVAL;
> > > +
> > > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > +	return proxy->processCaptureRequest(request);
> > > +}
> > > +
> > > +static void hal_dev_dump(const struct camera3_device *dev, int fd)
> > > +{
> > > +}
> > > +
> > > +static int hal_dev_flush(const struct camera3_device *dev)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static int hal_dev_close(hw_device_t *hw_device)
> > > +{
> > > +	if (!hw_device)
> > > +		return -EINVAL;
> > > +
> > > +	camera3_device_t *dev = reinterpret_cast<camera3_device_t *>(hw_device);
> > > +	CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > +
> > > +	proxy->close();
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static camera3_device_ops hal_dev_ops = {
> > > +	.initialize = hal_dev_initialize,
> > > +	.configure_streams = hal_dev_configure_streams,
> > > +	.register_stream_buffers = nullptr,
> > > +	.construct_default_request_settings = hal_dev_construct_default_request_settings,
> > > +	.process_capture_request = hal_dev_process_capture_request,
> > > +	.get_metadata_vendor_tag_ops = nullptr,
> > > +	.dump = hal_dev_dump,
> > > +	.flush = hal_dev_flush,
> > > +	.reserved = { nullptr },
> > > +};
> > > +
> > > +CameraProxy::CameraProxy(unsigned int id, std::shared_ptr<Camera> camera)
> > > +	: id_(id)
> > > +{
> > > +	cameraDevice_ = new CameraDevice(id, camera);
> > > +}
> > > +
> > > +CameraProxy::~CameraProxy()
> > > +{
> > > +	delete cameraDevice_;
> > > +}
> > > +
> > > +int CameraProxy::open(const hw_module_t *hardwareModule)
> > > +{
> > > +	int ret = cameraDevice_->open();
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Initialize the hw_device_t in the instance camera3_module_t. */
> > > +	camera3Device_.common.tag = HARDWARE_DEVICE_TAG;
> > > +	camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3;
> > > +	camera3Device_.common.module = (hw_module_t *)hardwareModule;
> > > +	camera3Device_.common.close = hal_dev_close;
> > > +
> > > +	/*
> > > +	 * The camera device operations. These actually implement
> > > +	 * the Android Camera HALv3 interface.
> > > +	 */
> > > +	camera3Device_.ops = &hal_dev_ops;
> > > +	camera3Device_.priv = this;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void CameraProxy::close()
> > > +{
> > > +	ThreadRpc rpcRequest;
> > > +	rpcRequest.tag = ThreadRpc::Close;
> > > +
> > > +	threadRpcCall(rpcRequest);
> > > +}
> > > +
> > > +void CameraProxy::initialize(const camera3_callback_ops_t *callbacks)
> > > +{
> > > +	cameraDevice_->setCallbacks(callbacks);
> > > +}
> > > +
> > > +const camera_metadata_t *CameraProxy::getStaticMetadata()
> > > +{
> > > +	return cameraDevice_->getStaticMetadata();
> > > +}
> > > +
> > > +const camera_metadata_t *CameraProxy::constructDefaultRequestSettings(int type)
> > > +{
> > > +	return cameraDevice_->constructDefaultRequestSettings(type);
> > > +}
> > > +
> > > +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)
> > > +{
> > > +	return cameraDevice_->configureStreams(stream_list);
> > > +}
> > > +
> > > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
> > > +{
> > > +	ThreadRpc rpcRequest;
> > > +	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> > > +	rpcRequest.request = request;
> > > +
> > > +	threadRpcCall(rpcRequest);
> > > +
> > > +	return 0;
> > > +}
> >
> > Repeating my question for the previous version, does this method need to
> > be synchronous ? We can keep it as-is for now, but I wonder if it
> > wouldn't make sense to later move (part of) the validation code here and
> > make the call asynchronous.
> 
> Are you suggesting to drop the wait on the delivery condition in the
> rpc call? How else would you make it asynchronous?

Yes, that's my question. Does it have to wait, or can
processCaptureRequest return before the message is delivered on the
other side ?

> > > +
> > > +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)
> > > +{
> > > +	std::unique_ptr<ThreadRpcMessage> message =
> > > +				utils::make_unique<ThreadRpcMessage>();
> > > +	message->rpc = &rpcRequest;
> > > +
> > > +	cameraDevice_->postMessage(std::move(message));
> > > +	rpcRequest.waitDelivery();
> > > +}
> > > diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h
> > > new file mode 100644
> > > index 000000000000..da63bfa79fc9
> > > --- /dev/null
> > > +++ b/src/android/camera_proxy.h
> > > @@ -0,0 +1,45 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_proxy.h - Proxy to camera devices
> > > + */
> > > +#ifndef __ANDROID_CAMERA_PROXY_H__
> > > +#define __ANDROID_CAMERA_PROXY_H__
> > > +
> > > +#include <memory>
> > > +
> > > +#include <hardware/camera3.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +
> > > +class CameraDevice;
> > > +class ThreadRpc;
> > > +
> > > +class CameraProxy
> > > +{
> > > +public:
> > > +	CameraProxy(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> > > +	~CameraProxy();
> > > +
> > > +	int open(const hw_module_t *hardwareModule);
> > > +	void close();
> > > +
> > > +	void initialize(const camera3_callback_ops_t *callbacks);
> > > +	const camera_metadata_t *getStaticMetadata();
> > > +	const camera_metadata_t *constructDefaultRequestSettings(int type);
> > > +	int configureStreams(camera3_stream_configuration_t *stream_list);
> > > +	int processCaptureRequest(camera3_capture_request_t *request);
> > > +
> > > +	unsigned int id() const { return id_; }
> > > +	camera3_device_t *camera3Device() { return &camera3Device_; }
> > > +
> > > +private:
> > > +	void threadRpcCall(ThreadRpc &rpcRequest);
> > > +
> > > +	unsigned int id_;
> > > +	CameraDevice *cameraDevice_;
> > > +	camera3_device_t camera3Device_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_PROXY_H__ */
> > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > index 1f242953db37..26537794bc29 100644
> > > --- a/src/android/meson.build
> > > +++ b/src/android/meson.build
> > > @@ -1,3 +1,11 @@
> > > +android_hal_sources = files([
> > > +    'camera3_hal.cpp',
> > > +    'camera_hal_manager.cpp',
> > > +    'camera_device.cpp',
> > > +    'camera_proxy.cpp',
> > > +    'thread_rpc.cpp'
> > > +])
> > > +
> > >  android_camera_metadata_sources = files([
> > >      'metadata/camera_metadata.c',
> > >  ])
> > > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp
> > > new file mode 100644
> > > index 000000000000..295a05d7c676
> > > --- /dev/null
> > > +++ b/src/android/thread_rpc.cpp
> > > @@ -0,0 +1,42 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * thread_rpc.cpp - Inter-thread procedure call
> > > + */
> > > +
> > > +#include "thread_rpc.h"
> > > +
> > > +#include "message.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;
> > > +
> > > +ThreadRpcMessage::ThreadRpcMessage()
> > > +	: Message(type())
> > > +{
> > > +}
> > > +
> > > +void ThreadRpc::notifyReception()
> > > +{
> > > +	{
> > > +		libcamera::MutexLocker locker(mutex_);
> > > +		delivered_ = true;
> > > +	}
> > > +	cv_.notify_one();
> > > +}
> > > +
> > > +void ThreadRpc::waitDelivery()
> > > +{
> > > +	libcamera::MutexLocker locker(mutex_);
> > > +	cv_.wait(locker, [&] { return delivered_; });
> > > +}
> > > +
> > > +Message::Type ThreadRpcMessage::type()
> > > +{
> > > +	if (ThreadRpcMessage::rpcType_ == Message::Type::None)
> > > +		rpcType_ = Message::registerMessageType();
> > > +
> > > +	return rpcType_;
> > > +}
> > > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h
> > > new file mode 100644
> > > index 000000000000..6d8992839d0b
> > > --- /dev/null
> > > +++ b/src/android/thread_rpc.h
> > > @@ -0,0 +1,54 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * thread_rpc.h - Inter-thread procedure call
> > > + */
> > > +#ifndef __ANDROID_THREAD_RPC_H__
> > > +#define __ANDROID_THREAD_RPC_H__
> > > +
> > > +#include <condition_variable>
> > > +#include <mutex>
> > > +
> > > +#include <hardware/camera3.h>
> > > +
> > > +#include "message.h"
> > > +#include "thread.h"
> > > +
> > > +class ThreadRpc
> > > +{
> > > +public:
> > > +	enum RpcTag {
> > > +		ProcessCaptureRequest,
> > > +		Close,
> > > +	};
> > > +
> > > +	ThreadRpc()
> > > +		: delivered_(false) {}
> > > +
> > > +	void notifyReception();
> > > +	void waitDelivery();
> > > +
> > > +	RpcTag tag;
> > > +
> > > +	camera3_capture_request_t *request;
> > > +
> > > +private:
> > > +	bool delivered_;
> > > +	std::mutex mutex_;
> > > +	std::condition_variable cv_;
> > > +};
> > > +
> > > +class ThreadRpcMessage : public libcamera::Message
> > > +{
> > > +public:
> > > +	ThreadRpcMessage();
> > > +	ThreadRpc *rpc;
> > > +
> > > +	static Message::Type type();
> > > +
> > > +private:
> > > +	static libcamera::Message::Type rpcType_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_THREAD_RPC_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index a09b23d60022..7d5d3c04fba0 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -103,9 +103,18 @@ libcamera_deps = [
> > >      dependency('threads'),
> > >  ]
> > >
> > > +libcamera_link_with = []
> > > +
> > > +if get_option('android')
> > > +    libcamera_sources += android_hal_sources
> > > +    includes += android_includes
> > > +    libcamera_link_with += android_camera_metadata
> > > +endif
> > > +
> > >  libcamera = shared_library('camera',
> > >                             libcamera_sources,
> > >                             install : true,
> > > +                           link_with : libcamera_link_with,
> > >                             include_directories : includes,
> > >                             dependencies : libcamera_deps)
> > >
> > > diff --git a/src/meson.build b/src/meson.build
> > > index 7148baee3eda..67ad20aab86b 100644
> > > --- a/src/meson.build
> > > +++ b/src/meson.build
> > > @@ -1,4 +1,7 @@
> > > -subdir('android')
> > > +if get_option('android')
> > > +    subdir('android')
> > > +endif
> > > +
> > >  subdir('libcamera')
> > >  subdir('ipa')
> > >  subdir('cam')

Hi Jacopo,

On Mon, Aug 12, 2019 at 11:21:02AM +0200, Jacopo Mondi wrote:
> Hi Laurent, one additiona note
> 
> On Sat, Aug 10, 2019 at 07:51:51PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> 
> [snip]
> 
> > > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
> > > +{
> > > +	ThreadRpc rpcRequest;
> > > +	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> > > +	rpcRequest.request = request;
> > > +
> > > +	threadRpcCall(rpcRequest);
> > > +
> > > +	return 0;
> > > +}
> >
> > Repeating my question for the previous version, does this method need to
> > be synchronous ? We can keep it as-is for now, but I wonder if it
> > wouldn't make sense to later move (part of) the validation code here and
> > make the call asynchronous.
> 
> Quoting the camera3.h header description of the expected operation
> sequence:
> 
>  * 7. The framework constructs and sends the first capture request to the HAL,
>  *    with settings based on one of the sets of default settings, and with at
>  *    least one output stream, which has been registered earlier by the
>  *    framework. This is sent to the HAL with
>  *    camera3_device_t->ops->process_capture_request(). The HAL must block the
>  *    return of this call until it is ready for the next request to be sent.
> 
> The last statement makes me wonder if making the method asynchronous
> would not then require serializin the requests on our side, as if we
> return earlier, there might be a chance (no idea how large the
> windonw could be) we receive a new capture request while the previous
> one still has to be handled.
> 
> Consider this, I'm not sure it's a good idea to make this call
> asynchronous.

As the calls are queued and processed in order in our HAL implementation
I think we could still make it asynchronous, but it may not be worth it
indeed. Let's just keep it in mind for now.