1097835 - [Camera] App doesn't listen for "closed" events

Status: VERIFIED FIXED

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

Description

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

The mediaserver process can fail, and when it does, it takes down the camera hardware with it. The app currently doesn't detect this state.

STR (to sim mediaserver crashing):
1. open the Camera app, note the working viewfinder
2. run 'adb shell ps | grep mediaserver' to get the PID of the mediaserver
3. run 'adb shell kill -9 $PID'

Expected: the system will restart mediaserver automatically. Once that happens, the app can reopen (and reconfigure) the camera.

Observed: the viewfinder goes black, and eventually the autofocus ring appears in the middle of the screen. If you touch the shutter button, the app will think it is trying to take a picture that is taking too long and the spinner appears.

Fatality: Homescreening doesn't fix the Camera app, but killing and restarting it does.

To fix this, the Camera app needs to either attach a callback to the CameraControl.onClosed attribute, or add a listener for the "close" event.

Comment 1

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

[Blocking Requested - why for this release]: blocking a blocker (bug 1094070).

Comment 2

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

Unit tests pending

Comment 3

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

Comment on attachment 8521661 [details] [review]
Pull Request

LGTM.

Comment 4

[Hema Koka [:hema]]

Blocking: Because the bug that this blocks on causes camera screen to go black: https://bugzilla.mozilla.org/show_bug.cgi?id=1094070#c9

Comment 5

[Diego Marcos [:dmarcos]]

Mike, what are the scenarios where onClose callback is invoked? In this bug scenario onClose is invoked on on an error situation: MediaServer is killed. Users of the Camera Controller API should be able to discriminate between errors and the camera closing after normal operation. What about adding an onError callback to the Camera Controller API?

Comment 6

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

