[libcamera-devel] [PATCH] android: camera_hal_manager: Fail on no cameras

Wed Jul 22 13:12:21 CEST 2020

Hi Kieran,
   I think the conversation diverged, as I was clearly overlooking
the UVC camera use case and you're missing the intended use case of
this patch. I'll try to reply and summarize at the end.

On Tue, Jul 21, 2020 at 09:59:10PM +0100, Kieran Bingham wrote:
> On 21/07/2020 14:09, Jacopo Mondi wrote:
> > On Tue, Jul 21, 2020 at 01:47:29PM +0100, Kieran Bingham wrote:
> >> Hi Jacopo,
> >>
> >> On 21/07/2020 13:13, Jacopo Mondi wrote:
> >>> Hi Kieran,
> >>>
> >>> On Tue, Jul 21, 2020 at 12:41:16PM +0100, Kieran Bingham wrote:
> >>>> Hi Jacopo,
> >>>>
> >>>> On 21/07/2020 12:26, Jacopo Mondi wrote:
> >>>>> The CameraHalManager initialization tries to start the
> >>>>> libcamera::CameraManager which enumerate the registered cameras.
> >>>>>
> >>>>> When initialization is called too early during system boot, it might
> >>>>> happen that the media graphs are still being registered to user-space
> >>>>> preventing any pipeline handler to match and register cameras.
> >>>>>
> >>>>> If that happens, the CameraHalManager silently accepts that no
> >>>>> cameras are available in the system, reporting that information
> >>>>> to the camera stack:
> >>>>>
> >>>>> cros_camera_service[2054]: (5) StartOnThread(): Camera module "libcamera camera HALv3 module" has 0 built-in camera(s)
> >>>>> cros_camera_service[2054]: (5) StartOnThread(): SuperHAL started with 1 modules and 0 built-in cameras
> >>>>> CameraProviderManager: Camera provider legacy/0 ready with 0 camera devices
> >>>>>
> >>>>
> >>>> Hrm ... should this be part of the hotplug work that as cameras become
> >>>> available this gets updated somehow? - Even on *non* hotpluggable pipelines?
> >>>>
> >>>
> >>> I don't get what your comment is about here, I'm sorry
> >>
> >>
> >> Your patch prevents loading of libcamera completely unless there is at
> >> least one camera right ?
> >>
> >> What happens on say an x86 platform which will only have UVC cameras,
> >> yet will use libcamera...
> >>
> >> When will it be acceptable to load the camera service.
> >>
> >> Should the service itself continually fail, and respawn until someone
> >> plugs in a camera?
> >>
> >
> > That's my understanding
>
> That sounds like that would cause a very bad implementation to me.
>
> Waiting for upstart to decide when to retry launching the camera-service
> wouldn't be an acceptable time to wait IMO.
>
> Either it tries every second, and is flooding the system, or it tries
> every minute and it takes a minute before you can view your newly
> attached UVC camera ...
>
> I would expect event driven camera attachment, not polled ...
>

For UVC we will have to register non-active cameras and notify to
android they're available when hotplug is detected

For non-pluggable cameras, as shall wait until the pipeline handler is
matched and has registered cameras

> >> So taking that further, what I'm saying is, perhaps this should be
> >> perfectly able to launch with zero cameras, and /when/ cameras are
> >> created they should notify android that a new camera is now available.
> >
> > Do you know if that's even possible ?
> >
> > I think Android expects the number of possible cameras to be
> > statically defined, there's a callback to notify a change of status of
> > a device, but we're dealing here with -creating- cameras based on the
> > number of cameras detected by android.
>
> So, looking at the USB HAL in cros - it does look like they pre-allocate
> some cameras or such and only activate them on device connection.
>
> I fear we might have to do the same somehow in the future, as indeed it
> might not be possible to tell android there is a 'new' camera. Only an
> update to an existing one... not sure it will require more investigation.
>

Yes, but that's for UVC. Built-in cameras are different, and Android
bets on the fact if you have a camera service running then your system
has cameras which are expected to be registered.

