[libcamera-devel] [PATCH v2 6/6] android: hal: Add Camera3 HAL
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 8 22:09:52 CEST 2019
Hi Jacopo,
On Thu, Aug 08, 2019 at 05:38:15PM +0200, Jacopo Mondi wrote:
> On Wed, Aug 07, 2019 at 06:32:08PM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 06, 2019 at 09:55:18PM +0200, Jacopo Mondi wrote:
> > > Add libcamera Android Camera HALv3 implementation.
> > >
> > > The initial camera HAL implementation supports the LIMITED hardware
> > > level and uses statically defined metadata and camera characteristics.
> > >
> > > Add a build option named 'android' and adjust the build system to
> > > selectively compile the Android camera HAL and link it against the
> > > required Android libraries.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > meson_options.txt | 5 +
> > > src/android/camera3_hal.cpp | 130 +++++
> > > src/android/camera_hal_manager.cpp | 173 +++++++
> > > src/android/camera_hal_manager.h | 56 ++
> > > src/android/camera_module.cpp | 799 +++++++++++++++++++++++++++++
> > > src/android/camera_module.h | 75 +++
> > > src/android/camera_proxy.cpp | 181 +++++++
> > > src/android/camera_proxy.h | 41 ++
> > > src/android/meson.build | 8 +
> > > src/android/thread_rpc.cpp | 41 ++
> > > src/android/thread_rpc.h | 56 ++
> > > src/libcamera/meson.build | 9 +
> > > src/meson.build | 5 +-
> > > 13 files changed, 1578 insertions(+), 1 deletion(-)
> > > create mode 100644 src/android/camera3_hal.cpp
> > > create mode 100644 src/android/camera_hal_manager.cpp
> > > create mode 100644 src/android/camera_hal_manager.h
> > > create mode 100644 src/android/camera_module.cpp
> > > create mode 100644 src/android/camera_module.h
> > > create mode 100644 src/android/camera_proxy.cpp
> > > create mode 100644 src/android/camera_proxy.h
> > > create mode 100644 src/android/thread_rpc.cpp
> > > create mode 100644 src/android/thread_rpc.h
> >
> > [snip]
> >
> > > diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp
> > > new file mode 100644
> > > index 000000000000..0a97a9333d20
> > > --- /dev/null
> > > +++ b/src/android/camera3_hal.cpp
> > > @@ -0,0 +1,130 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera3_hal.cpp - Android Camera3 HAL module
> > > + */
> > > +
> > > +#include <iostream>
> > > +#include <memory>
> > > +#include <vector>
> > > +
> > > +#include <hardware/hardware.h>
> > > +#include <system/camera_metadata.h>
> >
> > Do you need camera_metadata.h ? Isn't it enough to include
> > camera_common.h ? I think hardware.h can also be dropped.
>
> camera_common.h includes camera_metadata.h, so I could just include
> the one I need.
>
> > > +
> > > +#include <libcamera/libcamera.h>
> >
> > To speed up compilation, it would be better to include only the header
> > files we need (here and everywhere else).
>
> Indeed
>
> > > +
> > > +#include "log.h"
> > > +
> > > +#include "camera_hal_manager.h"
> > > +#include "camera_module.h"
> > > +#include "camera_proxy.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DEFINE_CATEGORY(HAL)
> > > +
> > > +static CameraHalManager cameraManager;
> > > +
> > > +/*------------------------------------------------------------------------------
> > > + * Android Camera HAL callbacks
> > > + */
> > > +
> > > +static int hal_get_number_of_cameras(void)
> > > +{
> > > + return cameraManager.numCameras();
> > > +}
> > > +
> > > +static int hal_get_camera_info(int id, struct camera_info *info)
> > > +{
> > > + return cameraManager.getCameraInfo(id, info);
> > > +}
> > > +
> > > +static int hal_set_callbacks(const camera_module_callbacks_t *callbacks)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int hal_open_legacy(const struct hw_module_t *module, const char *id,
> > > + uint32_t halVersion, struct hw_device_t **device)
> > > +{
> > > + return -ENOSYS;
> > > +}
> > > +
> > > +static int hal_set_torch_mode(const char *camera_id, bool enabled)
> > > +{
> > > + return -ENOSYS;
> > > +}
> > > +
> > > +/*
> > > + * First entry point of the camera HAL module.
> > > + *
> > > + * Initialize the HAL but does not open any camera device yet (see hal_dev_open)
> > > + */
> > > +static int hal_init()
> > > +{
> > > + LOG(HAL, Info) << "Initialising Android camera HAL";
> > > +
> > > + cameraManager.initialize();
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/*------------------------------------------------------------------------------
> > > + * Android Camera Device
> > > + */
> > > +
> > > +static int hal_dev_close(hw_device_t *hw_device)
> > > +{
> > > + if (!hw_device)
> > > + return -EINVAL;
> > > +
> > > + camera3_device_t *dev = reinterpret_cast<camera3_device_t *>(hw_device);
> > > + CameraProxy *cameraModule = reinterpret_cast<CameraProxy *>(dev->priv);
> >
> > s/cameraModule/proxy/ otherwise it's very confusing to have a
> > CameraProxy instance called cameraModule when there's a CameraModule
> > class.
> >
> > This comment applies to all other similar instances.
>
> Sorry, leftover. I don't see any other module/proxy in this file
>
> > > +
> > > + return cameraManager.close(cameraModule);
> >
> > As cameraManager.close() only calls proxy->close(), how about defining
> > the hal_dev_close() method as a static method of CameraProxy, and
> > setting proxy->device()->common.close inside the CameraProxy constructor
> > ? It would avoid touching the internals of the CameraProxy in
> > hal_dev_open() below.
>
> I liked having hal_dev_open and hal_dev_close here, as part of the HAL
> module. But yes, we can save one function call and poking into the
> device.common fields below..
I agree that grouped hal_dev_open() and hal_dev_close() is nice, but
it's offset by the needed to then poke into
proxy->device()->common.close. Let's also remember that the HAL API
separates open from all the other operations, so we should be sad to
separate them here.
> > > +}
> > > +
> > > +static int hal_dev_open(const hw_module_t* module, const char* name,
> > > + hw_device_t** device)
> > > +{
> > > + LOG(HAL, Debug) << "Open camera: " << name;
> >
> > s/camera:/camera/
> >
> > > +
> > > + int id = atoi(name);
> > > + CameraProxy *proxy = cameraManager.open(id, module);
> > > + if (!proxy) {
> > > + LOG(HAL, Error) << "Failed to open camera module " << id;
> > > + return -ENODEV;
> > > + }
> > > + proxy->device()->common.close = hal_dev_close;
> > > + *device = &proxy->device()->common;
> > > +
> > > + return 0;
> > > +}
> >
> > Would it make sense to define all these methods as static methods of the
> > CameraHalManager class, and turn the class into a singleton with an
> > instance() method ?
>
> Which methods? hal_dev_open? All the hal_dev_ ones which are used to
> initialize the camera_module_t ? I'm not sure I see a clear win...
hal_dev_open(), but also all the methods defined in camera_module_t. In
general I think it's nice to avoid global methods, and use singletons
instead of global variables. This could be addressed later too.
> > > +
> > > +static struct hw_module_methods_t hal_module_methods = {
> > > + .open = hal_dev_open,
> > > +};
> > > +
> > > +camera_module_t HAL_MODULE_INFO_SYM = {
> > > + .common = {
> > > + .tag = HARDWARE_MODULE_TAG,
> > > + .module_api_version = CAMERA_MODULE_API_VERSION_2_4,
> > > + .hal_api_version = HARDWARE_HAL_API_VERSION,
> > > + .id = CAMERA_HARDWARE_MODULE_ID,
> > > + .name = "Libcamera Camera3HAL Module",
> >
> > s/Libcamera/libcamera/
> >
> > > + .author = "libcamera",
> > > + .methods = &hal_module_methods,
> > > + .dso = nullptr,
> > > + .reserved = {},
> > > + },
> > > +
> > > + .get_number_of_cameras = hal_get_number_of_cameras,
> > > + .get_camera_info = hal_get_camera_info,
> > > + .set_callbacks = hal_set_callbacks,
> > > + .get_vendor_tag_ops = nullptr,
> > > + .open_legacy = hal_open_legacy,
> > > + .set_torch_mode = hal_set_torch_mode,
> > > + .init = hal_init,
> > > + .reserved = {},
> > > +};
> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > > new file mode 100644
> > > index 000000000000..734b5eebd9a5
> > > --- /dev/null
> > > +++ b/src/android/camera_hal_manager.cpp
> > > @@ -0,0 +1,173 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_hal_manager.cpp - Libcamera Android Camera Manager
> > > + */
> > > +
> > > +#include "camera_hal_manager.h"
> > > +
> > > +#include <map>
> >
> > This shouldn't be needed.
>
> Yes
>
> > > +
> > > +#include <hardware/hardware.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <libcamera/libcamera.h>
> > > +#include <libcamera/camera.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +#include "camera_module.h"
> > > +#include "camera_proxy.h"
> > > +
> > > +using namespace libcamera;
> >
> > Missing blank line.
> >
> > > +LOG_DECLARE_CATEGORY(HAL);
> > > +
> > > +CameraHalManager::~CameraHalManager()
> > > +{
> > > + for (auto metadata : staticMetadataMap_)
> > > + free_camera_metadata(metadata.second);
> > > +}
> > > +
> > > +int CameraHalManager::initialize()
> > > +{
> > > + /*
> > > + * Thread::start() will create a std::thread which will execute
> > > + * CameraHalManager::run()
> > > + */
> >
> > That's documenting the libcamera::Thread class (and the std::thread is
> > an internal implementation detail). I would just state
> >
> > /*
> > * Start the camera HAL manager thread and wait until its
> > * initialisation completes to be fully operational before
> > * receiving calls from the camera stack.
> > */
> >
> > (thus dropping the next comment)
>
> As you might have noticed, the camera HAL still has comments and
> annotations I added while developing, I'll clear them up better.
>
> > > + start();
> > > +
> > > + /*
> > > + * Wait for run() to notify us before returning to the caller to
> > > + * make sure libcamera is fully initialized before we start handling
> > > + * calls from the camera stack.
> > > + */
> > > + MutexLocker locker(mutex_);
> > > + cv_.wait(locker);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void CameraHalManager::run()
> > > +{
> > > + /*
> > > + * The run() operation is scheduled in its own thread;
> > > + * It exec() to run the even dispatcher loop and initialize libcamera.
> >
> > s/even/event/
> >
> > > + *
> > > + * All the libcamera components initialized from here will be tied to
> > > + * the same thread as the here below run event dispatcher.
> >
> > The first paragraph also mostly documents the Thread class. I would drop
> > it, and write the second paragraph as
> >
> > /*
> > * All the libcamera components must be initialised here, in
> > * order to bind them to the camera HAL manager thread that
> > * executes the event dispatcher.
> > /
> >
> > > + */
> > > + libcameraManager_ = libcamera::CameraManager::instance();
> > > +
> > > + int ret = libcameraManager_->start();
> > > + if (ret) {
> > > + LOG(HAL, Error) << "Failed to start camera manager: "
> > > + << strerror(-ret);
> > > + return;
> > > + }
> > > +
> > > + /*
> > > + * For each Camera registered in the system, a CameraModule
> > > + * get created here with an associated Camera and a proxy.
> >
> > s/get/gets/
> >
> > > + *
> > > + * \todo Support camera hotplug.
> > > + */
> > > + unsigned int cameraIndex = 0;
> >
> > Maybe just index ?
> >
> > > + for (auto camera : libcameraManager_->cameras()) {
> >
> > auto &camera
> >
> > otherwise camera will be a copy of the shared pointer instead of a
> > reference to it.
> >
> > > + cameras_.push_back(camera);
> >
> > The cameras_ vector isn't used after this. I would drop it, and store
> > the shared pointer in CameraModule.
>
> Correct, I missed this in the the latest rework of the proxies
> lifecycle.
>
> > > +
> > > + CameraModule *module = new CameraModule(cameraIndex,
> > > + camera.get());
> > > + modules_.emplace_back(module);
> > > +
> > > + CameraProxy *proxy = new CameraProxy(cameraIndex,
> > > + module);
> > > + proxies_.emplace_back(proxy);
> >
> > Similarly, how about sotring the module pointer inside the CameraProxy
> > class ? That would ensure that all accesses to the module go through the
> > proxy, which I think would be a good idea.
>
> As I use the modules_ only to access the map of static metadata which
> should now got away, I think it's doable
>
> > CameraProxy *proxy = new CameraProxy(index, camera);
> > proxies_.emplace_back(proxy);
> >
> > > +
> > > + ++cameraIndex;
> > > + }
> > > +
> > > + /*
> > > + * libcamera has been initialized! Unlock the initialize() caller
> > > + * as we're now ready to handle calls from the framework.
> > > + */
> > > + cv_.notify_one();
> > > +
> > > + /* Now start processing events and messages! */
> >
> > s/!/./ in the above two messages, there's no need to be
> > over-enthusiastic :-)
> >
> > > + exec();
> > > +}
> > > +
> > > +CameraProxy *CameraHalManager::open(unsigned int id,
> > > + const hw_module_t *hardwareModule)
> > > +{
> > > + if (id < 0 || id >= numCameras()) {
> > > + LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> > > + return nullptr;
> > > + }
> > > +
> > > + CameraProxy *proxy = proxies_[id].get();
> > > + if (proxy->open(hardwareModule))
> > > + return proxy;
> >
> > Shouldn't you return nullprtr here ?
>
> Ideed
>
> > > +
> > > + LOG(HAL, Info) << "Camera: '" << id << "' opened";
> >
> > s/Camera:/Camera/
> >
> > > +
> > > + return proxy;
> > > +}
> > > +
> > > +int CameraHalManager::close(CameraProxy *proxy)
> > > +{
> > > + proxy->close();
> > > + LOG(HAL, Info) << "Close camera: " << proxy->id();
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +unsigned int CameraHalManager::numCameras() const
> > > +{
> > > + return libcameraManager_->cameras().size();
> > > +}
> > > +
> > > +/*
> > > + * Before the camera framework even tries to open a camera device with
> > > + * hal_dev_open() it requires the camera HAL to report a list of static
> > > + * informations on the camera device with id \a id in the hal_get_camera_info()
> > > + * method.
> > > + *
> > > + * The static metadata should be then generated probably from a
> > > + * platform-specific module, as we cannot operate on the camera at this time as
> > > + * it has not yet been open by the framework.
> >
> > s/open/opened/
> >
> > What prevents us from opening the camera internally ? I don't think we
>
> It would complicate the lifecycle handling a bit, but I guess it's
> doable. AS of now, where all metadata are static it is not worth it
> imo
We don't need to do it now. I was mostly addressing the comment above
that states we can't operate on the camera, which I think isn't correct
as we can decouple opening the libcamera::Camera and the HAL camera.
> > need a platform-specific module.
>
> We surely won't be able to get from kernel at this point all the
> information we need, in example, the camera position.
I can't dispute that :-) We should however get it from the
libcamera::Camera.
> > > + *
> > > + * This is what the Intel XML file is used for, it is parsed and the data there
> > > + * combined with informations from the PSL service (which I -think- it's what
> > > + * our IPA is) to generate a list of static metadata per-camera device.
> >
> > I'd drop this paragraph.
> >
> > > + */
> > > +int CameraHalManager::getCameraInfo(int id, struct camera_info *info)
> > > +{
> > > + int cameras = numCameras();
> > > + if (id >= cameras || id < 0 || !info) {
> >
> > Can we be called with info == nullptr ? I think I'd drop that part of
> > the check.
>
> All (most?) the extra-paranoid checks I have added here are error conditions
> tested by the cros_camera_test suite.
Lovely test suite... I suppose we should consider ourselves lucky that
it doesn't test with info = 0xdeadbeef or other random pointer values
:-)
> > > + LOG(HAL, Error) << "Invalid camera id: " << id;
> >
> > s/id:/id/
> >
> > You may also want to add quotes around the value, as done in
> > CameraHalManager::open() (or drop them there).
> >
> > > + return -EINVAL;
> > > + }
> > > +
> > > + camera_metadata_t *staticMetadata;
> > > + auto it = staticMetadataMap_.find(id);
> > > + if (it != staticMetadataMap_.end()) {
> > > + staticMetadata = it->second;
> > > + } else {
> > > + CameraModule *cameraModule = modules_[id].get();
> > > +
> > > + staticMetadata = cameraModule->getStaticMetadata();
> > > + staticMetadataMap_[id] = staticMetadata;
> >
> > Why do you need the map, why can't you always retrieve the static
> > metadata from the CameraModule ?
>
> I should, yes, so we can frop modules_
>
> > > + }
> > > +
> > > + /* \todo: Get these info dynamically inspecting the camera module. */
> >
> > s/://
> >
> > > + info->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK;
> > > + info->orientation = 0;
> > > + info->device_version = 0;
> > > + info->resource_cost = 0;
> > > + info->static_camera_characteristics = staticMetadata;
> > > + info->conflicting_devices = nullptr;
> > > + info->conflicting_devices_length = 0;
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > > new file mode 100644
> > > index 000000000000..65d6fa3150ef
> > > --- /dev/null
> > > +++ b/src/android/camera_hal_manager.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_hal_manager.h - Libcamera Android Camera Manager
> > > + */
> > > +#ifndef __ANDROID_CAMERA_MANAGER_H__
> > > +#define __ANDROID_CAMERA_MANAGER_H__
> > > +
> > > +#include <condition_variable>
> > > +#include <cstddef>
> > > +#include <map>
> > > +#include <memory>
> > > +#include <vector>
> > > +
> > > +#include <hardware/hardware.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <libcamera/libcamera.h>
> > > +
> > > +#include "thread.h"
> > > +#include "thread_rpc.h"
> > > +
> > > +class CameraModule;
> > > +class CameraProxy;
> > > +
> > > +class CameraHalManager : public libcamera::Thread
> > > +{
> > > +public:
> > > + ~CameraHalManager();
> > > +
> > > + int initialize();
> >
> > Our initialisation methods are usually called init(), not initialize().
>
> ok
>
> > > +
> > > + CameraProxy *open(unsigned int id, const hw_module_t *module);
> > > + int close(CameraProxy *proxy);
> > > +
> > > + unsigned int numCameras() const;
> > > + int getCameraInfo(int id, struct camera_info *info);
> > > +
> > > +private:
> > > + void run() override;
> > > + camera_metadata_t *getStaticMetadata(unsigned int id);
> > > +
> > > + libcamera::CameraManager *libcameraManager_;
> >
> > I think you can call this manager_.
> >
> > > +
> > > + std::mutex mutex_;
> > > + std::condition_variable cv_;
> > > +
> > > + std::vector<std::shared_ptr<libcamera::Camera>> cameras_;
> > > + std::vector<std::unique_ptr<CameraModule>> modules_;
> > > + std::vector<std::unique_ptr<CameraProxy>> proxies_;
> > > +
> > > + std::map<unsigned int, camera_metadata_t *> staticMetadataMap_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_MANAGER_H__ */
> > > diff --git a/src/android/camera_module.cpp b/src/android/camera_module.cpp
> > > new file mode 100644
> > > index 000000000000..b64e377fb439
> > > --- /dev/null
> > > +++ b/src/android/camera_module.cpp
> > > @@ -0,0 +1,799 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_module.cpp - Libcamera Android Camera Module
> > > + */
> > > +
> > > +#include "camera_module.h"
> > > +
> > > +#include <iostream>
> > > +#include <memory>
> > > +
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <hardware/camera3.h>
> >
> > I would group all the Android headers together, so
> >
> > #include <hardware/camera3.h>
> > #include <system/camera_metadata.h>
> >
> > #include <libcamera/libcamera.h>
> >
> > > +#include <libcamera/libcamera.h>
> > > +
> > > +#include "message.h"
> > > +
> > > +#include "log.h"
> >
> > #include "log.h"
> > #include "message.h"
> >
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL);
> > > +
> > > +CameraModule::CameraModule(unsigned int id, libcamera::Camera *camera)
> > > + : id_(id), camera_(camera), cameraState_(CameraStopped),
> > > + requestTemplate_(nullptr)
> > > +{
> > > + camera_->requestCompleted.connect(this,
> > > + &CameraModule::requestComplete);
> >
> > This holds on a single line.
> >
> > > +}
> > > +
> > > +CameraModule::~CameraModule()
> > > +{
> > > + if (requestTemplate_)
> > > + free_camera_metadata(requestTemplate_);
> > > + requestTemplate_ = nullptr;
> > > +}
> > > +
> > > +void CameraModule::message(Message *message)
> > > +{
> > > + ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>
> > > + (message);
> > > +
> > > + if (rpcMessage->type() != ThreadRpcMessage::type())
> > > + return Object::message(message);
> >
> > As Message may not be a ThreadRpcMessage, you should write this as
> >
> > if (message->type() != ThreadRpcMessage::type())
> > return Object::message(message);
> >
> > ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message);
> >
> > > +
> > > + ThreadRpc *rpc = rpcMessage->rpc;
> > > +
> > > + switch (rpc->tag) {
> > > + case ThreadRpc::ProcessCaptureRequest:
> > > + processCaptureRequest(rpc->request);
> > > + break;
> > > + case ThreadRpc::Close:
> > > + close();
> > > + break;
> > > + default:
> > > + LOG(HAL, Error) << "Unknown RPC operation: " << rpc->tag;
> > > + }
> > > +
> > > + rpc->notifyReception();
> > > +}
> > > +
> > > +int CameraModule::open()
> > > +{
> > > + int ret = camera_->acquire();
> > > + if (ret) {
> > > + LOG(HAL, Error) << "Failed to acquire the camera";
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void CameraModule::close()
> > > +{
> > > + camera_->stop();
> > > +
> > > + camera_->freeBuffers();
> > > + camera_->release();
> > > +
> > > + cameraState_ = CameraStopped;
> > > +}
> > > +
> > > +void CameraModule::setCallbacks(const struct camera3_device *camera3Device,
> > > + const camera3_callback_ops_t *callbacks)
> > > +{
> > > + camera3Device_ = camera3Device;
> >
> > camera3Device_ seems unused.
> >
> > > + callbacks_ = callbacks;
> > > +}
> > > +
> > > +camera_metadata_t *CameraModule::getStaticMetadata()
> > > +{
> > > + int ret;
> > > +
> > > + /*
> > > + * The below metadata has been added by inspecting the Intel XML
> > > + * file and it's associated parsing routines in the Intel IPU3 HAL.
> > > + *
> > > + * The here below metadata are enough to satisfy the camera3-test
> > > + * provided by ChromeOS, but a real camera implementation might require
> >
> > s/might/will/
> >
> > > + * more.
> > > + *
> > > + * See CameraConf::AiqConf class implementation in the Intel HAL
> > > + * to get a list of the static metadata registered by parsing the
> > > + * XML config file in AiqConf::fillStaticMetadataFromCMC
> > > + */
> > > +
> > > + /*
> > > + * FIXME: this sizes come from the Intel IPU3 HAL, and are way too large for
> >
> > s/FIXME:/\todo/
> > s/this/these/
> >
> > > + * this intial HAL version.
> > > + */
> > > + #define STATIC_ENTRY_CAP 256
> > > + #define STATIC_DATA_CAP 6688
> > > + camera_metadata_t *staticMetadata =
> > > + allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> > > +
> > > + /* Sensor static metadata. */
> > > + int32_t pixelArraySize[] = {
> > > + 2592, 1944,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > > + &pixelArraySize, 2);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t sensorSizes[] = {
> > > + 0, 0, 2560, 1920,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > > + &sensorSizes, 4);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t sensitivityRange[] = {
> > > + 32, 2400,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> > > + &sensitivityRange, 2);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> > > + &filterArr, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int64_t exposureTimeRange[] = {
> > > + 100000, 200000000,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> > > + &exposureTimeRange, 2);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t orientation = 0;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SENSOR_ORIENTATION,
> > > + &orientation, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /* Flash static metadata. */
> > > + char flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_FLASH_INFO_AVAILABLE,
> > > + &flashAvailable, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /* Lens static metadata. */
> > > + float fn = 2.53 / 100;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_LENS_INFO_AVAILABLE_APERTURES, &fn, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /* Control metadata. */
> > > + char controlMetadata = ANDROID_CONTROL_MODE_AUTO;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_CONTROL_AVAILABLE_MODES,
> > > + &controlMetadata, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + char availableAntiBandingModes[] = {
> > > + ANDROID_CONTROL_AE_ANTIBANDING_MODE_OFF,
> > > + ANDROID_CONTROL_AE_ANTIBANDING_MODE_50HZ,
> > > + ANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,
> > > + ANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> > > + availableAntiBandingModes, 4);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + char aeAvailableModes[] = {
> > > + ANDROID_CONTROL_AE_MODE_ON,
> > > + ANDROID_CONTROL_AE_MODE_OFF,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > > + aeAvailableModes, 2);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + controlMetadata = ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > + &controlMetadata, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > + &awbLockAvailable, 1);
> > > +
> > > + /* Scaler static metadata. */
> > > + std::vector<uint32_t> availableStreamFormats = {
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_BLOB,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS,
> > > + availableStreamFormats.data(),
> > > + availableStreamFormats.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + std::vector<uint32_t> availableStreamConfigurations = {
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,
> > > + ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920,
> > > + ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,
> > > + ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> > > + availableStreamConfigurations.data(),
> > > + availableStreamConfigurations.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + std::vector<int64_t> availableStallDurations = {
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> > > + availableStallDurations.data(),
> > > + availableStallDurations.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + std::vector<int64_t> minFrameDurations = {
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,
> > > + ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,
> > > + };
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> > > + minFrameDurations.data(), minFrameDurations.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /* Info static metadata. */
> > > + uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > + ret = add_camera_metadata_entry(staticMetadata,
> > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > > + &supportedHWLevel, 1);
> > > +
> > > + return staticMetadata;
> > > +}
> > > +
> > > +const camera_metadata_t *CameraModule::constructDefaultRequestMetadata(int type)
> > > +{
> > > + int ret;
> > > +
> > > + /*
> > > + * TODO: inspect type and pick the right metadata pack.
> >
> > s/TODO:/\todo/
> >
> > > + * As of now just use a single one for all templates
> > > + */
> > > + uint8_t captureIntent;
> > > + switch (type) {
> > > + case CAMERA3_TEMPLATE_PREVIEW:
> > > + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> > > + break;
> > > + case CAMERA3_TEMPLATE_STILL_CAPTURE:
> > > + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> > > + break;
> > > + case CAMERA3_TEMPLATE_VIDEO_RECORD:
> > > + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> > > + break;
> > > + case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> > > + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> > > + break;
> > > + case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> > > + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> > > + break;
> > > + case CAMERA3_TEMPLATE_MANUAL:
> > > + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> > > + break;
> > > + default:
> > > + LOG(HAL, Error) << "Invalid template request type: " << type;
> > > + return nullptr;
> > > + }
> > > +
> > > + if (requestTemplate_)
> > > + return requestTemplate_;
> > > +
> > > + /* FIXME: this sizes are quite arbitrary ones */
> > > + #define REQUEST_TEMPLATE_ENTRIES 30
> > > + #define REQUEST_TEMPLATE_DATA 2048
> > > + requestTemplate_ = allocate_camera_metadata(REQUEST_TEMPLATE_ENTRIES,
> > > + REQUEST_TEMPLATE_DATA);
> > > + if (!requestTemplate_) {
> > > + LOG(HAL, Error) << "Failed to allocate template metadata";
> > > + return nullptr;
> > > + }
> > > +
> > > + /*
> > > + * Fill in the required Request metadata.
> > > + *
> > > + * Part (most?) of this entries comes from inspecting the Intel's IPU3
> > > + * HAL xml configuration file.
> > > + */
> > > +
> > > + /* Set to 0 the number of 'processed and stalling' streams (ie JPEG). */
> > > + int32_t maxOutStream[] = { 0, 2, 0 };
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
> > > + maxOutStream, 3);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t maxPipelineDepth = 5;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > > + &maxPipelineDepth, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t inputStreams = 0;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,
> > > + &inputStreams, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t partialResultCount = 1;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> > > + &partialResultCount, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t availableCapabilities[] = {
> > > + ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> > > + };
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> > > + availableCapabilities, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AE_MODE,
> > > + &aeMode, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t aeExposureCompensation = 0;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > > + &aeExposureCompensation, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > + &aePrecaptureTrigger, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AE_LOCK,
> > > + &aeLock, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AF_TRIGGER,
> > > + &afTrigger, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AWB_MODE,
> > > + &awbMode, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AWB_LOCK,
> > > + &awbLock, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > + &awbLockAvailable, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t flashMode = ANDROID_FLASH_MODE_OFF;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_FLASH_MODE,
> > > + &flashMode, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > + &faceDetectMode, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_CONTROL_CAPTURE_INTENT,
> > > + &captureIntent, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /*
> > > + * This is quite hard to list at the moment wihtout knowing what
> > > + * we could control.
> > > + *
> > > + * For now, just list in the available Request keys and in the available
> > > + * result keys the control and reporting of the AE algorithm.
> > > + */
> > > + std::vector<int32_t> availableRequestKeys = {
> > > + ANDROID_CONTROL_AE_MODE,
> > > + ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > > + ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > + ANDROID_CONTROL_AE_LOCK,
> > > + ANDROID_CONTROL_AF_TRIGGER,
> > > + ANDROID_CONTROL_AWB_MODE,
> > > + ANDROID_CONTROL_AWB_LOCK,
> > > + ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > + ANDROID_CONTROL_CAPTURE_INTENT,
> > > + ANDROID_FLASH_MODE,
> > > + ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > + };
> > > +
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
> > > + availableRequestKeys.data(),
> > > + availableRequestKeys.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + std::vector<int32_t> availableResultKeys = {
> > > + ANDROID_CONTROL_AE_MODE,
> > > + ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> > > + ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> > > + ANDROID_CONTROL_AE_LOCK,
> > > + ANDROID_CONTROL_AF_TRIGGER,
> > > + ANDROID_CONTROL_AWB_MODE,
> > > + ANDROID_CONTROL_AWB_LOCK,
> > > + ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > + ANDROID_CONTROL_CAPTURE_INTENT,
> > > + ANDROID_FLASH_MODE,
> > > + ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > + };
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,
> > > + availableResultKeys.data(),
> > > + availableResultKeys.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /*
> > > + * The available characteristics should be the tags reported
> > > + * as part of the static metadata reported at hal_get_camera_info()
> > > + * time. The xml file sets those to 0 though.
> > > + */
> > > + std::vector<int32_t> availableCharacteristicsKeys = {};
> > > + ret = add_camera_metadata_entry(requestTemplate_,
> > > + ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> > > + availableCharacteristicsKeys.data(),
> > > + availableCharacteristicsKeys.size());
> > > + METADATA_ASSERT(ret);
> > > +
> > > + return requestTemplate_;
> > > +}
> > > +
> > > +/**
> > > + * Inspect the stream_list to produce a list of StreamConfiguration to
> > > + * be use to configure the Camera.
> > > + */
> > > +int CameraModule::configureStreams(camera3_stream_configuration_t *stream_list)
> > > +{
> > > +
> >
> > Extra blank line.
> >
> > > + for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > > + camera3_stream_t *stream = stream_list->streams[i];
> > > +
> > > + LOG(HAL, Info) << "Stream #" << i
> > > + << ": direction: " << stream->stream_type
> > > + << " - width: " << stream->width
> > > + << " - height: " << stream->height
> > > + << " - format: " << std::hex << stream->format;
> >
> > s/ -/,/ in the above three lines
> >
> > And I would print size: ...x... instead of width and height.
> >
> > > + }
> > > +
> > > + /* Hardcode viewfinder role, collecting sizes from the stream config. */
> > > + if (stream_list->num_streams != 1) {
> > > + LOG(HAL, Error) << "Only one stream supported";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + StreamRoles roles = {};
> > > + roles.push_back(StreamRole::StillCapture);
> >
> > I think you can write this as
> >
> > StreamRoles roles{ StreamRole::StillCapture };
> >
> > > +
> > > + config_ = camera_->generateConfiguration(roles);
> > > + if (!config_ || config_->empty()) {
> > > + LOG(HAL, Error) << "Failed to generate camera configuration";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Only one stream is supported. */
> > > + camera3_stream_t *camera3Stream = stream_list->streams[0];
> > > + StreamConfiguration *streamConfiguration = &config_->at(0);
> > > + streamConfiguration->size.width = camera3Stream->width;
> > > + streamConfiguration->size.height = camera3Stream->height;
> > > + streamConfiguration->memoryType = ExternalMemory;
> > > +
> > > + /*
> > > + * FIXME: do not change the pixel format from the one returned
> > > + * from the Camera::generateConfiguration();
> > > + *
> > > + * We'll need to translate from Android defined pixel format codes
> > > + * to whatever libcamera will use.
> > > + */
> > > +
> > > + switch (config_->validate()) {
> > > + case CameraConfiguration::Valid:
> > > + break;
> > > + case CameraConfiguration::Adjusted:
> > > + LOG(HAL, Info) << "Camera configuration adjusted";
> >
> > Missing config_.reset() ?
> >
> > > + return -EINVAL;
> > > + case CameraConfiguration::Invalid:
> > > + LOG(HAL, Info) << "Camera configuration invalid";
> > > + config_.reset();
> > > + return -EINVAL;
> > > + }
> > > +
> > > + camera3Stream->max_buffers = streamConfiguration->bufferCount;
> > > +
> > > + /*
> > > + * Once the CameraConfiguration has been adjusted/validated
> > > + * it can be applied to the camera.
> > > + */
> > > + int ret = camera_->configure(config_.get());
> > > + if (ret) {
> > > + LOG(HAL, Error) << "Failed to configure camera '"
> > > + << camera_->name() << "'";
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int CameraModule::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > > +{
> > > + StreamConfiguration *streamConfiguration = &config_->at(0);
> > > + Stream *stream = streamConfiguration->stream();
> > > +
> > > + if (camera3Request->num_output_buffers != 1) {
> > > + LOG(HAL, Error) << "Invalid number of output buffers: "
> > > + << camera3Request->num_output_buffers;
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Start the camera if that's the first request we handle. */
> > > + if (cameraState_ == CameraStopped) {
> > > + int ret = camera_->allocateBuffers();
> > > + if (ret) {
> > > + LOG(HAL, Error) << "Failed to allocate buffers";
> > > + return ret;
> > > + }
> > > +
> > > + ret = camera_->start();
> > > + if (ret) {
> > > + LOG(HAL, Error) << "Failed to start camera";
> > > + camera_->freeBuffers();
> > > + return ret;
> > > + }
> > > +
> > > + cameraState_ = CameraRunning;
> > > + }
> > > +
> > > + /*
> > > + * Queue a request for the Camera with the provided dmabuf file
> > > + * descriptors.
> > > + */
> > > + const camera3_stream_buffer_t *camera3Buffer =
> > > + &camera3Request->output_buffers[0];
> > > + const buffer_handle_t camera3Handle = *camera3Buffer->buffer;
> > > +
> > > + std::array<int, 3> fds = {
> > > + camera3Handle->data[0],
> > > + camera3Handle->data[1],
> > > + camera3Handle->data[2],
> > > + };
> > > + std::unique_ptr<Buffer> buffer = stream->createBuffer(fds);
> > > + if (!buffer) {
> > > + LOG(HAL, Error) << "Failed to create buffer";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + Request *request = camera_->createRequest();
> > > + request->addBuffer(std::move(buffer));
> > > + int ret = camera_->queueRequest(request);
> > > + if (ret) {
> > > + LOG(HAL, Error) << "Failed to queue request";
> > > + return ret;
> > > + }
> > > +
> > > + /* Save the request descriptors for use at completion time. */
> > > + Camera3RequestDescriptor descriptor = {};
> > > + descriptor.frameNumber = camera3Request->frame_number,
> > > + descriptor.numBuffers = camera3Request->num_output_buffers,
> >
> > s/,$/;/ for the two lines above.
> >
> > I don't think you need to store numBuffers in the descriptors, it can be
> > retrieved from buffers.size() in requestComplete().
>
> numBuffers is the framework required number of buffers, buffers.size()
> is the number of buffers completed in the request. They -should- be
> the same, but I would keep them separate to make sure the completed
> request is correct.
It boils down to how much paranoia we need to build in :-) How could
they be different ?
> > > + descriptor.buffers = new camera3_stream_buffer_t[descriptor.numBuffers];
> > > + for (unsigned int i = 0; i < descriptor.numBuffers; ++i) {
> > > + camera3_stream_buffer_t &buffer = descriptor.buffers[i];
> > > + buffer = camera3Request->output_buffers[i];
> >
> > Just
> >
> > descriptor.buffers[i] = camera3Request->output_buffers[i];
> > > + }
> > > +
> > > + requestMap_[request] = descriptor;
> >
> > How about using the request cookie (passed to Camera::createRequest())
> > for this purpose ?
>
> Can we do that on top?
Sure, but please then keep track of the task, with a \todo here.
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void CameraModule::requestComplete(Request *request,
> > > + const std::map<Stream *, Buffer *> &buffers)
> > > +{
> > > + Buffer *libcameraBuffer = buffers.begin()->second;
> > > + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> > > + camera_metadata_t *resultMetadata = nullptr;
> > > +
> > > + if (request->status() != Request::RequestComplete) {
> > > + LOG(HAL, Error) << "Request not succesfully completed: "
> > > + << request->status();
> > > + status = CAMERA3_BUFFER_STATUS_ERROR;
> > > + }
> > > +
> > > + /* Prepare to call back the Android camera stack. */
> > > + Camera3RequestDescriptor &descriptor = requestMap_[request];
> > > +
> > > + camera3_capture_result_t captureResult = {};
> > > + captureResult.frame_number = descriptor.frameNumber;
> > > + captureResult.num_output_buffers = descriptor.numBuffers;
> > > +
> > > + camera3_stream_buffer_t *resultBuffers =
> > > + new camera3_stream_buffer_t[descriptor.numBuffers];
> > > + for (unsigned int i = 0; i < descriptor.numBuffers; ++i) {
> > > + camera3_stream_buffer_t resultBuffer;
> > > +
> > > + resultBuffer = descriptor.buffers[i];
> > > + resultBuffer.acquire_fence = -1;
> > > + resultBuffer.release_fence = -1;
> > > + resultBuffer.status = status;
> > > +
> > > + resultBuffers[i] = resultBuffer;
> > > + }
> > > + captureResult.output_buffers =
> > > + const_cast<const camera3_stream_buffer_t *>(resultBuffers);
> > > +
> > > + if (status == CAMERA3_BUFFER_STATUS_ERROR) {
> > > + /* \todo Improve error handling. */
> > > + notifyError(descriptor.frameNumber, resultBuffers->stream);
> > > + } else {
> > > + notifyShutter(descriptor.frameNumber, libcameraBuffer->timestamp());
> > > +
> > > + captureResult.partial_result = 1;
> > > + resultMetadata = getResultMetadata(descriptor.frameNumber,
> > > + libcameraBuffer->timestamp());
> > > + captureResult.result = resultMetadata;
> > > + }
> > > +
> > > + callbacks_->process_capture_result(callbacks_, &captureResult);
> > > +
> > > + if (resultMetadata)
> > > + free_camera_metadata(resultMetadata);
> > > +
> > > + delete[] resultBuffers;
> > > + delete[] descriptor.buffers;
> > > + requestMap_.erase(request);
> > > +
> > > + return;
> > > +}
> > > +
> > > +void CameraModule::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> > > +{
> > > + camera3_notify_msg_t notify = {};
> > > +
> > > + notify.type = CAMERA3_MSG_SHUTTER;
> > > + notify.message.shutter.frame_number = frameNumber;
> > > + notify.message.shutter.timestamp = timestamp;
> > > +
> > > + callbacks_->notify(callbacks_, ¬ify);
> > > +}
> > > +
> > > +void CameraModule::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> > > +{
> > > + camera3_notify_msg_t notify = {};
> > > +
> > > + notify.type = CAMERA3_MSG_ERROR;
> > > + notify.message.error.error_stream = stream;
> > > + notify.message.error.frame_number = frameNumber;
> > > + notify.message.error.error_code = CAMERA3_MSG_ERROR_REQUEST;
> > > +
> > > + callbacks_->notify(callbacks_, ¬ify);
> > > +}
> > > +
> > > +/*
> > > + * Fixed result metadata, mostly imported from the UVC camera HAL, which
> > > + * does not produces metadata and thus needs to generate a fixed set.
> > > + */
> > > +camera_metadata_t *CameraModule::getResultMetadata(int frame_number,
> > > + int64_t timestamp)
> > > +{
> > > + int ret;
> > > +
> > > + /* FIXME: random "big enough" values. */
> > > + #define RESULT_ENTRY_CAP 256
> > > + #define RESULT_DATA_CAP 6688
> > > + camera_metadata_t *resultMetadata =
> > > + allocate_camera_metadata(STATIC_ENTRY_CAP, STATIC_DATA_CAP);
> > > +
> > > + const uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;
> > > + ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,
> > > + &ae_state, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
> > > + ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,
> > > + &ae_lock, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + uint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;
> > > + ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,
> > > + &af_state, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + const uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_CONTROL_AWB_STATE,
> > > + &awb_state, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + const uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_CONTROL_AWB_LOCK,
> > > + &awb_lock, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + const uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_LENS_STATE,
> > > + &lens_state, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + int32_t sensorSizes[] = {
> > > + 0, 0, 2560, 1920,
> > > + };
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_SCALER_CROP_REGION,
> > > + sensorSizes, 4);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_SENSOR_TIMESTAMP,
> > > + ×tamp, 1);
> > > +
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /* 33.3 msec */
> > > + const int64_t rolling_shutter_skew = 33300000;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
> > > + &rolling_shutter_skew, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + /* 16.6 msec */
> > > + const int64_t exposure_time = 16600000;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_SENSOR_EXPOSURE_TIME,
> > > + &exposure_time, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + const uint8_t lens_shading_map_mode =
> > > + ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,
> > > + &lens_shading_map_mode, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + const uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;
> > > + ret = add_camera_metadata_entry(resultMetadata,
> > > + ANDROID_STATISTICS_SCENE_FLICKER,
> > > + &scene_flicker, 1);
> > > + METADATA_ASSERT(ret);
> > > +
> > > + return resultMetadata;
> > > +}
> > > diff --git a/src/android/camera_module.h b/src/android/camera_module.h
> > > new file mode 100644
> > > index 000000000000..67488d5940b1
> > > --- /dev/null
> > > +++ b/src/android/camera_module.h
> > > @@ -0,0 +1,75 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_module.h - Libcamera Android Camera Module
> >
> > s/Libcamera/libcamera/
> >
> > (here and everywhere else, libcamera is always written in all lowercase)
> >
> > > + */
> > > +#ifndef __ANDROID_CAMERA_MODULE_H__
> > > +#define __ANDROID_CAMERA_MODULE_H__
> > > +
> > > +#include <memory>
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <libcamera/libcamera.h>
> > > +
> > > +#include "message.h"
> > > +
> > > +#include "camera_hal_manager.h"
> > > +
> > > +#define METADATA_ASSERT(_r) \
> > > + do { \
> > > + if (!(_r)) break; \
> > > + LOG(HAL, Error) << "Error: " << __func__ << ":" << __LINE__; \
> > > + return nullptr; \
> > > + } while(0);
> >
> > I really, really dislike hiding return statements in macros. We can keep
> > it for now, but please add a comment telling that it should be removed.
>
> All the metadata handling will be nuked in the next versions, so I
> won't bother for now.
>
> > > +
> >
> > Even if we don't document the whole implementation with Doxygen, I think
> > a few short comments that explain what each class models would be
> > useful. Otherwise the reader is left to wonder what CameraModule or
> > CameraProxy stand for. A comment that explains how the various objects
> > work together would be useful too.
> >
> > > +class CameraModule : public libcamera::Object
> >
> > I don't think CameraModule is a good name :-/ In HAL terminology, the
> > module refers to camera_module_t, and corresponds more or less to
> > libcamera::CameraManager, not libcamera::Camera.
>
> Correct
>
> > One option would be to name this class Camera in a HAL namespace. That
> > would make it quite clear that it corresponds to a libcamra::Camera.
>
> I had a go at isolating namespaces and I have problems with the
> logging infrastructure,
Ah yes I rememeber that.
> and I would not duplicate the Camera name to be honest. CameraDevice ?
As it corresponds to camera3_device_t, CameraDevice makes sense.
This just struck my mind now, should CameraHalManager be named
CameraModule to match camera_module_t ? I'm fine either way.
> > > +{
> > > +public:
> > > + CameraModule(unsigned int id, libcamera::Camera *camera);
> > > + ~CameraModule();
> > > +
> > > + void message(libcamera::Message *message);
> > > +
> > > + int open();
> > > + void close();
> > > + void setCallbacks(const struct camera3_device *cameraDevice,
> > > + const camera3_callback_ops_t *callbacks);
> > > + camera_metadata_t *getStaticMetadata();
> > > + const camera_metadata_t *constructDefaultRequestMetadata(int type);
> > > + int configureStreams(camera3_stream_configuration_t *stream_list);
> > > + int processCaptureRequest(camera3_capture_request_t *request);
> > > + void requestComplete(libcamera::Request *request,
> > > + const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
> > > +
> > > + unsigned int id() const { return id_; }
> >
> > I think this method is not used.
> >
> > > +
> > > +private:
> > > + struct Camera3RequestDescriptor {
> > > + uint32_t frameNumber;
> > > + uint32_t numBuffers;
> > > + camera3_stream_buffer_t *buffers;
> > > + };
> > > +
> > > + enum CameraState {
> > > + CameraStopped,
> > > + CameraRunning,
> > > + };
> >
> > With just two states you could instead have a bool running_ field.
> >
> > > +
> > > + void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > > + void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > > + camera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);
> > > +
> > > + unsigned int id_;
> > > + libcamera::Camera *camera_;
> > > + std::unique_ptr<libcamera::CameraConfiguration> config_;
> > > + CameraState cameraState_;
> > > +
> > > + camera_metadata_t *requestTemplate_;
> > > + const camera3_callback_ops_t *callbacks_;
> > > + const struct camera3_device *camera3Device_;
> > > +
> > > + std::map<libcamera::Request *, Camera3RequestDescriptor> requestMap_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_MODULE_H__ */
> > > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp
> > > new file mode 100644
> > > index 000000000000..af7817f29137
> > > --- /dev/null
> > > +++ b/src/android/camera_proxy.cpp
> > > @@ -0,0 +1,181 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_proxy.cpp - Proxy to camera module instances
> > > + */
> > > +
> > > +#include "camera_proxy.h"
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <libcamera/libcamera.h>
> > > +#include <system/camera_metadata.h>
> > > +
> > > +#include <memory>
> > > +
> > > +#include "log.h"
> > > +#include "message.h"
> > > +#include "thread_rpc.h"
> > > +#include "utils.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(HAL);
> > > +
> > > +#define LIBCAMERA_HAL_FUNC_DBG \
> > > + LOG(HAL, Debug);
> >
> > If we need to trace calls, I think something better than debug messages
> > would be useful. I'm not sure what yet though.
>
> Yeah, I'm not even sure we want to trace calls actually. The fact is
> that the camera HAL is somewhat a weird beast, as it is a very system
> specific libcamera extension, and I tried to mimic what is usually
> found in other HAL implementations I've seen. Should we drop tracing
> completely and make this more libcamera-like ?
I think so. I also think we'll need tracing in the future, but that's
for later.
> > > +
> > > +static int hal_dev_initialize(const struct camera3_device *dev,
> > > + const camera3_callback_ops_t *callback_ops)
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + if (!dev)
> > > + return -EINVAL;
> >
> > Can this happen ?
>
> I think cros_camera_test exercises this.
> >
> > > +
> > > + CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > + proxy->setCallbacks(callback_ops);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int hal_dev_configure_streams(const struct camera3_device *dev,
> > > + camera3_stream_configuration_t *stream_list)
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + if (!dev)
> > > + return -EINVAL;
> > > +
> > > + CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > + proxy->configureStreams(stream_list);
> > > +
> > > + return 0;
> >
> > Shouldn't you propagate the error value from proxy->configureStreams() ?
> >
> > > +}
> > > +
> > > +static const camera_metadata_t *
> > > +hal_dev_construct_default_request_settings(const struct camera3_device *dev,
> > > + int type)
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + if (!dev)
> > > + return nullptr;
> > > +
> > > + CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > + return proxy->constructDefaultRequest(type);
> >
> > How about renaming constructDefaultRequest() and
> > constructDefaultRequestMetadata() to constructDefaultRequestSettings(),
> > in order to match the HAL operation name ?
>
> Ok...
>
> > > +}
> > > +
> > > +static int hal_dev_process_capture_request(const struct camera3_device *dev,
> > > + camera3_capture_request_t *request)
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + if (!dev)
> > > + return -EINVAL;
> > > +
> > > + CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > > + return proxy->processCaptureRequest(request);
> > > +}
> > > +
> > > +static void hal_dev_dump(const struct camera3_device *dev, int fd)
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +}
> > > +
> > > +static int hal_dev_flush(const struct camera3_device *dev)
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + return 0;
> > > +}
> >
> > You could define these methods as static methods of the CameraProxy
> > class. For instance, hal_dev_configure_streams would become
> >
> > int CameraProxy::configureStreams(const struct camera3_device *dev,
> > camera3_stream_configuration_t *stream_list)
> > {
> > CameraProxy *proxy = reinterpret_cast<CameraProxy *>(dev->priv);
> > proxy->configureStreams(stream_list);
> > }
>
> To avoid the static C methods you mean ? Not sure what is the gain
> though.
Correct. The gain is that all related code would be grouped in the same
class. It's not a big deal, and could be done on top of this series. I
just think it would be a bit cleaner (but I could be wrong).
> > > +
> > > +static camera3_device_ops hal_dev_ops = {
> >
> > I wish this could be const :-(
> >
> > > + .initialize = hal_dev_initialize,
> > > + .configure_streams = hal_dev_configure_streams,
> > > + .register_stream_buffers = nullptr,
> > > + .construct_default_request_settings = hal_dev_construct_default_request_settings,
> > > + .process_capture_request = hal_dev_process_capture_request,
> > > + .get_metadata_vendor_tag_ops = nullptr,
> > > + .dump = hal_dev_dump,
> > > + .flush = hal_dev_flush,
> > > + .reserved = { 0 },
> >
> > s/0/nullptr/ ?
> >
> > > +};
> > > +
> > > +CameraProxy::CameraProxy(unsigned int id, CameraModule *cameraModule)
> > > + : open_(false), id_(id), cameraModule_(cameraModule)
> > > +{
> > > +}
> > > +
> > > +int CameraProxy::open(const hw_module_t *hardwareModule)
> > > +{
> > > + int ret = cameraModule_->open();
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Initialize the hw_device_t in the instance camera3_module_t. */
> > > + cameraDevice_.common.tag = HARDWARE_DEVICE_TAG;
> > > + cameraDevice_.common.version = CAMERA_DEVICE_API_VERSION_3_3;
> > > + cameraDevice_.common.module = (hw_module_t *)hardwareModule;
> > > +
> > > + /*
> > > + * The camera device operations. These actually implement
> > > + * the Android Camera HALv3 interface.
> > > + */
> > > + cameraDevice_.ops = &hal_dev_ops;
> > > + cameraDevice_.priv = this;
> > > +
> > > + open_ = true;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void CameraProxy::close()
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + ThreadRpc rpcRequest;
> > > + rpcRequest.tag = ThreadRpc::Close;
> > > +
> > > + threadRpcCall(rpcRequest);
> > > +
> > > + open_ = false;
> > > +}
> > > +
> > > +void CameraProxy::setCallbacks(const camera3_callback_ops_t *callbacks)
> >
> > I would name this initialize() to match hal_dev_initialize().
> >
> > > +{
> > > + LIBCAMERA_HAL_FUNC_DBG
> > > +
> > > + cameraModule_->setCallbacks(&cameraDevice_, callbacks);
> > > +}
> > > +
> > > +const camera_metadata_t *CameraProxy::constructDefaultRequest(int type)
> > > +{
> > > + return cameraModule_->constructDefaultRequestMetadata(type);
> > > +}
> > > +
> > > +int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)
> > > +{
> > > + return cameraModule_->configureStreams(stream_list);
> > > +}
> > > +
> > > +int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
> > > +{
> > > + ThreadRpc rpcRequest;
> > > + rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> > > + rpcRequest.request = request;
> > > +
> > > + threadRpcCall(rpcRequest);
> > > +
> > > + return 0;
> >
> > Does this method need to be synchronous ? We can keep it as-is for now,
> > but I wonder if it wouldn't make sense to later move (part of) the
> > validation code here and make the call asynchronous.
> >
> > > +}
> > > +
> > > +void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)
> > > +{
> > > + std::unique_ptr<ThreadRpcMessage> message =
> > > + utils::make_unique<ThreadRpcMessage>();
> > > + message->rpc = &rpcRequest;
> > > +
> > > + cameraModule_->postMessage(std::move(message));
> > > + rpcRequest.waitDelivery();
> > > +}
> > > diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h
> > > new file mode 100644
> > > index 000000000000..69e6878c4352
> > > --- /dev/null
> > > +++ b/src/android/camera_proxy.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * camera_proxy.h - Proxy to camera module instances
> > > + */
> > > +#ifndef __ANDROID_CAMERA_PROXY_H__
> > > +#define __ANDROID_CAMERA_PROXY_H__
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <libcamera/libcamera.h>
> > > +
> > > +#include "camera_module.h"
> > > +
> > > +class CameraProxy
> > > +{
> > > +public:
> > > + CameraProxy(unsigned int id, CameraModule *cameraModule);
> > > +
> > > + int open(const hw_module_t *hardwareModule);
> > > + void close();
> > > +
> > > + void setCallbacks(const camera3_callback_ops_t *callbacks);
> > > + const camera_metadata_t *constructDefaultRequest(int type);
> > > + int configureStreams(camera3_stream_configuration_t *stream_list);
> > > + int processCaptureRequest(camera3_capture_request_t *request);
> > > +
> > > + bool isOpen() const { return open_; }
> >
> > This isn't used.
> >
> > > + unsigned int id() const { return id_; }
> > > + camera3_device_t *device() { return &cameraDevice_; }
> > > +
> > > +private:
> > > + void threadRpcCall(ThreadRpc &rpcRequest);
> > > +
> > > + bool open_;
> >
> > And neither is this, if you drop the isOpen() method.
> >
> > > + unsigned int id_;
> > > + CameraModule *cameraModule_;
> > > + camera3_device_t cameraDevice_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_PROXY_H__ */
> > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > index 1f242953db37..63b45832c0d2 100644
> > > --- a/src/android/meson.build
> > > +++ b/src/android/meson.build
> > > @@ -1,3 +1,11 @@
> > > +android_hal_sources = files([
> > > + 'camera3_hal.cpp',
> > > + 'camera_hal_manager.cpp',
> > > + 'camera_module.cpp',
> > > + 'camera_proxy.cpp',
> > > + 'thread_rpc.cpp'
> > > +])
> > > +
> > > android_camera_metadata_sources = files([
> > > 'metadata/camera_metadata.c',
> > > ])
> > > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp
> > > new file mode 100644
> > > index 000000000000..f13db59d0d75
> > > --- /dev/null
> > > +++ b/src/android/thread_rpc.cpp
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * thread_call.cpp - Inter-thread procedure call
> >
> > thread_rpc.cpp
> >
> > > + */
> > > +
> > > +#include "message.h"
> > > +#include "thread_rpc.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None;
> > > +
> > > +ThreadRpcMessage::ThreadRpcMessage()
> > > + : Message(type())
> > > +{
> > > +}
> > > +
> > > +void ThreadRpc::notifyReception()
> > > +{
> > > + {
> > > + libcamera::MutexLocker locker(mutex_);
> > > + delivered_ = true;
> > > + }
> > > + cv_.notify_one();
> > > +}
> > > +
> > > +void ThreadRpc::waitDelivery()
> > > +{
> > > + libcamera::MutexLocker locker(mutex_);
> > > + cv_.wait(locker, [&]{ return delivered_; });
> > > +}
> > > +
> > > +Message::Type ThreadRpcMessage::type()
> > > +{
> > > + if (ThreadRpcMessage::rpcType_ == Message::Type::None)
> > > + rpcType_ = Message::registerMessageType();
> > > +
> > > + return rpcType_;
> > > +}
> > > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h
> > > new file mode 100644
> > > index 000000000000..173caba1a515
> > > --- /dev/null
> > > +++ b/src/android/thread_rpc.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * thread_call.h - Inter-thread procedure call
> > > + */
> > > +#ifndef __ANDROID_THREAD_CALL_H__
> > > +#define __ANDROID_THREAD_CALL_H__
> >
> > s/CALL/RPC/
> >
> > > +
> > > +#include <condition_variable>
> > > +#include <string>
> >
> > Do you need string ?
> >
> > You should #include <mutex>
> >
> > > +
> > > +#include <hardware/camera3.h>
> > > +#include <libcamera/libcamera.h>
> > > +
> > > +#include "message.h"
> > > +#include "thread.h"
> > > +
> > > +class ThreadRpc
> > > +{
> > > +public:
> > > + enum RpcTag {
> > > + ProcessCaptureRequest,
> > > + Close,
> > > + };
> > > +
> > > + ThreadRpc()
> > > + : delivered_(false) {}
> > > +
> > > + void notifyReception();
> > > + void waitDelivery();
> > > +
> > > + RpcTag tag;
> > > +
> > > + camera3_capture_request_t *request;
> > > +
> > > +private:
> > > + bool delivered_;
> > > + std::mutex mutex_;
> > > + std::condition_variable cv_;
> > > +};
> > > +
> > > +class ThreadRpcMessage : public libcamera::Message
> > > +{
> > > +public:
> > > + ThreadRpcMessage();
> > > + ThreadRpc *rpc;
> > > +
> > > + static Message::Type type();
> > > +
> > > +private:
> > > + static libcamera::Message::Type rpcType_;
> > > +};
> >
> > FYI, I'll probably move part of this to the libcamera core.
>
> Let's merge the HAL first, if you're not working on this already
> please.
I'm not, don't worry :-) It will be a change on top.
> > > +
> > > +#endif /* __ANDROID_THREAD_CALL_H__ */
> > > +
> >
> > [snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list