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

Status: VERIFIED FIXED

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

Description

[Robert Chira [:RobertC]]

Attachment: 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

Comment 1

[Viorela Ioia [:viorela]]

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

[Zac C (:zac)]

ni? Wilson

Comment 3

[Jason Smith [:jsmith]]

[Blocking Requested - why for this release]:

QA automation blocker - this causes a permafail in existing on device camera test.

Comment 4

[Hema Koka [:hema]]

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

Comment 5

[Wilson Page [:wilsonpage]]

Passing over to Justin :)

Comment 6

[Justin D'Arcangelo [:justindarc]]

Attachment: 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 7

[Robert Chira [:RobertC]]

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.

Comment 8

[Wilson Page [:wilsonpage]]

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.

Comment 9

[Wilson Page [:wilsonpage]]

Attachment: 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 :)

Comment 10

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Comment 11

[Wilson Page [:wilsonpage]]

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

Nice! Lots of horrible hacks erased :)

Comment 12

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/7896a5122aeb6b83038b115db255f55f70c9f07f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.1 approval on this patch when you get a chance.

Comment 14

[Justin D'Arcangelo [:justindarc]]

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]:

Comment 15

[Justin D'Arcangelo [:justindarc]]

(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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/9356107a50b941da9e818d3581fd57c8067bd5df

Comment 17

[Sue]

Attachment: 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