Keep in mind so far there's been an HAL for UVC and one for built-in
cameras. We're handling both and that's a new use case afaict

>
>
> >>>>> Fix this by returning an error code if no camera is registered in the
> >>>>> system at the time CameraHalManager::init() is called. The camera
> >>>>> framework then tries to re-load the HAL module later in time, hopefully
> >>>>> after the media device dependencies have been registered:
> >>>>>
> >>>>> 2020-07-21T12:26:37.903456+02:00 INFO cros_camera_service[2054]: (5) StartOnThread(): Camera module "libcamera camera HALv3 module" has 0 built-in camera(s)
> >>>>> 2020-07-21T12:26:37.903521+02:00 INFO cros_camera_service[2054]: (5) StartOnThread(): SuperHAL started with 1 modules and 0 built-in cameras
> >>>>> ....
> >>>>> 2020-07-21T12:30:36.662877+02:00 INFO cros_camera_service[5908]: (5910) StartOnThread(): Camera module "libcamera camera HALv3 module" has 2 built-in camera(s)
> >>>>> 2020-07-21T12:30:36.663196+02:00 INFO cros_camera_service[5908]: (5910) StartOnThread(): SuperHAL started with 1 modules and 2 built-in cameras
> >>>>>
> >>>>> Return -ENODEV as according to camera_common.h specification is the
> >>>>> only supported error code. The CrOS HAL adapter does not distinguish
> >>>>> between that and other negative values, so it does not really make
> >>>>> a difference.
> >>>>
> >>>> This feels a bit punchy/racey still. And what happens in the instance
> >>>> that we really don't yet have any cameras? (I.e. a UVC only platform,
> >>>> which doesn't yet have any cameras connected).
> >>>
> >>> You keep defferring ? I think the framework handles that
> >>
> >> I would be surprised if it would be expected behaviour to just poll
> >> launching of the camera service until there is a camera there to satisfy
> >> it ...
> >>
> >>
> >>>> How many times will we defer? How long does it defer for for instance?
> >>>>
> >>>
> >>> Usually 1 is enough from my testing. I don't think we have to care
> >>> what is the timeout in the framework, as afaict android services are
> >>> restarted by default.
> >>
> >>
> >> Ok, so this is mostly dealing with the race at startup ...
> >>
> >>
> >>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>>>> ---
> >>>>>  src/android/camera_hal_manager.cpp | 11 +++++++++++
> >>>>>  1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> >>>>> index 02b6418fb36d..e967d210e547 100644
> >>>>> --- a/src/android/camera_hal_manager.cpp
> >>>>> +++ b/src/android/camera_hal_manager.cpp
> >>>>> @@ -73,6 +73,17 @@ int CameraHalManager::init()
> >>>>>  		++index;
> >>>>>  	}
> >>>>>
> >>>>> +	/*
> >>>>> +	 * If no pipeline has registered cameras, defer initialization to give
> >>>>> +	 * time to media devices to register to user-space.
> >>>>> +	 */
> >>>>> +	if (index == 0) {
> >>>>> +		LOG(HAL, Debug) << "Defer CameraHALManager initialization";
> >>>>
> >>>> I think perhaps this needs a higher level than Debug to make sure it
> >>>> always prints somewhere.
> >>>
> >>> I think that's kind of expected to happen, I don't think this should
> >>> be an error
> >>
> >>
> >> Info? Warning?
> >>
> >> If libcamera has prevented loading, I think it would be good to know
> >> about it by default...
> >>
> >>
> >>>> I fear this will be deflecting the issue, and introduces races (i.e.
> >>>> this would go through if only one camera of two in the system is available).
> >>>
> >>> If the pipeline handler match (ie the device enumerator matches the
> >>> dependencies) than the match() function continues and register
> >>> cameras. I don't think we can found ourselves in a semi-initialized
> >>> state.
> >>
> >>
> >> Ok, so it really is about having at least one camera available?
> >
> > It's about at least a pipeline being matched.
>
> Ok, but I don't think it's that difficult to conjur up a scenario where
> there won't be a device

Not for what Android has been thought to work on. You integrate a
system with cameras, it's fair to defer until those cameras are
registered. You have no cameras, either you disable the camera stack
or you register 0 cameras and then the camera subsystem is silent and
not accessible.

>
>
> >
> >>
> >> I think looking at CameraHalManager::init(), as it only creates a
> >> CameraDevice for cameras available in cameraManager_->cameras(), it
> >> /could/ be a race, it's just that creating two cameras is fast, so we're
> >> unlikely to get in here between the point that one has been available
> >> and another is still loading ...
> >
> > I don't think that's possible.
> >
> > If a all the dependencies of a pipeline handler are matched, the
> > camera are registered. What is the code flow that registers one
> > camera, returns to the camera manager, then calls the pipeline handler
> > again to register more ?
>
>
> At least PipelineHandlerUVC will call PipelineHandler::match() once for
> each camera attached.
>
> But yes, perhaps due to the threading model, it won't be a possible race.
>
> I was thinking of when a pipeline starts and only 'one' of it's media
> devices is available.

Yes, for UVC that's different I agree. A call to
CameraManager::start() might match one uvc media device but miss a
second one which still have to appear to userspace.

As said, UVC needs some special handling and pre-register cameras

>
>
>
>
> >> But still ... I think this is stuff that will get dealt with by hotplug
> >> being added to the HAL.
> >
> > I agree, we should support inserting and removing new cameras, but I
> > think at this point, we should really start only if cameras are
> > registered and any pipeline handler is matched.
> >
> > I'm not sure I got what other alternative approach you are proposing
> > to be honest.
>
> I'm suggesting that when hotplug support is available, this patch, if
> integrated, might need to be reverted in effect ... and thus a todo note
> should be added to say that or such.
>
> I believe we should be able to start the camera service successfully
> with zero cameras if there are zero cameras at the time the service starts.

I don't think so for built-in cameras. We start the service when
cameras are registered, otherwise if you have to pre-allocate cameras
you would need to know how many of them the pipeline handler expects
to register and that's not known before it has been matched.

UVC, again, is a different story as it supports hotplug.

>
>
> >>>> ..
> >>>>
> >>>> Perhaps it can still go in if it solves an immediate problem, but in
> >>>> that case I think it needs a \todo or a warning of some kind...
> >>>>
> >>>
> >>> A \todo item to record this should be fixed how ?
> >>>
> >>> Unless we expect the CameraManager to stall until any of the pipeline
> >>> handler it tries match, but this seems not a good idea to me.
> >>
> >> No, i don't expect it to stall ... I expect it to complete successfully,
> >> and if there are only 0 cameras, it will only have zero cameras - until
> >
> > That means Android/CrOS starts without the camera being active (ie.
> > the camera application icon is not available, in CrOS in example).
>
>
> Hrm, so it disables the app completely?
>
> Anyway, essentially I'm thinking - whatever happens with the existing
> UVC stack is what we would need to be able to mirror.
>
>
>
> >> they become available at which point they'll be registered through the
> >> same mechanisms as the hotplugging ...
> >
> > Still I don't get if you are talking about libcamera hotplug or
> > android hotplug, which, to my understanding, expects anyway a camera
> > device to be registered but marked as 'not present' (looking at the
> > camera_device_status enumeration)
>
>
> I was going to ask how does it register camera's it doesn't know will
> exist, but I think I saw that the UVC HAL just pre-allocates some
> cameras or such, so that explains how that one works.
>

I see three cases:
1) No uvc support
   Defer until a pipeline handler is matched and has registered
   cameras

2) UVC only
   Need to pre-register cameras and activate on hot-plug. Knowing when
   this has to happen is tricky, as the HAL needs to know it should
   not wait for built-in cameras and can proceed to register
   non-active USB cameras

3) built-in + UVC
   Simmilarly, knowing we have to wait for built-in to appear, we
   defer until cameras are available, then pre-register UVC cameras.

The hard part I think it is to define how to instruct the HAL it has
to wait for built-in to appear or not before registering UVC
non-active cameras.