(In reply to Diego Marcos [:dmarcos] from comment #5)

> Mike, what are the scenarios where onClose callback is invoked?

There are currently two cases where onClose is invoked:
1. If the camera hardware currently in use by the app is closed for any reason other than calling the release() method on CameraControl. Currently the only reason is mediaserver crashing.
2. If the camera hardware currently in use by the app is closed by calling the release() method, but no onSuccess callback is specified (Andrew, can you confirm? -- it looks like we'll always have a promise, no? What happens if the caller doesn't grab the returned promise from release()?)

> In this bug scenario onClose is invoked on on an error situation: MediaServer is killed.
> Users of the Camera Controller API should be able to discriminate between
> errors and the camera closing after normal operation. What about adding an
> onError callback to the Camera Controller API?

onClosed is meant to capture the cases where the hardware is closed in any unsolicited fashion, including error cases. I would actually like to remove the release() corner-case, and add an argument to onClosed (and the "close" event) identifying the reason -- though for now it will always be "SystemFailure" or something similar.

Comment 7

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

(In reply to Mike Habicher [:mikeh] from comment #6)
> 2. If the camera hardware currently in use by the app is closed by calling
> the release() method, but no onSuccess callback is specified (Andrew, can
> you confirm? -- it looks like we'll always have a promise, no? What happens
> if the caller doesn't grab the returned promise from release()?)

In the solicited case only the promise and onSuccess callback if supplied will be triggered in the solicited case. What anybody else does with the promise is irrelevant. onClose is only triggered in the unsolicited case. The only existing way to restore the original behaviour would be to store a weak reference to the promise (as it supports that) and see if it got freed. Oversight on my part I guess (although I may have been misled by the comments saying onClose is for unsolicited).

Comment 8

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8521661 [details] [review]
Pull Request

I left a comment on the PR asking if we can do `window.location.reload()` instead of `window.close()`. I'm not sure if maybe that would be enough to "reboot" the app?

Comment 9

[Diego Marcos [:dmarcos]]

I updated the PR to show a dialog when media server is not available before rebooting the application

Comment 10

[Diego Marcos [:dmarcos]]

Is it possible to have the reason why onClose was called as a callback argument?

Comment 11

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

(In reply to Diego Marcos [:dmarcos] from comment #10)

> Is it possible to have the reason why onClose was called as a callback
> argument?

Yes. { "Released" | "SystemFailure" }

I'll add it next week.

Comment 12

[Hema Koka [:hema]]

Diego, can we get this closed by 11/19 (2.1 cc is 11/21) and we need blockers wrapped up earlier in the week to be verified in time.

Thanks
Hema

Comment 13

[Justin D'Arcangelo [:justindarc]]

(In reply to Diego Marcos [:dmarcos] from comment #9)
> I updated the PR to show a dialog when media server is not available before
> rebooting the application

I left some comments on the PR. Ping me on IRC if you want to discuss.

Comment 14

[Diego Marcos [:dmarcos]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #13)
> (In reply to Diego Marcos [:dmarcos] from comment #9)
> > I updated the PR to show a dialog when media server is not available before
> > rebooting the application
> 
> I left some comments on the PR. Ping me on IRC if you want to discuss.

I read your comments about the new strings. Reusing the strings for camera unavailable would be misleading for the user. They don't represent what really happened:

camera-unavailable-title = Camera Unavailable
camera-unavailable-text = The camera is in use by another app.

Also we're adding the reboot button that needs to be translated as well.

Comment 15

[Diego Marcos [:dmarcos]]

(In reply to Mike Habicher [:mikeh] from comment #11)
> (In reply to Diego Marcos [:dmarcos] from comment #10)
> 
> > Is it possible to have the reason why onClose was called as a callback
> > argument?
> 
> Yes. { "Released" | "SystemFailure" }
> 
> I'll add it next week.

What is the bug tracking this change? What would be the best way to handle a SystemFailure? The PR attached to this bug presents a dialog to the user with the error information and a button to reboot the app. Wouldn't be better to be more specifc in the case of "SystemFailure" so the app can handle those differently?

Comment 16

[Diego Marcos [:dmarcos]]

Requesting the camera HW automatically after failure is not trivial to implement and I'm worried about regressions on 2.1. On the other hand, presenting a dialog to the user with an error message and a reboot button is trivial to implement. I know this is not ideal from the UX standpoint but the only reason that the camera might have to close in the middle of a session is after mediaserver going down which is an extremely rare scenario.  

I'm willing to refine this solution for 2.2+. We can create a follow up bug. ni UX

Comment 17

[David Flanagan [:djf]]

Diego,

Are you proposing that this dialog will contain another untranslated string like we have for the "another app is using the camera" error message?

What about just restarting the app without displaying the message or the button? Would that work? What would happen if the camera app restarted itself more quickly than the mediaserver process could restart itself? 

I think that it might be better for the user to see the screen go blank, and then see a spinner appear while the camera tries to restart than to see an untranslated error message and then see the camera try to restart.

Comment 18

[Stephany Wilkes]

Flagging Tif, who is UX for Media.

Comment 19

[Tiffanie Shakespeare [:tif]]

I agree with David - we don't want untranslated strings in our apps.

What is the harm in just restarting the camera ourselves? I may be missing something, but I don't see the benefit of displaying an error message and having the user restart the camera themselves.

David - what's this about an untranslated string? Is it new? We need to make sure that this gets translated in the next release. Shall I open a bug?

Comment 20

[Diego Marcos [:dmarcos]]

Attachment: Alternative Pull Request

This PR is a simplified version of the previous one. The application reboots automatically when the camera controller closed. No dialog is presented to the user.

Comment 21

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8524904 [details] [review]
Alternative Pull Request

LGTM. Made a comment about waiting 500ms. If you think we should do that, go ahead and add it. If not, that's fine. I'm not absolutely certain its necessary since we wait between retries at startup anyway.

Comment 22

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

(In reply to Diego Marcos [:dmarcos] from comment #15)

> What is the bug tracking this change? What would be the best way to handle a
> SystemFailure? The PR attached to this bug presents a dialog to the user
> with the error information and a button to reboot the app. Wouldn't be
> better to be more specifc in the case of "SystemFailure" so the app can
> handle those differently?

It's in bug 1099390.

The best way to handle a "SystemFailure" is to try again in a bit. AOSP seems to wait 500ms for the camera service (provided by mediaserver) to become available.

The "HardwareReleased" reason doesn't need to be handled beyond being a possible confirmation that release() succeeded.

Bug 1099390 also adds a "NotAvailable" reason, which is used in bug 1099381 to handle the bizarre case in bug 1099375 where, while getUserMedia() has the user-facing camera, we seem to be able to open the not-user-facing camera and configure it, but the preview fails to start. The best way to handle this is to prompt the user to close "the other app" using the camera and try again.

Comment 23

[Diego Marcos [:dmarcos]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/50be808cea194641ac520ee96b760c5d6bb9bae4

Comment 24

[Diego Marcos [:dmarcos]]

Comment on attachment 8524904 [details] [review]
Alternative Pull Request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1090501
[User impact] if declined: The camera app can get to an unusable state if media server goes down: Black preview and the application must be rebooted to recover from the error
[Testing completed]: Unit tests to account for the changes
[Risk to taking this patch] (and alternatives if risky): Low. We just added a callback to the camera API to detect when the camera closes due to a failure.
[String changes made]: None

Comment 25

[David Flanagan [:djf]]

(In reply to Tiffanie Shakespeare from comment #19)
>
> David - what's this about an untranslated string? Is it new? We need to make
> sure that this gets translated in the next release. Shall I open a bug?

There were to 2.1+ bugs in camera and video where if a Hello (webrtc) call was in progress, these two apps would not run. The only thing we could do is display a message saying basically "the hardware is in use". But it was so far past the late-l10n deadline that we ended up landing english-only error messages with the hope that our partners will be able to get late localizations in. Stephany knows all about it.

Comment 26

[Tiffanie Shakespeare [:tif]]

cool, thanks! I'll follow up with her and we'll make sure we get that fixed. Thanks David! :)

Comment 27

[Diego Marcos [:dmarcos]]

We ended up going with a solution that doesn't require new strings

Comment 28

[sarsenyev]

Verified the issue is fixed on 2.2 Flame

The system restars mediaserver automatically

"Flame 2.2

Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141120040205
Gaia: 1abe09b4925547699dfdb2d358aed019137c3aa6
Gecko: 6ce1b906c690
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0"
===================================================================
Leaving "verifyme" for 2.1 and 2.0 patch uplift

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

v2.1 https://github.com/mozilla-b2g/gaia/commit/2923ca8aa6763fe4b9a7d1894bf9510ae8df0e14

Comment 30

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame v2.1.MP4

This issue has been verified successfully on Flame 2.1.
See attachment: Verify_Video_Flame v2.1.MP4
Reproducing rate: 0/10

Flame v2.1 version:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 31

[Mike Lien[:mlien]]

set 2.0? to see if it needs to be uplifted to 2.0

Comment 32

[Rachelle Yang [:ryang][[e-mail]]]

no partner is requested this on 2.0; suggesting to denominate it for now...

Comment 33

[Mike Lien[:mlien]]

refer to comment 32, mark v2.0 status as wontfix