[libcamera-devel] [PATCH v3 4/5] android: camera_device: Get properties from configuration
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 6 15:53:21 CEST 2021
Hi Laurent,
On Sat, Apr 03, 2021 at 06:08:48AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Apr 01, 2021 at 11:02:30AM +0900, Hirokazu Honda wrote:
> > On Wed, Mar 31, 2021 at 8:08 PM Jacopo Mondi wrote:
> > > On Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:
> > > > On Tue, Mar 30, 2021 at 11:20 PM 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 at jmondi.org>
> > > > > ---
> > > > > src/android/camera_device.cpp | 9 +++------
> > > > > src/android/camera_device.h | 3 ++-
> > > > > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--
> > > > > src/android/camera_hal_manager.h | 3 +++
> > > > > 4 files changed, 27 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > index eb327978c84e..e59f25c68d6b 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"
> > > > >
> > > > > @@ -351,7 +352,7 @@ 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. */
> > > > > const ControlList &properties = camera_->properties();
> > > > > @@ -370,11 +371,7 @@ int CameraDevice::initialize()
> > > > > break;
> > > > > }
> > > > > } else {
> > > > > - /*
> > > > > - * \todo Retrieve the camera location from configuration file
> > > > > - * if not available from the library.
> > > > > - */
> > > > > - facing_ = CAMERA_FACING_FRONT;
> > > > > + facing_ = cameraProps.facing;
>
> Should we guard against the case where the property isn't specified in
> the configuration file either ? How about the case where it's specified
I think we should assume the configuration file is complete and
correct. It's structure is validated at parsing time, we should make
sure all mandatory information are there by validating it through a
schema ?
> but has a different value than the one retrieved from the firmware, I
> wonder if we should warn the user ?
>
If the information is available from firmware it takes precedence and
we don't get here at all
> > > > > }
> > > > >
> > > >
> > > > Can you explain the rule of updating facing_ and rotation_?
> > > > How do we prioritize camera_->properties() and cameraProps?
> > >
> > > Do you mean clarifying which one has precendence ?
> > >
> > > If the information is available from the Camera (which means it has
> > > been collected from the firmware interface) then it has prcedence.
> > > Otherwise the configuration file is used as a fallback.
> >
> > Yes, thanks for explaining.
> > Shall we add the comment in code about it?
>
> A comment could be nice indeed.
>
and that's the reason why the code layout is self-explanatory enough
that I considered it was not worth a comment. But it's cheap, I can
add one.
> > Regardless, the code change looks good.
> >
> > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > > > > /*
> > > > > 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;
> > > > > + }
>
> 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.
>
> We should however not make the configuration mandatory for external
> cameras, this would break UVC support.
>
> 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
More information about the libcamera-devel
mailing list