[v2.2][v2.1] Rapidly tapping record after switching from picture to camera mode will take photo instead of video

VERIFIED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::Camera
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: RobertC, Assigned: justindarc)

Tracking

({qablocker, regression})

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
qablocker, regression
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.1 verified, b2g-v2.2 fixed)

Details

(Whiteboard: [fromAutomation][xfail])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8487892 [details]
logcat.txt

If you switch from photo to camera mode the rapidly tap on the capture button, the app will take a picture instead of recording video.

This issue can be reproduced with both manual and automated tests. With automation we get a reproduction rate of 5 out of 5.
With manual testing it is hard to reproduce because it requires perfect timing.

STR:
1. open camera app
2. switch to video mode
3. rapidly tap record

Expected result:
A video should start recording

Actual result:
A photo is taken (attached logcat from when the steps were done manually)

Automated test link: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/camera/test_camera_capture_video.py

Last working
Gaia      bedd277bf39980bb6420bd83333448d4314a7451
Gecko     https://hg.mozilla.org/integration/b2g-inbound/rev/a74c178ec722
BuildID   20140909090257
Version   35.0a1
ro.build.date   Fri Jun 27 15:57:58 CST 2014
ro.bootloader   L1TC00011230
ro.build.version.incremental   110

First broken
Gaia      8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7
Gecko     https://hg.mozilla.org/mozilla-central/rev/6b3948d3413a
BuildID   20140909155914
Version   35.0a1
ro.build.date   Fri Jun 27 15:57:58 CST 2014
ro.bootloader   L1TC00011230
ro.build.version.incremental   110

This issue might be caused by the changes done in https://github.com/mozilla-b2g/gaia/commit/5a23b2065e8669a855a66464837b05ced1f51f22
I've been able to reproduce this failure manually on latest v2.1 too. I need to tap the capture button really quick after switching to video in order to take a photo.
Repro rate: 3 out of 5 times. 

Device firmware (date)     03 Sep 2014 00:36:26
Device firmware (incremental)     eng.cltbld.20140903.033618
Device firmware (release)     4.3
Device identifier     flame
Gaia date     10 Sep 2014 16:29:54
Gaia revision     d61264cd0c1f
Gecko build     20140911000225
Gecko revision     1d44dfce2e5b
Gecko version     34.0a2

Comment 2

4 years ago
ni? Wilson
Flags: needinfo?(wilsonpage)
(Reporter)

Updated

4 years ago
Summary: [v2.2] Rapidly tapping record after switching from picture to camera mode will take photo instead of video → [v2.2][v2.1] Rapidly tapping record after switching from picture to camera mode will take photo instead of video
[Blocking Requested - why for this release]:

QA automation blocker - this causes a permafail in existing on device camera test.
blocking-b2g: --- → 2.1?
Keywords: qablocker
Whiteboard: [fromAutomation] → [fromAutomation][xfail]

Updated

4 years ago
Blocks: 1061996

Comment 4

4 years ago
Blocking Reason: Regression and blocks test automation

Its too late in the day for Wilson. 

Justin, can you please help and unblock QA.

Thanks
Hema
Assignee: nobody → jdarcangelo
blocking-b2g: 2.1? → 2.1+
Passing over to Justin :)
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 6

4 years ago
Created attachment 8488140 [details] [review]
pull-request (master)

Robert: Let me know if this fixes the issue for you. When I run the integration tests on my local device with this patch, it seems to work.

Diego: Please review (one-liner to fix race condition)
Attachment #8488140 - Flags: review?(dmarcos)
Attachment #8488140 - Flags: feedback?(robert.chira)
(Reporter)

Comment 7

4 years ago
Comment on attachment 8488140 [details] [review]
pull-request (master)

I ran the test with your patch and I was still able to reproduce the issue, with both manual and automated testing.
Attachment #8488140 - Flags: feedback?(robert.chira) → feedback-
Attachment #8488140 - Flags: review-
I removed the line you added line as part of bug 1061996 as it can leave the controls permanently disabled. This is because in some edge cases the mode doesn't get changed and the 'ready' event never fires (required to re-enable the controls). We need to rely on the 'ready' and 'busy' events only otherwise things will break.

I'm guessing the root of the problem here is that in the mode change flow, camera is firing 'ready' too early, before the the entire process has finished. I think we need to be more careful where we fire 'ready' in camera.js.
Created attachment 8488531 [details] [review]
pull-request (master)

I've created an alternative patch that fires an app 'busy' event when the 'camera:willchange'. This will effectively disable all controls whilst the viewfinder is fading out, before the instruction to configure the camera is made. It was this ~300ms window that was allowing this race condition.

As 'busy' is central app event, I chose to fire it from app.js and not controllers/camera.js. My thinking was that things could get messy if we start firing 'busy' events all over our code base. Happy to change this if anyone thinks this is a bad idea though. I'm sure there are arguments to fire it from camera-controller.

The most import important thing is that we have central mechanism to declare the app 'busy' and 'ready', and that views like the Controls and HUD just obey and don't try to get clever, otherwise things will break :)
Assignee: jdarcangelo → wilsonpage
Attachment #8488140 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8488140 - Flags: review?(dmarcos)
Attachment #8488531 - Flags: review?(jdarcangelo)
(Assignee)

Comment 10

4 years ago
Created attachment 8488704 [details] [review]
pull-request (master)
Assignee: wilsonpage → jdarcangelo
Attachment #8488531 - Attachment is obsolete: true
Attachment #8488531 - Flags: review?(jdarcangelo)
Attachment #8488704 - Flags: review?(wilsonpage)
Comment on attachment 8488704 [details] [review]
pull-request (master)

Nice! Lots of horrible hacks erased :)
Attachment #8488704 - Flags: review?(wilsonpage) → review+

Updated

4 years ago
Target Milestone: --- → 2.1 S4 (12sep)
(Assignee)

Comment 12

4 years ago
Landed on master:

https://github.com/mozilla-b2g/gaia/commit/7896a5122aeb6b83038b115db255f55f70c9f07f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Please request Gaia v2.1 approval on this patch when you get a chance.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Flags: needinfo?(jdarcangelo)
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
(Assignee)

Comment 14

4 years ago
Comment on attachment 8488704 [details] [review]
pull-request (master)

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Attachment #8488704 - Flags: approval-gaia-v2.1?(fabrice)
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 15

4 years ago
(In reply to Justin D'Arcangelo [:justindarc] from comment #14)
> Comment on attachment 8488704 [details] [review]
> pull-request (master)
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> [User impact] if declined:
> [Testing completed]:
> [Risk to taking this patch] (and alternatives if risky):
> [String changes made]:

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a
[User impact] if declined: automated testing will be broken; its possible for user to take a picture in video mode and vice-versa
[Testing completed]: UI integration tests passed
[Risk to taking this patch] (and alternatives if risky): low-risk
[String changes made]: none
Attachment #8488704 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+

Updated

3 years ago
Status: RESOLVED → VERIFIED

Updated

3 years ago
status-b2g-v2.1: fixed → verified

Comment 17

3 years ago
Created attachment 8525818 [details]
video of issue verify

This issue has been successfully verified on Flame 2.1
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.1 versions:
Gaia-Rev        1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152
Build-ID        20141119001205
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141119.035246
FW-Date         Wed Nov 19 03:52:56 EST 2014
Bootloader      L1TC00011880
You need to log in before you can comment on or make changes to this bug.