| Message ID | 20210406154557.27303-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for the patch. On Wed, Apr 7, 2021 at 12:45 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Open the HAL configuration file in the Camera HAL manager and get > the camera properties for each created CameraDevice and initialize it > with them. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 29 ++++++++++++++++++++++------- > src/android/camera_device.h | 3 ++- > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++-- > src/android/camera_hal_manager.h | 3 +++ > 4 files changed, 46 insertions(+), 10 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 89044efa7ebe..66030c012db0 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -6,6 +6,7 @@ > */ > > #include "camera_device.h" > +#include "camera_hal_config.h" > #include "camera_ops.h" > #include "post_processor.h" > > @@ -446,9 +447,15 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, > * Initialize the camera static information. > * This method is called before the camera device is opened. > */ > -int CameraDevice::initialize() > +int CameraDevice::initialize(const CameraProps &cameraProps) > { > - /* Initialize orientation and facing side of the camera. */ > + /* > + * Initialize orientation and facing side of the camera. > + * > + * If the libcamera::Camera provides those information as retrieved > + * from firmware use that, otherwise fallback to values parsed from > + * the configuration file. > + */ > const ControlList &properties = camera_->properties(); > > if (properties.contains(properties::Location)) { > @@ -464,12 +471,14 @@ int CameraDevice::initialize() > facing_ = CAMERA_FACING_EXTERNAL; > break; > } > + > + if (facing_ != cameraProps.facing) > + LOG(HAL, Warning) > + << "Camera location does not match" > + << " configuration file. Use " << facing_; > + I would enclose by "{}" if a if-clause is more than one line. > } else { > - /* > - * \todo Retrieve the camera location from configuration file > - * if not available from the library. > - */ > - facing_ = CAMERA_FACING_FRONT; > + facing_ = cameraProps.facing; > } > > /* > @@ -483,6 +492,12 @@ int CameraDevice::initialize() > if (properties.contains(properties::Rotation)) { > int rotation = properties.get(properties::Rotation); > orientation_ = (360 - rotation) % 360; > + if (orientation_ != cameraProps.rotation) > + LOG(HAL, Warning) > + << "Camera orientation does not match" > + << " configuration file. Use " << orientation_; same here. > + } else { > + orientation_ = cameraProps.rotation; > } > > int ret = camera_->acquire(); > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 11bdfec8d587..ba3ec8770e11 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -29,6 +29,7 @@ > #include "camera_worker.h" > #include "jpeg/encoder.h" > > +class CameraProps; > class CameraDevice : protected libcamera::Loggable > { > public: > @@ -36,7 +37,7 @@ public: > std::shared_ptr<libcamera::Camera> cam); > ~CameraDevice(); > > - int initialize(); > + int initialize(const CameraProps &cameraProps); > > int open(const hw_module_t *hardwareModule); > void close(); > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index bf3fcda75237..a517727ea0b8 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default; > > int CameraHalManager::init() > { > + int ret = halConfig_.open(); > + if (ret) > + return ret; > + > cameraManager_ = std::make_unique<CameraManager>(); > > /* Support camera hotplug. */ > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > > - int ret = cameraManager_->start(); > + ret = cameraManager_->start(); > if (ret) { > LOG(HAL, Error) << "Failed to start camera manager: " > << strerror(-ret); > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > } > } > > + /* > + * Get camera properties from the configuration file. > + * > + * The camera properties as recorded in the configuration file > + * supplement information that cannot be retrieved from the > + * libcamera::Camera at run time. > + */ > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id()); > + if (!cameraProps.valid) { > + LOG(HAL, Error) << "Failed to register camera: " << cam->id(); > + return; > + } > + > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); > - int ret = camera->initialize(); > + int ret = camera->initialize(cameraProps); > if (ret) { > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > return; > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > index d9bf27989965..af1581da6579 100644 > --- a/src/android/camera_hal_manager.h > +++ b/src/android/camera_hal_manager.h > @@ -19,6 +19,8 @@ > > #include <libcamera/camera_manager.h> > > +#include "camera_hal_config.h" > + > class CameraDevice; > > class CameraHalManager > @@ -50,6 +52,7 @@ private: > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > + CameraHalConfig halConfig_; > > const camera_module_callbacks_t *callbacks_; > std::vector<std::unique_ptr<CameraDevice>> cameras_; Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Tue, Apr 06, 2021 at 05:45:56PM +0200, Jacopo Mondi wrote: > Open the HAL configuration file in the Camera HAL manager and get > the camera properties for each created CameraDevice and initialize it > with them. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 29 ++++++++++++++++++++++------- > src/android/camera_device.h | 3 ++- > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++-- > src/android/camera_hal_manager.h | 3 +++ > 4 files changed, 46 insertions(+), 10 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 89044efa7ebe..66030c012db0 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -6,6 +6,7 @@ > */ > > #include "camera_device.h" > +#include "camera_hal_config.h" > #include "camera_ops.h" > #include "post_processor.h" > > @@ -446,9 +447,15 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, > * Initialize the camera static information. > * This method is called before the camera device is opened. > */ > -int CameraDevice::initialize() > +int CameraDevice::initialize(const CameraProps &cameraProps) > { > - /* Initialize orientation and facing side of the camera. */ > + /* > + * Initialize orientation and facing side of the camera. > + * > + * If the libcamera::Camera provides those information as retrieved > + * from firmware use that, otherwise fallback to values parsed from > + * the configuration file. > + */ > const ControlList &properties = camera_->properties(); > > if (properties.contains(properties::Location)) { > @@ -464,12 +471,14 @@ int CameraDevice::initialize() > facing_ = CAMERA_FACING_EXTERNAL; > break; > } > + > + if (facing_ != cameraProps.facing) > + LOG(HAL, Warning) > + << "Camera location does not match" > + << " configuration file. Use " << facing_; s/Use/Using/ > + > } else { > - /* > - * \todo Retrieve the camera location from configuration file > - * if not available from the library. > - */ > - facing_ = CAMERA_FACING_FRONT; > + facing_ = cameraProps.facing; > } > > /* > @@ -483,6 +492,12 @@ int CameraDevice::initialize() > if (properties.contains(properties::Rotation)) { > int rotation = properties.get(properties::Rotation); > orientation_ = (360 - rotation) % 360; > + if (orientation_ != cameraProps.rotation) > + LOG(HAL, Warning) > + << "Camera orientation does not match" > + << " configuration file. Use " << orientation_; Same here. > + } else { > + orientation_ = cameraProps.rotation; > } > > int ret = camera_->acquire(); > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 11bdfec8d587..ba3ec8770e11 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -29,6 +29,7 @@ > #include "camera_worker.h" > #include "jpeg/encoder.h" > > +class CameraProps; > class CameraDevice : protected libcamera::Loggable > { > public: > @@ -36,7 +37,7 @@ public: > std::shared_ptr<libcamera::Camera> cam); > ~CameraDevice(); > > - int initialize(); > + int initialize(const CameraProps &cameraProps); > > int open(const hw_module_t *hardwareModule); > void close(); > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index bf3fcda75237..a517727ea0b8 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default; > > int CameraHalManager::init() > { > + int ret = halConfig_.open(); > + if (ret) > + return ret; > + > cameraManager_ = std::make_unique<CameraManager>(); > > /* Support camera hotplug. */ > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > > - int ret = cameraManager_->start(); > + ret = cameraManager_->start(); > if (ret) { > LOG(HAL, Error) << "Failed to start camera manager: " > << strerror(-ret); > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > } > } > > + /* > + * Get camera properties from the configuration file. > + * > + * The camera properties as recorded in the configuration file > + * supplement information that cannot be retrieved from the > + * libcamera::Camera at run time. > + */ > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id()); > + if (!cameraProps.valid) { > + LOG(HAL, Error) << "Failed to register camera: " << cam->id(); > + return; > + } Could you please reply to the comment in v4 regarding UVC cameras ? > + > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); > - int ret = camera->initialize(); > + int ret = camera->initialize(cameraProps); > if (ret) { > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > return; > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > index d9bf27989965..af1581da6579 100644 > --- a/src/android/camera_hal_manager.h > +++ b/src/android/camera_hal_manager.h > @@ -19,6 +19,8 @@ > > #include <libcamera/camera_manager.h> > > +#include "camera_hal_config.h" > + > class CameraDevice; > > class CameraHalManager > @@ -50,6 +52,7 @@ private: > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > + CameraHalConfig halConfig_; > > const camera_module_callbacks_t *callbacks_; > std::vector<std::unique_ptr<CameraDevice>> cameras_;
Hi Laurent, On Tue, Apr 13, 2021 at 07:22:56AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > > > > + /* > > + * Get camera properties from the configuration file. > > + * > > + * The camera properties as recorded in the configuration file > > + * supplement information that cannot be retrieved from the > > + * libcamera::Camera at run time. > > + */ > > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id()); > > + if (!cameraProps.valid) { > > + LOG(HAL, Error) << "Failed to register camera: " << cam->id(); > > + return; > > + } > > Could you please reply to the comment in v4 regarding UVC cameras ? > Sorry, I completely missed that part, I'll re-paste comment here > What if we have a fully populated libcamera::Camera (with properties > retrieved from the firmware) ? Should we still require a configuration > file then ? The configuration file will likely be mandatory in the > future to provide additional information that the firmware doesn't > provide (such as lens-related information for instance), so I suppose > it's fine to already make it mandatory. > I think so > We should however not make the configuration mandatory for external > cameras, this would break UVC support. I see. It won't be possible to provide a configuration file for pluggable cameras, although there might be vendors that embedds a UVC camera and want a configuration file. I'll see how this can be handled... we'll need to handle the case where !cameraProps.valid and default properties to something sensible for an external camera. I wonder if we ever get to the point were we could specialize CameraDevice for the external use case, but that's for later... > > Related to breakages, we need to also ensure that the configuration file > will get deployed correctly in Chrome OS builds of libcamera, or the > existing tests will start failing. I'm not sure how that's best handled, > Hiro, maybe this is something that you could help us with ? > > + > > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); > > - int ret = camera->initialize(); > > + int ret = camera->initialize(cameraProps); > > if (ret) { > > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > > return; > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > > index d9bf27989965..af1581da6579 100644 > > --- a/src/android/camera_hal_manager.h > > +++ b/src/android/camera_hal_manager.h > > @@ -19,6 +19,8 @@ > > > > #include <libcamera/camera_manager.h> > > > > +#include "camera_hal_config.h" > > + > > class CameraDevice; > > > > class CameraHalManager > > @@ -50,6 +52,7 @@ private: > > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > > + CameraHalConfig halConfig_; > > > > const camera_module_callbacks_t *callbacks_; > > std::vector<std::unique_ptr<CameraDevice>> cameras_; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 89044efa7ebe..66030c012db0 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -6,6 +6,7 @@ */ #include "camera_device.h" +#include "camera_hal_config.h" #include "camera_ops.h" #include "post_processor.h" @@ -446,9 +447,15 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, * Initialize the camera static information. * This method is called before the camera device is opened. */ -int CameraDevice::initialize() +int CameraDevice::initialize(const CameraProps &cameraProps) { - /* Initialize orientation and facing side of the camera. */ + /* + * Initialize orientation and facing side of the camera. + * + * If the libcamera::Camera provides those information as retrieved + * from firmware use that, otherwise fallback to values parsed from + * the configuration file. + */ const ControlList &properties = camera_->properties(); if (properties.contains(properties::Location)) { @@ -464,12 +471,14 @@ int CameraDevice::initialize() facing_ = CAMERA_FACING_EXTERNAL; break; } + + if (facing_ != cameraProps.facing) + LOG(HAL, Warning) + << "Camera location does not match" + << " configuration file. Use " << facing_; + } else { - /* - * \todo Retrieve the camera location from configuration file - * if not available from the library. - */ - facing_ = CAMERA_FACING_FRONT; + facing_ = cameraProps.facing; } /* @@ -483,6 +492,12 @@ int CameraDevice::initialize() if (properties.contains(properties::Rotation)) { int rotation = properties.get(properties::Rotation); orientation_ = (360 - rotation) % 360; + if (orientation_ != cameraProps.rotation) + LOG(HAL, Warning) + << "Camera orientation does not match" + << " configuration file. Use " << orientation_; + } else { + orientation_ = cameraProps.rotation; } int ret = camera_->acquire(); diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 11bdfec8d587..ba3ec8770e11 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -29,6 +29,7 @@ #include "camera_worker.h" #include "jpeg/encoder.h" +class CameraProps; class CameraDevice : protected libcamera::Loggable { public: @@ -36,7 +37,7 @@ public: std::shared_ptr<libcamera::Camera> cam); ~CameraDevice(); - int initialize(); + int initialize(const CameraProps &cameraProps); int open(const hw_module_t *hardwareModule); void close(); diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index bf3fcda75237..a517727ea0b8 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default; int CameraHalManager::init() { + int ret = halConfig_.open(); + if (ret) + return ret; + cameraManager_ = std::make_unique<CameraManager>(); /* Support camera hotplug. */ cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); - int ret = cameraManager_->start(); + ret = cameraManager_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera manager: " << strerror(-ret); @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) } } + /* + * Get camera properties from the configuration file. + * + * The camera properties as recorded in the configuration file + * supplement information that cannot be retrieved from the + * libcamera::Camera at run time. + */ + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id()); + if (!cameraProps.valid) { + LOG(HAL, Error) << "Failed to register camera: " << cam->id(); + return; + } + /* Create a CameraDevice instance to wrap the libcamera Camera. */ std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); - int ret = camera->initialize(); + int ret = camera->initialize(cameraProps); if (ret) { LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); return; diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h index d9bf27989965..af1581da6579 100644 --- a/src/android/camera_hal_manager.h +++ b/src/android/camera_hal_manager.h @@ -19,6 +19,8 @@ #include <libcamera/camera_manager.h> +#include "camera_hal_config.h" + class CameraDevice; class CameraHalManager @@ -50,6 +52,7 @@ private: CameraDevice *cameraDeviceFromHalId(unsigned int id); std::unique_ptr<libcamera::CameraManager> cameraManager_; + CameraHalConfig halConfig_; const camera_module_callbacks_t *callbacks_; std::vector<std::unique_ptr<CameraDevice>> cameras_;
Open the HAL configuration file in the Camera HAL manager and get the camera properties for each created CameraDevice and initialize it with them. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 29 ++++++++++++++++++++++------- src/android/camera_device.h | 3 ++- src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++-- src/android/camera_hal_manager.h | 3 +++ 4 files changed, 46 insertions(+), 10 deletions(-)