[libcamera-devel] [PATCH 2/2] android: camera_hal_manager: Support camera hotplug
Umang Jain
email at uajain.com
Thu Aug 6 10:26:45 CEST 2020
Hi Laurent,
Thanks for the detailed review.
I need a few clarifications.
On 8/6/20 3:04 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Wed, Aug 05, 2020 at 03:14:44PM +0000, Umang Jain wrote:
>> Extend the support for camera hotplug from libcamera's CameraManager
>> to CameraHalManager. Use camera module callbacks to let the framework
>> know about the hotplug events and change the status of cameras being
>> being hotplugged or unplugged via camera_device_status_change().
>>
>> Signed-off-by: Umang Jain <email at uajain.com>
>> ---
>> src/android/camera_device.h | 1 +
>> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++--
>> src/android/camera_hal_manager.h | 3 +++
>> 3 files changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 4e5fb98..fa9706c 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -47,6 +47,7 @@ public:
>>
>> unsigned int id() const { return id_; }
>> camera3_device_t *camera3Device() { return &camera3Device_; }
>> + const libcamera::Camera *getCamera() { return camera_.get(); };
>>
>> int facing() const { return facing_; }
>> int orientation() const { return orientation_; }
>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
>> index 3ddcab5..b498278 100644
>> --- a/src/android/camera_hal_manager.cpp
>> +++ b/src/android/camera_hal_manager.cpp
>> @@ -59,8 +59,6 @@ int CameraHalManager::init()
>> /*
>> * For each Camera registered in the system, a CameraDevice
>> * gets created here to wraps a libcamera Camera instance.
>> - *
>> - * \todo Support camera hotplug.
>> */
>> unsigned int index = 0;
>> for (auto &cam : cameraManager_->cameras()) {
>> @@ -73,6 +71,10 @@ int CameraHalManager::init()
>> ++index;
>> }
>>
>> + /* Support camera hotplug */
> s/hotplug/hotplug./
>
>> + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);
>> + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);
> This creates a race condition, as the hotplug (and hotunplug) could
> occur after the camera manager is started and before you connect the
> signals. I think you should connect the signals before starting the
> camera manager, and remove the loop above that creates CameraDevice
> instances. CameraHalManager::cameraAdded will add instances as needed.
I can agree with this suggestion, I don't see it being problematic.
>
>> +
>> return 0;
>> }
>>
>> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id,
>> return camera;
>> }
> You also need to update .get_number_of_cameras() to handle hotplug.
> According to its documentation ([1]),
>
> * The value here must be static, and must count only built-in cameras,
> * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values
> * (camera_info.facing). The HAL must not include the external cameras
> * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value
> * of this call. Frameworks will use camera_device_status_change callback
> * to manage number of external cameras.
>
> We need to count the number of internal cameras at initialization time,
> and cache the value. That's fairly easy, except for the fact that USB
> cameras could be either internal or external. There's no way we can
> figure this out without some sort a platform-specific configuration
> data. In theory some information could be reported through ACPI, but
> looking at what my laptop reports there for the internal camera, I think
> most vendors just provide bogus information.
>
> Internal USB cameras can be ignored for now I believe, we can just
> consider that all cameras detected when the HAL is started are external.
Do you mean "all cameras detected" or "all UVC cameras detected" as
external?
Because the next line of explanation confuses me a bit.
>
> .get_number_of_cameras() need to be updated accordingly, and we can then
> work on better handling of internal USB cameras.
I think you meant "all UVC cameras" just above, no?
>
> Or, after having thought a bit more about it, maybe we should check the
> Location property of the Camera instead. That should be an authoritative
> source of information. The UVC pipeline handler would need to be fixed
> to report the Location (it doesn't at the moment), and we can hardcode
> it to CameraLocationExternal there for now until we get support for
> platform-specific configuration data. What do you think ?
hmm, Is this Location property can be queried from any existing metadata or,
do you mean that we should declare it inside pipeline-handler/camera.
The way I am understanding things here is, you are suggesting that each
pipeline
handler sets the CameraLocationProperty for the camera being created.
For e.g.
if IPU3 is used, that particular camera will be created with
CameraLocationInternal
whereas UVC pipelinehandler will create a camera with
CameraLocationExternal;
which then can be queried by a convenient Camera API. Is this what are
you referring here?
>
> Also, the id >= numCameras() in open() and getCameraInfo() needs to be
> replaced, with checks that lookup the id in the cameras_ map.
>
>> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>> +{
>> + unsigned int id = cameras_.size();
> I think we need something a bit more advanced to create camera IDs.
> The constraints on the camera ID are not well documented. [2] states
> that
>
> "Non-removable cameras use integers starting at 0 for their identifiers,
> while removable cameras have a unique identifier for each individual
> device, even if they are the same model."
>
> Note that this is the Java API, so the camera service may perform ID
> mapping, although that would surprise me.
>
> As the .camera_device_status_change() callback uses an int camera_id, we
> need integers for external cameras too, unless there's something I'm
> missing. IDs of different cameras need to be unique, so I think we
> shouldn't reuse the same ID if we unplug a USB camera and plug another
> one.
>
> One way to handle this is to keep a may of Camera::id() (Niklas has just
> merged the patch series that adds this) to HAL integer IDs, for all
> cameras we see. If a new camera is added and was seen before, we can
> reuse the same HAL ID. Otherwise we assign a new ID (we thus also need a
> counter of used HAL IDs).
>
> Furthermore, as cameraAdded() would be called for all cameras when we
> start the camera manager (assuming you agree with my suggestion to do so
> above), we won't know here, when we encounter an external device, how
> many internal devices there will be. We thus can't assign the HAL ID for
> external devices right away, unless we set a large enough base number to
> make sure it will always be above the number of internal devices. Even
> if it's a bit of a hack, I'd go in that direction, starting at 1000 for
> instance.
>
>> + CameraDevice *camera = new CameraDevice(id, cam);
>> + int ret = camera->initialize();
>> + if (ret) {
>> + LOG(HAL, Error) << "Failed to initialize camera: " << cam->name();
>> + return;
>> + }
>> +
>> + cameras_.emplace_back(camera);
>> + callbacks_->camera_device_status_change(callbacks_, id,
>> + CAMERA_DEVICE_STATUS_PRESENT);
> You first need to check if the callbacks_ are not null, as a camera
> could be hotplugged before callbacks are set. Access to callbacks_ will
> need to be protected with a mutex.
>
>> + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id
>> + << " added and initialized successfully.";
>> +}
>> +
>> +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
>> +{
>> + auto iter = std::find_if(cameras_.begin(), cameras_.end(),
>> + [cam](std::unique_ptr<CameraDevice> &camera) {
>> + return cam.get() == camera->getCamera();
>> + });
>> + if (iter == cameras_.end())
>> + return;
>> +
>> + unsigned int id = (*iter)->id();
>> + callbacks_->camera_device_status_change(callbacks_, id,
>> + CAMERA_DEVICE_STATUS_NOT_PRESENT);
>> + cameras_.erase(iter);
> This will delete the CameraDevice instance, which is "slightly"
> problematic if the camera is already open. It also creates race
> conditions with the CameraHalManager::open() function. I wonder if we
> need to turn cameras_ in a vector of shared pointers to handle this.
>
> I had a look at how the Chrome OS USB HAL implements unplugging when the
> camera is already opened ([3]):
>
> // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel
> // panic.
> if (cameras_.find(id) != cameras_.end()) {
> LOGF(WARNING)
> << "Unplug an opening camera, exit the camera service to cleanup";
> // Upstart will start the service again.
> _exit(EIO);
> }
>
> I'm not sure what to say :-)
>
> Should we try to refcount CameraDevice with std::shared_ptr<>, and
> protect access to camera_ with a mutex ?
Can you explain a bit, why would we need to protect access to camera_
with a mutex?
>
>> + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id
>> + << " removed successfully.";
>> +}
> [1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881
> [2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList()
> [3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498
>
>> +
>> unsigned int CameraHalManager::numCameras() const
>> {
>> return cameraManager_->cameras().size();
>> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
>> index 4345b1e..52ab9e2 100644
>> --- a/src/android/camera_hal_manager.h
>> +++ b/src/android/camera_hal_manager.h
>> @@ -33,6 +33,9 @@ public:
>> int setCallbacks(const camera_module_callbacks_t *callbacks);
>>
>> private:
>> + void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
>> + void cameraRemoved(std::shared_ptr<libcamera::Camera> cam);
>> +
>> libcamera::CameraManager *cameraManager_;
>>
>> const camera_module_callbacks_t *callbacks_;
More information about the libcamera-devel
mailing list