1099381 - [Camera][Gecko] If pre-init camera SetConfiguration() fails, need to release CameraControl and call onClosed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

With the camera pre-initialization in bug 1090501, calling getCamera() now instead creates a DOMCameraControl wrapper around the ICameraControl object and if an initial configuration is specified, passes that configuration to SetConfiguration().

SetConfiguration() can fail (see bug 1099375), but there is no way to communicate that to the app.

The cleanest solution seems to be to call getCamera()s onError callback and reject the promise, while discarding the DOM/ICameraControl objects.

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

...unfortunately the getCamera() callbacks have already run. So have the hardware state change notification. Also, in the case of bug 1099375, we were even able to successfully apply the requested configuration! The failure doesn't occur until GCC::SetConfigurationImpl() tries to start the preview. It's this last step that fails.

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - close camera if initial config can't be applied to pre-init camera

I'm not entirely happy with this solution, but wanted to solicit some feedback.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8525386 [details] [diff] [review]
WIP - close camera if initial config can't be applied to pre-init camera

Review of attachment 8525386 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, although if you want a cleaner solution, why not redefine how/when we fulfill getCamera instead. Something like:
- Never fulfill getCamera promise on hardware open. Instead fulfill on configuration updated.
- Add configuration updated trigger when camera opened without being supplied an initial configuration by application. This makes some kind of logical sense because it still has an initial state as given by the driver.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Andrew Osmond [:aosmond] from comment #3)

> Looks fine to me, although if you want a cleaner solution, why not redefine
> how/when we fulfill getCamera instead. Something like:
> - Never fulfill getCamera promise on hardware open. Instead fulfill on
> configuration updated.
> - Add configuration updated trigger when camera opened without being
> supplied an initial configuration by application. This makes some kind of
> logical sense because it still has an initial state as given by the driver.

The reason for not doing this is that in general, this -will- succeed, and it allows the Camera app to start up faster if it gets the open hardware instance ASAP. i.e. it can grab recorder profiles, query additional supported features, etc.

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Close camera if initial config can't be applied to pre-init camera, v1

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4b8c547c8742

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Close camera if initial config can't be applied to pre-init camera, v1.1

Small change in StartInternal(): if there's no initial config, we don't call OnConfigurationChange() or OnPreviewStateChange().

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8528609 [details] [diff] [review]
Close camera if initial config can't be applied to pre-init camera, v1.1

Review of attachment 8528609 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment 8528609 [details] [diff] was included/modified as part of bug 1104913.

https://hg.mozilla.org/mozilla-central/rev/ce9707a48002
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ce9707a48002