[{"id":2332,"web_url":"https://patchwork.libcamera.org/comment/2332/","msgid":"<20190807153208.GG5048@pendragon.ideasonboard.com>","date":"2019-08-07T15:32:08","subject":"Re: [libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Aug 06, 2019 at 09:55:18PM +0200, Jacopo Mondi wrote:\n> Add libcamera Android Camera HALv3 implementation.\n> \n> The initial camera HAL implementation supports the LIMITED hardware\n> level and uses statically defined metadata and camera characteristics.\n> \n> Add a build option named 'android' and adjust the build system to\n> selectively compile the Android camera HAL and link it against the\n> required Android libraries.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> meson_options.txt | 5 +\n> src/android/camera3_hal.cpp | 130 +++++\n> src/android/camera_hal_manager.cpp | 173 +++++++\n> src/android/camera_hal_manager.h | 56 ++\n> src/android/camera_module.cpp | 799 +++++++++++++++++++++++++++++\n> src/android/camera_module.h | 75 +++\n> src/android/camera_proxy.cpp | 181 +++++++\n> src/android/camera_proxy.h | 41 ++\n> src/android/meson.build | 8 +\n> src/android/thread_rpc.cpp | 41 ++\n> src/android/thread_rpc.h | 56 ++\n> src/libcamera/meson.build | 9 +\n> src/meson.build | 5 +-\n> 13 files changed, 1578 insertions(+), 1 deletion(-)\n> create mode 100644 src/android/camera3_hal.cpp\n> create mode 100644 src/android/camera_hal_manager.cpp\n> create mode 100644 src/android/camera_hal_manager.h\n> create mode 100644 src/android/camera_module.cpp\n> create mode 100644 src/android/camera_module.h\n> create mode 100644 src/android/camera_proxy.cpp\n> create mode 100644 src/android/camera_proxy.h\n> create mode 100644 src/android/thread_rpc.cpp\n> create mode 100644 src/android/thread_rpc.h\n\n[snip]\n\n> diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp\n> new file mode 100644\n> index 000000000000..0a97a9333d20\n> --- /dev/null\n> +++ b/src/android/camera3_hal.cpp\n> @@ -0,0 +1,130 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera3_hal.cpp - Android Camera3 HAL module\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n\nDo you need camera_metadata.h ? Isn't it enough to include\ncamera_common.h ? I think hardware.h can also be dropped.\n\n> +\n> +#include \n\nTo speed up compilation, it would be better to include only the header\nfiles we need (here and everywhere else).\n\n> +\n> +#include \"log.h\"\n> +\n> +#include \"camera_hal_manager.h\"\n> +#include \"camera_module.h\"\n> +#include \"camera_proxy.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(HAL)\n> +\n> +static CameraHalManager cameraManager;\n> +\n> +/*------------------------------------------------------------------------------\n> + * Android Camera HAL callbacks\n> + */\n> +\n> +static int hal_get_number_of_cameras(void)\n> +{\n> +\treturn cameraManager.numCameras();\n> +}\n> +\n> +static int hal_get_camera_info(int id, struct camera_info *info)\n> +{\n> +\treturn cameraManager.getCameraInfo(id, info);\n> +}\n> +\n> +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +static int hal_open_legacy(const struct hw_module_t *module, const char *id,\n> +\t\t\t uint32_t halVersion, struct hw_device_t **device)\n> +{\n> +\treturn -ENOSYS;\n> +}\n> +\n> +static int hal_set_torch_mode(const char *camera_id, bool enabled)\n> +{\n> +\treturn -ENOSYS;\n> +}\n> +\n> +/*\n> + * First entry point of the camera HAL module.\n> + *\n> + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)\n> + */\n> +static int hal_init()\n> +{\n> +\tLOG(HAL, Info) << \"Initialising Android camera HAL\";\n> +\n> +\tcameraManager.initialize();\n> +\n> +\treturn 0;\n> +}\n> +\n> +/*------------------------------------------------------------------------------\n> + * Android Camera Device\n> + */\n> +\n> +static int hal_dev_close(hw_device_t *hw_device)\n> +{\n> +\tif (!hw_device)\n> +\t\treturn -EINVAL;\n> +\n> +\tcamera3_device_t *dev = reinterpret_cast(hw_device);\n> +\tCameraProxy *cameraModule = reinterpret_cast(dev->priv);\n\ns/cameraModule/proxy/ otherwise it's very confusing to have a\nCameraProxy instance called cameraModule when there's a CameraModule\nclass.\n\nThis comment applies to all other similar instances.\n\n> +\n> +\treturn cameraManager.close(cameraModule);\n\nAs cameraManager.close() only calls proxy->close(), how about defining\nthe hal_dev_close() method as a static method of CameraProxy, and\nsetting proxy->device()->common.close inside the CameraProxy constructor\n? It would avoid touching the internals of the CameraProxy in\nhal_dev_open() below.\n\n> +}\n> +\n> +static int hal_dev_open(const hw_module_t* module, const char* name,\n> +\t\t\thw_device_t** device)\n> +{\n> +\tLOG(HAL, Debug) << \"Open camera: \" << name;\n\ns/camera:/camera/\n\n> +\n> +\tint id = atoi(name);\n> +\tCameraProxy *proxy = cameraManager.open(id, module);\n> +\tif (!proxy) {\n> +\t\tLOG(HAL, Error) << \"Failed to open camera module \" << id;\n> +\t\treturn -ENODEV;\n> +\t}\n> +\tproxy->device()->common.close = hal_dev_close;\n> +\t*device = &proxy->device()->common;\n> +\n> +\treturn 0;\n> +}\n\nWould it make sense to define all these methods as static methods of the\nCameraHalManager class, and turn the class into a singleton with an\ninstance() method ?\n\n> +\n> +static struct hw_module_methods_t hal_module_methods = {\n> +\t.open = hal_dev_open,\n> +};\n> +\n> +camera_module_t HAL_MODULE_INFO_SYM = {\n> +\t.common = {\n> +\t\t.tag = HARDWARE_MODULE_TAG,\n> +\t\t.module_api_version = CAMERA_MODULE_API_VERSION_2_4,\n> +\t\t.hal_api_version = HARDWARE_HAL_API_VERSION,\n> +\t\t.id = CAMERA_HARDWARE_MODULE_ID,\n> +\t\t.name = \"Libcamera Camera3HAL Module\",\n\ns/Libcamera/libcamera/\n\n> +\t\t.author = \"libcamera\",\n> +\t\t.methods = &hal_module_methods,\n> +\t\t.dso = nullptr,\n> +\t\t.reserved = {},\n> +\t},\n> +\n> +\t.get_number_of_cameras = hal_get_number_of_cameras,\n> +\t.get_camera_info = hal_get_camera_info,\n> +\t.set_callbacks = hal_set_callbacks,\n> +\t.get_vendor_tag_ops = nullptr,\n> +\t.open_legacy = hal_open_legacy,\n> +\t.set_torch_mode = hal_set_torch_mode,\n> +\t.init = hal_init,\n> +\t.reserved = {},\n> +};\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> new file mode 100644\n> index 000000000000..734b5eebd9a5\n> --- /dev/null\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -0,0 +1,173 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_hal_manager.cpp - Libcamera Android Camera Manager\n> + */\n> +\n> +#include \"camera_hal_manager.h\"\n> +\n> +#include \n\nThis shouldn't be needed.\n\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +#include \"camera_module.h\"\n> +#include \"camera_proxy.h\"\n> +\n> +using namespace libcamera;\n\nMissing blank line.\n\n> +LOG_DECLARE_CATEGORY(HAL);\n> +\n> +CameraHalManager::~CameraHalManager()\n> +{\n> +\tfor (auto metadata : staticMetadataMap_)\n> +\t\tfree_camera_metadata(metadata.second);\n> +}\n> +\n> +int CameraHalManager::initialize()\n> +{\n> +\t/*\n> +\t * Thread::start() will create a std::thread which will execute\n> +\t * CameraHalManager::run()\n> +\t */\n\nThat's documenting the libcamera::Thread class (and the std::thread is\nan internal implementation detail). I would just state\n\n\t/*\n\t * Start the camera HAL manager thread and wait until its\n\t * initialisation completes to be fully operational before\n\t * receiving calls from the camera stack.\n\t */\n\n(thus dropping the next comment)\n\n> +\tstart();\n> +\n> +\t/*\n> +\t * Wait for run() to notify us before returning to the caller to\n> +\t * make sure libcamera is fully initialized before we start handling\n> +\t * calls from the camera stack.\n> +\t */\n> +\tMutexLocker locker(mutex_);\n> +\tcv_.wait(locker);\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CameraHalManager::run()\n> +{\n> +\t/*\n> +\t * The run() operation is scheduled in its own thread;\n> +\t * It exec() to run the even dispatcher loop and initialize libcamera.\n\ns/even/event/\n\n> +\t *\n> +\t * All the libcamera components initialized from here will be tied to\n> +\t * the same thread as the here below run event dispatcher.\n\nThe first paragraph also mostly documents the Thread class. I would drop\nit, and write the second paragraph as\n\n\t/*\n\t * All the libcamera components must be initialised here, in\n\t * order to bind them to the camera HAL manager thread that\n\t * executes the event dispatcher.\n\t /\n\n> +\t */\n> +\tlibcameraManager_ = libcamera::CameraManager::instance();\n> +\n> +\tint ret = libcameraManager_->start();\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n> +\t\t\t\t<< strerror(-ret);\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * For each Camera registered in the system, a CameraModule\n> +\t * get created here with an associated Camera and a proxy.\n\ns/get/gets/\n\n> +\t *\n> +\t * \\todo Support camera hotplug.\n> +\t */\n> +\tunsigned int cameraIndex = 0;\n\nMaybe just index ?\n\n> +\tfor (auto camera : libcameraManager_->cameras()) {\n\nauto &camera\n\notherwise camera will be a copy of the shared pointer instead of a\nreference to it.\n\n> +\t\tcameras_.push_back(camera);\n\nThe cameras_ vector isn't used after this. I would drop it, and store\nthe shared pointer in CameraModule.\n\n> +\n> +\t\tCameraModule *module = new CameraModule(cameraIndex,\n> +\t\t\t\t\t\t\tcamera.get());\n> +\t\tmodules_.emplace_back(module);\n> +\n> +\t\tCameraProxy *proxy = new CameraProxy(cameraIndex,\n> +\t\t\t\t\t\t module);\n> +\t\tproxies_.emplace_back(proxy);\n\nSimilarly, how about sotring the module pointer inside the CameraProxy\nclass ? That would ensure that all accesses to the module go through the\nproxy, which I think would be a good idea.\n\n\t\tCameraProxy *proxy = new CameraProxy(index, camera);\n\t\tproxies_.emplace_back(proxy);\n\n> +\n> +\t\t++cameraIndex;\n> +\t}\n> +\n> +\t/*\n> +\t * libcamera has been initialized! Unlock the initialize() caller\n> +\t * as we're now ready to handle calls from the framework.\n> +\t */\n> +\tcv_.notify_one();\n> +\n> +\t/* Now start processing events and messages! */\n\ns/!/./ in the above two messages, there's no need to be\nover-enthusiastic :-)\n\n> +\texec();\n> +}\n> +\n> +CameraProxy *CameraHalManager::open(unsigned int id,\n> +\t\t\t\t const hw_module_t *hardwareModule)\n> +{\n> +\tif (id < 0 || id >= numCameras()) {\n> +\t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tCameraProxy *proxy = proxies_[id].get();\n> +\tif (proxy->open(hardwareModule))\n> +\t\treturn proxy;\n\nShouldn't you return nullprtr here ?\n\n> +\n> +\tLOG(HAL, Info) << \"Camera: '\" << id << \"' opened\";\n\ns/Camera:/Camera/\n\n> +\n> +\treturn proxy;\n> +}\n> +\n> +int CameraHalManager::close(CameraProxy *proxy)\n> +{\n> +\tproxy->close();\n> +\tLOG(HAL, Info) << \"Close camera: \" << proxy->id();\n> +\n> +\treturn 0;\n> +}\n> +\n> +unsigned int CameraHalManager::numCameras() const\n> +{\n> +\treturn libcameraManager_->cameras().size();\n> +}\n> +\n> +/*\n> + * Before the camera framework even tries to open a camera device with\n> + * hal_dev_open() it requires the camera HAL to report a list of static\n> + * informations on the camera device with id \\a id in the hal_get_camera_info()\n> + * method.\n> + *\n> + * The static metadata should be then generated probably from a\n> + * platform-specific module, as we cannot operate on the camera at this time as\n> + * it has not yet been open by the framework.\n\ns/open/opened/\n\nWhat prevents us from opening the camera internally ? I don't think we\nneed a platform-specific module.\n\n> + *\n> + * This is what the Intel XML file is used for, it is parsed and the data there\n> + * combined with informations from the PSL service (which I -think- it's what\n> + * our IPA is) to generate a list of static metadata per-camera device.\n\nI'd drop this paragraph.\n\n> + */\n> +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)\n> +{\n> +\tint cameras = numCameras();\n> +\tif (id >= cameras || id < 0 || !info) {\n\nCan we be called with info == nullptr ? I think I'd drop that part of\nthe check.\n\n> +\t\tLOG(HAL, Error) << \"Invalid camera id: \" << id;\n\ns/id:/id/\n\nYou may also want to add quotes around the value, as done in\nCameraHalManager::open() (or drop them there).\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcamera_metadata_t *staticMetadata;\n> +\tauto it = staticMetadataMap_.find(id);\n> +\tif (it != staticMetadataMap_.end()) {\n> +\t\tstaticMetadata = it->second;\n> +\t} else {\n> +\t\tCameraModule *cameraModule = modules_[id].get();\n> +\n> +\t\tstaticMetadata = cameraModule->getStaticMetadata();\n> +\t\tstaticMetadataMap_[id] = staticMetadata;\n\nWhy do you need the map, why can't you always retrieve the static\nmetadata from the CameraModule ?\n\n> +\t}\n> +\n> +\t/* \\todo: Get these info dynamically inspecting the camera module. */\n\ns/://\n\n> +\tinfo->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;\n> +\tinfo->orientation = 0;\n> +\tinfo->device_version = 0;\n> +\tinfo->resource_cost = 0;\n> +\tinfo->static_camera_characteristics = staticMetadata;\n> +\tinfo->conflicting_devices = nullptr;\n> +\tinfo->conflicting_devices_length = 0;\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> new file mode 100644\n> index 000000000000..65d6fa3150ef\n> --- /dev/null\n> +++ b/src/android/camera_hal_manager.h\n> @@ -0,0 +1,56 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_hal_manager.h - Libcamera Android Camera Manager\n> + */\n> +#ifndef __ANDROID_CAMERA_MANAGER_H__\n> +#define __ANDROID_CAMERA_MANAGER_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"thread.h\"\n> +#include \"thread_rpc.h\"\n> +\n> +class CameraModule;\n> +class CameraProxy;\n> +\n> +class CameraHalManager : public libcamera::Thread\n> +{\n> +public:\n> +\t~CameraHalManager();\n> +\n> +\tint initialize();\n\nOur initialisation methods are usually called init(), not initialize().\n\n> +\n> +\tCameraProxy *open(unsigned int id, const hw_module_t *module);\n> +\tint close(CameraProxy *proxy);\n> +\n> +\tunsigned int numCameras() const;\n> +\tint getCameraInfo(int id, struct camera_info *info);\n> +\n> +private:\n> +\tvoid run() override;\n> +\tcamera_metadata_t *getStaticMetadata(unsigned int id);\n> +\n> +\tlibcamera::CameraManager *libcameraManager_;\n\nI think you can call this manager_.\n\n> +\n> +\tstd::mutex mutex_;\n> +\tstd::condition_variable cv_;\n> +\n> +\tstd::vector> cameras_;\n> +\tstd::vector> modules_;\n> +\tstd::vector> proxies_;\n> +\n> +\tstd::map staticMetadataMap_;\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_MANAGER_H__ */\n> diff --git a/src/android/camera_module.cpp b/src/android/camera_module.cpp\n> new file mode 100644\n> index 000000000000..b64e377fb439\n> --- /dev/null\n> +++ b/src/android/camera_module.cpp\n> @@ -0,0 +1,799 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_module.cpp - Libcamera Android Camera Module\n> + */\n> +\n> +#include \"camera_module.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n\nI would group all the Android headers together, so\n\n#include \n#include \n\n#include \n\n> +#include \n> +\n> +#include \"message.h\"\n> +\n> +#include \"log.h\"\n\n#include \"log.h\"\n#include \"message.h\"\n\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL);\n> +\n> +CameraModule::CameraModule(unsigned int id, libcamera::Camera *camera)\n> +\t: id_(id), camera_(camera), cameraState_(CameraStopped),\n> +\t requestTemplate_(nullptr)\n> +{\n> +\tcamera_->requestCompleted.connect(this,\n> +\t\t\t\t\t &CameraModule::requestComplete);\n\nThis holds on a single line.\n\n> +}\n> +\n> +CameraModule::~CameraModule()\n> +{\n> +\tif (requestTemplate_)\n> +\t\tfree_camera_metadata(requestTemplate_);\n> +\trequestTemplate_ = nullptr;\n> +}\n> +\n> +void CameraModule::message(Message *message)\n> +{\n> +\tThreadRpcMessage *rpcMessage = static_cast\n> +\t\t\t\t (message);\n> +\n> +\tif (rpcMessage->type() != ThreadRpcMessage::type())\n> +\t\treturn Object::message(message);\n\nAs Message may not be a ThreadRpcMessage, you should write this as\n\n\tif (message->type() != ThreadRpcMessage::type())\n\t\treturn Object::message(message);\n\n\tThreadRpcMessage *rpcMessage = static_cast(message);\n\n> +\n> +\tThreadRpc *rpc = rpcMessage->rpc;\n> +\n> +\tswitch (rpc->tag) {\n> +\tcase ThreadRpc::ProcessCaptureRequest:\n> +\t\tprocessCaptureRequest(rpc->request);\n> +\t\tbreak;\n> +\tcase ThreadRpc::Close:\n> +\t\tclose();\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(HAL, Error) << \"Unknown RPC operation: \" << rpc->tag;\n> +\t}\n> +\n> +\trpc->notifyReception();\n> +}\n> +\n> +int CameraModule::open()\n> +{\n> +\tint ret = camera_->acquire();\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to acquire the camera\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CameraModule::close()\n> +{\n> +\tcamera_->stop();\n> +\n> +\tcamera_->freeBuffers();\n> +\tcamera_->release();\n> +\n> +\tcameraState_ = CameraStopped;\n> +}\n> +\n> +void CameraModule::setCallbacks(const struct camera3_device *camera3Device,\n> +\t\t\t\tconst camera3_callback_ops_t *callbacks)\n> +{\n> +\tcamera3Device_ = camera3Device;\n\ncamera3Device_ seems unused.\n\n> +\tcallbacks_ = callbacks;\n> +}\n> +\n> +camera_metadata_t *CameraModule::getStaticMetadata()\n> +{\n> +\tint ret;\n> +\n> +\t/*\n> +\t * The below metadata has been added by inspecting the Intel XML\n> +\t * file and it's associated parsing routines in the Intel IPU3 HAL.\n> +\t *\n> +\t * The here below metadata are enough to satisfy the camera3-test\n> +\t * provided by ChromeOS, but a real camera implementation might require\n\ns/might/will/\n\n> +\t * more.\n> +\t *\n> +\t * See CameraConf::AiqConf class implementation in the Intel HAL\n> +\t * to get a list of the static metadata registered by parsing the\n> +\t * XML config file in AiqConf::fillStaticMetadataFromCMC\n> +\t */\n> +\n> +\t/*\n> +\t * FIXME: this sizes come from the Intel IPU3 HAL, and are way too large for\n\ns/FIXME:/\\todo/\ns/this/these/\n\n> +\t * this intial HAL version.\n> +\t */\n> +\t#define STATIC_ENTRY_CAP 256\n> +\t#define STATIC_DATA_CAP 6688\n> +\tcamera_metadata_t *staticMetadata =\n> +\t\tallocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);\n> +\n> +\t/* Sensor static metadata. */\n> +\tint32_t pixelArraySize[] = {\n> +\t\t2592, 1944,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> +\t\t\t\t&pixelArraySize, 2);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t sensorSizes[] = {\n> +\t\t0, 0, 2560, 1920,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> +\t\t\t\t&sensorSizes, 4);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t sensitivityRange[] = {\n> +\t\t32, 2400,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> +\t\t\t\t&sensitivityRange, 2);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> +\t\t\t\t&filterArr, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint64_t exposureTimeRange[] = {\n> +\t\t100000, 200000000,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> +\t\t\t\t&exposureTimeRange, 2);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t orientation = 0;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_SENSOR_ORIENTATION,\n> +\t\t\t\t&orientation, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/* Flash static metadata. */\n> +\tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_FLASH_INFO_AVAILABLE,\n> +\t\t\t&flashAvailable, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/* Lens static metadata. */\n> +\tfloat fn = 2.53 / 100;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/* Control metadata. */\n> +\tchar controlMetadata = ANDROID_CONTROL_MODE_AUTO;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> +\t\t\t&controlMetadata, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tchar availableAntiBandingModes[] = {\n> +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,\n> +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,\n> +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,\n> +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> +\t\t\tavailableAntiBandingModes, 4);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tchar aeAvailableModes[] = {\n> +\t\tANDROID_CONTROL_AE_MODE_ON,\n> +\t\tANDROID_CONTROL_AE_MODE_OFF,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> +\t\t\taeAvailableModes, 2);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tcontrolMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> +\t\t\t&controlMetadata, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> +\t\t\t&awbLockAvailable, 1);\n> +\n> +\t/* Scaler static metadata. */\n> +\tstd::vector availableStreamFormats = {\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB,\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> +\t\t\tavailableStreamFormats.data(),\n> +\t\t\tavailableStreamFormats.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tstd::vector availableStreamConfigurations = {\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,\n> +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,\n> +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,\n> +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> +\t\t\tavailableStreamConfigurations.data(),\n> +\t\t\tavailableStreamConfigurations.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tstd::vector availableStallDurations = {\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> +\t\t\tavailableStallDurations.data(),\n> +\t\t\tavailableStallDurations.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tstd::vector minFrameDurations = {\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,\n> +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,\n> +\t};\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> +\t\t\tminFrameDurations.data(), minFrameDurations.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/* Info static metadata. */\n> +\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\tret = add_camera_metadata_entry(staticMetadata,\n> +\t\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> +\t\t\t&supportedHWLevel, 1);\n> +\n> +\treturn staticMetadata;\n> +}\n> +\n> +const camera_metadata_t *CameraModule::constructDefaultRequestMetadata(int type)\n> +{\n> +\tint ret;\n> +\n> +\t/*\n> +\t * TODO: inspect type and pick the right metadata pack.\n\ns/TODO:/\\todo/\n\n> +\t * As of now just use a single one for all templates\n> +\t */\n> +\tuint8_t captureIntent;\n> +\tswitch (type) {\n> +\tcase CAMERA3_TEMPLATE_PREVIEW:\n> +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;\n> +\t\tbreak;\n> +\tcase CAMERA3_TEMPLATE_STILL_CAPTURE:\n> +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;\n> +\t\tbreak;\n> +\tcase CAMERA3_TEMPLATE_VIDEO_RECORD:\n> +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;\n> +\t\tbreak;\n> +\tcase CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:\n> +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;\n> +\t\tbreak;\n> +\tcase CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:\n> +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;\n> +\t\tbreak;\n> +\tcase CAMERA3_TEMPLATE_MANUAL:\n> +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(HAL, Error) << \"Invalid template request type: \" << type;\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tif (requestTemplate_)\n> +\t\treturn requestTemplate_;\n> +\n> +\t/* FIXME: this sizes are quite arbitrary ones */\n> +\t#define REQUEST_TEMPLATE_ENTRIES\t 30\n> +\t#define REQUEST_TEMPLATE_DATA\t\t2048\n> +\trequestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,\n> +\t\t\t\t\t\t REQUEST_TEMPLATE_DATA);\n> +\tif (!requestTemplate_) {\n> +\t\tLOG(HAL, Error) << \"Failed to allocate template metadata\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\t/*\n> +\t * Fill in the required Request metadata.\n> +\t *\n> +\t * Part (most?) of this entries comes from inspecting the Intel's IPU3\n> +\t * HAL xml configuration file.\n> +\t */\n> +\n> +\t/* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */\n> +\tint32_t maxOutStream[] = { 0, 2, 0 };\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> +\t\t\tmaxOutStream, 3);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t maxPipelineDepth = 5;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> +\t\t\t&maxPipelineDepth, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t inputStreams = 0;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> +\t\t\t&inputStreams, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t partialResultCount = 1;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> +\t\t\t&partialResultCount, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t availableCapabilities[] = {\n> +\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> +\t};\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> +\t\t\tavailableCapabilities, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AE_MODE,\n> +\t\t\t&aeMode, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t aeExposureCompensation = 0;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> +\t\t\t&aeExposureCompensation, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> +\t\t\t&aePrecaptureTrigger, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AE_LOCK,\n> +\t\t\t&aeLock, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AF_TRIGGER,\n> +\t\t\t&afTrigger, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AWB_MODE,\n> +\t\t\t&awbMode, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AWB_LOCK,\n> +\t\t\t&awbLock, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> +\t\t\t&awbLockAvailable, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t flashMode = ANDROID_FLASH_MODE_OFF;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_FLASH_MODE,\n> +\t\t\t&flashMode, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> +\t\t\t&faceDetectMode, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> +\t\t\t&captureIntent, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/*\n> +\t * This is quite hard to list at the moment wihtout knowing what\n> +\t * we could control.\n> +\t *\n> +\t * For now, just list in the available Request keys and in the available\n> +\t * result keys the control and reporting of the AE algorithm.\n> +\t */\n> +\tstd::vector availableRequestKeys = {\n> +\t\tANDROID_CONTROL_AE_MODE,\n> +\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> +\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> +\t\tANDROID_CONTROL_AE_LOCK,\n> +\t\tANDROID_CONTROL_AF_TRIGGER,\n> +\t\tANDROID_CONTROL_AWB_MODE,\n> +\t\tANDROID_CONTROL_AWB_LOCK,\n> +\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> +\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> +\t\tANDROID_FLASH_MODE,\n> +\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> +\t};\n> +\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,\n> +\t\t\tavailableRequestKeys.data(),\n> +\t\t\tavailableRequestKeys.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tstd::vector availableResultKeys = {\n> +\t\tANDROID_CONTROL_AE_MODE,\n> +\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> +\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> +\t\tANDROID_CONTROL_AE_LOCK,\n> +\t\tANDROID_CONTROL_AF_TRIGGER,\n> +\t\tANDROID_CONTROL_AWB_MODE,\n> +\t\tANDROID_CONTROL_AWB_LOCK,\n> +\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> +\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> +\t\tANDROID_FLASH_MODE,\n> +\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> +\t};\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n> +\t\t\tavailableResultKeys.data(),\n> +\t\t\tavailableResultKeys.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/*\n> +\t * The available characteristics should be the tags reported\n> +\t * as part of the static metadata reported at hal_get_camera_info()\n> +\t * time. The xml file sets those to 0 though.\n> +\t */\n> +\tstd::vector availableCharacteristicsKeys = {};\n> +\tret = add_camera_metadata_entry(requestTemplate_,\n> +\t\t\tANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> +\t\t\tavailableCharacteristicsKeys.data(),\n> +\t\t\tavailableCharacteristicsKeys.size());\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\treturn requestTemplate_;\n> +}\n> +\n> +/**\n> + * Inspect the stream_list to produce a list of StreamConfiguration to\n> + * be use to configure the Camera.\n> + */\n> +int CameraModule::configureStreams(camera3_stream_configuration_t *stream_list)\n> +{\n> +\n\nExtra blank line.\n\n> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> +\n> +\t\tLOG(HAL, Info) << \"Stream #\" << i\n> +\t\t\t << \": direction: \" << stream->stream_type\n> +\t\t\t << \" - width: \" << stream->width\n> +\t\t\t << \" - height: \" << stream->height\n> +\t\t\t << \" - format: \" << std::hex << stream->format;\n\ns/ -/,/ in the above three lines\n\nAnd I would print size: ...x... instead of width and height.\n\n> +\t}\n> +\n> +\t/* Hardcode viewfinder role, collecting sizes from the stream config. */\n> +\tif (stream_list->num_streams != 1) {\n> +\t\tLOG(HAL, Error) << \"Only one stream supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tStreamRoles roles = {};\n> +\troles.push_back(StreamRole::StillCapture);\n\nI think you can write this as\n\n\tStreamRoles roles{ StreamRole::StillCapture };\n\n> +\n> +\tconfig_ = camera_->generateConfiguration(roles);\n> +\tif (!config_ || config_->empty()) {\n> +\t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Only one stream is supported. */\n> +\tcamera3_stream_t *camera3Stream = stream_list->streams[0];\n> +\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> +\tstreamConfiguration->size.width = camera3Stream->width;\n> +\tstreamConfiguration->size.height = camera3Stream->height;\n> +\tstreamConfiguration->memoryType = ExternalMemory;\n> +\n> +\t/*\n> +\t * FIXME: do not change the pixel format from the one returned\n> +\t * from the Camera::generateConfiguration();\n> +\t *\n> +\t * We'll need to translate from Android defined pixel format codes\n> +\t * to whatever libcamera will use.\n> +\t */\n> +\n> +\tswitch (config_->validate()) {\n> +\tcase CameraConfiguration::Valid:\n> +\t\tbreak;\n> +\tcase CameraConfiguration::Adjusted:\n> +\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n\nMissing config_.reset() ?\n\n> +\t\treturn -EINVAL;\n> +\tcase CameraConfiguration::Invalid:\n> +\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> +\t\tconfig_.reset();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcamera3Stream->max_buffers = streamConfiguration->bufferCount;\n> +\n> +\t/*\n> +\t * Once the CameraConfiguration has been adjusted/validated\n> +\t * it can be applied to the camera.\n> +\t */\n> +\tint ret = camera_->configure(config_.get());\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> +\t\t\t\t<< camera_->name() << \"'\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraModule::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> +{\n> +\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> +\tStream *stream = streamConfiguration->stream();\n> +\n> +\tif (camera3Request->num_output_buffers != 1) {\n> +\t\tLOG(HAL, Error) << \"Invalid number of output buffers: \"\n> +\t\t\t\t<< camera3Request->num_output_buffers;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Start the camera if that's the first request we handle. */\n> +\tif (cameraState_ == CameraStopped) {\n> +\t\tint ret = camera_->allocateBuffers();\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to allocate buffers\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tret = camera_->start();\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> +\t\t\tcamera_->freeBuffers();\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tcameraState_ = CameraRunning;\n> +\t}\n> +\n> +\t/*\n> +\t * Queue a request for the Camera with the provided dmabuf file\n> +\t * descriptors.\n> +\t */\n> +\tconst camera3_stream_buffer_t *camera3Buffer =\n> +\t\t\t\t\t&camera3Request->output_buffers[0];\n> +\tconst buffer_handle_t camera3Handle = *camera3Buffer->buffer;\n> +\n> +\tstd::array fds = {\n> +\t\tcamera3Handle->data[0],\n> +\t\tcamera3Handle->data[1],\n> +\t\tcamera3Handle->data[2],\n> +\t};\n> +\tstd::unique_ptr buffer = stream->createBuffer(fds);\n> +\tif (!buffer) {\n> +\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tRequest *request = camera_->createRequest();\n> +\trequest->addBuffer(std::move(buffer));\n> +\tint ret = camera_->queueRequest(request);\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to queue request\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Save the request descriptors for use at completion time. */\n> +\tCamera3RequestDescriptor descriptor = {};\n> +\tdescriptor.frameNumber = camera3Request->frame_number,\n> +\tdescriptor.numBuffers = camera3Request->num_output_buffers,\n\ns/,$/;/ for the two lines above.\n\nI don't think you need to store numBuffers in the descriptors, it can be\nretrieved from buffers.size() in requestComplete().\n\n\n> +\tdescriptor.buffers = new camera3_stream_buffer_t[descriptor.numBuffers];\n> +\tfor (unsigned int i = 0; i < descriptor.numBuffers; ++i) {\n> +\t\tcamera3_stream_buffer_t &buffer = descriptor.buffers[i];\n> +\t\tbuffer = camera3Request->output_buffers[i];\n\nJust\n\n\t\tdescriptor.buffers[i] = camera3Request->output_buffers[i];\n> +\t}\n> +\n> +\trequestMap_[request] = descriptor;\n\nHow about using the request cookie (passed to Camera::createRequest())\nfor this purpose ?\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CameraModule::requestComplete(Request *request,\n> +\t\t\t\t const std::map &buffers)\n> +{\n> +\tBuffer *libcameraBuffer = buffers.begin()->second;\n> +\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> +\tcamera_metadata_t *resultMetadata = nullptr;\n> +\n> +\tif (request->status() != Request::RequestComplete) {\n> +\t\tLOG(HAL, Error) << \"Request not succesfully completed: \"\n> +\t\t\t\t<< request->status();\n> +\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t}\n> +\n> +\t/* Prepare to call back the Android camera stack. */\n> +\tCamera3RequestDescriptor &descriptor = requestMap_[request];\n> +\n> +\tcamera3_capture_result_t captureResult = {};\n> +\tcaptureResult.frame_number = descriptor.frameNumber;\n> +\tcaptureResult.num_output_buffers = descriptor.numBuffers;\n> +\n> +\tcamera3_stream_buffer_t *resultBuffers =\n> +\t\tnew camera3_stream_buffer_t[descriptor.numBuffers];\n> +\tfor (unsigned int i = 0; i < descriptor.numBuffers; ++i) {\n> +\t\tcamera3_stream_buffer_t resultBuffer;\n> +\n> +\t\tresultBuffer = descriptor.buffers[i];\n> +\t\tresultBuffer.acquire_fence = -1;\n> +\t\tresultBuffer.release_fence = -1;\n> +\t\tresultBuffer.status = status;\n> +\n> +\t\tresultBuffers[i] = resultBuffer;\n> +\t}\n> +\tcaptureResult.output_buffers =\n> +\t\tconst_cast(resultBuffers);\n> +\n> +\tif (status == CAMERA3_BUFFER_STATUS_ERROR) {\n> +\t\t/* \\todo Improve error handling. */\n> +\t\tnotifyError(descriptor.frameNumber, resultBuffers->stream);\n> +\t} else {\n> +\t\tnotifyShutter(descriptor.frameNumber, libcameraBuffer->timestamp());\n> +\n> +\t\tcaptureResult.partial_result = 1;\n> +\t\tresultMetadata = getResultMetadata(descriptor.frameNumber,\n> +\t\t\t\t\t\t libcameraBuffer->timestamp());\n> +\t\tcaptureResult.result = resultMetadata;\n> +\t}\n> +\n> +\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\n> +\tif (resultMetadata)\n> +\t\tfree_camera_metadata(resultMetadata);\n> +\n> +\tdelete[] resultBuffers;\n> +\tdelete[] descriptor.buffers;\n> +\trequestMap_.erase(request);\n> +\n> +\treturn;\n> +}\n> +\n> +void CameraModule::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> +{\n> +\tcamera3_notify_msg_t notify = {};\n> +\n> +\tnotify.type = CAMERA3_MSG_SHUTTER;\n> +\tnotify.message.shutter.frame_number = frameNumber;\n> +\tnotify.message.shutter.timestamp = timestamp;\n> +\n> +\tcallbacks_->notify(callbacks_, ¬ify);\n> +}\n> +\n> +void CameraModule::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> +{\n> +\tcamera3_notify_msg_t notify = {};\n> +\n> +\tnotify.type = CAMERA3_MSG_ERROR;\n> +\tnotify.message.error.error_stream = stream;\n> +\tnotify.message.error.frame_number = frameNumber;\n> +\tnotify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> +\n> +\tcallbacks_->notify(callbacks_, ¬ify);\n> +}\n> +\n> +/*\n> + * Fixed result metadata, mostly imported from the UVC camera HAL, which\n> + * does not produces metadata and thus needs to generate a fixed set.\n> + */\n> +camera_metadata_t *CameraModule::getResultMetadata(int frame_number,\n> +\t\t\t\t\t\t int64_t timestamp)\n> +{\n> +\tint ret;\n> +\n> +\t/* FIXME: random \"big enough\" values. */\n> +\t#define RESULT_ENTRY_CAP 256\n> +\t#define RESULT_DATA_CAP 6688\n> +\tcamera_metadata_t *resultMetadata =\n> +\t\tallocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);\n> +\n> +\tconst uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,\n> +\t\t\t\t\t&ae_state, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tconst uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;\n> +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,\n> +\t\t\t\t\t&ae_lock, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tuint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;\n> +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,\n> +\t\t\t\t\t&af_state, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tconst uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_CONTROL_AWB_STATE,\n> +\t\t\t\t\t&awb_state, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tconst uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_CONTROL_AWB_LOCK,\n> +\t\t\t\t\t&awb_lock, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tconst uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_LENS_STATE,\n> +\t\t\t\t\t&lens_state, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tint32_t sensorSizes[] = {\n> +\t\t0, 0, 2560, 1920,\n> +\t};\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_SCALER_CROP_REGION,\n> +\t\t\t\t\tsensorSizes, 4);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_SENSOR_TIMESTAMP,\n> +\t\t\t\t\t×tamp, 1);\n> +\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/* 33.3 msec */\n> +\tconst int64_t rolling_shutter_skew = 33300000;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_SENSOR_ROLLING_SHUTTER_SKEW,\n> +\t\t\t\t\t&rolling_shutter_skew, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\t/* 16.6 msec */\n> +\tconst int64_t exposure_time = 16600000;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_SENSOR_EXPOSURE_TIME,\n> +\t\t\t\t\t&exposure_time, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tconst uint8_t lens_shading_map_mode =\n> +\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> +\t\t\t\t\t&lens_shading_map_mode, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\tconst uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;\n> +\tret = add_camera_metadata_entry(resultMetadata,\n> +\t\t\t\t\tANDROID_STATISTICS_SCENE_FLICKER,\n> +\t\t\t\t\t&scene_flicker, 1);\n> +\tMETADATA_ASSERT(ret);\n> +\n> +\treturn resultMetadata;\n> +}\n> diff --git a/src/android/camera_module.h b/src/android/camera_module.h\n> new file mode 100644\n> index 000000000000..67488d5940b1\n> --- /dev/null\n> +++ b/src/android/camera_module.h\n> @@ -0,0 +1,75 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_module.h - Libcamera Android Camera Module\n\ns/Libcamera/libcamera/\n\n(here and everywhere else, libcamera is always written in all lowercase)\n\n> + */\n> +#ifndef __ANDROID_CAMERA_MODULE_H__\n> +#define __ANDROID_CAMERA_MODULE_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"message.h\"\n> +\n> +#include \"camera_hal_manager.h\"\n> +\n> +#define METADATA_ASSERT(_r)\t\t\\\n> +\tdo {\t\t\t\t\\\n> +\t\tif (!(_r)) break;\t\\\n> +\t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> +\t\treturn nullptr;\t\t\\\n> +\t} while(0);\n\nI really, really dislike hiding return statements in macros. We can keep\nit for now, but please add a comment telling that it should be removed.\n\n> +\n\nEven if we don't document the whole implementation with Doxygen, I think\na few short comments that explain what each class models would be\nuseful. Otherwise the reader is left to wonder what CameraModule or\nCameraProxy stand for. A comment that explains how the various objects\nwork together would be useful too.\n\n> +class CameraModule : public libcamera::Object\n\nI don't think CameraModule is a good name :-/ In HAL terminology, the\nmodule refers to camera_module_t, and corresponds more or less to\nlibcamera::CameraManager, not libcamera::Camera.\n\nOne option would be to name this class Camera in a HAL namespace. That\nwould make it quite clear that it corresponds to a libcamra::Camera.\n\n> +{\n> +public:\n> +\tCameraModule(unsigned int id, libcamera::Camera *camera);\n> +\t~CameraModule();\n> +\n> +\tvoid message(libcamera::Message *message);\n> +\n> +\tint open();\n> +\tvoid close();\n> +\tvoid setCallbacks(const struct camera3_device *cameraDevice,\n> +\t\t\t const camera3_callback_ops_t *callbacks);\n> +\tcamera_metadata_t *getStaticMetadata();\n> +\tconst camera_metadata_t *constructDefaultRequestMetadata(int type);\n> +\tint configureStreams(camera3_stream_configuration_t *stream_list);\n> +\tint processCaptureRequest(camera3_capture_request_t *request);\n> +\tvoid requestComplete(libcamera::Request *request,\n> +\t\t\t const std::map &buffers);\n> +\n> +\tunsigned int id() const { return id_; }\n\nI think this method is not used.\n\n> +\n> +private:\n> +\tstruct Camera3RequestDescriptor {\n> +\t\tuint32_t frameNumber;\n> +\t\tuint32_t numBuffers;\n> +\t\tcamera3_stream_buffer_t *buffers;\n> +\t};\n> +\n> +\tenum CameraState {\n> +\t\tCameraStopped,\n> +\t\tCameraRunning,\n> +\t};\n\nWith just two states you could instead have a bool running_ field.\n\n> +\n> +\tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> +\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> +\tcamera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);\n> +\n> +\tunsigned int id_;\n> +\tlibcamera::Camera *camera_;\n> +\tstd::unique_ptr config_;\n> +\tCameraState cameraState_;\n> +\n> +\tcamera_metadata_t *requestTemplate_;\n> +\tconst camera3_callback_ops_t *callbacks_;\n> +\tconst struct camera3_device *camera3Device_;\n> +\n> +\tstd::map requestMap_;\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_MODULE_H__ */\n> diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp\n> new file mode 100644\n> index 000000000000..af7817f29137\n> --- /dev/null\n> +++ b/src/android/camera_proxy.cpp\n> @@ -0,0 +1,181 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_proxy.cpp - Proxy to camera module instances\n> + */\n> +\n> +#include \"camera_proxy.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"message.h\"\n> +#include \"thread_rpc.h\"\n> +#include \"utils.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL);\n> +\n> +#define LIBCAMERA_HAL_FUNC_DBG\t\\\n> +\tLOG(HAL, Debug);\n\nIf we need to trace calls, I think something better than debug messages\nwould be useful. I'm not sure what yet though.\n\n> +\n> +static int hal_dev_initialize(const struct camera3_device *dev,\n> +\t\t\t const camera3_callback_ops_t *callback_ops)\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n\nCan this happen ?\n\n> +\n> +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> +\tproxy->setCallbacks(callback_ops);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int hal_dev_configure_streams(const struct camera3_device *dev,\n> +\t\t\t\t camera3_stream_configuration_t *stream_list)\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> +\tproxy->configureStreams(stream_list);\n> +\n> +\treturn 0;\n\nShouldn't you propagate the error value from proxy->configureStreams() ?\n\n> +}\n> +\n> +static const camera_metadata_t *\n> +hal_dev_construct_default_request_settings(const struct camera3_device *dev,\n> +\t\t\t\t\t int type)\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\tif (!dev)\n> +\t\treturn nullptr;\n> +\n> +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> +\treturn proxy->constructDefaultRequest(type);\n\nHow about renaming constructDefaultRequest() and\nconstructDefaultRequestMetadata() to constructDefaultRequestSettings(),\nin order to match the HAL operation name ?\n\n> +}\n> +\n> +static int hal_dev_process_capture_request(const struct camera3_device *dev,\n> +\t\t\t\t\t camera3_capture_request_t *request)\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> +\treturn proxy->processCaptureRequest(request);\n> +}\n> +\n> +static void hal_dev_dump(const struct camera3_device *dev, int fd)\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +}\n> +\n> +static int hal_dev_flush(const struct camera3_device *dev)\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\treturn 0;\n> +}\n\nYou could define these methods as static methods of the CameraProxy\nclass. For instance, hal_dev_configure_streams would become\n\nint CameraProxy::configureStreams(const struct camera3_device *dev,\n\t\t\t\t camera3_stream_configuration_t *stream_list)\n{\n\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n\tproxy->configureStreams(stream_list);\n}\n\n> +\n> +static camera3_device_ops hal_dev_ops = {\n\nI wish this could be const :-(\n\n> +\t.initialize = hal_dev_initialize,\n> +\t.configure_streams = hal_dev_configure_streams,\n> +\t.register_stream_buffers = nullptr,\n> +\t.construct_default_request_settings = hal_dev_construct_default_request_settings,\n> +\t.process_capture_request = hal_dev_process_capture_request,\n> +\t.get_metadata_vendor_tag_ops = nullptr,\n> +\t.dump = hal_dev_dump,\n> +\t.flush = hal_dev_flush,\n> +\t.reserved = { 0 },\n\ns/0/nullptr/ ?\n\n> +};\n> +\n> +CameraProxy::CameraProxy(unsigned int id, CameraModule *cameraModule)\n> +\t: open_(false), id_(id), cameraModule_(cameraModule)\n> +{\n> +}\n> +\n> +int CameraProxy::open(const hw_module_t *hardwareModule)\n> +{\n> +\tint ret = cameraModule_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Initialize the hw_device_t in the instance camera3_module_t. */\n> +\tcameraDevice_.common.tag = HARDWARE_DEVICE_TAG;\n> +\tcameraDevice_.common.version = CAMERA_DEVICE_API_VERSION_3_3;\n> +\tcameraDevice_.common.module = (hw_module_t *)hardwareModule;\n> +\n> +\t/*\n> +\t * The camera device operations. These actually implement\n> +\t * the Android Camera HALv3 interface.\n> +\t */\n> +\tcameraDevice_.ops = &hal_dev_ops;\n> +\tcameraDevice_.priv = this;\n> +\n> +\topen_ = true;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CameraProxy::close()\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\tThreadRpc rpcRequest;\n> +\trpcRequest.tag = ThreadRpc::Close;\n> +\n> +\tthreadRpcCall(rpcRequest);\n> +\n> +\topen_ = false;\n> +}\n> +\n> +void CameraProxy::setCallbacks(const camera3_callback_ops_t *callbacks)\n\nI would name this initialize() to match hal_dev_initialize().\n\n> +{\n> +\tLIBCAMERA_HAL_FUNC_DBG\n> +\n> +\tcameraModule_->setCallbacks(&cameraDevice_, callbacks);\n> +}\n> +\n> +const camera_metadata_t *CameraProxy::constructDefaultRequest(int type)\n> +{\n> +\treturn cameraModule_->constructDefaultRequestMetadata(type);\n> +}\n> +\n> +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)\n> +{\n> +\treturn cameraModule_->configureStreams(stream_list);\n> +}\n> +\n> +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)\n> +{\n> +\tThreadRpc rpcRequest;\n> +\trpcRequest.tag = ThreadRpc::ProcessCaptureRequest;\n> +\trpcRequest.request = request;\n> +\n> +\tthreadRpcCall(rpcRequest);\n> +\n> +\treturn 0;\n\nDoes this method need to be synchronous ? We can keep it as-is for now,\nbut I wonder if it wouldn't make sense to later move (part of) the\nvalidation code here and make the call asynchronous.\n\n> +}\n> +\n> +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)\n> +{\n> +\tstd::unique_ptr message =\n> +\t\t\t\tutils::make_unique();\n> +\tmessage->rpc = &rpcRequest;\n> +\n> +\tcameraModule_->postMessage(std::move(message));\n> +\trpcRequest.waitDelivery();\n> +}\n> diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h\n> new file mode 100644\n> index 000000000000..69e6878c4352\n> --- /dev/null\n> +++ b/src/android/camera_proxy.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * camera_proxy.h - Proxy to camera module instances\n> + */\n> +#ifndef __ANDROID_CAMERA_PROXY_H__\n> +#define __ANDROID_CAMERA_PROXY_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \"camera_module.h\"\n> +\n> +class CameraProxy\n> +{\n> +public:\n> +\tCameraProxy(unsigned int id, CameraModule *cameraModule);\n> +\n> +\tint open(const hw_module_t *hardwareModule);\n> +\tvoid close();\n> +\n> +\tvoid setCallbacks(const camera3_callback_ops_t *callbacks);\n> +\tconst camera_metadata_t *constructDefaultRequest(int type);\n> +\tint configureStreams(camera3_stream_configuration_t *stream_list);\n> +\tint processCaptureRequest(camera3_capture_request_t *request);\n> +\n> +\tbool isOpen() const { return open_; }\n\nThis isn't used.\n\n> +\tunsigned int id() const { return id_; }\n> +\tcamera3_device_t *device() { return &cameraDevice_; }\n> +\n> +private:\n> +\tvoid threadRpcCall(ThreadRpc &rpcRequest);\n> +\n> +\tbool open_;\n\nAnd neither is this, if you drop the isOpen() method.\n\n> +\tunsigned int id_;\n> +\tCameraModule *cameraModule_;\n> +\tcamera3_device_t cameraDevice_;\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_PROXY_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 1f242953db37..63b45832c0d2 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -1,3 +1,11 @@\n> +android_hal_sources = files([\n> + 'camera3_hal.cpp',\n> + 'camera_hal_manager.cpp',\n> + 'camera_module.cpp',\n> + 'camera_proxy.cpp',\n> + 'thread_rpc.cpp'\n> +])\n> +\n> android_camera_metadata_sources = files([\n> 'metadata/camera_metadata.c',\n> ])\n> diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp\n> new file mode 100644\n> index 000000000000..f13db59d0d75\n> --- /dev/null\n> +++ b/src/android/thread_rpc.cpp\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * thread_call.cpp - Inter-thread procedure call\n\nthread_rpc.cpp\n\n> + */\n> +\n> +#include \"message.h\"\n> +#include \"thread_rpc.h\"\n> +\n> +using namespace libcamera;\n> +\n> +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;\n> +\n> +ThreadRpcMessage::ThreadRpcMessage()\n> +\t: Message(type())\n> +{\n> +}\n> +\n> +void ThreadRpc::notifyReception()\n> +{\n> +\t{\n> +\t\tlibcamera::MutexLocker locker(mutex_);\n> +\t\tdelivered_ = true;\n> +\t}\n> +\tcv_.notify_one();\n> +}\n> +\n> +void ThreadRpc::waitDelivery()\n> +{\n> +\tlibcamera::MutexLocker locker(mutex_);\n> +\tcv_.wait(locker, [&]{ return delivered_; });\n> +}\n> +\n> +Message::Type ThreadRpcMessage::type()\n> +{\n> +\tif (ThreadRpcMessage::rpcType_ == Message::Type::None)\n> +\t\trpcType_ = Message::registerMessageType();\n> +\n> +\treturn rpcType_;\n> +}\n> diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h\n> new file mode 100644\n> index 000000000000..173caba1a515\n> --- /dev/null\n> +++ b/src/android/thread_rpc.h\n> @@ -0,0 +1,56 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * thread_call.h - Inter-thread procedure call\n> + */\n> +#ifndef __ANDROID_THREAD_CALL_H__\n> +#define __ANDROID_THREAD_CALL_H__\n\ns/CALL/RPC/\n\n> +\n> +#include \n> +#include \n\nDo you need string ?\n\nYou should #include \n\n> +\n> +#include \n> +#include \n> +\n> +#include \"message.h\"\n> +#include \"thread.h\"\n> +\n> +class ThreadRpc\n> +{\n> +public:\n> +\tenum RpcTag {\n> +\t\tProcessCaptureRequest,\n> +\t\tClose,\n> +\t};\n> +\n> +\tThreadRpc()\n> +\t\t: delivered_(false) {}\n> +\n> +\tvoid notifyReception();\n> +\tvoid waitDelivery();\n> +\n> +\tRpcTag tag;\n> +\n> +\tcamera3_capture_request_t *request;\n> +\n> +private:\n> +\tbool delivered_;\n> +\tstd::mutex mutex_;\n> +\tstd::condition_variable cv_;\n> +};\n> +\n> +class ThreadRpcMessage : public libcamera::Message\n> +{\n> +public:\n> +\tThreadRpcMessage();\n> +\tThreadRpc *rpc;\n> +\n> +\tstatic Message::Type type();\n> +\n> +private:\n> +\tstatic libcamera::Message::Type rpcType_;\n> +};\n\nFYI, I'll probably move part of this to the libcamera core.\n\n> +\n> +#endif /* __ANDROID_THREAD_CALL_H__ */\n> +\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 566B060E31\n\tfor ;\n\tWed, 7 Aug 2019 17:32:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7AAED814;\n\tWed, 7 Aug 2019 17:32:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565191930;\n\tbh=KuFOZLnKXNvTQ6NOtuSzsUy6OjNPtfISVPO6UXMzT8Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E+m28A8jlH2oFhakhTDyOgv/huB2DX6NuQPrXNGIoGeNNkiIOYXFdUTinGVB9YaqV\n\tTPKtUbuT/XeyA9stdF6iW6iscO+bWmZhqfTwSMpvG/RnZi0NWX3Enb+X4t8Etv7PZU\n\tNlpL1nx7O5qJzBBr/wEtZL7hco47uDTdRUrQORr8=","Date":"Wed, 7 Aug 2019 18:32:08 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190807153208.GG5048@pendragon.ideasonboard.com>","References":"<20190806195518.16739-1-jacopo@jmondi.org>\n\t<20190806195518.16739-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190806195518.16739-7-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 07 Aug 2019 15:32:11 -0000"}},{"id":2343,"web_url":"https://patchwork.libcamera.org/comment/2343/","msgid":"<20190808153806.3j5d72fwi5vayavw@uno.localdomain>","date":"2019-08-08T15:38:15","subject":"Re: [libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Aug 07, 2019 at 06:32:08PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Aug 06, 2019 at 09:55:18PM +0200, Jacopo Mondi wrote:\n> > Add libcamera Android Camera HALv3 implementation.\n> >\n> > The initial camera HAL implementation supports the LIMITED hardware\n> > level and uses statically defined metadata and camera characteristics.\n> >\n> > Add a build option named 'android' and adjust the build system to\n> > selectively compile the Android camera HAL and link it against the\n> > required Android libraries.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > meson_options.txt | 5 +\n> > src/android/camera3_hal.cpp | 130 +++++\n> > src/android/camera_hal_manager.cpp | 173 +++++++\n> > src/android/camera_hal_manager.h | 56 ++\n> > src/android/camera_module.cpp | 799 +++++++++++++++++++++++++++++\n> > src/android/camera_module.h | 75 +++\n> > src/android/camera_proxy.cpp | 181 +++++++\n> > src/android/camera_proxy.h | 41 ++\n> > src/android/meson.build | 8 +\n> > src/android/thread_rpc.cpp | 41 ++\n> > src/android/thread_rpc.h | 56 ++\n> > src/libcamera/meson.build | 9 +\n> > src/meson.build | 5 +-\n> > 13 files changed, 1578 insertions(+), 1 deletion(-)\n> > create mode 100644 src/android/camera3_hal.cpp\n> > create mode 100644 src/android/camera_hal_manager.cpp\n> > create mode 100644 src/android/camera_hal_manager.h\n> > create mode 100644 src/android/camera_module.cpp\n> > create mode 100644 src/android/camera_module.h\n> > create mode 100644 src/android/camera_proxy.cpp\n> > create mode 100644 src/android/camera_proxy.h\n> > create mode 100644 src/android/thread_rpc.cpp\n> > create mode 100644 src/android/thread_rpc.h\n>\n> [snip]\n>\n> > diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp\n> > new file mode 100644\n> > index 000000000000..0a97a9333d20\n> > --- /dev/null\n> > +++ b/src/android/camera3_hal.cpp\n> > @@ -0,0 +1,130 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera3_hal.cpp - Android Camera3 HAL module\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n>\n> Do you need camera_metadata.h ? Isn't it enough to include\n> camera_common.h ? I think hardware.h can also be dropped.\n\ncamera_common.h includes camera_metadata.h, so I could just include\nthe one I need.\n\n>\n> > +\n> > +#include \n>\n> To speed up compilation, it would be better to include only the header\n> files we need (here and everywhere else).\n>\n\nIndeed\n\n> > +\n> > +#include \"log.h\"\n> > +\n> > +#include \"camera_hal_manager.h\"\n> > +#include \"camera_module.h\"\n> > +#include \"camera_proxy.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(HAL)\n> > +\n> > +static CameraHalManager cameraManager;\n> > +\n> > +/*------------------------------------------------------------------------------\n> > + * Android Camera HAL callbacks\n> > + */\n> > +\n> > +static int hal_get_number_of_cameras(void)\n> > +{\n> > +\treturn cameraManager.numCameras();\n> > +}\n> > +\n> > +static int hal_get_camera_info(int id, struct camera_info *info)\n> > +{\n> > +\treturn cameraManager.getCameraInfo(id, info);\n> > +}\n> > +\n> > +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)\n> > +{\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int hal_open_legacy(const struct hw_module_t *module, const char *id,\n> > +\t\t\t uint32_t halVersion, struct hw_device_t **device)\n> > +{\n> > +\treturn -ENOSYS;\n> > +}\n> > +\n> > +static int hal_set_torch_mode(const char *camera_id, bool enabled)\n> > +{\n> > +\treturn -ENOSYS;\n> > +}\n> > +\n> > +/*\n> > + * First entry point of the camera HAL module.\n> > + *\n> > + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)\n> > + */\n> > +static int hal_init()\n> > +{\n> > +\tLOG(HAL, Info) << \"Initialising Android camera HAL\";\n> > +\n> > +\tcameraManager.initialize();\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/*------------------------------------------------------------------------------\n> > + * Android Camera Device\n> > + */\n> > +\n> > +static int hal_dev_close(hw_device_t *hw_device)\n> > +{\n> > +\tif (!hw_device)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tcamera3_device_t *dev = reinterpret_cast(hw_device);\n> > +\tCameraProxy *cameraModule = reinterpret_cast(dev->priv);\n>\n> s/cameraModule/proxy/ otherwise it's very confusing to have a\n> CameraProxy instance called cameraModule when there's a CameraModule\n> class.\n>\n> This comment applies to all other similar instances.\n>\n\nSorry, leftover. I don't see any other module/proxy in this file\n\n> > +\n> > +\treturn cameraManager.close(cameraModule);\n>\n> As cameraManager.close() only calls proxy->close(), how about defining\n> the hal_dev_close() method as a static method of CameraProxy, and\n> setting proxy->device()->common.close inside the CameraProxy constructor\n> ? It would avoid touching the internals of the CameraProxy in\n> hal_dev_open() below.\n>\n\nI liked having hal_dev_open and hal_dev_close here, as part of the HAL\nmodule. But yes, we can save one function call and poking into the\ndevice.common fields below..\n\n> > +}\n> > +\n> > +static int hal_dev_open(const hw_module_t* module, const char* name,\n> > +\t\t\thw_device_t** device)\n> > +{\n> > +\tLOG(HAL, Debug) << \"Open camera: \" << name;\n>\n> s/camera:/camera/\n>\n> > +\n> > +\tint id = atoi(name);\n> > +\tCameraProxy *proxy = cameraManager.open(id, module);\n> > +\tif (!proxy) {\n> > +\t\tLOG(HAL, Error) << \"Failed to open camera module \" << id;\n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\tproxy->device()->common.close = hal_dev_close;\n> > +\t*device = &proxy->device()->common;\n> > +\n> > +\treturn 0;\n> > +}\n>\n> Would it make sense to define all these methods as static methods of the\n> CameraHalManager class, and turn the class into a singleton with an\n> instance() method ?\n>\n\nWhich methods? hal_dev_open? All the hal_dev_ ones which are used to\ninitialize the camera_module_t ? I'm not sure I see a clear win...\n\n> > +\n> > +static struct hw_module_methods_t hal_module_methods = {\n> > +\t.open = hal_dev_open,\n> > +};\n> > +\n> > +camera_module_t HAL_MODULE_INFO_SYM = {\n> > +\t.common = {\n> > +\t\t.tag = HARDWARE_MODULE_TAG,\n> > +\t\t.module_api_version = CAMERA_MODULE_API_VERSION_2_4,\n> > +\t\t.hal_api_version = HARDWARE_HAL_API_VERSION,\n> > +\t\t.id = CAMERA_HARDWARE_MODULE_ID,\n> > +\t\t.name = \"Libcamera Camera3HAL Module\",\n>\n> s/Libcamera/libcamera/\n>\n> > +\t\t.author = \"libcamera\",\n> > +\t\t.methods = &hal_module_methods,\n> > +\t\t.dso = nullptr,\n> > +\t\t.reserved = {},\n> > +\t},\n> > +\n> > +\t.get_number_of_cameras = hal_get_number_of_cameras,\n> > +\t.get_camera_info = hal_get_camera_info,\n> > +\t.set_callbacks = hal_set_callbacks,\n> > +\t.get_vendor_tag_ops = nullptr,\n> > +\t.open_legacy = hal_open_legacy,\n> > +\t.set_torch_mode = hal_set_torch_mode,\n> > +\t.init = hal_init,\n> > +\t.reserved = {},\n> > +};\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > new file mode 100644\n> > index 000000000000..734b5eebd9a5\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -0,0 +1,173 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_hal_manager.cpp - Libcamera Android Camera Manager\n> > + */\n> > +\n> > +#include \"camera_hal_manager.h\"\n> > +\n> > +#include \n>\n> This shouldn't be needed.\n>\n\nYes\n\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +\n> > +#include \"camera_module.h\"\n> > +#include \"camera_proxy.h\"\n> > +\n> > +using namespace libcamera;\n>\n> Missing blank line.\n>\n> > +LOG_DECLARE_CATEGORY(HAL);\n> > +\n> > +CameraHalManager::~CameraHalManager()\n> > +{\n> > +\tfor (auto metadata : staticMetadataMap_)\n> > +\t\tfree_camera_metadata(metadata.second);\n> > +}\n> > +\n> > +int CameraHalManager::initialize()\n> > +{\n> > +\t/*\n> > +\t * Thread::start() will create a std::thread which will execute\n> > +\t * CameraHalManager::run()\n> > +\t */\n>\n> That's documenting the libcamera::Thread class (and the std::thread is\n> an internal implementation detail). I would just state\n>\n> \t/*\n> \t * Start the camera HAL manager thread and wait until its\n> \t * initialisation completes to be fully operational before\n> \t * receiving calls from the camera stack.\n> \t */\n>\n> (thus dropping the next comment)\n>\n\nAs you might have noticed, the camera HAL still has comments and\nannotations I added while developing, I'll clear them up better.\n\n> > +\tstart();\n> > +\n> > +\t/*\n> > +\t * Wait for run() to notify us before returning to the caller to\n> > +\t * make sure libcamera is fully initialized before we start handling\n> > +\t * calls from the camera stack.\n> > +\t */\n> > +\tMutexLocker locker(mutex_);\n> > +\tcv_.wait(locker);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CameraHalManager::run()\n> > +{\n> > +\t/*\n> > +\t * The run() operation is scheduled in its own thread;\n> > +\t * It exec() to run the even dispatcher loop and initialize libcamera.\n>\n> s/even/event/\n>\n> > +\t *\n> > +\t * All the libcamera components initialized from here will be tied to\n> > +\t * the same thread as the here below run event dispatcher.\n>\n> The first paragraph also mostly documents the Thread class. I would drop\n> it, and write the second paragraph as\n>\n> \t/*\n> \t * All the libcamera components must be initialised here, in\n> \t * order to bind them to the camera HAL manager thread that\n> \t * executes the event dispatcher.\n> \t /\n>\n> > +\t */\n> > +\tlibcameraManager_ = libcamera::CameraManager::instance();\n> > +\n> > +\tint ret = libcameraManager_->start();\n> > +\tif (ret) {\n> > +\t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n> > +\t\t\t\t<< strerror(-ret);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * For each Camera registered in the system, a CameraModule\n> > +\t * get created here with an associated Camera and a proxy.\n>\n> s/get/gets/\n>\n> > +\t *\n> > +\t * \\todo Support camera hotplug.\n> > +\t */\n> > +\tunsigned int cameraIndex = 0;\n>\n> Maybe just index ?\n>\n> > +\tfor (auto camera : libcameraManager_->cameras()) {\n>\n> auto &camera\n>\n> otherwise camera will be a copy of the shared pointer instead of a\n> reference to it.\n>\n> > +\t\tcameras_.push_back(camera);\n>\n> The cameras_ vector isn't used after this. I would drop it, and store\n> the shared pointer in CameraModule.\n>\n\nCorrect, I missed this in the the latest rework of the proxies\nlifecycle.\n\n> > +\n> > +\t\tCameraModule *module = new CameraModule(cameraIndex,\n> > +\t\t\t\t\t\t\tcamera.get());\n> > +\t\tmodules_.emplace_back(module);\n> > +\n> > +\t\tCameraProxy *proxy = new CameraProxy(cameraIndex,\n> > +\t\t\t\t\t\t module);\n> > +\t\tproxies_.emplace_back(proxy);\n>\n> Similarly, how about sotring the module pointer inside the CameraProxy\n> class ? That would ensure that all accesses to the module go through the\n> proxy, which I think would be a good idea.\n\nAs I use the modules_ only to access the map of static metadata which\nshould now got away, I think it's doable\n\n>\n> \t\tCameraProxy *proxy = new CameraProxy(index, camera);\n> \t\tproxies_.emplace_back(proxy);\n>\n> > +\n> > +\t\t++cameraIndex;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * libcamera has been initialized! Unlock the initialize() caller\n> > +\t * as we're now ready to handle calls from the framework.\n> > +\t */\n> > +\tcv_.notify_one();\n> > +\n> > +\t/* Now start processing events and messages! */\n>\n> s/!/./ in the above two messages, there's no need to be\n> over-enthusiastic :-)\n>\n> > +\texec();\n> > +}\n> > +\n> > +CameraProxy *CameraHalManager::open(unsigned int id,\n> > +\t\t\t\t const hw_module_t *hardwareModule)\n> > +{\n> > +\tif (id < 0 || id >= numCameras()) {\n> > +\t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tCameraProxy *proxy = proxies_[id].get();\n> > +\tif (proxy->open(hardwareModule))\n> > +\t\treturn proxy;\n>\n> Shouldn't you return nullprtr here ?\n>\n\nIdeed\n\n> > +\n> > +\tLOG(HAL, Info) << \"Camera: '\" << id << \"' opened\";\n>\n> s/Camera:/Camera/\n>\n> > +\n> > +\treturn proxy;\n> > +}\n> > +\n> > +int CameraHalManager::close(CameraProxy *proxy)\n> > +{\n> > +\tproxy->close();\n> > +\tLOG(HAL, Info) << \"Close camera: \" << proxy->id();\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +unsigned int CameraHalManager::numCameras() const\n> > +{\n> > +\treturn libcameraManager_->cameras().size();\n> > +}\n> > +\n> > +/*\n> > + * Before the camera framework even tries to open a camera device with\n> > + * hal_dev_open() it requires the camera HAL to report a list of static\n> > + * informations on the camera device with id \\a id in the hal_get_camera_info()\n> > + * method.\n> > + *\n> > + * The static metadata should be then generated probably from a\n> > + * platform-specific module, as we cannot operate on the camera at this time as\n> > + * it has not yet been open by the framework.\n>\n> s/open/opened/\n>\n> What prevents us from opening the camera internally ? I don't think we\n\nIt would complicate the lifecycle handling a bit, but I guess it's\ndoable. AS of now, where all metadata are static it is not worth it\nimo\n\n> need a platform-specific module.\n>\n\nWe surely won't be able to get from kernel at this point all the\ninformation we need, in example, the camera position.\n\n> > + *\n> > + * This is what the Intel XML file is used for, it is parsed and the data there\n> > + * combined with informations from the PSL service (which I -think- it's what\n> > + * our IPA is) to generate a list of static metadata per-camera device.\n>\n> I'd drop this paragraph.\n>\n> > + */\n> > +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)\n> > +{\n> > +\tint cameras = numCameras();\n> > +\tif (id >= cameras || id < 0 || !info) {\n>\n> Can we be called with info == nullptr ? I think I'd drop that part of\n> the check.\n\nAll (most?) the extra-paranoid checks I have added here are error conditions\ntested by the cros_camera_test suite.\n\n>\n> > +\t\tLOG(HAL, Error) << \"Invalid camera id: \" << id;\n>\n> s/id:/id/\n>\n> You may also want to add quotes around the value, as done in\n> CameraHalManager::open() (or drop them there).\n>\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcamera_metadata_t *staticMetadata;\n> > +\tauto it = staticMetadataMap_.find(id);\n> > +\tif (it != staticMetadataMap_.end()) {\n> > +\t\tstaticMetadata = it->second;\n> > +\t} else {\n> > +\t\tCameraModule *cameraModule = modules_[id].get();\n> > +\n> > +\t\tstaticMetadata = cameraModule->getStaticMetadata();\n> > +\t\tstaticMetadataMap_[id] = staticMetadata;\n>\n> Why do you need the map, why can't you always retrieve the static\n> metadata from the CameraModule ?\n>\n\nI should, yes, so we can frop modules_\n\n> > +\t}\n> > +\n> > +\t/* \\todo: Get these info dynamically inspecting the camera module. */\n>\n> s/://\n>\n> > +\tinfo->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;\n> > +\tinfo->orientation = 0;\n> > +\tinfo->device_version = 0;\n> > +\tinfo->resource_cost = 0;\n> > +\tinfo->static_camera_characteristics = staticMetadata;\n> > +\tinfo->conflicting_devices = nullptr;\n> > +\tinfo->conflicting_devices_length = 0;\n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > new file mode 100644\n> > index 000000000000..65d6fa3150ef\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_manager.h\n> > @@ -0,0 +1,56 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_hal_manager.h - Libcamera Android Camera Manager\n> > + */\n> > +#ifndef __ANDROID_CAMERA_MANAGER_H__\n> > +#define __ANDROID_CAMERA_MANAGER_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"thread.h\"\n> > +#include \"thread_rpc.h\"\n> > +\n> > +class CameraModule;\n> > +class CameraProxy;\n> > +\n> > +class CameraHalManager : public libcamera::Thread\n> > +{\n> > +public:\n> > +\t~CameraHalManager();\n> > +\n> > +\tint initialize();\n>\n> Our initialisation methods are usually called init(), not initialize().\n\nok\n\n>\n> > +\n> > +\tCameraProxy *open(unsigned int id, const hw_module_t *module);\n> > +\tint close(CameraProxy *proxy);\n> > +\n> > +\tunsigned int numCameras() const;\n> > +\tint getCameraInfo(int id, struct camera_info *info);\n> > +\n> > +private:\n> > +\tvoid run() override;\n> > +\tcamera_metadata_t *getStaticMetadata(unsigned int id);\n> > +\n> > +\tlibcamera::CameraManager *libcameraManager_;\n>\n> I think you can call this manager_.\n>\n> > +\n> > +\tstd::mutex mutex_;\n> > +\tstd::condition_variable cv_;\n> > +\n> > +\tstd::vector> cameras_;\n> > +\tstd::vector> modules_;\n> > +\tstd::vector> proxies_;\n> > +\n> > +\tstd::map staticMetadataMap_;\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_MANAGER_H__ */\n> > diff --git a/src/android/camera_module.cpp b/src/android/camera_module.cpp\n> > new file mode 100644\n> > index 000000000000..b64e377fb439\n> > --- /dev/null\n> > +++ b/src/android/camera_module.cpp\n> > @@ -0,0 +1,799 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_module.cpp - Libcamera Android Camera Module\n> > + */\n> > +\n> > +#include \"camera_module.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n>\n> I would group all the Android headers together, so\n>\n> #include \n> #include \n>\n> #include \n>\n> > +#include \n> > +\n> > +#include \"message.h\"\n> > +\n> > +#include \"log.h\"\n>\n> #include \"log.h\"\n> #include \"message.h\"\n>\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL);\n> > +\n> > +CameraModule::CameraModule(unsigned int id, libcamera::Camera *camera)\n> > +\t: id_(id), camera_(camera), cameraState_(CameraStopped),\n> > +\t requestTemplate_(nullptr)\n> > +{\n> > +\tcamera_->requestCompleted.connect(this,\n> > +\t\t\t\t\t &CameraModule::requestComplete);\n>\n> This holds on a single line.\n>\n> > +}\n> > +\n> > +CameraModule::~CameraModule()\n> > +{\n> > +\tif (requestTemplate_)\n> > +\t\tfree_camera_metadata(requestTemplate_);\n> > +\trequestTemplate_ = nullptr;\n> > +}\n> > +\n> > +void CameraModule::message(Message *message)\n> > +{\n> > +\tThreadRpcMessage *rpcMessage = static_cast\n> > +\t\t\t\t (message);\n> > +\n> > +\tif (rpcMessage->type() != ThreadRpcMessage::type())\n> > +\t\treturn Object::message(message);\n>\n> As Message may not be a ThreadRpcMessage, you should write this as\n>\n> \tif (message->type() != ThreadRpcMessage::type())\n> \t\treturn Object::message(message);\n>\n> \tThreadRpcMessage *rpcMessage = static_cast(message);\n>\n> > +\n> > +\tThreadRpc *rpc = rpcMessage->rpc;\n> > +\n> > +\tswitch (rpc->tag) {\n> > +\tcase ThreadRpc::ProcessCaptureRequest:\n> > +\t\tprocessCaptureRequest(rpc->request);\n> > +\t\tbreak;\n> > +\tcase ThreadRpc::Close:\n> > +\t\tclose();\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tLOG(HAL, Error) << \"Unknown RPC operation: \" << rpc->tag;\n> > +\t}\n> > +\n> > +\trpc->notifyReception();\n> > +}\n> > +\n> > +int CameraModule::open()\n> > +{\n> > +\tint ret = camera_->acquire();\n> > +\tif (ret) {\n> > +\t\tLOG(HAL, Error) << \"Failed to acquire the camera\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CameraModule::close()\n> > +{\n> > +\tcamera_->stop();\n> > +\n> > +\tcamera_->freeBuffers();\n> > +\tcamera_->release();\n> > +\n> > +\tcameraState_ = CameraStopped;\n> > +}\n> > +\n> > +void CameraModule::setCallbacks(const struct camera3_device *camera3Device,\n> > +\t\t\t\tconst camera3_callback_ops_t *callbacks)\n> > +{\n> > +\tcamera3Device_ = camera3Device;\n>\n> camera3Device_ seems unused.\n>\n> > +\tcallbacks_ = callbacks;\n> > +}\n> > +\n> > +camera_metadata_t *CameraModule::getStaticMetadata()\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * The below metadata has been added by inspecting the Intel XML\n> > +\t * file and it's associated parsing routines in the Intel IPU3 HAL.\n> > +\t *\n> > +\t * The here below metadata are enough to satisfy the camera3-test\n> > +\t * provided by ChromeOS, but a real camera implementation might require\n>\n> s/might/will/\n>\n> > +\t * more.\n> > +\t *\n> > +\t * See CameraConf::AiqConf class implementation in the Intel HAL\n> > +\t * to get a list of the static metadata registered by parsing the\n> > +\t * XML config file in AiqConf::fillStaticMetadataFromCMC\n> > +\t */\n> > +\n> > +\t/*\n> > +\t * FIXME: this sizes come from the Intel IPU3 HAL, and are way too large for\n>\n> s/FIXME:/\\todo/\n> s/this/these/\n>\n> > +\t * this intial HAL version.\n> > +\t */\n> > +\t#define STATIC_ENTRY_CAP 256\n> > +\t#define STATIC_DATA_CAP 6688\n> > +\tcamera_metadata_t *staticMetadata =\n> > +\t\tallocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);\n> > +\n> > +\t/* Sensor static metadata. */\n> > +\tint32_t pixelArraySize[] = {\n> > +\t\t2592, 1944,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > +\t\t\t\t&pixelArraySize, 2);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t sensorSizes[] = {\n> > +\t\t0, 0, 2560, 1920,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > +\t\t\t\t&sensorSizes, 4);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t sensitivityRange[] = {\n> > +\t\t32, 2400,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> > +\t\t\t\t&sensitivityRange, 2);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > +\t\t\t\t&filterArr, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint64_t exposureTimeRange[] = {\n> > +\t\t100000, 200000000,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > +\t\t\t\t&exposureTimeRange, 2);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t orientation = 0;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_SENSOR_ORIENTATION,\n> > +\t\t\t\t&orientation, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/* Flash static metadata. */\n> > +\tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_FLASH_INFO_AVAILABLE,\n> > +\t\t\t&flashAvailable, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/* Lens static metadata. */\n> > +\tfloat fn = 2.53 / 100;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/* Control metadata. */\n> > +\tchar controlMetadata = ANDROID_CONTROL_MODE_AUTO;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> > +\t\t\t&controlMetadata, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tchar availableAntiBandingModes[] = {\n> > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,\n> > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,\n> > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,\n> > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > +\t\t\tavailableAntiBandingModes, 4);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tchar aeAvailableModes[] = {\n> > +\t\tANDROID_CONTROL_AE_MODE_ON,\n> > +\t\tANDROID_CONTROL_AE_MODE_OFF,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > +\t\t\taeAvailableModes, 2);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tcontrolMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > +\t\t\t&controlMetadata, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > +\t\t\t&awbLockAvailable, 1);\n> > +\n> > +\t/* Scaler static metadata. */\n> > +\tstd::vector availableStreamFormats = {\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB,\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> > +\t\t\tavailableStreamFormats.data(),\n> > +\t\t\tavailableStreamFormats.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tstd::vector availableStreamConfigurations = {\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,\n> > +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,\n> > +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,\n> > +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> > +\t\t\tavailableStreamConfigurations.data(),\n> > +\t\t\tavailableStreamConfigurations.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tstd::vector availableStallDurations = {\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> > +\t\t\tavailableStallDurations.data(),\n> > +\t\t\tavailableStallDurations.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tstd::vector minFrameDurations = {\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,\n> > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> > +\t\t\tminFrameDurations.data(), minFrameDurations.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/* Info static metadata. */\n> > +\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\tret = add_camera_metadata_entry(staticMetadata,\n> > +\t\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > +\t\t\t&supportedHWLevel, 1);\n> > +\n> > +\treturn staticMetadata;\n> > +}\n> > +\n> > +const camera_metadata_t *CameraModule::constructDefaultRequestMetadata(int type)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * TODO: inspect type and pick the right metadata pack.\n>\n> s/TODO:/\\todo/\n>\n> > +\t * As of now just use a single one for all templates\n> > +\t */\n> > +\tuint8_t captureIntent;\n> > +\tswitch (type) {\n> > +\tcase CAMERA3_TEMPLATE_PREVIEW:\n> > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;\n> > +\t\tbreak;\n> > +\tcase CAMERA3_TEMPLATE_STILL_CAPTURE:\n> > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;\n> > +\t\tbreak;\n> > +\tcase CAMERA3_TEMPLATE_VIDEO_RECORD:\n> > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;\n> > +\t\tbreak;\n> > +\tcase CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:\n> > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;\n> > +\t\tbreak;\n> > +\tcase CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:\n> > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;\n> > +\t\tbreak;\n> > +\tcase CAMERA3_TEMPLATE_MANUAL:\n> > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tLOG(HAL, Error) << \"Invalid template request type: \" << type;\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tif (requestTemplate_)\n> > +\t\treturn requestTemplate_;\n> > +\n> > +\t/* FIXME: this sizes are quite arbitrary ones */\n> > +\t#define REQUEST_TEMPLATE_ENTRIES\t 30\n> > +\t#define REQUEST_TEMPLATE_DATA\t\t2048\n> > +\trequestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,\n> > +\t\t\t\t\t\t REQUEST_TEMPLATE_DATA);\n> > +\tif (!requestTemplate_) {\n> > +\t\tLOG(HAL, Error) << \"Failed to allocate template metadata\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Fill in the required Request metadata.\n> > +\t *\n> > +\t * Part (most?) of this entries comes from inspecting the Intel's IPU3\n> > +\t * HAL xml configuration file.\n> > +\t */\n> > +\n> > +\t/* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */\n> > +\tint32_t maxOutStream[] = { 0, 2, 0 };\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > +\t\t\tmaxOutStream, 3);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t maxPipelineDepth = 5;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> > +\t\t\t&maxPipelineDepth, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t inputStreams = 0;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> > +\t\t\t&inputStreams, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t partialResultCount = 1;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > +\t\t\t&partialResultCount, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t availableCapabilities[] = {\n> > +\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > +\t\t\tavailableCapabilities, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AE_MODE,\n> > +\t\t\t&aeMode, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t aeExposureCompensation = 0;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > +\t\t\t&aeExposureCompensation, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> > +\t\t\t&aePrecaptureTrigger, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AE_LOCK,\n> > +\t\t\t&aeLock, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AF_TRIGGER,\n> > +\t\t\t&afTrigger, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AWB_MODE,\n> > +\t\t\t&awbMode, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AWB_LOCK,\n> > +\t\t\t&awbLock, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > +\t\t\t&awbLockAvailable, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t flashMode = ANDROID_FLASH_MODE_OFF;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_FLASH_MODE,\n> > +\t\t\t&flashMode, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> > +\t\t\t&faceDetectMode, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> > +\t\t\t&captureIntent, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/*\n> > +\t * This is quite hard to list at the moment wihtout knowing what\n> > +\t * we could control.\n> > +\t *\n> > +\t * For now, just list in the available Request keys and in the available\n> > +\t * result keys the control and reporting of the AE algorithm.\n> > +\t */\n> > +\tstd::vector availableRequestKeys = {\n> > +\t\tANDROID_CONTROL_AE_MODE,\n> > +\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > +\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> > +\t\tANDROID_CONTROL_AE_LOCK,\n> > +\t\tANDROID_CONTROL_AF_TRIGGER,\n> > +\t\tANDROID_CONTROL_AWB_MODE,\n> > +\t\tANDROID_CONTROL_AWB_LOCK,\n> > +\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > +\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> > +\t\tANDROID_FLASH_MODE,\n> > +\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> > +\t};\n> > +\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,\n> > +\t\t\tavailableRequestKeys.data(),\n> > +\t\t\tavailableRequestKeys.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tstd::vector availableResultKeys = {\n> > +\t\tANDROID_CONTROL_AE_MODE,\n> > +\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > +\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> > +\t\tANDROID_CONTROL_AE_LOCK,\n> > +\t\tANDROID_CONTROL_AF_TRIGGER,\n> > +\t\tANDROID_CONTROL_AWB_MODE,\n> > +\t\tANDROID_CONTROL_AWB_LOCK,\n> > +\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > +\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> > +\t\tANDROID_FLASH_MODE,\n> > +\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n> > +\t\t\tavailableResultKeys.data(),\n> > +\t\t\tavailableResultKeys.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/*\n> > +\t * The available characteristics should be the tags reported\n> > +\t * as part of the static metadata reported at hal_get_camera_info()\n> > +\t * time. The xml file sets those to 0 though.\n> > +\t */\n> > +\tstd::vector availableCharacteristicsKeys = {};\n> > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > +\t\t\tANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > +\t\t\tavailableCharacteristicsKeys.data(),\n> > +\t\t\tavailableCharacteristicsKeys.size());\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\treturn requestTemplate_;\n> > +}\n> > +\n> > +/**\n> > + * Inspect the stream_list to produce a list of StreamConfiguration to\n> > + * be use to configure the Camera.\n> > + */\n> > +int CameraModule::configureStreams(camera3_stream_configuration_t *stream_list)\n> > +{\n> > +\n>\n> Extra blank line.\n>\n> > +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> > +\n> > +\t\tLOG(HAL, Info) << \"Stream #\" << i\n> > +\t\t\t << \": direction: \" << stream->stream_type\n> > +\t\t\t << \" - width: \" << stream->width\n> > +\t\t\t << \" - height: \" << stream->height\n> > +\t\t\t << \" - format: \" << std::hex << stream->format;\n>\n> s/ -/,/ in the above three lines\n>\n> And I would print size: ...x... instead of width and height.\n>\n> > +\t}\n> > +\n> > +\t/* Hardcode viewfinder role, collecting sizes from the stream config. */\n> > +\tif (stream_list->num_streams != 1) {\n> > +\t\tLOG(HAL, Error) << \"Only one stream supported\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tStreamRoles roles = {};\n> > +\troles.push_back(StreamRole::StillCapture);\n>\n> I think you can write this as\n>\n> \tStreamRoles roles{ StreamRole::StillCapture };\n>\n> > +\n> > +\tconfig_ = camera_->generateConfiguration(roles);\n> > +\tif (!config_ || config_->empty()) {\n> > +\t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/* Only one stream is supported. */\n> > +\tcamera3_stream_t *camera3Stream = stream_list->streams[0];\n> > +\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> > +\tstreamConfiguration->size.width = camera3Stream->width;\n> > +\tstreamConfiguration->size.height = camera3Stream->height;\n> > +\tstreamConfiguration->memoryType = ExternalMemory;\n> > +\n> > +\t/*\n> > +\t * FIXME: do not change the pixel format from the one returned\n> > +\t * from the Camera::generateConfiguration();\n> > +\t *\n> > +\t * We'll need to translate from Android defined pixel format codes\n> > +\t * to whatever libcamera will use.\n> > +\t */\n> > +\n> > +\tswitch (config_->validate()) {\n> > +\tcase CameraConfiguration::Valid:\n> > +\t\tbreak;\n> > +\tcase CameraConfiguration::Adjusted:\n> > +\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n>\n> Missing config_.reset() ?\n>\n> > +\t\treturn -EINVAL;\n> > +\tcase CameraConfiguration::Invalid:\n> > +\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> > +\t\tconfig_.reset();\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcamera3Stream->max_buffers = streamConfiguration->bufferCount;\n> > +\n> > +\t/*\n> > +\t * Once the CameraConfiguration has been adjusted/validated\n> > +\t * it can be applied to the camera.\n> > +\t */\n> > +\tint ret = camera_->configure(config_.get());\n> > +\tif (ret) {\n> > +\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> > +\t\t\t\t<< camera_->name() << \"'\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraModule::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > +{\n> > +\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> > +\tStream *stream = streamConfiguration->stream();\n> > +\n> > +\tif (camera3Request->num_output_buffers != 1) {\n> > +\t\tLOG(HAL, Error) << \"Invalid number of output buffers: \"\n> > +\t\t\t\t<< camera3Request->num_output_buffers;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/* Start the camera if that's the first request we handle. */\n> > +\tif (cameraState_ == CameraStopped) {\n> > +\t\tint ret = camera_->allocateBuffers();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(HAL, Error) << \"Failed to allocate buffers\";\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tret = camera_->start();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > +\t\t\tcamera_->freeBuffers();\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tcameraState_ = CameraRunning;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Queue a request for the Camera with the provided dmabuf file\n> > +\t * descriptors.\n> > +\t */\n> > +\tconst camera3_stream_buffer_t *camera3Buffer =\n> > +\t\t\t\t\t&camera3Request->output_buffers[0];\n> > +\tconst buffer_handle_t camera3Handle = *camera3Buffer->buffer;\n> > +\n> > +\tstd::array fds = {\n> > +\t\tcamera3Handle->data[0],\n> > +\t\tcamera3Handle->data[1],\n> > +\t\tcamera3Handle->data[2],\n> > +\t};\n> > +\tstd::unique_ptr buffer = stream->createBuffer(fds);\n> > +\tif (!buffer) {\n> > +\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tRequest *request = camera_->createRequest();\n> > +\trequest->addBuffer(std::move(buffer));\n> > +\tint ret = camera_->queueRequest(request);\n> > +\tif (ret) {\n> > +\t\tLOG(HAL, Error) << \"Failed to queue request\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Save the request descriptors for use at completion time. */\n> > +\tCamera3RequestDescriptor descriptor = {};\n> > +\tdescriptor.frameNumber = camera3Request->frame_number,\n> > +\tdescriptor.numBuffers = camera3Request->num_output_buffers,\n>\n> s/,$/;/ for the two lines above.\n>\n> I don't think you need to store numBuffers in the descriptors, it can be\n> retrieved from buffers.size() in requestComplete().\n\n\nnumBuffers is the framework required number of buffers, buffers.size()\nis the number of buffers completed in the request. They -should- be\nthe same, but I would keep them separate to make sure the completed\nrequest is correct.\n\n>\n>\n> > +\tdescriptor.buffers = new camera3_stream_buffer_t[descriptor.numBuffers];\n> > +\tfor (unsigned int i = 0; i < descriptor.numBuffers; ++i) {\n> > +\t\tcamera3_stream_buffer_t &buffer = descriptor.buffers[i];\n> > +\t\tbuffer = camera3Request->output_buffers[i];\n>\n> Just\n>\n> \t\tdescriptor.buffers[i] = camera3Request->output_buffers[i];\n> > +\t}\n> > +\n> > +\trequestMap_[request] = descriptor;\n>\n> How about using the request cookie (passed to Camera::createRequest())\n> for this purpose ?\n>\n\nCan we do that on top?\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CameraModule::requestComplete(Request *request,\n> > +\t\t\t\t const std::map &buffers)\n> > +{\n> > +\tBuffer *libcameraBuffer = buffers.begin()->second;\n> > +\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > +\tcamera_metadata_t *resultMetadata = nullptr;\n> > +\n> > +\tif (request->status() != Request::RequestComplete) {\n> > +\t\tLOG(HAL, Error) << \"Request not succesfully completed: \"\n> > +\t\t\t\t<< request->status();\n> > +\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t}\n> > +\n> > +\t/* Prepare to call back the Android camera stack. */\n> > +\tCamera3RequestDescriptor &descriptor = requestMap_[request];\n> > +\n> > +\tcamera3_capture_result_t captureResult = {};\n> > +\tcaptureResult.frame_number = descriptor.frameNumber;\n> > +\tcaptureResult.num_output_buffers = descriptor.numBuffers;\n> > +\n> > +\tcamera3_stream_buffer_t *resultBuffers =\n> > +\t\tnew camera3_stream_buffer_t[descriptor.numBuffers];\n> > +\tfor (unsigned int i = 0; i < descriptor.numBuffers; ++i) {\n> > +\t\tcamera3_stream_buffer_t resultBuffer;\n> > +\n> > +\t\tresultBuffer = descriptor.buffers[i];\n> > +\t\tresultBuffer.acquire_fence = -1;\n> > +\t\tresultBuffer.release_fence = -1;\n> > +\t\tresultBuffer.status = status;\n> > +\n> > +\t\tresultBuffers[i] = resultBuffer;\n> > +\t}\n> > +\tcaptureResult.output_buffers =\n> > +\t\tconst_cast(resultBuffers);\n> > +\n> > +\tif (status == CAMERA3_BUFFER_STATUS_ERROR) {\n> > +\t\t/* \\todo Improve error handling. */\n> > +\t\tnotifyError(descriptor.frameNumber, resultBuffers->stream);\n> > +\t} else {\n> > +\t\tnotifyShutter(descriptor.frameNumber, libcameraBuffer->timestamp());\n> > +\n> > +\t\tcaptureResult.partial_result = 1;\n> > +\t\tresultMetadata = getResultMetadata(descriptor.frameNumber,\n> > +\t\t\t\t\t\t libcameraBuffer->timestamp());\n> > +\t\tcaptureResult.result = resultMetadata;\n> > +\t}\n> > +\n> > +\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > +\n> > +\tif (resultMetadata)\n> > +\t\tfree_camera_metadata(resultMetadata);\n> > +\n> > +\tdelete[] resultBuffers;\n> > +\tdelete[] descriptor.buffers;\n> > +\trequestMap_.erase(request);\n> > +\n> > +\treturn;\n> > +}\n> > +\n> > +void CameraModule::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> > +{\n> > +\tcamera3_notify_msg_t notify = {};\n> > +\n> > +\tnotify.type = CAMERA3_MSG_SHUTTER;\n> > +\tnotify.message.shutter.frame_number = frameNumber;\n> > +\tnotify.message.shutter.timestamp = timestamp;\n> > +\n> > +\tcallbacks_->notify(callbacks_, ¬ify);\n> > +}\n> > +\n> > +void CameraModule::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > +{\n> > +\tcamera3_notify_msg_t notify = {};\n> > +\n> > +\tnotify.type = CAMERA3_MSG_ERROR;\n> > +\tnotify.message.error.error_stream = stream;\n> > +\tnotify.message.error.frame_number = frameNumber;\n> > +\tnotify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > +\n> > +\tcallbacks_->notify(callbacks_, ¬ify);\n> > +}\n> > +\n> > +/*\n> > + * Fixed result metadata, mostly imported from the UVC camera HAL, which\n> > + * does not produces metadata and thus needs to generate a fixed set.\n> > + */\n> > +camera_metadata_t *CameraModule::getResultMetadata(int frame_number,\n> > +\t\t\t\t\t\t int64_t timestamp)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/* FIXME: random \"big enough\" values. */\n> > +\t#define RESULT_ENTRY_CAP 256\n> > +\t#define RESULT_DATA_CAP 6688\n> > +\tcamera_metadata_t *resultMetadata =\n> > +\t\tallocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);\n> > +\n> > +\tconst uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> > +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,\n> > +\t\t\t\t\t&ae_state, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tconst uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;\n> > +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,\n> > +\t\t\t\t\t&ae_lock, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tuint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;\n> > +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,\n> > +\t\t\t\t\t&af_state, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tconst uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_CONTROL_AWB_STATE,\n> > +\t\t\t\t\t&awb_state, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tconst uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_CONTROL_AWB_LOCK,\n> > +\t\t\t\t\t&awb_lock, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tconst uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_LENS_STATE,\n> > +\t\t\t\t\t&lens_state, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tint32_t sensorSizes[] = {\n> > +\t\t0, 0, 2560, 1920,\n> > +\t};\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_SCALER_CROP_REGION,\n> > +\t\t\t\t\tsensorSizes, 4);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_SENSOR_TIMESTAMP,\n> > +\t\t\t\t\t×tamp, 1);\n> > +\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/* 33.3 msec */\n> > +\tconst int64_t rolling_shutter_skew = 33300000;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_SENSOR_ROLLING_SHUTTER_SKEW,\n> > +\t\t\t\t\t&rolling_shutter_skew, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\t/* 16.6 msec */\n> > +\tconst int64_t exposure_time = 16600000;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_SENSOR_EXPOSURE_TIME,\n> > +\t\t\t\t\t&exposure_time, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tconst uint8_t lens_shading_map_mode =\n> > +\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> > +\t\t\t\t\t&lens_shading_map_mode, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\tconst uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;\n> > +\tret = add_camera_metadata_entry(resultMetadata,\n> > +\t\t\t\t\tANDROID_STATISTICS_SCENE_FLICKER,\n> > +\t\t\t\t\t&scene_flicker, 1);\n> > +\tMETADATA_ASSERT(ret);\n> > +\n> > +\treturn resultMetadata;\n> > +}\n> > diff --git a/src/android/camera_module.h b/src/android/camera_module.h\n> > new file mode 100644\n> > index 000000000000..67488d5940b1\n> > --- /dev/null\n> > +++ b/src/android/camera_module.h\n> > @@ -0,0 +1,75 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_module.h - Libcamera Android Camera Module\n>\n> s/Libcamera/libcamera/\n>\n> (here and everywhere else, libcamera is always written in all lowercase)\n>\n> > + */\n> > +#ifndef __ANDROID_CAMERA_MODULE_H__\n> > +#define __ANDROID_CAMERA_MODULE_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"message.h\"\n> > +\n> > +#include \"camera_hal_manager.h\"\n> > +\n> > +#define METADATA_ASSERT(_r)\t\t\\\n> > +\tdo {\t\t\t\t\\\n> > +\t\tif (!(_r)) break;\t\\\n> > +\t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> > +\t\treturn nullptr;\t\t\\\n> > +\t} while(0);\n>\n> I really, really dislike hiding return statements in macros. We can keep\n> it for now, but please add a comment telling that it should be removed.\n>\n\nAll the metadata handling will be nuked in the next versions, so I\nwon't bother for now.\n\n> > +\n>\n> Even if we don't document the whole implementation with Doxygen, I think\n> a few short comments that explain what each class models would be\n> useful. Otherwise the reader is left to wonder what CameraModule or\n> CameraProxy stand for. A comment that explains how the various objects\n> work together would be useful too.\n>\n> > +class CameraModule : public libcamera::Object\n>\n> I don't think CameraModule is a good name :-/ In HAL terminology, the\n> module refers to camera_module_t, and corresponds more or less to\n> libcamera::CameraManager, not libcamera::Camera.\n\nCorrect\n\n>\n> One option would be to name this class Camera in a HAL namespace. That\n> would make it quite clear that it corresponds to a libcamra::Camera.\n\nI had a go at isolating namespaces and I have problems with the\nlogging infrastructure, and I would not duplicate the Camera name to\nbe honest. CameraDevice ?\n\n>\n> > +{\n> > +public:\n> > +\tCameraModule(unsigned int id, libcamera::Camera *camera);\n> > +\t~CameraModule();\n> > +\n> > +\tvoid message(libcamera::Message *message);\n> > +\n> > +\tint open();\n> > +\tvoid close();\n> > +\tvoid setCallbacks(const struct camera3_device *cameraDevice,\n> > +\t\t\t const camera3_callback_ops_t *callbacks);\n> > +\tcamera_metadata_t *getStaticMetadata();\n> > +\tconst camera_metadata_t *constructDefaultRequestMetadata(int type);\n> > +\tint configureStreams(camera3_stream_configuration_t *stream_list);\n> > +\tint processCaptureRequest(camera3_capture_request_t *request);\n> > +\tvoid requestComplete(libcamera::Request *request,\n> > +\t\t\t const std::map &buffers);\n> > +\n> > +\tunsigned int id() const { return id_; }\n>\n> I think this method is not used.\n>\n> > +\n> > +private:\n> > +\tstruct Camera3RequestDescriptor {\n> > +\t\tuint32_t frameNumber;\n> > +\t\tuint32_t numBuffers;\n> > +\t\tcamera3_stream_buffer_t *buffers;\n> > +\t};\n> > +\n> > +\tenum CameraState {\n> > +\t\tCameraStopped,\n> > +\t\tCameraRunning,\n> > +\t};\n>\n> With just two states you could instead have a bool running_ field.\n>\n> > +\n> > +\tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > +\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > +\tcamera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);\n> > +\n> > +\tunsigned int id_;\n> > +\tlibcamera::Camera *camera_;\n> > +\tstd::unique_ptr config_;\n> > +\tCameraState cameraState_;\n> > +\n> > +\tcamera_metadata_t *requestTemplate_;\n> > +\tconst camera3_callback_ops_t *callbacks_;\n> > +\tconst struct camera3_device *camera3Device_;\n> > +\n> > +\tstd::map requestMap_;\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_MODULE_H__ */\n> > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp\n> > new file mode 100644\n> > index 000000000000..af7817f29137\n> > --- /dev/null\n> > +++ b/src/android/camera_proxy.cpp\n> > @@ -0,0 +1,181 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_proxy.cpp - Proxy to camera module instances\n> > + */\n> > +\n> > +#include \"camera_proxy.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"message.h\"\n> > +#include \"thread_rpc.h\"\n> > +#include \"utils.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(HAL);\n> > +\n> > +#define LIBCAMERA_HAL_FUNC_DBG\t\\\n> > +\tLOG(HAL, Debug);\n>\n> If we need to trace calls, I think something better than debug messages\n> would be useful. I'm not sure what yet though.\n>\n\nYeah, I'm not even sure we want to trace calls actually. The fact is\nthat the camera HAL is somewhat a weird beast, as it is a very system\nspecific libcamera extension, and I tried to mimic what is usually\nfound in other HAL implementations I've seen. Should we drop tracing\ncompletely and make this more libcamera-like ?\n\n> > +\n> > +static int hal_dev_initialize(const struct camera3_device *dev,\n> > +\t\t\t const camera3_callback_ops_t *callback_ops)\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\tif (!dev)\n> > +\t\treturn -EINVAL;\n>\n> Can this happen ?\n\nI think cros_camera_test exercises this.\n>\n> > +\n> > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > +\tproxy->setCallbacks(callback_ops);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int hal_dev_configure_streams(const struct camera3_device *dev,\n> > +\t\t\t\t camera3_stream_configuration_t *stream_list)\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\tif (!dev)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > +\tproxy->configureStreams(stream_list);\n> > +\n> > +\treturn 0;\n>\n> Shouldn't you propagate the error value from proxy->configureStreams() ?\n>\n> > +}\n> > +\n> > +static const camera_metadata_t *\n> > +hal_dev_construct_default_request_settings(const struct camera3_device *dev,\n> > +\t\t\t\t\t int type)\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\tif (!dev)\n> > +\t\treturn nullptr;\n> > +\n> > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > +\treturn proxy->constructDefaultRequest(type);\n>\n> How about renaming constructDefaultRequest() and\n> constructDefaultRequestMetadata() to constructDefaultRequestSettings(),\n> in order to match the HAL operation name ?\n\nOk...\n\n>\n> > +}\n> > +\n> > +static int hal_dev_process_capture_request(const struct camera3_device *dev,\n> > +\t\t\t\t\t camera3_capture_request_t *request)\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\tif (!dev)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > +\treturn proxy->processCaptureRequest(request);\n> > +}\n> > +\n> > +static void hal_dev_dump(const struct camera3_device *dev, int fd)\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +}\n> > +\n> > +static int hal_dev_flush(const struct camera3_device *dev)\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\treturn 0;\n> > +}\n>\n> You could define these methods as static methods of the CameraProxy\n> class. For instance, hal_dev_configure_streams would become\n>\n> int CameraProxy::configureStreams(const struct camera3_device *dev,\n> \t\t\t\t camera3_stream_configuration_t *stream_list)\n> {\n> \tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> \tproxy->configureStreams(stream_list);\n> }\n\nTo avoid the static C methods you mean ? Not sure what is the gain\nthough.\n\n>\n> > +\n> > +static camera3_device_ops hal_dev_ops = {\n>\n> I wish this could be const :-(\n>\n> > +\t.initialize = hal_dev_initialize,\n> > +\t.configure_streams = hal_dev_configure_streams,\n> > +\t.register_stream_buffers = nullptr,\n> > +\t.construct_default_request_settings = hal_dev_construct_default_request_settings,\n> > +\t.process_capture_request = hal_dev_process_capture_request,\n> > +\t.get_metadata_vendor_tag_ops = nullptr,\n> > +\t.dump = hal_dev_dump,\n> > +\t.flush = hal_dev_flush,\n> > +\t.reserved = { 0 },\n>\n> s/0/nullptr/ ?\n>\n> > +};\n> > +\n> > +CameraProxy::CameraProxy(unsigned int id, CameraModule *cameraModule)\n> > +\t: open_(false), id_(id), cameraModule_(cameraModule)\n> > +{\n> > +}\n> > +\n> > +int CameraProxy::open(const hw_module_t *hardwareModule)\n> > +{\n> > +\tint ret = cameraModule_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Initialize the hw_device_t in the instance camera3_module_t. */\n> > +\tcameraDevice_.common.tag = HARDWARE_DEVICE_TAG;\n> > +\tcameraDevice_.common.version = CAMERA_DEVICE_API_VERSION_3_3;\n> > +\tcameraDevice_.common.module = (hw_module_t *)hardwareModule;\n> > +\n> > +\t/*\n> > +\t * The camera device operations. These actually implement\n> > +\t * the Android Camera HALv3 interface.\n> > +\t */\n> > +\tcameraDevice_.ops = &hal_dev_ops;\n> > +\tcameraDevice_.priv = this;\n> > +\n> > +\topen_ = true;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CameraProxy::close()\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\tThreadRpc rpcRequest;\n> > +\trpcRequest.tag = ThreadRpc::Close;\n> > +\n> > +\tthreadRpcCall(rpcRequest);\n> > +\n> > +\topen_ = false;\n> > +}\n> > +\n> > +void CameraProxy::setCallbacks(const camera3_callback_ops_t *callbacks)\n>\n> I would name this initialize() to match hal_dev_initialize().\n>\n> > +{\n> > +\tLIBCAMERA_HAL_FUNC_DBG\n> > +\n> > +\tcameraModule_->setCallbacks(&cameraDevice_, callbacks);\n> > +}\n> > +\n> > +const camera_metadata_t *CameraProxy::constructDefaultRequest(int type)\n> > +{\n> > +\treturn cameraModule_->constructDefaultRequestMetadata(type);\n> > +}\n> > +\n> > +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)\n> > +{\n> > +\treturn cameraModule_->configureStreams(stream_list);\n> > +}\n> > +\n> > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)\n> > +{\n> > +\tThreadRpc rpcRequest;\n> > +\trpcRequest.tag = ThreadRpc::ProcessCaptureRequest;\n> > +\trpcRequest.request = request;\n> > +\n> > +\tthreadRpcCall(rpcRequest);\n> > +\n> > +\treturn 0;\n>\n> Does this method need to be synchronous ? We can keep it as-is for now,\n> but I wonder if it wouldn't make sense to later move (part of) the\n> validation code here and make the call asynchronous.\n>\n> > +}\n> > +\n> > +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)\n> > +{\n> > +\tstd::unique_ptr message =\n> > +\t\t\t\tutils::make_unique();\n> > +\tmessage->rpc = &rpcRequest;\n> > +\n> > +\tcameraModule_->postMessage(std::move(message));\n> > +\trpcRequest.waitDelivery();\n> > +}\n> > diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h\n> > new file mode 100644\n> > index 000000000000..69e6878c4352\n> > --- /dev/null\n> > +++ b/src/android/camera_proxy.h\n> > @@ -0,0 +1,41 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * camera_proxy.h - Proxy to camera module instances\n> > + */\n> > +#ifndef __ANDROID_CAMERA_PROXY_H__\n> > +#define __ANDROID_CAMERA_PROXY_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_module.h\"\n> > +\n> > +class CameraProxy\n> > +{\n> > +public:\n> > +\tCameraProxy(unsigned int id, CameraModule *cameraModule);\n> > +\n> > +\tint open(const hw_module_t *hardwareModule);\n> > +\tvoid close();\n> > +\n> > +\tvoid setCallbacks(const camera3_callback_ops_t *callbacks);\n> > +\tconst camera_metadata_t *constructDefaultRequest(int type);\n> > +\tint configureStreams(camera3_stream_configuration_t *stream_list);\n> > +\tint processCaptureRequest(camera3_capture_request_t *request);\n> > +\n> > +\tbool isOpen() const { return open_; }\n>\n> This isn't used.\n>\n> > +\tunsigned int id() const { return id_; }\n> > +\tcamera3_device_t *device() { return &cameraDevice_; }\n> > +\n> > +private:\n> > +\tvoid threadRpcCall(ThreadRpc &rpcRequest);\n> > +\n> > +\tbool open_;\n>\n> And neither is this, if you drop the isOpen() method.\n>\n> > +\tunsigned int id_;\n> > +\tCameraModule *cameraModule_;\n> > +\tcamera3_device_t cameraDevice_;\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_PROXY_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 1f242953db37..63b45832c0d2 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -1,3 +1,11 @@\n> > +android_hal_sources = files([\n> > + 'camera3_hal.cpp',\n> > + 'camera_hal_manager.cpp',\n> > + 'camera_module.cpp',\n> > + 'camera_proxy.cpp',\n> > + 'thread_rpc.cpp'\n> > +])\n> > +\n> > android_camera_metadata_sources = files([\n> > 'metadata/camera_metadata.c',\n> > ])\n> > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp\n> > new file mode 100644\n> > index 000000000000..f13db59d0d75\n> > --- /dev/null\n> > +++ b/src/android/thread_rpc.cpp\n> > @@ -0,0 +1,41 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * thread_call.cpp - Inter-thread procedure call\n>\n> thread_rpc.cpp\n>\n> > + */\n> > +\n> > +#include \"message.h\"\n> > +#include \"thread_rpc.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;\n> > +\n> > +ThreadRpcMessage::ThreadRpcMessage()\n> > +\t: Message(type())\n> > +{\n> > +}\n> > +\n> > +void ThreadRpc::notifyReception()\n> > +{\n> > +\t{\n> > +\t\tlibcamera::MutexLocker locker(mutex_);\n> > +\t\tdelivered_ = true;\n> > +\t}\n> > +\tcv_.notify_one();\n> > +}\n> > +\n> > +void ThreadRpc::waitDelivery()\n> > +{\n> > +\tlibcamera::MutexLocker locker(mutex_);\n> > +\tcv_.wait(locker, [&]{ return delivered_; });\n> > +}\n> > +\n> > +Message::Type ThreadRpcMessage::type()\n> > +{\n> > +\tif (ThreadRpcMessage::rpcType_ == Message::Type::None)\n> > +\t\trpcType_ = Message::registerMessageType();\n> > +\n> > +\treturn rpcType_;\n> > +}\n> > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h\n> > new file mode 100644\n> > index 000000000000..173caba1a515\n> > --- /dev/null\n> > +++ b/src/android/thread_rpc.h\n> > @@ -0,0 +1,56 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * thread_call.h - Inter-thread procedure call\n> > + */\n> > +#ifndef __ANDROID_THREAD_CALL_H__\n> > +#define __ANDROID_THREAD_CALL_H__\n>\n> s/CALL/RPC/\n>\n> > +\n> > +#include \n> > +#include \n>\n> Do you need string ?\n>\n> You should #include \n>\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"message.h\"\n> > +#include \"thread.h\"\n> > +\n> > +class ThreadRpc\n> > +{\n> > +public:\n> > +\tenum RpcTag {\n> > +\t\tProcessCaptureRequest,\n> > +\t\tClose,\n> > +\t};\n> > +\n> > +\tThreadRpc()\n> > +\t\t: delivered_(false) {}\n> > +\n> > +\tvoid notifyReception();\n> > +\tvoid waitDelivery();\n> > +\n> > +\tRpcTag tag;\n> > +\n> > +\tcamera3_capture_request_t *request;\n> > +\n> > +private:\n> > +\tbool delivered_;\n> > +\tstd::mutex mutex_;\n> > +\tstd::condition_variable cv_;\n> > +};\n> > +\n> > +class ThreadRpcMessage : public libcamera::Message\n> > +{\n> > +public:\n> > +\tThreadRpcMessage();\n> > +\tThreadRpc *rpc;\n> > +\n> > +\tstatic Message::Type type();\n> > +\n> > +private:\n> > +\tstatic libcamera::Message::Type rpcType_;\n> > +};\n>\n> FYI, I'll probably move part of this to the libcamera core.\n>\n\nLet's merge the HAL first, if you're not working on this already\nplease.\n\nThanks\n j\n\n> > +\n> > +#endif /* __ANDROID_THREAD_CALL_H__ */\n> > +\n>\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C258615FF\n\tfor ;\n\tThu, 8 Aug 2019 17:37:02 +0200 (CEST)","from uno.localdomain\n\t(host150-24-dynamic.51-79-r.retail.telecomitalia.it [79.51.24.150])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id EF8E240008;\n\tThu, 8 Aug 2019 15:36:58 +0000 (UTC)"],"X-Originating-IP":"79.51.24.150","Date":"Thu, 8 Aug 2019 17:38:15 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190808153806.3j5d72fwi5vayavw@uno.localdomain>","References":"<20190806195518.16739-1-jacopo@jmondi.org>\n\t<20190806195518.16739-7-jacopo@jmondi.org>\n\t<20190807153208.GG5048@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ga52nagm6mzpvsfv\"","Content-Disposition":"inline","In-Reply-To":"<20190807153208.GG5048@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 08 Aug 2019 15:37:02 -0000"}},{"id":2345,"web_url":"https://patchwork.libcamera.org/comment/2345/","msgid":"<20190808200952.GC6055@pendragon.ideasonboard.com>","date":"2019-08-08T20:09:52","subject":"Re: [libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Aug 08, 2019 at 05:38:15PM +0200, Jacopo Mondi wrote:\n> On Wed, Aug 07, 2019 at 06:32:08PM +0300, Laurent Pinchart wrote:\n> > On Tue, Aug 06, 2019 at 09:55:18PM +0200, Jacopo Mondi wrote:\n> > > Add libcamera Android Camera HALv3 implementation.\n> > >\n> > > The initial camera HAL implementation supports the LIMITED hardware\n> > > level and uses statically defined metadata and camera characteristics.\n> > >\n> > > Add a build option named 'android' and adjust the build system to\n> > > selectively compile the Android camera HAL and link it against the\n> > > required Android libraries.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > meson_options.txt | 5 +\n> > > src/android/camera3_hal.cpp | 130 +++++\n> > > src/android/camera_hal_manager.cpp | 173 +++++++\n> > > src/android/camera_hal_manager.h | 56 ++\n> > > src/android/camera_module.cpp | 799 +++++++++++++++++++++++++++++\n> > > src/android/camera_module.h | 75 +++\n> > > src/android/camera_proxy.cpp | 181 +++++++\n> > > src/android/camera_proxy.h | 41 ++\n> > > src/android/meson.build | 8 +\n> > > src/android/thread_rpc.cpp | 41 ++\n> > > src/android/thread_rpc.h | 56 ++\n> > > src/libcamera/meson.build | 9 +\n> > > src/meson.build | 5 +-\n> > > 13 files changed, 1578 insertions(+), 1 deletion(-)\n> > > create mode 100644 src/android/camera3_hal.cpp\n> > > create mode 100644 src/android/camera_hal_manager.cpp\n> > > create mode 100644 src/android/camera_hal_manager.h\n> > > create mode 100644 src/android/camera_module.cpp\n> > > create mode 100644 src/android/camera_module.h\n> > > create mode 100644 src/android/camera_proxy.cpp\n> > > create mode 100644 src/android/camera_proxy.h\n> > > create mode 100644 src/android/thread_rpc.cpp\n> > > create mode 100644 src/android/thread_rpc.h\n> >\n> > [snip]\n> >\n> > > diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp\n> > > new file mode 100644\n> > > index 000000000000..0a97a9333d20\n> > > --- /dev/null\n> > > +++ b/src/android/camera3_hal.cpp\n> > > @@ -0,0 +1,130 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera3_hal.cpp - Android Camera3 HAL module\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> >\n> > Do you need camera_metadata.h ? Isn't it enough to include\n> > camera_common.h ? I think hardware.h can also be dropped.\n> \n> camera_common.h includes camera_metadata.h, so I could just include\n> the one I need.\n> \n> > > +\n> > > +#include \n> >\n> > To speed up compilation, it would be better to include only the header\n> > files we need (here and everywhere else).\n> \n> Indeed\n> \n> > > +\n> > > +#include \"log.h\"\n> > > +\n> > > +#include \"camera_hal_manager.h\"\n> > > +#include \"camera_module.h\"\n> > > +#include \"camera_proxy.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(HAL)\n> > > +\n> > > +static CameraHalManager cameraManager;\n> > > +\n> > > +/*------------------------------------------------------------------------------\n> > > + * Android Camera HAL callbacks\n> > > + */\n> > > +\n> > > +static int hal_get_number_of_cameras(void)\n> > > +{\n> > > +\treturn cameraManager.numCameras();\n> > > +}\n> > > +\n> > > +static int hal_get_camera_info(int id, struct camera_info *info)\n> > > +{\n> > > +\treturn cameraManager.getCameraInfo(id, info);\n> > > +}\n> > > +\n> > > +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)\n> > > +{\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +static int hal_open_legacy(const struct hw_module_t *module, const char *id,\n> > > +\t\t\t uint32_t halVersion, struct hw_device_t **device)\n> > > +{\n> > > +\treturn -ENOSYS;\n> > > +}\n> > > +\n> > > +static int hal_set_torch_mode(const char *camera_id, bool enabled)\n> > > +{\n> > > +\treturn -ENOSYS;\n> > > +}\n> > > +\n> > > +/*\n> > > + * First entry point of the camera HAL module.\n> > > + *\n> > > + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)\n> > > + */\n> > > +static int hal_init()\n> > > +{\n> > > +\tLOG(HAL, Info) << \"Initialising Android camera HAL\";\n> > > +\n> > > +\tcameraManager.initialize();\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/*------------------------------------------------------------------------------\n> > > + * Android Camera Device\n> > > + */\n> > > +\n> > > +static int hal_dev_close(hw_device_t *hw_device)\n> > > +{\n> > > +\tif (!hw_device)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tcamera3_device_t *dev = reinterpret_cast(hw_device);\n> > > +\tCameraProxy *cameraModule = reinterpret_cast(dev->priv);\n> >\n> > s/cameraModule/proxy/ otherwise it's very confusing to have a\n> > CameraProxy instance called cameraModule when there's a CameraModule\n> > class.\n> >\n> > This comment applies to all other similar instances.\n> \n> Sorry, leftover. I don't see any other module/proxy in this file\n> \n> > > +\n> > > +\treturn cameraManager.close(cameraModule);\n> >\n> > As cameraManager.close() only calls proxy->close(), how about defining\n> > the hal_dev_close() method as a static method of CameraProxy, and\n> > setting proxy->device()->common.close inside the CameraProxy constructor\n> > ? It would avoid touching the internals of the CameraProxy in\n> > hal_dev_open() below.\n> \n> I liked having hal_dev_open and hal_dev_close here, as part of the HAL\n> module. But yes, we can save one function call and poking into the\n> device.common fields below..\n\nI agree that grouped hal_dev_open() and hal_dev_close() is nice, but\nit's offset by the needed to then poke into\nproxy->device()->common.close. Let's also remember that the HAL API\nseparates open from all the other operations, so we should be sad to\nseparate them here.\n\n> > > +}\n> > > +\n> > > +static int hal_dev_open(const hw_module_t* module, const char* name,\n> > > +\t\t\thw_device_t** device)\n> > > +{\n> > > +\tLOG(HAL, Debug) << \"Open camera: \" << name;\n> >\n> > s/camera:/camera/\n> >\n> > > +\n> > > +\tint id = atoi(name);\n> > > +\tCameraProxy *proxy = cameraManager.open(id, module);\n> > > +\tif (!proxy) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to open camera module \" << id;\n> > > +\t\treturn -ENODEV;\n> > > +\t}\n> > > +\tproxy->device()->common.close = hal_dev_close;\n> > > +\t*device = &proxy->device()->common;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> >\n> > Would it make sense to define all these methods as static methods of the\n> > CameraHalManager class, and turn the class into a singleton with an\n> > instance() method ?\n> \n> Which methods? hal_dev_open? All the hal_dev_ ones which are used to\n> initialize the camera_module_t ? I'm not sure I see a clear win...\n\nhal_dev_open(), but also all the methods defined in camera_module_t. In\ngeneral I think it's nice to avoid global methods, and use singletons\ninstead of global variables. This could be addressed later too.\n\n> > > +\n> > > +static struct hw_module_methods_t hal_module_methods = {\n> > > +\t.open = hal_dev_open,\n> > > +};\n> > > +\n> > > +camera_module_t HAL_MODULE_INFO_SYM = {\n> > > +\t.common = {\n> > > +\t\t.tag = HARDWARE_MODULE_TAG,\n> > > +\t\t.module_api_version = CAMERA_MODULE_API_VERSION_2_4,\n> > > +\t\t.hal_api_version = HARDWARE_HAL_API_VERSION,\n> > > +\t\t.id = CAMERA_HARDWARE_MODULE_ID,\n> > > +\t\t.name = \"Libcamera Camera3HAL Module\",\n> >\n> > s/Libcamera/libcamera/\n> >\n> > > +\t\t.author = \"libcamera\",\n> > > +\t\t.methods = &hal_module_methods,\n> > > +\t\t.dso = nullptr,\n> > > +\t\t.reserved = {},\n> > > +\t},\n> > > +\n> > > +\t.get_number_of_cameras = hal_get_number_of_cameras,\n> > > +\t.get_camera_info = hal_get_camera_info,\n> > > +\t.set_callbacks = hal_set_callbacks,\n> > > +\t.get_vendor_tag_ops = nullptr,\n> > > +\t.open_legacy = hal_open_legacy,\n> > > +\t.set_torch_mode = hal_set_torch_mode,\n> > > +\t.init = hal_init,\n> > > +\t.reserved = {},\n> > > +};\n> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > new file mode 100644\n> > > index 000000000000..734b5eebd9a5\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -0,0 +1,173 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_hal_manager.cpp - Libcamera Android Camera Manager\n> > > + */\n> > > +\n> > > +#include \"camera_hal_manager.h\"\n> > > +\n> > > +#include \n> >\n> > This shouldn't be needed.\n> \n> Yes\n> \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +\n> > > +#include \"camera_module.h\"\n> > > +#include \"camera_proxy.h\"\n> > > +\n> > > +using namespace libcamera;\n> >\n> > Missing blank line.\n> >\n> > > +LOG_DECLARE_CATEGORY(HAL);\n> > > +\n> > > +CameraHalManager::~CameraHalManager()\n> > > +{\n> > > +\tfor (auto metadata : staticMetadataMap_)\n> > > +\t\tfree_camera_metadata(metadata.second);\n> > > +}\n> > > +\n> > > +int CameraHalManager::initialize()\n> > > +{\n> > > +\t/*\n> > > +\t * Thread::start() will create a std::thread which will execute\n> > > +\t * CameraHalManager::run()\n> > > +\t */\n> >\n> > That's documenting the libcamera::Thread class (and the std::thread is\n> > an internal implementation detail). I would just state\n> >\n> > \t/*\n> > \t * Start the camera HAL manager thread and wait until its\n> > \t * initialisation completes to be fully operational before\n> > \t * receiving calls from the camera stack.\n> > \t */\n> >\n> > (thus dropping the next comment)\n> \n> As you might have noticed, the camera HAL still has comments and\n> annotations I added while developing, I'll clear them up better.\n> \n> > > +\tstart();\n> > > +\n> > > +\t/*\n> > > +\t * Wait for run() to notify us before returning to the caller to\n> > > +\t * make sure libcamera is fully initialized before we start handling\n> > > +\t * calls from the camera stack.\n> > > +\t */\n> > > +\tMutexLocker locker(mutex_);\n> > > +\tcv_.wait(locker);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void CameraHalManager::run()\n> > > +{\n> > > +\t/*\n> > > +\t * The run() operation is scheduled in its own thread;\n> > > +\t * It exec() to run the even dispatcher loop and initialize libcamera.\n> >\n> > s/even/event/\n> >\n> > > +\t *\n> > > +\t * All the libcamera components initialized from here will be tied to\n> > > +\t * the same thread as the here below run event dispatcher.\n> >\n> > The first paragraph also mostly documents the Thread class. I would drop\n> > it, and write the second paragraph as\n> >\n> > \t/*\n> > \t * All the libcamera components must be initialised here, in\n> > \t * order to bind them to the camera HAL manager thread that\n> > \t * executes the event dispatcher.\n> > \t /\n> >\n> > > +\t */\n> > > +\tlibcameraManager_ = libcamera::CameraManager::instance();\n> > > +\n> > > +\tint ret = libcameraManager_->start();\n> > > +\tif (ret) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n> > > +\t\t\t\t<< strerror(-ret);\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * For each Camera registered in the system, a CameraModule\n> > > +\t * get created here with an associated Camera and a proxy.\n> >\n> > s/get/gets/\n> >\n> > > +\t *\n> > > +\t * \\todo Support camera hotplug.\n> > > +\t */\n> > > +\tunsigned int cameraIndex = 0;\n> >\n> > Maybe just index ?\n> >\n> > > +\tfor (auto camera : libcameraManager_->cameras()) {\n> >\n> > auto &camera\n> >\n> > otherwise camera will be a copy of the shared pointer instead of a\n> > reference to it.\n> >\n> > > +\t\tcameras_.push_back(camera);\n> >\n> > The cameras_ vector isn't used after this. I would drop it, and store\n> > the shared pointer in CameraModule.\n> \n> Correct, I missed this in the the latest rework of the proxies\n> lifecycle.\n> \n> > > +\n> > > +\t\tCameraModule *module = new CameraModule(cameraIndex,\n> > > +\t\t\t\t\t\t\tcamera.get());\n> > > +\t\tmodules_.emplace_back(module);\n> > > +\n> > > +\t\tCameraProxy *proxy = new CameraProxy(cameraIndex,\n> > > +\t\t\t\t\t\t module);\n> > > +\t\tproxies_.emplace_back(proxy);\n> >\n> > Similarly, how about sotring the module pointer inside the CameraProxy\n> > class ? That would ensure that all accesses to the module go through the\n> > proxy, which I think would be a good idea.\n> \n> As I use the modules_ only to access the map of static metadata which\n> should now got away, I think it's doable\n> \n> > \t\tCameraProxy *proxy = new CameraProxy(index, camera);\n> > \t\tproxies_.emplace_back(proxy);\n> >\n> > > +\n> > > +\t\t++cameraIndex;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * libcamera has been initialized! Unlock the initialize() caller\n> > > +\t * as we're now ready to handle calls from the framework.\n> > > +\t */\n> > > +\tcv_.notify_one();\n> > > +\n> > > +\t/* Now start processing events and messages! */\n> >\n> > s/!/./ in the above two messages, there's no need to be\n> > over-enthusiastic :-)\n> >\n> > > +\texec();\n> > > +}\n> > > +\n> > > +CameraProxy *CameraHalManager::open(unsigned int id,\n> > > +\t\t\t\t const hw_module_t *hardwareModule)\n> > > +{\n> > > +\tif (id < 0 || id >= numCameras()) {\n> > > +\t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tCameraProxy *proxy = proxies_[id].get();\n> > > +\tif (proxy->open(hardwareModule))\n> > > +\t\treturn proxy;\n> >\n> > Shouldn't you return nullprtr here ?\n> \n> Ideed\n> \n> > > +\n> > > +\tLOG(HAL, Info) << \"Camera: '\" << id << \"' opened\";\n> >\n> > s/Camera:/Camera/\n> >\n> > > +\n> > > +\treturn proxy;\n> > > +}\n> > > +\n> > > +int CameraHalManager::close(CameraProxy *proxy)\n> > > +{\n> > > +\tproxy->close();\n> > > +\tLOG(HAL, Info) << \"Close camera: \" << proxy->id();\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +unsigned int CameraHalManager::numCameras() const\n> > > +{\n> > > +\treturn libcameraManager_->cameras().size();\n> > > +}\n> > > +\n> > > +/*\n> > > + * Before the camera framework even tries to open a camera device with\n> > > + * hal_dev_open() it requires the camera HAL to report a list of static\n> > > + * informations on the camera device with id \\a id in the hal_get_camera_info()\n> > > + * method.\n> > > + *\n> > > + * The static metadata should be then generated probably from a\n> > > + * platform-specific module, as we cannot operate on the camera at this time as\n> > > + * it has not yet been open by the framework.\n> >\n> > s/open/opened/\n> >\n> > What prevents us from opening the camera internally ? I don't think we\n> \n> It would complicate the lifecycle handling a bit, but I guess it's\n> doable. AS of now, where all metadata are static it is not worth it\n> imo\n\nWe don't need to do it now. I was mostly addressing the comment above\nthat states we can't operate on the camera, which I think isn't correct\nas we can decouple opening the libcamera::Camera and the HAL camera.\n\n> > need a platform-specific module.\n> \n> We surely won't be able to get from kernel at this point all the\n> information we need, in example, the camera position.\n\nI can't dispute that :-) We should however get it from the\nlibcamera::Camera.\n\n> > > + *\n> > > + * This is what the Intel XML file is used for, it is parsed and the data there\n> > > + * combined with informations from the PSL service (which I -think- it's what\n> > > + * our IPA is) to generate a list of static metadata per-camera device.\n> >\n> > I'd drop this paragraph.\n> >\n> > > + */\n> > > +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)\n> > > +{\n> > > +\tint cameras = numCameras();\n> > > +\tif (id >= cameras || id < 0 || !info) {\n> >\n> > Can we be called with info == nullptr ? I think I'd drop that part of\n> > the check.\n> \n> All (most?) the extra-paranoid checks I have added here are error conditions\n> tested by the cros_camera_test suite.\n\nLovely test suite... I suppose we should consider ourselves lucky that\nit doesn't test with info = 0xdeadbeef or other random pointer values\n:-)\n\n> > > +\t\tLOG(HAL, Error) << \"Invalid camera id: \" << id;\n> >\n> > s/id:/id/\n> >\n> > You may also want to add quotes around the value, as done in\n> > CameraHalManager::open() (or drop them there).\n> >\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tcamera_metadata_t *staticMetadata;\n> > > +\tauto it = staticMetadataMap_.find(id);\n> > > +\tif (it != staticMetadataMap_.end()) {\n> > > +\t\tstaticMetadata = it->second;\n> > > +\t} else {\n> > > +\t\tCameraModule *cameraModule = modules_[id].get();\n> > > +\n> > > +\t\tstaticMetadata = cameraModule->getStaticMetadata();\n> > > +\t\tstaticMetadataMap_[id] = staticMetadata;\n> >\n> > Why do you need the map, why can't you always retrieve the static\n> > metadata from the CameraModule ?\n> \n> I should, yes, so we can frop modules_\n> \n> > > +\t}\n> > > +\n> > > +\t/* \\todo: Get these info dynamically inspecting the camera module. */\n> >\n> > s/://\n> >\n> > > +\tinfo->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;\n> > > +\tinfo->orientation = 0;\n> > > +\tinfo->device_version = 0;\n> > > +\tinfo->resource_cost = 0;\n> > > +\tinfo->static_camera_characteristics = staticMetadata;\n> > > +\tinfo->conflicting_devices = nullptr;\n> > > +\tinfo->conflicting_devices_length = 0;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > new file mode 100644\n> > > index 000000000000..65d6fa3150ef\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_manager.h\n> > > @@ -0,0 +1,56 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_hal_manager.h - Libcamera Android Camera Manager\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_MANAGER_H__\n> > > +#define __ANDROID_CAMERA_MANAGER_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"thread.h\"\n> > > +#include \"thread_rpc.h\"\n> > > +\n> > > +class CameraModule;\n> > > +class CameraProxy;\n> > > +\n> > > +class CameraHalManager : public libcamera::Thread\n> > > +{\n> > > +public:\n> > > +\t~CameraHalManager();\n> > > +\n> > > +\tint initialize();\n> >\n> > Our initialisation methods are usually called init(), not initialize().\n> \n> ok\n> \n> > > +\n> > > +\tCameraProxy *open(unsigned int id, const hw_module_t *module);\n> > > +\tint close(CameraProxy *proxy);\n> > > +\n> > > +\tunsigned int numCameras() const;\n> > > +\tint getCameraInfo(int id, struct camera_info *info);\n> > > +\n> > > +private:\n> > > +\tvoid run() override;\n> > > +\tcamera_metadata_t *getStaticMetadata(unsigned int id);\n> > > +\n> > > +\tlibcamera::CameraManager *libcameraManager_;\n> >\n> > I think you can call this manager_.\n> >\n> > > +\n> > > +\tstd::mutex mutex_;\n> > > +\tstd::condition_variable cv_;\n> > > +\n> > > +\tstd::vector> cameras_;\n> > > +\tstd::vector> modules_;\n> > > +\tstd::vector> proxies_;\n> > > +\n> > > +\tstd::map staticMetadataMap_;\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_CAMERA_MANAGER_H__ */\n> > > diff --git a/src/android/camera_module.cpp b/src/android/camera_module.cpp\n> > > new file mode 100644\n> > > index 000000000000..b64e377fb439\n> > > --- /dev/null\n> > > +++ b/src/android/camera_module.cpp\n> > > @@ -0,0 +1,799 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_module.cpp - Libcamera Android Camera Module\n> > > + */\n> > > +\n> > > +#include \"camera_module.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> >\n> > I would group all the Android headers together, so\n> >\n> > #include \n> > #include \n> >\n> > #include \n> >\n> > > +#include \n> > > +\n> > > +#include \"message.h\"\n> > > +\n> > > +#include \"log.h\"\n> >\n> > #include \"log.h\"\n> > #include \"message.h\"\n> >\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL);\n> > > +\n> > > +CameraModule::CameraModule(unsigned int id, libcamera::Camera *camera)\n> > > +\t: id_(id), camera_(camera), cameraState_(CameraStopped),\n> > > +\t requestTemplate_(nullptr)\n> > > +{\n> > > +\tcamera_->requestCompleted.connect(this,\n> > > +\t\t\t\t\t &CameraModule::requestComplete);\n> >\n> > This holds on a single line.\n> >\n> > > +}\n> > > +\n> > > +CameraModule::~CameraModule()\n> > > +{\n> > > +\tif (requestTemplate_)\n> > > +\t\tfree_camera_metadata(requestTemplate_);\n> > > +\trequestTemplate_ = nullptr;\n> > > +}\n> > > +\n> > > +void CameraModule::message(Message *message)\n> > > +{\n> > > +\tThreadRpcMessage *rpcMessage = static_cast\n> > > +\t\t\t\t (message);\n> > > +\n> > > +\tif (rpcMessage->type() != ThreadRpcMessage::type())\n> > > +\t\treturn Object::message(message);\n> >\n> > As Message may not be a ThreadRpcMessage, you should write this as\n> >\n> > \tif (message->type() != ThreadRpcMessage::type())\n> > \t\treturn Object::message(message);\n> >\n> > \tThreadRpcMessage *rpcMessage = static_cast(message);\n> >\n> > > +\n> > > +\tThreadRpc *rpc = rpcMessage->rpc;\n> > > +\n> > > +\tswitch (rpc->tag) {\n> > > +\tcase ThreadRpc::ProcessCaptureRequest:\n> > > +\t\tprocessCaptureRequest(rpc->request);\n> > > +\t\tbreak;\n> > > +\tcase ThreadRpc::Close:\n> > > +\t\tclose();\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tLOG(HAL, Error) << \"Unknown RPC operation: \" << rpc->tag;\n> > > +\t}\n> > > +\n> > > +\trpc->notifyReception();\n> > > +}\n> > > +\n> > > +int CameraModule::open()\n> > > +{\n> > > +\tint ret = camera_->acquire();\n> > > +\tif (ret) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to acquire the camera\";\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void CameraModule::close()\n> > > +{\n> > > +\tcamera_->stop();\n> > > +\n> > > +\tcamera_->freeBuffers();\n> > > +\tcamera_->release();\n> > > +\n> > > +\tcameraState_ = CameraStopped;\n> > > +}\n> > > +\n> > > +void CameraModule::setCallbacks(const struct camera3_device *camera3Device,\n> > > +\t\t\t\tconst camera3_callback_ops_t *callbacks)\n> > > +{\n> > > +\tcamera3Device_ = camera3Device;\n> >\n> > camera3Device_ seems unused.\n> >\n> > > +\tcallbacks_ = callbacks;\n> > > +}\n> > > +\n> > > +camera_metadata_t *CameraModule::getStaticMetadata()\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\t/*\n> > > +\t * The below metadata has been added by inspecting the Intel XML\n> > > +\t * file and it's associated parsing routines in the Intel IPU3 HAL.\n> > > +\t *\n> > > +\t * The here below metadata are enough to satisfy the camera3-test\n> > > +\t * provided by ChromeOS, but a real camera implementation might require\n> >\n> > s/might/will/\n> >\n> > > +\t * more.\n> > > +\t *\n> > > +\t * See CameraConf::AiqConf class implementation in the Intel HAL\n> > > +\t * to get a list of the static metadata registered by parsing the\n> > > +\t * XML config file in AiqConf::fillStaticMetadataFromCMC\n> > > +\t */\n> > > +\n> > > +\t/*\n> > > +\t * FIXME: this sizes come from the Intel IPU3 HAL, and are way too large for\n> >\n> > s/FIXME:/\\todo/\n> > s/this/these/\n> >\n> > > +\t * this intial HAL version.\n> > > +\t */\n> > > +\t#define STATIC_ENTRY_CAP 256\n> > > +\t#define STATIC_DATA_CAP 6688\n> > > +\tcamera_metadata_t *staticMetadata =\n> > > +\t\tallocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);\n> > > +\n> > > +\t/* Sensor static metadata. */\n> > > +\tint32_t pixelArraySize[] = {\n> > > +\t\t2592, 1944,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > +\t\t\t\t&pixelArraySize, 2);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t sensorSizes[] = {\n> > > +\t\t0, 0, 2560, 1920,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > +\t\t\t\t&sensorSizes, 4);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t sensitivityRange[] = {\n> > > +\t\t32, 2400,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> > > +\t\t\t\t&sensitivityRange, 2);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > > +\t\t\t\t&filterArr, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint64_t exposureTimeRange[] = {\n> > > +\t\t100000, 200000000,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > > +\t\t\t\t&exposureTimeRange, 2);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t orientation = 0;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_SENSOR_ORIENTATION,\n> > > +\t\t\t\t&orientation, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/* Flash static metadata. */\n> > > +\tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_FLASH_INFO_AVAILABLE,\n> > > +\t\t\t&flashAvailable, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/* Lens static metadata. */\n> > > +\tfloat fn = 2.53 / 100;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/* Control metadata. */\n> > > +\tchar controlMetadata = ANDROID_CONTROL_MODE_AUTO;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> > > +\t\t\t&controlMetadata, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tchar availableAntiBandingModes[] = {\n> > > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,\n> > > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,\n> > > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,\n> > > +\t\tANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > > +\t\t\tavailableAntiBandingModes, 4);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tchar aeAvailableModes[] = {\n> > > +\t\tANDROID_CONTROL_AE_MODE_ON,\n> > > +\t\tANDROID_CONTROL_AE_MODE_OFF,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > +\t\t\taeAvailableModes, 2);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tcontrolMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > +\t\t\t&controlMetadata, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > +\t\t\t&awbLockAvailable, 1);\n> > > +\n> > > +\t/* Scaler static metadata. */\n> > > +\tstd::vector availableStreamFormats = {\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB,\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> > > +\t\t\tavailableStreamFormats.data(),\n> > > +\t\t\tavailableStreamFormats.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tstd::vector availableStreamConfigurations = {\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,\n> > > +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,\n> > > +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,\n> > > +\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> > > +\t\t\tavailableStreamConfigurations.data(),\n> > > +\t\t\tavailableStreamConfigurations.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tstd::vector availableStallDurations = {\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> > > +\t\t\tavailableStallDurations.data(),\n> > > +\t\t\tavailableStallDurations.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tstd::vector minFrameDurations = {\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,\n> > > +\t\tANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> > > +\t\t\tminFrameDurations.data(), minFrameDurations.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/* Info static metadata. */\n> > > +\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\tret = add_camera_metadata_entry(staticMetadata,\n> > > +\t\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > +\t\t\t&supportedHWLevel, 1);\n> > > +\n> > > +\treturn staticMetadata;\n> > > +}\n> > > +\n> > > +const camera_metadata_t *CameraModule::constructDefaultRequestMetadata(int type)\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\t/*\n> > > +\t * TODO: inspect type and pick the right metadata pack.\n> >\n> > s/TODO:/\\todo/\n> >\n> > > +\t * As of now just use a single one for all templates\n> > > +\t */\n> > > +\tuint8_t captureIntent;\n> > > +\tswitch (type) {\n> > > +\tcase CAMERA3_TEMPLATE_PREVIEW:\n> > > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;\n> > > +\t\tbreak;\n> > > +\tcase CAMERA3_TEMPLATE_STILL_CAPTURE:\n> > > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;\n> > > +\t\tbreak;\n> > > +\tcase CAMERA3_TEMPLATE_VIDEO_RECORD:\n> > > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;\n> > > +\t\tbreak;\n> > > +\tcase CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:\n> > > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;\n> > > +\t\tbreak;\n> > > +\tcase CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:\n> > > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;\n> > > +\t\tbreak;\n> > > +\tcase CAMERA3_TEMPLATE_MANUAL:\n> > > +\t\tcaptureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tLOG(HAL, Error) << \"Invalid template request type: \" << type;\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tif (requestTemplate_)\n> > > +\t\treturn requestTemplate_;\n> > > +\n> > > +\t/* FIXME: this sizes are quite arbitrary ones */\n> > > +\t#define REQUEST_TEMPLATE_ENTRIES\t 30\n> > > +\t#define REQUEST_TEMPLATE_DATA\t\t2048\n> > > +\trequestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,\n> > > +\t\t\t\t\t\t REQUEST_TEMPLATE_DATA);\n> > > +\tif (!requestTemplate_) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to allocate template metadata\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Fill in the required Request metadata.\n> > > +\t *\n> > > +\t * Part (most?) of this entries comes from inspecting the Intel's IPU3\n> > > +\t * HAL xml configuration file.\n> > > +\t */\n> > > +\n> > > +\t/* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */\n> > > +\tint32_t maxOutStream[] = { 0, 2, 0 };\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > > +\t\t\tmaxOutStream, 3);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t maxPipelineDepth = 5;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> > > +\t\t\t&maxPipelineDepth, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t inputStreams = 0;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,\n> > > +\t\t\t&inputStreams, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t partialResultCount = 1;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > > +\t\t\t&partialResultCount, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t availableCapabilities[] = {\n> > > +\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > +\t\t\tavailableCapabilities, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AE_MODE,\n> > > +\t\t\t&aeMode, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t aeExposureCompensation = 0;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > > +\t\t\t&aeExposureCompensation, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> > > +\t\t\t&aePrecaptureTrigger, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AE_LOCK,\n> > > +\t\t\t&aeLock, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AF_TRIGGER,\n> > > +\t\t\t&afTrigger, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AWB_MODE,\n> > > +\t\t\t&awbMode, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AWB_LOCK,\n> > > +\t\t\t&awbLock, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > +\t\t\t&awbLockAvailable, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t flashMode = ANDROID_FLASH_MODE_OFF;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_FLASH_MODE,\n> > > +\t\t\t&flashMode, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> > > +\t\t\t&faceDetectMode, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> > > +\t\t\t&captureIntent, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/*\n> > > +\t * This is quite hard to list at the moment wihtout knowing what\n> > > +\t * we could control.\n> > > +\t *\n> > > +\t * For now, just list in the available Request keys and in the available\n> > > +\t * result keys the control and reporting of the AE algorithm.\n> > > +\t */\n> > > +\tstd::vector availableRequestKeys = {\n> > > +\t\tANDROID_CONTROL_AE_MODE,\n> > > +\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > > +\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> > > +\t\tANDROID_CONTROL_AE_LOCK,\n> > > +\t\tANDROID_CONTROL_AF_TRIGGER,\n> > > +\t\tANDROID_CONTROL_AWB_MODE,\n> > > +\t\tANDROID_CONTROL_AWB_LOCK,\n> > > +\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > +\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> > > +\t\tANDROID_FLASH_MODE,\n> > > +\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> > > +\t};\n> > > +\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,\n> > > +\t\t\tavailableRequestKeys.data(),\n> > > +\t\t\tavailableRequestKeys.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tstd::vector availableResultKeys = {\n> > > +\t\tANDROID_CONTROL_AE_MODE,\n> > > +\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > > +\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n> > > +\t\tANDROID_CONTROL_AE_LOCK,\n> > > +\t\tANDROID_CONTROL_AF_TRIGGER,\n> > > +\t\tANDROID_CONTROL_AWB_MODE,\n> > > +\t\tANDROID_CONTROL_AWB_LOCK,\n> > > +\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > +\t\tANDROID_CONTROL_CAPTURE_INTENT,\n> > > +\t\tANDROID_FLASH_MODE,\n> > > +\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_AVAILABLE_RESULT_KEYS,\n> > > +\t\t\tavailableResultKeys.data(),\n> > > +\t\t\tavailableResultKeys.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/*\n> > > +\t * The available characteristics should be the tags reported\n> > > +\t * as part of the static metadata reported at hal_get_camera_info()\n> > > +\t * time. The xml file sets those to 0 though.\n> > > +\t */\n> > > +\tstd::vector availableCharacteristicsKeys = {};\n> > > +\tret = add_camera_metadata_entry(requestTemplate_,\n> > > +\t\t\tANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > > +\t\t\tavailableCharacteristicsKeys.data(),\n> > > +\t\t\tavailableCharacteristicsKeys.size());\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\treturn requestTemplate_;\n> > > +}\n> > > +\n> > > +/**\n> > > + * Inspect the stream_list to produce a list of StreamConfiguration to\n> > > + * be use to configure the Camera.\n> > > + */\n> > > +int CameraModule::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > +{\n> > > +\n> >\n> > Extra blank line.\n> >\n> > > +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > > +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> > > +\n> > > +\t\tLOG(HAL, Info) << \"Stream #\" << i\n> > > +\t\t\t << \": direction: \" << stream->stream_type\n> > > +\t\t\t << \" - width: \" << stream->width\n> > > +\t\t\t << \" - height: \" << stream->height\n> > > +\t\t\t << \" - format: \" << std::hex << stream->format;\n> >\n> > s/ -/,/ in the above three lines\n> >\n> > And I would print size: ...x... instead of width and height.\n> >\n> > > +\t}\n> > > +\n> > > +\t/* Hardcode viewfinder role, collecting sizes from the stream config. */\n> > > +\tif (stream_list->num_streams != 1) {\n> > > +\t\tLOG(HAL, Error) << \"Only one stream supported\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tStreamRoles roles = {};\n> > > +\troles.push_back(StreamRole::StillCapture);\n> >\n> > I think you can write this as\n> >\n> > \tStreamRoles roles{ StreamRole::StillCapture };\n> >\n> > > +\n> > > +\tconfig_ = camera_->generateConfiguration(roles);\n> > > +\tif (!config_ || config_->empty()) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\t/* Only one stream is supported. */\n> > > +\tcamera3_stream_t *camera3Stream = stream_list->streams[0];\n> > > +\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> > > +\tstreamConfiguration->size.width = camera3Stream->width;\n> > > +\tstreamConfiguration->size.height = camera3Stream->height;\n> > > +\tstreamConfiguration->memoryType = ExternalMemory;\n> > > +\n> > > +\t/*\n> > > +\t * FIXME: do not change the pixel format from the one returned\n> > > +\t * from the Camera::generateConfiguration();\n> > > +\t *\n> > > +\t * We'll need to translate from Android defined pixel format codes\n> > > +\t * to whatever libcamera will use.\n> > > +\t */\n> > > +\n> > > +\tswitch (config_->validate()) {\n> > > +\tcase CameraConfiguration::Valid:\n> > > +\t\tbreak;\n> > > +\tcase CameraConfiguration::Adjusted:\n> > > +\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n> >\n> > Missing config_.reset() ?\n> >\n> > > +\t\treturn -EINVAL;\n> > > +\tcase CameraConfiguration::Invalid:\n> > > +\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> > > +\t\tconfig_.reset();\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tcamera3Stream->max_buffers = streamConfiguration->bufferCount;\n> > > +\n> > > +\t/*\n> > > +\t * Once the CameraConfiguration has been adjusted/validated\n> > > +\t * it can be applied to the camera.\n> > > +\t */\n> > > +\tint ret = camera_->configure(config_.get());\n> > > +\tif (ret) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> > > +\t\t\t\t<< camera_->name() << \"'\";\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraModule::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > > +{\n> > > +\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> > > +\tStream *stream = streamConfiguration->stream();\n> > > +\n> > > +\tif (camera3Request->num_output_buffers != 1) {\n> > > +\t\tLOG(HAL, Error) << \"Invalid number of output buffers: \"\n> > > +\t\t\t\t<< camera3Request->num_output_buffers;\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\t/* Start the camera if that's the first request we handle. */\n> > > +\tif (cameraState_ == CameraStopped) {\n> > > +\t\tint ret = camera_->allocateBuffers();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(HAL, Error) << \"Failed to allocate buffers\";\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +\n> > > +\t\tret = camera_->start();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > > +\t\t\tcamera_->freeBuffers();\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +\n> > > +\t\tcameraState_ = CameraRunning;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Queue a request for the Camera with the provided dmabuf file\n> > > +\t * descriptors.\n> > > +\t */\n> > > +\tconst camera3_stream_buffer_t *camera3Buffer =\n> > > +\t\t\t\t\t&camera3Request->output_buffers[0];\n> > > +\tconst buffer_handle_t camera3Handle = *camera3Buffer->buffer;\n> > > +\n> > > +\tstd::array fds = {\n> > > +\t\tcamera3Handle->data[0],\n> > > +\t\tcamera3Handle->data[1],\n> > > +\t\tcamera3Handle->data[2],\n> > > +\t};\n> > > +\tstd::unique_ptr buffer = stream->createBuffer(fds);\n> > > +\tif (!buffer) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tRequest *request = camera_->createRequest();\n> > > +\trequest->addBuffer(std::move(buffer));\n> > > +\tint ret = camera_->queueRequest(request);\n> > > +\tif (ret) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to queue request\";\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\t/* Save the request descriptors for use at completion time. */\n> > > +\tCamera3RequestDescriptor descriptor = {};\n> > > +\tdescriptor.frameNumber = camera3Request->frame_number,\n> > > +\tdescriptor.numBuffers = camera3Request->num_output_buffers,\n> >\n> > s/,$/;/ for the two lines above.\n> >\n> > I don't think you need to store numBuffers in the descriptors, it can be\n> > retrieved from buffers.size() in requestComplete().\n> \n> numBuffers is the framework required number of buffers, buffers.size()\n> is the number of buffers completed in the request. They -should- be\n> the same, but I would keep them separate to make sure the completed\n> request is correct.\n\nIt boils down to how much paranoia we need to build in :-) How could\nthey be different ?\n\n> > > +\tdescriptor.buffers = new camera3_stream_buffer_t[descriptor.numBuffers];\n> > > +\tfor (unsigned int i = 0; i < descriptor.numBuffers; ++i) {\n> > > +\t\tcamera3_stream_buffer_t &buffer = descriptor.buffers[i];\n> > > +\t\tbuffer = camera3Request->output_buffers[i];\n> >\n> > Just\n> >\n> > \t\tdescriptor.buffers[i] = camera3Request->output_buffers[i];\n> > > +\t}\n> > > +\n> > > +\trequestMap_[request] = descriptor;\n> >\n> > How about using the request cookie (passed to Camera::createRequest())\n> > for this purpose ?\n> \n> Can we do that on top?\n\nSure, but please then keep track of the task, with a \\todo here.\n\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void CameraModule::requestComplete(Request *request,\n> > > +\t\t\t\t const std::map &buffers)\n> > > +{\n> > > +\tBuffer *libcameraBuffer = buffers.begin()->second;\n> > > +\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > +\tcamera_metadata_t *resultMetadata = nullptr;\n> > > +\n> > > +\tif (request->status() != Request::RequestComplete) {\n> > > +\t\tLOG(HAL, Error) << \"Request not succesfully completed: \"\n> > > +\t\t\t\t<< request->status();\n> > > +\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +\t}\n> > > +\n> > > +\t/* Prepare to call back the Android camera stack. */\n> > > +\tCamera3RequestDescriptor &descriptor = requestMap_[request];\n> > > +\n> > > +\tcamera3_capture_result_t captureResult = {};\n> > > +\tcaptureResult.frame_number = descriptor.frameNumber;\n> > > +\tcaptureResult.num_output_buffers = descriptor.numBuffers;\n> > > +\n> > > +\tcamera3_stream_buffer_t *resultBuffers =\n> > > +\t\tnew camera3_stream_buffer_t[descriptor.numBuffers];\n> > > +\tfor (unsigned int i = 0; i < descriptor.numBuffers; ++i) {\n> > > +\t\tcamera3_stream_buffer_t resultBuffer;\n> > > +\n> > > +\t\tresultBuffer = descriptor.buffers[i];\n> > > +\t\tresultBuffer.acquire_fence = -1;\n> > > +\t\tresultBuffer.release_fence = -1;\n> > > +\t\tresultBuffer.status = status;\n> > > +\n> > > +\t\tresultBuffers[i] = resultBuffer;\n> > > +\t}\n> > > +\tcaptureResult.output_buffers =\n> > > +\t\tconst_cast(resultBuffers);\n> > > +\n> > > +\tif (status == CAMERA3_BUFFER_STATUS_ERROR) {\n> > > +\t\t/* \\todo Improve error handling. */\n> > > +\t\tnotifyError(descriptor.frameNumber, resultBuffers->stream);\n> > > +\t} else {\n> > > +\t\tnotifyShutter(descriptor.frameNumber, libcameraBuffer->timestamp());\n> > > +\n> > > +\t\tcaptureResult.partial_result = 1;\n> > > +\t\tresultMetadata = getResultMetadata(descriptor.frameNumber,\n> > > +\t\t\t\t\t\t libcameraBuffer->timestamp());\n> > > +\t\tcaptureResult.result = resultMetadata;\n> > > +\t}\n> > > +\n> > > +\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > > +\n> > > +\tif (resultMetadata)\n> > > +\t\tfree_camera_metadata(resultMetadata);\n> > > +\n> > > +\tdelete[] resultBuffers;\n> > > +\tdelete[] descriptor.buffers;\n> > > +\trequestMap_.erase(request);\n> > > +\n> > > +\treturn;\n> > > +}\n> > > +\n> > > +void CameraModule::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> > > +{\n> > > +\tcamera3_notify_msg_t notify = {};\n> > > +\n> > > +\tnotify.type = CAMERA3_MSG_SHUTTER;\n> > > +\tnotify.message.shutter.frame_number = frameNumber;\n> > > +\tnotify.message.shutter.timestamp = timestamp;\n> > > +\n> > > +\tcallbacks_->notify(callbacks_, ¬ify);\n> > > +}\n> > > +\n> > > +void CameraModule::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > > +{\n> > > +\tcamera3_notify_msg_t notify = {};\n> > > +\n> > > +\tnotify.type = CAMERA3_MSG_ERROR;\n> > > +\tnotify.message.error.error_stream = stream;\n> > > +\tnotify.message.error.frame_number = frameNumber;\n> > > +\tnotify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;\n> > > +\n> > > +\tcallbacks_->notify(callbacks_, ¬ify);\n> > > +}\n> > > +\n> > > +/*\n> > > + * Fixed result metadata, mostly imported from the UVC camera HAL, which\n> > > + * does not produces metadata and thus needs to generate a fixed set.\n> > > + */\n> > > +camera_metadata_t *CameraModule::getResultMetadata(int frame_number,\n> > > +\t\t\t\t\t\t int64_t timestamp)\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\t/* FIXME: random \"big enough\" values. */\n> > > +\t#define RESULT_ENTRY_CAP 256\n> > > +\t#define RESULT_DATA_CAP 6688\n> > > +\tcamera_metadata_t *resultMetadata =\n> > > +\t\tallocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);\n> > > +\n> > > +\tconst uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> > > +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,\n> > > +\t\t\t\t\t&ae_state, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tconst uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;\n> > > +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,\n> > > +\t\t\t\t\t&ae_lock, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tuint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;\n> > > +\tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,\n> > > +\t\t\t\t\t&af_state, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tconst uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_CONTROL_AWB_STATE,\n> > > +\t\t\t\t\t&awb_state, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tconst uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_CONTROL_AWB_LOCK,\n> > > +\t\t\t\t\t&awb_lock, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tconst uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_LENS_STATE,\n> > > +\t\t\t\t\t&lens_state, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tint32_t sensorSizes[] = {\n> > > +\t\t0, 0, 2560, 1920,\n> > > +\t};\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_SCALER_CROP_REGION,\n> > > +\t\t\t\t\tsensorSizes, 4);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_SENSOR_TIMESTAMP,\n> > > +\t\t\t\t\t×tamp, 1);\n> > > +\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/* 33.3 msec */\n> > > +\tconst int64_t rolling_shutter_skew = 33300000;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_SENSOR_ROLLING_SHUTTER_SKEW,\n> > > +\t\t\t\t\t&rolling_shutter_skew, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\t/* 16.6 msec */\n> > > +\tconst int64_t exposure_time = 16600000;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_SENSOR_EXPOSURE_TIME,\n> > > +\t\t\t\t\t&exposure_time, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tconst uint8_t lens_shading_map_mode =\n> > > +\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> > > +\t\t\t\t\t&lens_shading_map_mode, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\tconst uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;\n> > > +\tret = add_camera_metadata_entry(resultMetadata,\n> > > +\t\t\t\t\tANDROID_STATISTICS_SCENE_FLICKER,\n> > > +\t\t\t\t\t&scene_flicker, 1);\n> > > +\tMETADATA_ASSERT(ret);\n> > > +\n> > > +\treturn resultMetadata;\n> > > +}\n> > > diff --git a/src/android/camera_module.h b/src/android/camera_module.h\n> > > new file mode 100644\n> > > index 000000000000..67488d5940b1\n> > > --- /dev/null\n> > > +++ b/src/android/camera_module.h\n> > > @@ -0,0 +1,75 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_module.h - Libcamera Android Camera Module\n> >\n> > s/Libcamera/libcamera/\n> >\n> > (here and everywhere else, libcamera is always written in all lowercase)\n> >\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_MODULE_H__\n> > > +#define __ANDROID_CAMERA_MODULE_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"message.h\"\n> > > +\n> > > +#include \"camera_hal_manager.h\"\n> > > +\n> > > +#define METADATA_ASSERT(_r)\t\t\\\n> > > +\tdo {\t\t\t\t\\\n> > > +\t\tif (!(_r)) break;\t\\\n> > > +\t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> > > +\t\treturn nullptr;\t\t\\\n> > > +\t} while(0);\n> >\n> > I really, really dislike hiding return statements in macros. We can keep\n> > it for now, but please add a comment telling that it should be removed.\n> \n> All the metadata handling will be nuked in the next versions, so I\n> won't bother for now.\n> \n> > > +\n> >\n> > Even if we don't document the whole implementation with Doxygen, I think\n> > a few short comments that explain what each class models would be\n> > useful. Otherwise the reader is left to wonder what CameraModule or\n> > CameraProxy stand for. A comment that explains how the various objects\n> > work together would be useful too.\n> >\n> > > +class CameraModule : public libcamera::Object\n> >\n> > I don't think CameraModule is a good name :-/ In HAL terminology, the\n> > module refers to camera_module_t, and corresponds more or less to\n> > libcamera::CameraManager, not libcamera::Camera.\n> \n> Correct\n> \n> > One option would be to name this class Camera in a HAL namespace. That\n> > would make it quite clear that it corresponds to a libcamra::Camera.\n> \n> I had a go at isolating namespaces and I have problems with the\n> logging infrastructure,\n\nAh yes I rememeber that.\n\n> and I would not duplicate the Camera name to be honest. CameraDevice ?\n\nAs it corresponds to camera3_device_t, CameraDevice makes sense.\n\nThis just struck my mind now, should CameraHalManager be named\nCameraModule to match camera_module_t ? I'm fine either way.\n\n> > > +{\n> > > +public:\n> > > +\tCameraModule(unsigned int id, libcamera::Camera *camera);\n> > > +\t~CameraModule();\n> > > +\n> > > +\tvoid message(libcamera::Message *message);\n> > > +\n> > > +\tint open();\n> > > +\tvoid close();\n> > > +\tvoid setCallbacks(const struct camera3_device *cameraDevice,\n> > > +\t\t\t const camera3_callback_ops_t *callbacks);\n> > > +\tcamera_metadata_t *getStaticMetadata();\n> > > +\tconst camera_metadata_t *constructDefaultRequestMetadata(int type);\n> > > +\tint configureStreams(camera3_stream_configuration_t *stream_list);\n> > > +\tint processCaptureRequest(camera3_capture_request_t *request);\n> > > +\tvoid requestComplete(libcamera::Request *request,\n> > > +\t\t\t const std::map &buffers);\n> > > +\n> > > +\tunsigned int id() const { return id_; }\n> >\n> > I think this method is not used.\n> >\n> > > +\n> > > +private:\n> > > +\tstruct Camera3RequestDescriptor {\n> > > +\t\tuint32_t frameNumber;\n> > > +\t\tuint32_t numBuffers;\n> > > +\t\tcamera3_stream_buffer_t *buffers;\n> > > +\t};\n> > > +\n> > > +\tenum CameraState {\n> > > +\t\tCameraStopped,\n> > > +\t\tCameraRunning,\n> > > +\t};\n> >\n> > With just two states you could instead have a bool running_ field.\n> >\n> > > +\n> > > +\tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > +\tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > > +\tcamera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);\n> > > +\n> > > +\tunsigned int id_;\n> > > +\tlibcamera::Camera *camera_;\n> > > +\tstd::unique_ptr config_;\n> > > +\tCameraState cameraState_;\n> > > +\n> > > +\tcamera_metadata_t *requestTemplate_;\n> > > +\tconst camera3_callback_ops_t *callbacks_;\n> > > +\tconst struct camera3_device *camera3Device_;\n> > > +\n> > > +\tstd::map requestMap_;\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_CAMERA_MODULE_H__ */\n> > > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp\n> > > new file mode 100644\n> > > index 000000000000..af7817f29137\n> > > --- /dev/null\n> > > +++ b/src/android/camera_proxy.cpp\n> > > @@ -0,0 +1,181 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_proxy.cpp - Proxy to camera module instances\n> > > + */\n> > > +\n> > > +#include \"camera_proxy.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +#include \"message.h\"\n> > > +#include \"thread_rpc.h\"\n> > > +#include \"utils.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(HAL);\n> > > +\n> > > +#define LIBCAMERA_HAL_FUNC_DBG\t\\\n> > > +\tLOG(HAL, Debug);\n> >\n> > If we need to trace calls, I think something better than debug messages\n> > would be useful. I'm not sure what yet though.\n> \n> Yeah, I'm not even sure we want to trace calls actually. The fact is\n> that the camera HAL is somewhat a weird beast, as it is a very system\n> specific libcamera extension, and I tried to mimic what is usually\n> found in other HAL implementations I've seen. Should we drop tracing\n> completely and make this more libcamera-like ?\n\nI think so. I also think we'll need tracing in the future, but that's\nfor later.\n\n> > > +\n> > > +static int hal_dev_initialize(const struct camera3_device *dev,\n> > > +\t\t\t const camera3_callback_ops_t *callback_ops)\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\tif (!dev)\n> > > +\t\treturn -EINVAL;\n> >\n> > Can this happen ?\n> \n> I think cros_camera_test exercises this.\n> >\n> > > +\n> > > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > > +\tproxy->setCallbacks(callback_ops);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +static int hal_dev_configure_streams(const struct camera3_device *dev,\n> > > +\t\t\t\t camera3_stream_configuration_t *stream_list)\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\tif (!dev)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > > +\tproxy->configureStreams(stream_list);\n> > > +\n> > > +\treturn 0;\n> >\n> > Shouldn't you propagate the error value from proxy->configureStreams() ?\n> >\n> > > +}\n> > > +\n> > > +static const camera_metadata_t *\n> > > +hal_dev_construct_default_request_settings(const struct camera3_device *dev,\n> > > +\t\t\t\t\t int type)\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\tif (!dev)\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > > +\treturn proxy->constructDefaultRequest(type);\n> >\n> > How about renaming constructDefaultRequest() and\n> > constructDefaultRequestMetadata() to constructDefaultRequestSettings(),\n> > in order to match the HAL operation name ?\n> \n> Ok...\n> \n> > > +}\n> > > +\n> > > +static int hal_dev_process_capture_request(const struct camera3_device *dev,\n> > > +\t\t\t\t\t camera3_capture_request_t *request)\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\tif (!dev)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > > +\treturn proxy->processCaptureRequest(request);\n> > > +}\n> > > +\n> > > +static void hal_dev_dump(const struct camera3_device *dev, int fd)\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +}\n> > > +\n> > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\treturn 0;\n> > > +}\n> >\n> > You could define these methods as static methods of the CameraProxy\n> > class. For instance, hal_dev_configure_streams would become\n> >\n> > int CameraProxy::configureStreams(const struct camera3_device *dev,\n> > \t\t\t\t camera3_stream_configuration_t *stream_list)\n> > {\n> > \tCameraProxy *proxy = reinterpret_cast(dev->priv);\n> > \tproxy->configureStreams(stream_list);\n> > }\n> \n> To avoid the static C methods you mean ? Not sure what is the gain\n> though.\n\nCorrect. The gain is that all related code would be grouped in the same\nclass. It's not a big deal, and could be done on top of this series. I\njust think it would be a bit cleaner (but I could be wrong).\n\n> > > +\n> > > +static camera3_device_ops hal_dev_ops = {\n> >\n> > I wish this could be const :-(\n> >\n> > > +\t.initialize = hal_dev_initialize,\n> > > +\t.configure_streams = hal_dev_configure_streams,\n> > > +\t.register_stream_buffers = nullptr,\n> > > +\t.construct_default_request_settings = hal_dev_construct_default_request_settings,\n> > > +\t.process_capture_request = hal_dev_process_capture_request,\n> > > +\t.get_metadata_vendor_tag_ops = nullptr,\n> > > +\t.dump = hal_dev_dump,\n> > > +\t.flush = hal_dev_flush,\n> > > +\t.reserved = { 0 },\n> >\n> > s/0/nullptr/ ?\n> >\n> > > +};\n> > > +\n> > > +CameraProxy::CameraProxy(unsigned int id, CameraModule *cameraModule)\n> > > +\t: open_(false), id_(id), cameraModule_(cameraModule)\n> > > +{\n> > > +}\n> > > +\n> > > +int CameraProxy::open(const hw_module_t *hardwareModule)\n> > > +{\n> > > +\tint ret = cameraModule_->open();\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\t/* Initialize the hw_device_t in the instance camera3_module_t. */\n> > > +\tcameraDevice_.common.tag = HARDWARE_DEVICE_TAG;\n> > > +\tcameraDevice_.common.version = CAMERA_DEVICE_API_VERSION_3_3;\n> > > +\tcameraDevice_.common.module = (hw_module_t *)hardwareModule;\n> > > +\n> > > +\t/*\n> > > +\t * The camera device operations. These actually implement\n> > > +\t * the Android Camera HALv3 interface.\n> > > +\t */\n> > > +\tcameraDevice_.ops = &hal_dev_ops;\n> > > +\tcameraDevice_.priv = this;\n> > > +\n> > > +\topen_ = true;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void CameraProxy::close()\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\tThreadRpc rpcRequest;\n> > > +\trpcRequest.tag = ThreadRpc::Close;\n> > > +\n> > > +\tthreadRpcCall(rpcRequest);\n> > > +\n> > > +\topen_ = false;\n> > > +}\n> > > +\n> > > +void CameraProxy::setCallbacks(const camera3_callback_ops_t *callbacks)\n> >\n> > I would name this initialize() to match hal_dev_initialize().\n> >\n> > > +{\n> > > +\tLIBCAMERA_HAL_FUNC_DBG\n> > > +\n> > > +\tcameraModule_->setCallbacks(&cameraDevice_, callbacks);\n> > > +}\n> > > +\n> > > +const camera_metadata_t *CameraProxy::constructDefaultRequest(int type)\n> > > +{\n> > > +\treturn cameraModule_->constructDefaultRequestMetadata(type);\n> > > +}\n> > > +\n> > > +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > +{\n> > > +\treturn cameraModule_->configureStreams(stream_list);\n> > > +}\n> > > +\n> > > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)\n> > > +{\n> > > +\tThreadRpc rpcRequest;\n> > > +\trpcRequest.tag = ThreadRpc::ProcessCaptureRequest;\n> > > +\trpcRequest.request = request;\n> > > +\n> > > +\tthreadRpcCall(rpcRequest);\n> > > +\n> > > +\treturn 0;\n> >\n> > Does this method need to be synchronous ? We can keep it as-is for now,\n> > but I wonder if it wouldn't make sense to later move (part of) the\n> > validation code here and make the call asynchronous.\n> >\n> > > +}\n> > > +\n> > > +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)\n> > > +{\n> > > +\tstd::unique_ptr message =\n> > > +\t\t\t\tutils::make_unique();\n> > > +\tmessage->rpc = &rpcRequest;\n> > > +\n> > > +\tcameraModule_->postMessage(std::move(message));\n> > > +\trpcRequest.waitDelivery();\n> > > +}\n> > > diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h\n> > > new file mode 100644\n> > > index 000000000000..69e6878c4352\n> > > --- /dev/null\n> > > +++ b/src/android/camera_proxy.h\n> > > @@ -0,0 +1,41 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * camera_proxy.h - Proxy to camera module instances\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_PROXY_H__\n> > > +#define __ANDROID_CAMERA_PROXY_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"camera_module.h\"\n> > > +\n> > > +class CameraProxy\n> > > +{\n> > > +public:\n> > > +\tCameraProxy(unsigned int id, CameraModule *cameraModule);\n> > > +\n> > > +\tint open(const hw_module_t *hardwareModule);\n> > > +\tvoid close();\n> > > +\n> > > +\tvoid setCallbacks(const camera3_callback_ops_t *callbacks);\n> > > +\tconst camera_metadata_t *constructDefaultRequest(int type);\n> > > +\tint configureStreams(camera3_stream_configuration_t *stream_list);\n> > > +\tint processCaptureRequest(camera3_capture_request_t *request);\n> > > +\n> > > +\tbool isOpen() const { return open_; }\n> >\n> > This isn't used.\n> >\n> > > +\tunsigned int id() const { return id_; }\n> > > +\tcamera3_device_t *device() { return &cameraDevice_; }\n> > > +\n> > > +private:\n> > > +\tvoid threadRpcCall(ThreadRpc &rpcRequest);\n> > > +\n> > > +\tbool open_;\n> >\n> > And neither is this, if you drop the isOpen() method.\n> >\n> > > +\tunsigned int id_;\n> > > +\tCameraModule *cameraModule_;\n> > > +\tcamera3_device_t cameraDevice_;\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_CAMERA_PROXY_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 1f242953db37..63b45832c0d2 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -1,3 +1,11 @@\n> > > +android_hal_sources = files([\n> > > + 'camera3_hal.cpp',\n> > > + 'camera_hal_manager.cpp',\n> > > + 'camera_module.cpp',\n> > > + 'camera_proxy.cpp',\n> > > + 'thread_rpc.cpp'\n> > > +])\n> > > +\n> > > android_camera_metadata_sources = files([\n> > > 'metadata/camera_metadata.c',\n> > > ])\n> > > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp\n> > > new file mode 100644\n> > > index 000000000000..f13db59d0d75\n> > > --- /dev/null\n> > > +++ b/src/android/thread_rpc.cpp\n> > > @@ -0,0 +1,41 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * thread_call.cpp - Inter-thread procedure call\n> >\n> > thread_rpc.cpp\n> >\n> > > + */\n> > > +\n> > > +#include \"message.h\"\n> > > +#include \"thread_rpc.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;\n> > > +\n> > > +ThreadRpcMessage::ThreadRpcMessage()\n> > > +\t: Message(type())\n> > > +{\n> > > +}\n> > > +\n> > > +void ThreadRpc::notifyReception()\n> > > +{\n> > > +\t{\n> > > +\t\tlibcamera::MutexLocker locker(mutex_);\n> > > +\t\tdelivered_ = true;\n> > > +\t}\n> > > +\tcv_.notify_one();\n> > > +}\n> > > +\n> > > +void ThreadRpc::waitDelivery()\n> > > +{\n> > > +\tlibcamera::MutexLocker locker(mutex_);\n> > > +\tcv_.wait(locker, [&]{ return delivered_; });\n> > > +}\n> > > +\n> > > +Message::Type ThreadRpcMessage::type()\n> > > +{\n> > > +\tif (ThreadRpcMessage::rpcType_ == Message::Type::None)\n> > > +\t\trpcType_ = Message::registerMessageType();\n> > > +\n> > > +\treturn rpcType_;\n> > > +}\n> > > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h\n> > > new file mode 100644\n> > > index 000000000000..173caba1a515\n> > > --- /dev/null\n> > > +++ b/src/android/thread_rpc.h\n> > > @@ -0,0 +1,56 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * thread_call.h - Inter-thread procedure call\n> > > + */\n> > > +#ifndef __ANDROID_THREAD_CALL_H__\n> > > +#define __ANDROID_THREAD_CALL_H__\n> >\n> > s/CALL/RPC/\n> >\n> > > +\n> > > +#include \n> > > +#include \n> >\n> > Do you need string ?\n> >\n> > You should #include \n> >\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"message.h\"\n> > > +#include \"thread.h\"\n> > > +\n> > > +class ThreadRpc\n> > > +{\n> > > +public:\n> > > +\tenum RpcTag {\n> > > +\t\tProcessCaptureRequest,\n> > > +\t\tClose,\n> > > +\t};\n> > > +\n> > > +\tThreadRpc()\n> > > +\t\t: delivered_(false) {}\n> > > +\n> > > +\tvoid notifyReception();\n> > > +\tvoid waitDelivery();\n> > > +\n> > > +\tRpcTag tag;\n> > > +\n> > > +\tcamera3_capture_request_t *request;\n> > > +\n> > > +private:\n> > > +\tbool delivered_;\n> > > +\tstd::mutex mutex_;\n> > > +\tstd::condition_variable cv_;\n> > > +};\n> > > +\n> > > +class ThreadRpcMessage : public libcamera::Message\n> > > +{\n> > > +public:\n> > > +\tThreadRpcMessage();\n> > > +\tThreadRpc *rpc;\n> > > +\n> > > +\tstatic Message::Type type();\n> > > +\n> > > +private:\n> > > +\tstatic libcamera::Message::Type rpcType_;\n> > > +};\n> >\n> > FYI, I'll probably move part of this to the libcamera core.\n> \n> Let's merge the HAL first, if you're not working on this already\n> please.\n\nI'm not, don't worry :-) It will be a change on top.\n\n> > > +\n> > > +#endif /* __ANDROID_THREAD_CALL_H__ */\n> > > +\n> >\n> > [snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF3F0615FF\n\tfor ;\n\tThu, 8 Aug 2019 22:09:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0D87BCC;\n\tThu, 8 Aug 2019 22:09:54 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1565294995;\n\tbh=v6QGP5mbL0hwdVnP0C9iQ92srpXnCSx1SLAOEN3AEkA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pUU7BgZPnbiezzpjByfeJhP/tSwAlh4eCTBsav1Wllg2Fvc1HUb3yy37r4UydF6rD\n\tAZ+zk4lfgPICFltEOqza6IWmiwicxMpTFYr+yeCedm1XpHuk+RccpgYNb3K1czltYU\n\tamnozhe9AdEs+/cZYC5Upj6ph75bjG5DyamDUxlU=","Date":"Thu, 8 Aug 2019 23:09:52 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190808200952.GC6055@pendragon.ideasonboard.com>","References":"<20190806195518.16739-1-jacopo@jmondi.org>\n\t<20190806195518.16739-7-jacopo@jmondi.org>\n\t<20190807153208.GG5048@pendragon.ideasonboard.com>\n\t<20190808153806.3j5d72fwi5vayavw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190808153806.3j5d72fwi5vayavw@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 08 Aug 2019 20:09:56 -0000"}}]