[libcamera-devel] [PATCH] android: Add flag for external camera support
Jacopo Mondi
jacopo at jmondi.org
Thu Mar 4 10:47:36 CET 2021
Hi Paul,
On Thu, Mar 04, 2021 at 05:56:59PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Jacopo,
>
> On Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:
> > > In the static metadata, we claim that we don't support external cameras,
> > > yet libcamera core defaults the camera location to external. In the HAL
> > > CameraDevice we defaulted external to front, but we hadn't handled
> > > CameraHalManager when counting the internal vs external cameras for
> > > hal_get_number_of_cameras().
> > >
> > > To solve this, add a flag to hold if we have external camera support
> > > (hardcoded to false until we do), and add that to the condition in
> > > CameraHalManager when deciding whether a camera should be counted as
> > > internal or external. CameraDevice also uses this flag to decide on the
> > > default location.
> >
> > I have missed what condition should trigger for us setting
> > externalCameraSupport_ to true in the camera_hal_manager.
>
> Once we support the external hardware level...?
>
> >
> > and once we do so, won't cameras that report location=external because
> > of the missing information from the firmware be reported as external
> > even if they're not ?
>
> Yes that will happen, but it should be fine since the hardware level
> says that we support it.
>
> This is just one step before adding HAL config support (unless we should
> go for that directly instead?).
>
> >
> > I think we're a bit running in circles here. In my opinion the best
> > course of actions would have been not registering properties::Location
> > if libcamera cannot find that information from firmware (or from
>
> Yes.
>
> > somewhere else in future), and the camera HAL should resort to a
> > configuration file in that case, and fail registering the camera if
> > said file is not available.
>
> Oh, okay. I guess the question then is, in the event that the HAL config
> file isn't available and no location is specified, should 1) we fail to
> register the camera, or 2) default to external (or front, depending on
> the hardware level).
I would refrain from registering the camera but it might be too harsh.
True that Android is not a regular distro and integrators should know
what they're doing and what they need to do.
I'm skeptical about defaulting to anything as it's proving to be doing
more harm than good, but again, I might be too harsh.
>
> I'm fine with having HAL config files. I was thinking of this patch more
> like an intermediate step.
>
>
> Paul
>
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > > src/android/camera_device.cpp | 24 ++++++++++++++----------
> > > src/android/camera_device.h | 8 ++++++--
> > > src/android/camera_hal_manager.cpp | 11 +++++++----
> > > src/android/camera_hal_manager.h | 2 ++
> > > 4 files changed, 29 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index ae01c362..bb33c922 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> > > * back to the framework using the designated callbacks.
> > > */
> > >
> > > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> > > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera,
> > > + bool externalCameraSupport)
> > > : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> > > - facing_(CAMERA_FACING_FRONT), orientation_(0)
> > > + facing_(CAMERA_FACING_FRONT), orientation_(0),
> > > + externalCameraSupport_(externalCameraSupport)
> > > {
> > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > >
> > > @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()
> > > }
> > >
> > > std::shared_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > > - const std::shared_ptr<Camera> &cam)
> > > + const std::shared_ptr<Camera> &cam,
> > > + bool externalCameraSupport)
> > > {
> > > - CameraDevice *camera = new CameraDevice(id, cam);
> > > + CameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);
> > > return std::shared_ptr<CameraDevice>(camera);
> > > }
> > >
> > > @@ -375,11 +378,10 @@ int CameraDevice::initialize()
> > > facing_ = CAMERA_FACING_BACK;
> > > break;
> > > case properties::CameraLocationExternal:
> > > - /*
> > > - * \todo Set this to EXTERNAL once we support
> > > - * HARDWARE_LEVEL_EXTERNAL
> > > - */
> > > - facing_ = CAMERA_FACING_FRONT;
> > > + if (externalCameraSupport_)
> > > + facing_ = CAMERA_FACING_EXTERNAL;
> > > + else
> > > + facing_ = CAMERA_FACING_FRONT;
> > > break;
> > > }
> > > }
> > > @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > > staticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);
> > >
> > > /* Info static metadata. */
> > > - uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > + uint8_t supportedHWLevel = externalCameraSupport_ ?
> > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :
> > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > > &supportedHWLevel, 1);
> > >
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 4905958e..f96934db 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable
> > > {
> > > public:
> > > static std::shared_ptr<CameraDevice> create(unsigned int id,
> > > - const std::shared_ptr<libcamera::Camera> &cam);
> > > + const std::shared_ptr<libcamera::Camera> &cam,
> > > + bool externalCameraSupport);
> > > ~CameraDevice();
> > >
> > > int initialize();
> > > @@ -66,7 +67,8 @@ protected:
> > > std::string logPrefix() const override;
> > >
> > > private:
> > > - CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> > > + CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera,
> > > + bool externalCameraSupport);
> > >
> > > struct Camera3RequestDescriptor {
> > > Camera3RequestDescriptor(libcamera::Camera *camera,
> > > @@ -130,6 +132,8 @@ private:
> > > unsigned int maxJpegBufferSize_;
> > >
> > > CameraMetadata lastSettings_;
> > > +
> > > + const bool externalCameraSupport_;
> > > };
> > >
> > > #endif /* __ANDROID_CAMERA_DEVICE_H__ */
> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > > index 189eda2b..e4ca4c82 100644
> > > --- a/src/android/camera_hal_manager.cpp
> > > +++ b/src/android/camera_hal_manager.cpp
> > > @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)
> > > */
> > >
> > > CameraHalManager::CameraHalManager()
> > > - : cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),
> > > + : cameraManager_(nullptr), callbacks_(nullptr),
> > > + externalCameraSupport_(false), numInternalCameras_(0),
> > > nextExternalCameraId_(firstExternalCameraId_)
> > > {
> > > }
> > > @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> > > * Now check if this is an external camera and assign
> > > * its id accordingly.
> > > */
> > > - if (cameraLocation(cam.get()) == properties::CameraLocationExternal) {
> > > + if (cameraLocation(cam.get()) == properties::CameraLocationExternal &&
> > > + externalCameraSupport_) {
> > > isCameraExternal = true;
> > > id = nextExternalCameraId_;
> > > } else {
> > > @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> > > }
> > >
> > > /* Create a CameraDevice instance to wrap the libcamera Camera. */
> > > - std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> > > + std::shared_ptr<CameraDevice> camera =
> > > + CameraDevice::create(id, std::move(cam), externalCameraSupport_);
> > > int ret = camera->initialize();
> > > if (ret) {
> > > LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> > > @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> > > if (isCameraNew) {
> > > cameraIdsMap_.emplace(cam->id(), id);
> > >
> > > - if (isCameraExternal)
> > > + if (isCameraExternal && externalCameraSupport_)
> > > nextExternalCameraId_++;
> > > else
> > > numInternalCameras_++;
> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> > > index a91decc7..74dc6b3e 100644
> > > --- a/src/android/camera_hal_manager.h
> > > +++ b/src/android/camera_hal_manager.h
> > > @@ -54,6 +54,8 @@ private:
> > > std::map<std::string, unsigned int> cameraIdsMap_;
> > > Mutex mutex_;
> > >
> > > + const bool externalCameraSupport_;
> > > +
> > > unsigned int numInternalCameras_;
> > > unsigned int nextExternalCameraId_;
> > > };
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list