All users were logged out of Bugzilla on October 13th, 2018

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

VERIFIED FIXED

Status

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: mikeh, Assigned: dmarcos)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 wontfix, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: eta 11/19)

Attachments

(3 attachments)

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.
[Blocking Requested - why for this release]: blocking a blocker (bug 1094070).
blocking-b2g: --- → 2.1?
(Assignee)

Updated

4 years ago
Assignee: nobody → dmarcos
Flags: needinfo?(mhabicher)
(Assignee)

Comment 2

4 years ago
Created attachment 8521661 [details] [review]
Pull Request

Unit tests pending
Attachment #8521661 - Flags: review?(jdarcangelo)
(Assignee)

Updated

4 years ago
Attachment #8521661 - Flags: feedback?(mhabicher)
Comment on attachment 8521661 [details] [review]
Pull Request

LGTM.
Flags: needinfo?(mhabicher)
Attachment #8521661 - Flags: feedback?(mhabicher) → feedback+

Comment 4

4 years ago
Blocking: Because the bug that this blocks on causes camera screen to go black: https://bugzilla.mozilla.org/show_bug.cgi?id=1094070#c9
blocking-b2g: 2.1? → 2.1+
(Assignee)

Comment 5

4 years ago
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?
Flags: needinfo?(mhabicher)
(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.
Flags: needinfo?(mhabicher) → needinfo?(aosmond)
(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).
Flags: needinfo?(aosmond)
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?
Flags: needinfo?(dmarcos)
(Assignee)

Comment 9

4 years ago
I updated the PR to show a dialog when media server is not available before rebooting the application
Flags: needinfo?(dmarcos) → needinfo?(jdarcangelo)
(Assignee)

Comment 10

4 years ago
Is it possible to have the reason why onClose was called as a callback argument?
Flags: needinfo?(mhabicher)
Flags: needinfo?(aosmond)
(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.
Flags: needinfo?(mhabicher)
Flags: needinfo?(aosmond)
See Also: → bug 1099390

Comment 12

4 years ago
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
Whiteboard: eta 11/19
(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.
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 14

4 years ago
(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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 15

4 years ago
(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?
Flags: needinfo?(mhabicher)
(Assignee)

Comment 16

4 years ago
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
Flags: needinfo?(swilkes)
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.
Flags: needinfo?(dmarcos)

Comment 18

4 years ago
Flagging Tif, who is UX for Media.
Flags: needinfo?(swilkes) → needinfo?(tshakespeare)
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?
Flags: needinfo?(tshakespeare) → needinfo?(dflanagan)
(Assignee)

Comment 20

4 years ago
Created attachment 8524904 [details] [review]
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.
Flags: needinfo?(dmarcos)
Attachment #8524904 - Flags: review?(jdarcangelo)
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.
Flags: needinfo?(jdarcangelo)
Attachment #8524904 - Flags: review?(jdarcangelo) → review+
(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.
Flags: needinfo?(mhabicher)
(Assignee)

Comment 23

4 years ago
Landed on master:

https://github.com/mozilla-b2g/gaia/commit/50be808cea194641ac520ee96b760c5d6bb9bae4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

4 years ago
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
Attachment #8524904 - Flags: approval-gaia-v2.1?(fabrice)
(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.
Flags: needinfo?(dflanagan)
cool, thanks! I'll follow up with her and we'll make sure we get that fixed. Thanks David! :)
(Assignee)

Comment 27

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

Updated

4 years ago
Keywords: verifyme

Comment 28

4 years ago
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Updated

4 years ago
Attachment #8524904 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
Created attachment 8527428 [details]
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
status-b2g-v2.1: fixed → verified

Comment 31

4 years ago
set 2.0? to see if it needs to be uplifted to 2.0
blocking-b2g: 2.1+ → 2.0?
no partner is requested this on 2.0; suggesting to denominate it for now...
blocking-b2g: 2.0? → ---

Comment 33

4 years ago
refer to comment 32, mark v2.0 status as wontfix
status-b2g-v2.0: affected → wontfix
Keywords: verifyme
Attachment #8521661 - Flags: review?(jdarcangelo)
You need to log in before you can comment on or make changes to this bug.