[libcamera-devel] [PATCH] android: Add flag for external camera support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 8 04:01:55 CET 2021
Hello everybody,
On Thu, Mar 04, 2021 at 11:20:16PM +0900, Tomasz Figa wrote:
> On Thu, Mar 4, 2021 at 6:47 PM Jacopo Mondi wrote:
> > On Thu, Mar 04, 2021 at 05:56:59PM +0900, wrote:
> > > On Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:
> > > > 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.
>
> Hmm, wouldn't the camera end up with the EXTERNAL capability level
> rather than LIMITED or FULL we're targeting? Also on Chrome OS we
> count internal cameras and things would error out if we don't get the
> right count.
In the context of Chrome OS, external cameras are limited to USB
webcams, right ? In that case, the EXTERNAL hardware level seems more
appropriate than LIMITED or FULL, as external UVC devices can't support
LIMITED or FULL (they don't provide all the information required by
those hardware levels). Is there something I'm missing ?
> > > 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.
I'm fine with this.
> > > > 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.
>
> Agreed with Jacopo. I'd log an error and fail to allow detecting problems early.
Once we have support for a configuration file, I'm fine with this, but I
think we can let the HAL default to front if the location isn't reported
by the libcamera core in the meantime.
> > > I'm fine with having HAL config files. I was thinking of this patch more
> > > like an intermediate step.
> > >
> > > > > 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_;
> > > > > };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list