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
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?
Whiteboard: [fromAutomation] → [fromAutomation][xfail]
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 :)
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)
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-
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 :)
Created attachment 8488704 [details] [review] pull-request (master)
Comment on attachment 8488704 [details] [review] pull-request (master) Nice! Lots of horrible hacks erased :)
Attachment #8488704 - Flags: review?(wilsonpage) → review+
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
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
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)
(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+
status-b2g-v2.1: affected → fixed
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.