[libcamera-devel] [PATCH v3 4/5] android: camera_device: Get properties from configuration
Han-lin Chen
hanlinchen at google.com
Mon Apr 19 13:50:26 CEST 2021
On Sat, Apr 3, 2021 at 11:09 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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
> 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 ?
Usually the configuration file is submitted to the device overlay (or
it's parent overlay).
An example (poppy is the parent overlay of soraka):
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/camera3_profiles.xml
in the case, libcamera may load from a static file path, but the
actual files are installed by different device overlays when built,
and thus they have different contents.
>
> > > > > +
> > > > > /* 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Cheers.
Hanlin Chen
More information about the libcamera-devel
mailing list