| Message ID | 20210413145042.48185-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Apr 13, 2021 at 04:50:41PM +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 | 56 +++++++++++++++++++++++++----- > src/android/camera_device.h | 3 +- > src/android/camera_hal_manager.cpp | 37 +++++++++++++++++++- > src/android/camera_hal_manager.h | 3 ++ > 4 files changed, 89 insertions(+), 10 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 89044efa7ebe..2e93936fdb4b 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" > > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, > } > > /* > - * Initialize the camera static information. > + * Initialize the camera static information retrieved from the > + * Camera::properties or from the cameraPros. > + * > + * cameraProps is optional for external camera devices and is defaulted to > + * nullptr. > + * > * 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 them, otherwise fallback to values parsed from > + * the configuration file. If the configuration file is not available > + * the camera is external so its location and rotation can be safely > + * defaulted. > + */ > const ControlList &properties = camera_->properties(); > > if (properties.contains(properties::Location)) { > @@ -464,12 +478,22 @@ int CameraDevice::initialize() > facing_ = CAMERA_FACING_EXTERNAL; > break; > } > + > + if (cameraProps && cameraProps->facing != -1 && > + facing_ != cameraProps->facing) { > + LOG(HAL, Warning) > + << "Camera location does not match" > + << " configuration file. Using " << facing_; > + } > + } else if (cameraProps) { > + if (cameraProps->facing == -1) { > + LOG(HAL, Error) > + << "Camera facing not in configuration file"; > + return -EINVAL; > + } > + facing_ = cameraProps->facing; > } else { > - /* > - * \todo Retrieve the camera location from configuration file > - * if not available from the library. > - */ > - facing_ = CAMERA_FACING_FRONT; > + facing_ = CAMERA_FACING_EXTERNAL; > } > > /* > @@ -483,8 +507,24 @@ int CameraDevice::initialize() > if (properties.contains(properties::Rotation)) { > int rotation = properties.get(properties::Rotation); > orientation_ = (360 - rotation) % 360; > + if (cameraProps && cameraProps->rotation != -1 && > + orientation_ != cameraProps->rotation) { > + LOG(HAL, Warning) > + << "Camera orientation does not match" > + << " configuration file. Using " << orientation_; > + } > + } else if (cameraProps) { > + if (cameraProps->rotation == -1) { > + LOG(HAL, Error) > + << "Camera rotation not in configuration file"; > + return -EINVAL; > + } > + orientation_ = cameraProps->rotation; > + } else { > + orientation_ = 0; > } > > + /* Acquire the camera and initialize available stream configurations. */ > int ret = camera_->acquire(); > if (ret) { > LOG(HAL, Error) << "Failed to temporarily acquire the camera"; > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 11bdfec8d587..598d89f1cff0 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 = nullptr); You can drop the = nullptr if you agree with the change below. > > 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..1defc3f9c5bd 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -41,6 +41,16 @@ int CameraHalManager::init() > { > cameraManager_ = std::make_unique<CameraManager>(); > > + /* > + * Open and parse configuration file. > + * > + * If the configuration file is not available the HAL only supports > + * external cameras. If it exists but it's not valid then error out. > + */ > + halConfig_.open(); > + if (halConfig_.exists() && !halConfig_.valid()) > + return -EINVAL; > + > /* Support camera hotplug. */ > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > @@ -100,6 +110,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > auto iter = cameraIdsMap_.find(cam->id()); > if (iter != cameraIdsMap_.end()) { > id = iter->second; > + if (id >= firstExternalCameraId_) > + isCameraExternal = true; > } else { > isCameraNew = true; > > @@ -117,7 +129,30 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); > - int ret = camera->initialize(); > + > + /* > + * The configuration file must be valid for internal cameras. > + * External cameras can be initialized without configuration file. > + */ > + int ret = -EINVAL; > + if (!halConfig_.exists()) { > + if (isCameraExternal) > + ret = camera->initialize(); > + } else { > + /* > + * Get camera properties from the configuration file which > + * exists and is valid. > + * > + * Internal cameras are required to have a corresponding entry > + * in the configuration file. External cameras are not required > + * to. > + */ > + const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); > + if (cameraProps->valid) > + ret = camera->initialize(cameraProps); > + else if (isCameraExternal) > + ret = camera->initialize(); > + } > if (ret) { > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > return; If the configuration file does not exist and the camera is internal, we'll only print "Failed to initialize camera", which will be confusing. How about the following ? /* * The configuration file must be valid, and contain a corresponding * entry for internal cameras. External cameras can be initialized * without configuration file. */ if (!isCameraExternal && !halConfig_.exists()) { LOG(HALConfig, Error) << "HAL configuration file is mandatory for internal cameras"; return -EINVAL; } const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); if (!isCameraExternal && !cameraProps) { LOG(HALConfig, Error) << "HAL configuration entry for camera " << cam->id() << " is missing"; return -EINVAL; } ret = camera->initialize(cameraProps); if (ret) { LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); return; } Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 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 Jacopo, On 13/04/2021 21:00, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Apr 13, 2021 at 04:50:41PM +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 | 56 +++++++++++++++++++++++++----- >> src/android/camera_device.h | 3 +- >> src/android/camera_hal_manager.cpp | 37 +++++++++++++++++++- >> src/android/camera_hal_manager.h | 3 ++ >> 4 files changed, 89 insertions(+), 10 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 89044efa7ebe..2e93936fdb4b 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" >> >> @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, >> } >> >> /* >> - * Initialize the camera static information. >> + * Initialize the camera static information retrieved from the >> + * Camera::properties or from the cameraPros. >> + * >> + * cameraProps is optional for external camera devices and is defaulted to >> + * nullptr. >> + * >> * 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 them, otherwise fallback to values parsed from >> + * the configuration file. If the configuration file is not available >> + * the camera is external so its location and rotation can be safely >> + * defaulted. >> + */ >> const ControlList &properties = camera_->properties(); >> >> if (properties.contains(properties::Location)) { >> @@ -464,12 +478,22 @@ int CameraDevice::initialize() >> facing_ = CAMERA_FACING_EXTERNAL; >> break; >> } >> + >> + if (cameraProps && cameraProps->facing != -1 && >> + facing_ != cameraProps->facing) { >> + LOG(HAL, Warning) >> + << "Camera location does not match" >> + << " configuration file. Using " << facing_; >> + } >> + } else if (cameraProps) { >> + if (cameraProps->facing == -1) { >> + LOG(HAL, Error) >> + << "Camera facing not in configuration file"; >> + return -EINVAL; >> + } >> + facing_ = cameraProps->facing; >> } else { >> - /* >> - * \todo Retrieve the camera location from configuration file >> - * if not available from the library. >> - */ >> - facing_ = CAMERA_FACING_FRONT; >> + facing_ = CAMERA_FACING_EXTERNAL; >> } >> >> /* >> @@ -483,8 +507,24 @@ int CameraDevice::initialize() >> if (properties.contains(properties::Rotation)) { >> int rotation = properties.get(properties::Rotation); >> orientation_ = (360 - rotation) % 360; >> + if (cameraProps && cameraProps->rotation != -1 && >> + orientation_ != cameraProps->rotation) { >> + LOG(HAL, Warning) >> + << "Camera orientation does not match" >> + << " configuration file. Using " << orientation_; >> + } >> + } else if (cameraProps) { >> + if (cameraProps->rotation == -1) { >> + LOG(HAL, Error) >> + << "Camera rotation not in configuration file"; >> + return -EINVAL; >> + } >> + orientation_ = cameraProps->rotation; >> + } else { >> + orientation_ = 0; >> } >> >> + /* Acquire the camera and initialize available stream configurations. */ >> int ret = camera_->acquire(); >> if (ret) { >> LOG(HAL, Error) << "Failed to temporarily acquire the camera"; >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >> index 11bdfec8d587..598d89f1cff0 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; This triggers a compiler warning on GCC In file included from ../src/android/camera_device.cpp:9: ../src/android/camera_hal_config.h:16:1: error: 'CameraProps' defined as a struct here but previously declared as a class; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Werror,-Wmismatched-tags] struct CameraProps { ^ ../src/android/camera_device.h:32:1: note: did you mean struct here? 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 = nullptr); > > You can drop the = nullptr if you agree with the change below. > >> >> 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..1defc3f9c5bd 100644 >> --- a/src/android/camera_hal_manager.cpp >> +++ b/src/android/camera_hal_manager.cpp >> @@ -41,6 +41,16 @@ int CameraHalManager::init() >> { >> cameraManager_ = std::make_unique<CameraManager>(); >> >> + /* >> + * Open and parse configuration file. >> + * >> + * If the configuration file is not available the HAL only supports >> + * external cameras. If it exists but it's not valid then error out. >> + */ >> + halConfig_.open(); >> + if (halConfig_.exists() && !halConfig_.valid()) >> + return -EINVAL; >> + >> /* Support camera hotplug. */ >> cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); >> cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); >> @@ -100,6 +110,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >> auto iter = cameraIdsMap_.find(cam->id()); >> if (iter != cameraIdsMap_.end()) { >> id = iter->second; >> + if (id >= firstExternalCameraId_) >> + isCameraExternal = true; >> } else { >> isCameraNew = true; >> >> @@ -117,7 +129,30 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >> >> /* Create a CameraDevice instance to wrap the libcamera Camera. */ >> std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); >> - int ret = camera->initialize(); >> + >> + /* >> + * The configuration file must be valid for internal cameras. >> + * External cameras can be initialized without configuration file. >> + */ >> + int ret = -EINVAL; >> + if (!halConfig_.exists()) { >> + if (isCameraExternal) >> + ret = camera->initialize(); >> + } else { >> + /* >> + * Get camera properties from the configuration file which >> + * exists and is valid. >> + * >> + * Internal cameras are required to have a corresponding entry >> + * in the configuration file. External cameras are not required >> + * to. >> + */ >> + const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); >> + if (cameraProps->valid) >> + ret = camera->initialize(cameraProps); >> + else if (isCameraExternal) >> + ret = camera->initialize(); >> + } >> if (ret) { >> LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); >> return; > > If the configuration file does not exist and the camera is internal, > we'll only print "Failed to initialize camera", which will be confusing. > How about the following ? > > /* > * The configuration file must be valid, and contain a corresponding > * entry for internal cameras. External cameras can be initialized > * without configuration file. > */ > if (!isCameraExternal && !halConfig_.exists()) { > LOG(HALConfig, Error) > << "HAL configuration file is mandatory for internal cameras"; > return -EINVAL; > } > > const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); > if (!isCameraExternal && !cameraProps) { > LOG(HALConfig, Error) > << "HAL configuration entry for camera " << cam->id() > << " is missing"; > return -EINVAL; > } > > ret = camera->initialize(cameraProps); > if (ret) { > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > return; > } > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >> 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 Kieran, On Wed, Apr 14, 2021 at 07:33:26PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 13/04/2021 21:00, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Tue, Apr 13, 2021 at 04:50:41PM +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 | 56 +++++++++++++++++++++++++----- > >> src/android/camera_device.h | 3 +- > >> src/android/camera_hal_manager.cpp | 37 +++++++++++++++++++- > >> src/android/camera_hal_manager.h | 3 ++ > >> 4 files changed, 89 insertions(+), 10 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 89044efa7ebe..2e93936fdb4b 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" > >> > >> @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, > >> } > >> > >> /* > >> - * Initialize the camera static information. > >> + * Initialize the camera static information retrieved from the > >> + * Camera::properties or from the cameraPros. > >> + * > >> + * cameraProps is optional for external camera devices and is defaulted to > >> + * nullptr. > >> + * > >> * 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 them, otherwise fallback to values parsed from > >> + * the configuration file. If the configuration file is not available > >> + * the camera is external so its location and rotation can be safely > >> + * defaulted. > >> + */ > >> const ControlList &properties = camera_->properties(); > >> > >> if (properties.contains(properties::Location)) { > >> @@ -464,12 +478,22 @@ int CameraDevice::initialize() > >> facing_ = CAMERA_FACING_EXTERNAL; > >> break; > >> } > >> + > >> + if (cameraProps && cameraProps->facing != -1 && > >> + facing_ != cameraProps->facing) { > >> + LOG(HAL, Warning) > >> + << "Camera location does not match" > >> + << " configuration file. Using " << facing_; > >> + } > >> + } else if (cameraProps) { > >> + if (cameraProps->facing == -1) { > >> + LOG(HAL, Error) > >> + << "Camera facing not in configuration file"; > >> + return -EINVAL; > >> + } > >> + facing_ = cameraProps->facing; > >> } else { > >> - /* > >> - * \todo Retrieve the camera location from configuration file > >> - * if not available from the library. > >> - */ > >> - facing_ = CAMERA_FACING_FRONT; > >> + facing_ = CAMERA_FACING_EXTERNAL; > >> } > >> > >> /* > >> @@ -483,8 +507,24 @@ int CameraDevice::initialize() > >> if (properties.contains(properties::Rotation)) { > >> int rotation = properties.get(properties::Rotation); > >> orientation_ = (360 - rotation) % 360; > >> + if (cameraProps && cameraProps->rotation != -1 && > >> + orientation_ != cameraProps->rotation) { > >> + LOG(HAL, Warning) > >> + << "Camera orientation does not match" > >> + << " configuration file. Using " << orientation_; > >> + } > >> + } else if (cameraProps) { > >> + if (cameraProps->rotation == -1) { > >> + LOG(HAL, Error) > >> + << "Camera rotation not in configuration file"; > >> + return -EINVAL; > >> + } > >> + orientation_ = cameraProps->rotation; > >> + } else { > >> + orientation_ = 0; > >> } > >> > >> + /* Acquire the camera and initialize available stream configurations. */ > >> int ret = camera_->acquire(); > >> if (ret) { > >> LOG(HAL, Error) << "Failed to temporarily acquire the camera"; > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >> index 11bdfec8d587..598d89f1cff0 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; > > This triggers a compiler warning on GCC > > In file included from ../src/android/camera_device.cpp:9: > ../src/android/camera_hal_config.h:16:1: error: 'CameraProps' defined as > a struct here but previously declared as a class; this is valid, but may > result in linker errors under the Microsoft C++ ABI > [-Werror,-Wmismatched-tags] > struct CameraProps { > ^ > ../src/android/camera_device.h:32:1: note: did you mean struct here? > class CameraProps; > Uuuups, yes indeed! Thanks for spotting! > > > >> 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 = nullptr); > > > > You can drop the = nullptr if you agree with the change below. > > > >> > >> 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..1defc3f9c5bd 100644 > >> --- a/src/android/camera_hal_manager.cpp > >> +++ b/src/android/camera_hal_manager.cpp > >> @@ -41,6 +41,16 @@ int CameraHalManager::init() > >> { > >> cameraManager_ = std::make_unique<CameraManager>(); > >> > >> + /* > >> + * Open and parse configuration file. > >> + * > >> + * If the configuration file is not available the HAL only supports > >> + * external cameras. If it exists but it's not valid then error out. > >> + */ > >> + halConfig_.open(); > >> + if (halConfig_.exists() && !halConfig_.valid()) > >> + return -EINVAL; > >> + > >> /* Support camera hotplug. */ > >> cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > >> cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > >> @@ -100,6 +110,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > >> auto iter = cameraIdsMap_.find(cam->id()); > >> if (iter != cameraIdsMap_.end()) { > >> id = iter->second; > >> + if (id >= firstExternalCameraId_) > >> + isCameraExternal = true; > >> } else { > >> isCameraNew = true; > >> > >> @@ -117,7 +129,30 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > >> > >> /* Create a CameraDevice instance to wrap the libcamera Camera. */ > >> std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); > >> - int ret = camera->initialize(); > >> + > >> + /* > >> + * The configuration file must be valid for internal cameras. > >> + * External cameras can be initialized without configuration file. > >> + */ > >> + int ret = -EINVAL; > >> + if (!halConfig_.exists()) { > >> + if (isCameraExternal) > >> + ret = camera->initialize(); > >> + } else { > >> + /* > >> + * Get camera properties from the configuration file which > >> + * exists and is valid. > >> + * > >> + * Internal cameras are required to have a corresponding entry > >> + * in the configuration file. External cameras are not required > >> + * to. > >> + */ > >> + const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); > >> + if (cameraProps->valid) > >> + ret = camera->initialize(cameraProps); > >> + else if (isCameraExternal) > >> + ret = camera->initialize(); > >> + } > >> if (ret) { > >> LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > >> return; > > > > If the configuration file does not exist and the camera is internal, > > we'll only print "Failed to initialize camera", which will be confusing. > > How about the following ? > > > > /* > > * The configuration file must be valid, and contain a corresponding > > * entry for internal cameras. External cameras can be initialized > > * without configuration file. > > */ > > if (!isCameraExternal && !halConfig_.exists()) { > > LOG(HALConfig, Error) > > << "HAL configuration file is mandatory for internal cameras"; > > return -EINVAL; > > } > > > > const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); > > if (!isCameraExternal && !cameraProps) { > > LOG(HALConfig, Error) > > << "HAL configuration entry for camera " << cam->id() > > << " is missing"; > > return -EINVAL; > > } > > > > ret = camera->initialize(cameraProps); > > if (ret) { > > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); > > return; > > } > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > >> 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 > -- > Kieran
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 89044efa7ebe..2e93936fdb4b 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" @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, } /* - * Initialize the camera static information. + * Initialize the camera static information retrieved from the + * Camera::properties or from the cameraPros. + * + * cameraProps is optional for external camera devices and is defaulted to + * nullptr. + * * 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 them, otherwise fallback to values parsed from + * the configuration file. If the configuration file is not available + * the camera is external so its location and rotation can be safely + * defaulted. + */ const ControlList &properties = camera_->properties(); if (properties.contains(properties::Location)) { @@ -464,12 +478,22 @@ int CameraDevice::initialize() facing_ = CAMERA_FACING_EXTERNAL; break; } + + if (cameraProps && cameraProps->facing != -1 && + facing_ != cameraProps->facing) { + LOG(HAL, Warning) + << "Camera location does not match" + << " configuration file. Using " << facing_; + } + } else if (cameraProps) { + if (cameraProps->facing == -1) { + LOG(HAL, Error) + << "Camera facing not in configuration file"; + return -EINVAL; + } + facing_ = cameraProps->facing; } else { - /* - * \todo Retrieve the camera location from configuration file - * if not available from the library. - */ - facing_ = CAMERA_FACING_FRONT; + facing_ = CAMERA_FACING_EXTERNAL; } /* @@ -483,8 +507,24 @@ int CameraDevice::initialize() if (properties.contains(properties::Rotation)) { int rotation = properties.get(properties::Rotation); orientation_ = (360 - rotation) % 360; + if (cameraProps && cameraProps->rotation != -1 && + orientation_ != cameraProps->rotation) { + LOG(HAL, Warning) + << "Camera orientation does not match" + << " configuration file. Using " << orientation_; + } + } else if (cameraProps) { + if (cameraProps->rotation == -1) { + LOG(HAL, Error) + << "Camera rotation not in configuration file"; + return -EINVAL; + } + orientation_ = cameraProps->rotation; + } else { + orientation_ = 0; } + /* Acquire the camera and initialize available stream configurations. */ int ret = camera_->acquire(); if (ret) { LOG(HAL, Error) << "Failed to temporarily acquire the camera"; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 11bdfec8d587..598d89f1cff0 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 = nullptr); 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..1defc3f9c5bd 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -41,6 +41,16 @@ int CameraHalManager::init() { cameraManager_ = std::make_unique<CameraManager>(); + /* + * Open and parse configuration file. + * + * If the configuration file is not available the HAL only supports + * external cameras. If it exists but it's not valid then error out. + */ + halConfig_.open(); + if (halConfig_.exists() && !halConfig_.valid()) + return -EINVAL; + /* Support camera hotplug. */ cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); @@ -100,6 +110,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) auto iter = cameraIdsMap_.find(cam->id()); if (iter != cameraIdsMap_.end()) { id = iter->second; + if (id >= firstExternalCameraId_) + isCameraExternal = true; } else { isCameraNew = true; @@ -117,7 +129,30 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) /* Create a CameraDevice instance to wrap the libcamera Camera. */ std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); - int ret = camera->initialize(); + + /* + * The configuration file must be valid for internal cameras. + * External cameras can be initialized without configuration file. + */ + int ret = -EINVAL; + if (!halConfig_.exists()) { + if (isCameraExternal) + ret = camera->initialize(); + } else { + /* + * Get camera properties from the configuration file which + * exists and is valid. + * + * Internal cameras are required to have a corresponding entry + * in the configuration file. External cameras are not required + * to. + */ + const CameraProps *cameraProps = &halConfig_.cameraProps(cam->id()); + if (cameraProps->valid) + ret = camera->initialize(cameraProps); + else if (isCameraExternal) + ret = camera->initialize(); + } 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 | 56 +++++++++++++++++++++++++----- src/android/camera_device.h | 3 +- src/android/camera_hal_manager.cpp | 37 +++++++++++++++++++- src/android/camera_hal_manager.h | 3 ++ 4 files changed, 89 insertions(+), 10 deletions(-)