[B2G][Camera][Gaia] Handle auto focus error callbacks, restore flash/focus modes correctly

RESOLVED FIXED in 2.1 S3 (29aug)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

({branch-patch-needed})

unspecified
2.1 S3 (29aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.0 affected, b2g-v2.1 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

STR:

1) Take a picture where by the application will attempt to focus first (F1). Application will wait for an onSuccess callback before proceeding with the actual take picture.

2) Quickly interrupt the focus (F1) above by triggering a touch to focus (F2). F1 will generate an onError callback with AutoFocusInterrupted status which is ignored.

3) F2 generates an onSuccess callback but nothing happens aside from the focus rings appearing/disappearing.

4) UI controls are now locked because F1 did not receive the onSuccess callback it expected. User must exit and reenter the application. No picture will be taken.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Posted file Gaia pull request, v5 (obsolete) —
Attachment #8476017 - Flags: review?(dmarcos)
Posted patch Gecko debug improvement, v1 (obsolete) — Splinter Review
Attachment #8476018 - Flags: review?(mhabicher)
Comment on attachment 8476018 [details] [diff] [review]
Gecko debug improvement, v1

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

r+ with the nit fixed.

::: dom/camera/DOMCameraControl.cpp
@@ +738,3 @@
>      , mMessage(aMessage)
>    { }
>    

nit: extra whitespace on line.
Attachment #8476018 - Flags: review?(mhabicher) → review+
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Cancel review, actually this breaks the display of interrupted auto focusing (at the same time as fixing the UI block). Might need a different behaviour for take picture (i.e. should continue if interrupted) vs the rest (i.e. touch to focus should ignore interrupted).
Attachment #8476017 - Flags: review?(dmarcos)
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Updated to handle interruptions properly (and checked for callers to hopefully ensure I captured all use cases this time :)).
Attachment #8476017 - Attachment description: Gaia pull request, v1 → Gaia pull request, v2
Attachment #8476017 - Flags: review?(dmarcos)
Duplicate of this bug: 1055290
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Thanks Andrew! It looks great. We need to add unit tests though
Attachment #8476017 - Flags: review?(dmarcos) → review-
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Added tests for the focus APIs.
Attachment #8476017 - Attachment description: Gaia pull request, v2 → Gaia pull request, v3
Attachment #8476017 - Flags: review- → review?(dmarcos)
NO-jun mentioned ew hit this consistently on KK and JB https://bugzilla.mozilla.org/show_bug.cgi?id=1055290 and said this is regression and was working before in 2.0, so blocking on this given the behaviour.
blocking-b2g: --- → 2.0+
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

This is looking great. A little bit more polish and we can land. Made some comments on GH. One of the changes is not covered by unit tests
Attachment #8476017 - Flags: review?(dmarcos) → review-
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Fixed the other issue I mentioned with respect to flash. I think we should still handle the error cases properly. While I can't see the interruptions which were the original cause of the issue, in theory auto focus can fail for other reasons.
Attachment #8476017 - Attachment description: Gaia pull request, v3 → Gaia pull request, v4
Attachment #8476017 - Flags: review- → review?(dmarcos)
Depends on: 1042072
New STR for the issues tracked by this since one can no longer lock the UI easily (although it can still happen if an error occurs when focusing, a test case was written for this scenario):

Flash mode forgotten:
1) Set flash mode to on or auto
2) Touch-to-focus
3) Change flash mode before focus completes
4) Old flash mode restored, UI now incorrect

Flash mode stuck to off:
1) Set flash mode to on or auto
2) Touch-to-focus
3) Interrupt touch-to-focus
4) Wait for focus to complete
5) UI now incorrect for flash mode setting
6) Take a picture, flash will not fire

Focus mode incorrect:
1) Switch to picture mode; focus mode is continuous-picture
2) Touch-to-focus; focus mode is auto
3) Switch to record mode before or after focus completes but in less than 10 seconds
4) Focus mode is continuous-video
4) Wait 10 seconds
5) Focus mode is continuous-picture (in record mode!)
No longer depends on: 1042072
Summary: [B2G][Camera][Gaia] Interrupting auto focus when taking a picture causes controls to lock → [B2G][Camera][Gaia] Handle auto focus error callbacks, restore flash/focus modes correctly
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Add fix for restore focus mode restoration (see STR above).
Attachment #8476017 - Attachment description: Gaia pull request, v4 → Gaia pull request, v5
Attachment #8476017 - Flags: review?(dmarcos) → review?(jdarcangelo)
Original issue found in bug 1042072 appears to be resolved by this as well (complex interactions with between the driver, flash and focus modes I can only presume).

STR:
1) Turn on flash in picture mode
2) Touch-to-focus, flash turns on
3) Touch-to-focus, interrupting previous, flash remains on
4) Focus completes, flash remains on
Comment on attachment 8476017 [details] [review]
Gaia pull request, v5

Conditional R+ based on our IRC conversation about cleaning up the commits in this PR. Please omit unrelated commits from the PR and squash your commits into a single one before landing. Otherwise, looks good. Thanks for adding the unit tests that Diego requested!
Attachment #8476017 - Flags: review?(jdarcangelo) → review+
Updated pull request with all of the commits squashed into one.
Attachment #8476017 - Attachment is obsolete: true
Attachment #8480277 - Flags: review+
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/43c2ec709cc4dd5b902f65d9c824719732e5e84e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
Needs rebasing for v2.0 uplift.
Flags: needinfo?(aosmond)
[Blocking Requested - why for this release]:

I believe this should be changed to be blocking for 2.1 only. While it *does* resolve the original problem of the UI locking up, another change in bug 1056347 was introduced which made this same scenario unreproducible in 2.0/2.1. Its still there in theory (along with the other less serious issues I've resolved in the patch for this bug) but in practice users should not encounter it. (Additionally, while I've completed/tested the backporting, it relied on a number of other changes that were not backported to 2.0, and would make future integrations a pain.)
blocking-b2g: 2.0+ → 2.1?
Flags: needinfo?(aosmond)
Landed on master prior to 2.1-branching; clearning 2.1-nom as it is not required.
blocking-b2g: 2.1? → ---
This issue has been verified successfully on Flame2.1
Verify video:"verify_1056173.mp4".
**Video step is follow comment 3

Flame2.1 build:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141202.034824
FW-Date         Tue Dec  2 03:48:34 EST 2014
Bootloader      L1TC00011880
You need to log in before you can comment on or make changes to this bug.