[libcamera-devel] [PATCH] android: Add flag for external camera support
Tomasz Figa
tfiga at chromium.org
Mon Mar 8 04:09:08 CET 2021
On Mon, Mar 8, 2021 at 12:02 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
>
I thought we were talking here about internal cameras that just lack
the location information because of an old or otherwise buggy firmware
implementation? In any case, my comment refers specifically to
internal MIPI cameras.
> > > > 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 thought Paul was specifically referring to the case when we have the
configuration file implemented, but it's not provided by the system
integrator.
Best regards,
Tomasz
> > > > 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