[libcamera-devel] [PATCH v3 4/5] android: camera_device: Get properties from configuration

[Laurent Pinchart]

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
but has a different value than the one retrieved from the firmware, I
wonder if we should warn the user ?

> > > >         }
> > > >
> > >
> > > 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.

> 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