1066885 - [Camera] Record button flashes blue initially; video does not record, produces error message

Status: RESOLVED FIXED

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

Description

[Oliver Nelson [:oliverthor]]

Attachment: log.txt

Occurred after full flash to build:
20140911064110

on base image:
v123

Description:
When the user attempts to record a video, they may encounter the capture button flashing blue (similar to how the camera capture button would behave), before resulting in an error message display of the video recording being unavailable.
This issue was only seen once, with uncertain repro steps.Issue appeared unexpectedly when attempting to capture video (directly after capturing 3+ photos) to produce media for a profile prior to an OTA case.

Repro Steps:
1) Update a Flame device to BuildID: 20140911064110 
2) Change Settings:
	- Settings
		+ Connect to WiFi
		+ Battery settings: Turn on at 15% battery left
		+ Display: Screen Timeout set to 5 minutes
		+ Sound: Change randomly
		+ Set Screen Lock: 1111
3) Open Camera
4) Capture 3+ photos.
5) Switch to video
6) Attempt to capture
  
Actual:
Video capture button flashes blue; produces error that states video is disabled.

Expected: 
Video capture begins.
  
  
Environmental Variables:
Device: Flame Master (319mb)
Build ID: 20140911064110
Gaia: e3b9d0d6516177636965d97c63c60981a24a0662
Gecko: 98ea98c8191a
Version: 35.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
  

Repro frequency: 2/3
See attached: 
logcat
youtube- http://youtu.be/8-mlqgAvaUY
potentially related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1057980

Comment 1

[Peter Bylenga [:PBylenga]]

Steps-wanted to see if we can reduce the STRs to a more reproductive state.

Comment 2

[Cody Roesch [:croesch]]

This bug repro's on Flame KK builds: Flame 2.2

Actual Results: Camera error when trying to record a video "Video not recorded. An error prevented Camera from recording the video."

STR:
1. Launch the Camera app.
2. Switch to camera and quickly tap record before the camera app is ready to record.
3. Repeat step 2 if bug does not occur.

Actual Results: Error is produced.

Repro Rate: 5/6

Environmental Variables:
Device: Flame Master
BuildID: 20140915091417
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: d08c31df0c1a
Version: 35.0a1 (Master) 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

-----------------------------------------------------------------
-----------------------------------------------------------------

This bug does NOT repro on Flame kk build: Flame 2.1, Flame 2.0, Flame 1.4 Base, OpenC 2.2

Actual Result: No error is seen when taking a video.

Repro Rate: 0/10 attempts

Environmental Variables:
Device: Flame 2.1
BuildID: 20140915073605
Gaia: 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko: 2697f51cfe95
Version: 34.0a2 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------
Enviromental Variables:
Device: Flame 2.0
BuildID: 20140914162307
Gaia: 7edd3b0b9f65c3dde235c732d270e43e055a1254
Gecko: 13e04ab68621
Version: 32.0 (2.0)
Firmware: v165
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------------
Device: Flame 1.4
BuildID: 20140814202332
Gaia: 129211661489feb60bbd6772a44081d23b374f17
Gecko: 
Version: 30.0 (1.4)
Firmware: v165
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0
-----------------------------------------------------------------
Environmental Variables:
Device: Open_C Master
BuildID: 20140915091417
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: d08c31df0c1a
Version: 35.0a1 (Master) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 3

[Joshua Mitchell (Inactive)]

[Blocking Requested - why for this release]: broken functionality in a major feature (Camera). 

Notes: When the error message is encountered (claiming an error occurred) the device is actually still recording video in the background. 
       This repro rate seemed lower for me - about 25% 
       This bug may have a higher repro rate when the device is plugged in
       

not calling for regression window based on low repro rate / difficulty to repro consistently

Comment 4

[Hema Koka [:hema]]

Lets investigate whats going on (I was not able to reproduce based on STR in comment 2, may be I am not fast enough). 

Justin or Wilson, please investigate. 


Thanks
Hema

Comment 5

[Wilson Page [:wilsonpage]]

Unable to reproduce 10/10 on 319mb Flame.

Comment 6

[Joshua Mitchell (Inactive)]

qa-wanted: to test again in the latest 2.2 (master), KK base Flame. If it repros please update STR and repro rate if any changes.

Comment 7

[Cody Roesch [:croesch]]

Redoing branch checks because I was able to repro this on builds I was unable to last check on 319mb. The STR's are exactly how you do it. Tap the camera to video button and quickly keep tapping the video capture button. Error occurs.

***NOTE: Only branch I could not get it on was the 2.1 KK branch. 0/20 attempts seems enough to be able to get it to occur compared to the other branches.
***NOTE: In 2.0, the error window appears but instead of a message, I just see a period with no text. Attached screenshot.

Repro Rate: 5/12
Environmental Variables:
Device: Flame Master KK
BuildID: 20140917212258
Gaia: d37950eb09e28aa18d0e01df9ff90574bd4337e0
Gecko: 426497473505
Version: 35.0a1 (Master) 
Firmware Version: L1TC00011650
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
-----------------------------------------------------------------
Repro Rate: 1/12
Environmental Variables:
Device: Flame 2.0 KK
BuildID: 20140918082321
Gaia: 31434a3949556171f3565ca47ac2b44e810e95e6
Gecko: 5cf783171d5c
Version: 32.0 (2.0) 
Firmware Version: L1TC00011650
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------------
Repro Rate: 4/12
Environmental Variables:
Device: Flame 1.4 KK
BuildID: 20140814202332
Gaia: 129211661489feb60bbd6772a44081d23b374f17
Gecko: 
Version: 30.0 (1.4)
Firmware: v165
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0
-----------------------------------------------------------------
Repro Rate: 5/12
Device: Open_C 2.2
BuildID: 20140917212258
Gaia: d37950eb09e28aa18d0e01df9ff90574bd4337e0
Gecko: 426497473505
Version: 35.0a1 (2.2)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

-----------------------------------------------------------------
-----------------------------------------------------------------

Repro 0/20 attempts
Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20140918073020
Gaia: 379e68fe729a684fa2fcddb30ea1e65508db73e1
Gecko: 44eec4673c25
Version: 34.0a2
Firmware Version: L1TC00011650
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 8

[Cody Roesch [:croesch]]

Attachment: Flame 2.1 KK.png

Comment 9

[Cody Roesch [:croesch]]

Sorry the screenshot is for 2.0 not 2.1. :(

Comment 10

[Joshua Mitchell (Inactive)]

[Blocking Requested - why for this release]: just swapping from 2.1 to 2.0

It seems we are looking at some really low repro rates - this might be affecting some of the 'no-repro' posts.

Comment 11

[Justin D'Arcangelo [:justindarc]]

From the video in comment 0 ( http://youtu.be/8-mlqgAvaUY ), you can see that other gfx glitch where the capture button becomes a square during the animation. I've seen that happen a few times now, but I've never been able to nail down the STR to consistently reproduce it. This seems like there may be some underlying gecko issue going on here with the CSS.

Comment 12

[Hema Koka [:hema]]

2.0 Bug triage is on TAMs from what I hear, adding Wesly

Comment 13

[Wesly Huang (EPM)]

Partner has no comment about this issue. Per triage pls Bhavana help judge, tks.

Comment 14

[Dietrich Ayala (:dietrich)]

Triage group reviewed, blocking+ due to regression, the fact that it occurs randomly across all versions and multiple devices, and the terrible scenario described in comment #3 where it *continues recording*. That's bad in multiple ways.

Comment 15

[David Flanagan [:djf]]

Taking this to see if I can reproduce or move it forward at all.

Comment 16

[David Flanagan [:djf]]

It sounds to me like there are possibly two or three different bugs being described here:

1) There are some visual issues (flash of blue in record button, flash of square in record button), as shown in the video.

2) The video does not actually record, and we see: "An error prevented the camera from recording the video" as shown in the attached video.

3) Sometimes we see an error screen with no text but just a period, as shown in the attached screenshot.

For #3, I'm pretty sure that the problem is in camera/js/lib/camera/camera.js:1068, where we are assigning req.onerror=onRecordingError. That would pass an event object to a function that expects a string, and the stringified event would cause a localization error that would result in no text being displayed.  

I'll fix that, but it is a failure mode that really just should not happen. So that particular blank screen issue was probably a side effect of a device storage bug we were having (which, I'm told, has since been fixed.)

For #2, that particular error message could be caused by three different failures in camera.js, so I'll add some logging and try to reproduce the bug. If we can see what exactly is failing we can figure out how to fix it.

For #1, I've got no idea. But it seems like the blocking issue here is #2, so I'll focus on fixing that and hope that that provides some insight into #1.

Comment 17

[David Flanagan [:djf]]

I've flashed my Flame with KK build 180, and today's flame-kk nightly.  I have 319mb memory.

Here's what I see:

1) If I switch to video mode and tap on the shutter button as fast as I can, it is pretty easy to see the square shutter button bug. That seems to happen every 10 to 20 taps. I would guess it is a gecko bug, but I have not looked at the CSS.

2) In order to reproduce the error message, I find that I need to turn off the flash and then tap rapidly on the shutter button. (I have no idea why that would make a difference, but that's what I'm seeing right now) But this bug is hard to observe because the rapid tapping also dismisses the error message, so it goes by in an instant.  Once the error message has happened and been dismissed, none of the controls work: the camera is wedged in a state where all of the buttons have been disabled (I think).  Going to the homescreen and returning to the camera is enough to make the controls work again.

Comment 18

[David Flanagan [:djf]]

The error message displayed to the user is coming from the onError handler at camera.js:987. That is the error callback for the gecko startRecording() function on the gecko camera object.  

Inserting logging into onError, I see that gecko is reporting "StartRecordingInProgress" as the error message.

I'll talk to Mike about what this means, but I guess it means that there is already a pending call to startRecording.  The safest thing in that case might be to ignore the error completely and wait for the pending call to complete.  Ideally, though, we would be better about disabling the shutter button immediately so that concurrent calls just can not happen.

Comment 19

[David Flanagan [:djf]]

Andrew confirms that the "StartRecordingInProgress" error message means that there is a pending startRecording call that has not yet called its error or success callback.

So it seems to me that this issue is happening because we are not disabling the shutter button quickly enough. (Or are not converting it from a start recording to a stop recording button). I'll try to sort out the sequence of events that occurs when the user taps on the button to start recording.

If we can't figure that out, a temporary fix might be to just ignore the StartRecordingInProgress error.

Comment 20

[David Flanagan [:djf]]

Okay, so the click event handler for the capture button uses js/lib/debounce.js, which means that if the button is tapped and then tapped again within 300ms the second tap won't have any effect except to extend the wait by another 300ms. So in theory if you tap the button every 200ms, none of the taps after the first will ever do anything.  I'm not sure that is exactly the behavior we want... but I don't think it is behind the bugs here.  It might be possible to resolve this bug by changing the 300ms constant to 500ms or something.

Anyway, if it has been more than 300ms since the last tap, the button emits a "click:capture" event. (this is from views/controls.js)

The click:capture event is handled in only one place, in controllers/controls.js:onCaptureClick().

onCaptureClick() calls a method to change the visual appearance of the button, and emits a 'capture' event.

The capture event is handled in only one place, in controllers/camera.js in the capture() method at line 150. That method calls the capture method in js/lib/camera/camera.js, and that method (assuming we're in video mode) calls toggleRecording in the same camera library.  toggleRecording() calls this.get('recording') to find out whether the camera is currently recording or not, and then calls startRecording() or stopRecording() to change the state.  I note here that toggleRecording does not itself set this state variable, so perhaps it is possible that two calls to toggleRecording very close together could call startRecording() twice, which seems to be what was causing our problem...

Assuming that we were not recording when we started, the tap on the capture button has now reached startRecording() in the camera library.  The startRecording function() does some async stuff and does not set the recording state variable until that async stuff is done. So this does mean that there is a window of time when subsequent calls to toggleRecording would both call startRecording, which would presumably lead to the StartRecordingInProgress error we're seeing.

However, startRecording does synchronously call the busy() method, which emits an event that is supposed to disable buttons. So if that busy() method is working correctly, then the capture button should become disabled and we should not be able to tap on it again to create that extra toggleRecording() call.  Before I investigate what busy() is actually doing, I'll note that will probably not work to set the recording state variable synchronously in startRecording, because then we might end up calling stopRecording before recording has actually started.  So perhaps the recording state variable should not be a boolean, but a three-state variable that can be true, false, and 'toggling'. If it is in the toggling state, then calls to toggleRecording should be ignored.  And if it is true or false, then toggleRecording should set it to toggling before calling start or stopRecording. That would probably patch the bug.

But let's try going a bit deeper to see what the busy() method is actually doing... Calling busy() in the camera library object emits a 'busy' event. The camera controller listens for busy events from the camera library, and fires a 'busy' event on the app.  Various things listen to the app's busy event. In the controls controller, it calls onCameraBusy(), which calls the disable() method on the controls view.

The disable() method seems to set data-disabled=true on the controls view, and also add the class 'disabled' to its class list.  In the css for the controls, the disabled class sets pointer-events:none for the disabled elements but does not change their visual style at all to indicate that they are disabled.  Also (and I think this is a separate bug) setting pointer-events:none on these elements makes them transparent to touches, which means that the events go through and are registered on the containing elements. I suspect this is why we have other bugs like moving the touch focus region behind the shutter button when the shutter is disabled.

I think maybe we're getting somewhere here... I don't know for sure how low-level event processing works, but if the user is tapping really fast on the buttons, I imagine that we might be queueing up multiple events before they are all processed.  When the first tap is processed, it can set pointer-events:none to prevent future events from being generated for that event, but maybe it doesn't stop already queued events from being processed, and these could go through and result in multiple calls to startRecording.

Maybe the disable() method on the view could actually use the HTML disabled attribute, (or does that only work for form elements?) Or maybe simpler and safer, the onButtonClick() method could check data-disabled and discard events when the button is disabled.

(Hmm. I wonder if removing the debounce code in the controls view would make this bug easier to reproduce. If so, that would help us to find a robust fix that is not dependent on the debounce timing.)

As for the square capture button that sometimes appears, these lines in controls.css look suspicious to me:

.controls.mode-video.capture-active .capture-button .center:before {
  border-radius: 3px;
  background-color: white;
}

Maybe it is not an actual square, but a square with 3px rounded edges? Since the square is relatively easy to reproduce, changing this 3px to 15px or something should show me whether that is what I'm seeing. The capture-active class is added to the view when the shutter button is pressed. I don't know how multiple presses would affect anything, but this is the first place I'd start investigating.

Comment 21

[David Flanagan [:djf]]

Nevermind about my border-radius 3px suspicions above. I now see that is for the little white square inside the record button when it is active. So that has nothing to do with the overall square button.

Removing the debounce() call in the camera controls seems to make it a little easier to make the error message occur when startRecording is called twice in a row. It still isn't every time, but removing it would probably be a good way to test a fix.

Wilson, Diego, or Justin: your feedback on my findings in comment #20 would be helpful here. What do you think of my idea to check data-disabled in onButtonClick? (And if any of you want to take this bug and finish it up, that would be great!)

Comment 22

[Justin D'Arcangelo [:justindarc]]

David: I may have some bandwidth free up tomorrow afternoon if no one else wants to finish this up. I don't have the code in front of me right now, but your analysis sounds correct (from what I remember at least). One thing that was suspiciously absent though was that I seem to remember at some point having a 1000ms wait period between startRecording() and stopRecording(). This was mainly to avoid getting an invalid recording (videos seem to need to be at least 1 second long), but also to avoid getting caught in this in-between state you're seeing. Either way, it sounds like your idea of making a 3-state variable for recording status might cure the problem.

Also, as for the app events (using evt.js), their event handlers are called synchronously. So, I don't think it should be possible for toggleRecording() to get called twice before the `recording` state can get updated. Unless, of course, we're waiting on the async CameraControl API before updating the state... in which case we come back to the idea of making it a 3-state variable.

Comment 23

[David Flanagan [:djf]]

I know the app events are synchronous, but I'm not certain that the touch events that get turned into click events that trigger toggleRecording() are. There is an event queue, after all, and I think that multiple taps on the capture button could be cued up before the touch-events:none property is set. It is pretty clearly getting called twice, somehow, despite the debounce code and the disabled class setting touch-events:none.

Also note that the recording state is not updated synchronously. Once a click event arrives, things go synchronously to startRecording. But that method is async and does not update the recording state synchronously. So if toggleRecording() is called right away again, startRecording() will be called

Comment 24

[Wilson Page [:wilsonpage]]

It seems this is the biggest area of Camera app's weakness. We have state in three places:

- UI
- Settings
- Camera

Things can get out of sync largely due to:

- Async calls to camera having a 'pending' stage whereby the state is neither true nor false.
- `pointer-events: none` perhaps not taking effect instantly. I wonder if we can produce a reduced test case case to prove that 'click' events can sometimes register after `pointer-events: none` has been set.

SOLUTION

I think we should deal with this in a way that we can use to apply to other parts to the app, as button bashing will always be a problem.

I suggest that we should have an app 'busy' boolean that we can check in places like this. So before we fire a 'capture' event, we should first check 'is the camera/app busy', if it is, we should just `return`.

onCaptureClick = function() {
  if (app.get('busy')) return;

  ...
}

This way we're not relying on UI state to determine the underlying issue, which is: 'The camera is busy'. If this works, David is right, we may be able to remove the debounce wrapper, which is a less accurate way of preventing a capture request being made when the camera is currently in the middle of processing one.

Comment 25

[David Flanagan [:djf]]

(In reply to Wilson Page [:wilsonpage] from comment #24)
> It seems this is the biggest area of Camera app's weakness. We have state in
> three places:
> 
> - UI
> - Settings
> - Camera

I'd go further and argue that the problem isn't so much that the state is spread across various modules, but that the state handling and transition logic is spread across multiple controllers. I think it might be cleaner if there was a single (large!) controller class that handled all events and state transitions. Given the complexity of the camera API and of the app's UI, the most robust way to do this would probably be with a state machine.

> 
> Things can get out of sync largely due to:
> 
> - Async calls to camera having a 'pending' stage whereby the state is
> neither true nor false.
> - `pointer-events: none` perhaps not taking effect instantly. I wonder if we
> can produce a reduced test case case to prove that 'click' events can
> sometimes register after `pointer-events: none` has been set.
> 

Maybe there is a platform bug lurking there. But in any case, I think that you need to stop using pointer-events for other reasons. Currently when the buttons are disabled, tapping them moves the focus region. I've already filed a bug about this, and I wouldn't be surprised to see a partner escalate it to blocking status. By using pointer-events:none, you lose the opportunity to call stopPropagation on the touch events and prevent them from being handled by the viewfinder controller.

> SOLUTION
> 
> I think we should deal with this in a way that we can use to apply to other
> parts to the app, as button bashing will always be a problem.
> 

Agreed.

> I suggest that we should have an app 'busy' boolean that we can check in
> places like this. So before we fire a 'capture' event, we should first check
> 'is the camera/app busy', if it is, we should just `return`.
> 

You already have that boolean in the camera library, and it would be easy to add it to the app.

I'm not convinced that a single true/false state variable for busy might not be enough.  For it to work, you have to be sure that every operation that sets busy to true eventually (on success or failure) restores the flag to false again. And you have to be sure that it is never possible for two of these operations to occur concurrently.  Otherwise, the first operation might set the flag to true, then the second operation sets it to true again, then one of them completes and sets the flag to false, but the other operation is still pending and the camera is still busy, but its state variable indicates that it is not busy.

If the operations that use the busy flag result only from user events that can be cancelled, then things are okay. We can ignore extra taps on the shutter button when the camera is busy, and prevent concurrent operations from happening.

But, what about things like the Home button and incoming calls? Those aren't things we can't ignore. We have to deal with those events even when the camera is busy. And if dealing with those events requires us to do anything that sets the busy flag, then we've got concurrent operations. In that case, then busy can't be a two state boolean, and it needs to be a counter. We increment it each time we start an operation and decrement it each time we complete the operation. And then if it is non-zero then we are busy.  But then if anything goes wrong (like the user pressing the home button prevents and operation from completing normally) we can get stuck in a non-zero state and the app would be non-responsive.

Also, in addition to questions about whether we need a boolean or a counter for the busy state variable, I also wonder whether one state variable is actually enough.  When the camera is completely busy starting or stopping recording, for example, it makes sense to disable all the on-screen controls.  But what about while the camera is busy during an autofocus cycle, for example? In that case we might want to prevent the user from zooming or moving the focus region to a different part of the screen, but still allow other on-screen controls to work.  I don't know enough about the details of the app, but I can imagine that we might want one busy state variable and another focusing state variable.   

So another way to handle state would be to have a different state variable for each async operation. One for acquiring the camera, one for switching modes, one for capturing a photo, one for starting recording, one for stopping recording, one for focusing, and so on.  Then each operation is responsible for setting and clearing its own state variable and concurrent operations won't overwrite each other. Then there could be an app.isBusy() method that checks all of the state variables.  Or UI elements that can work during some kinds of busyiness but not others can do their own checks for what is going on.

I guess what I'm describing here is beyond the scope of this bug, however.

> onCaptureClick = function() {
>   if (app.get('busy')) return;
> 
>   ...
> }
> 
> This way we're not relying on UI state to determine the underlying issue,
> which is: 'The camera is busy'. If this works, David is right, we may be
> able to remove the debounce wrapper, which is a less accurate way of
> preventing a capture request being made when the camera is currently in the
> middle of processing one.

Comment 26

[David Flanagan [:djf]]

Here's my plan for this bug:

I noted in comment #16 that there were three different bugs here: the error message, the blank error screen and the square button. The initial bug report and video show a fourth bug: that the record button turns blue as if it still in still photo mode.

- error message: following the discussion in comment #20 and comment #24, I think we can fix this by being more careful about disabling the shutter button right away.  A proper fix would probably be far reaching and touch a lot of code. So I'll try for a band-aid fix suitable for uplift to 2.0 and will file a new bug to get this fixed right.

- blank error message: I think it was a side effect of an unrelated device storage bug that is now gone, but I'll include a fix that prevents it from happening again if the device storage issue comes back somehow.

- square shutter button: it is ugly but probably not something we'd block on. I'll file a new bug for it.

- blue shutter button: I can't reproduce this, but I don't think it is something we'd block on. I'll file a new bug for it.

Comment 27

[David Flanagan [:djf]]

Hmm. If I take out the debounce and the pointer-events:none code, that makes it easy to get weird results when hammering on the buttons. And I was able to reproduce the blank error message bug. So maybe it wasn't a device storage issue. I need to trace that a bit more.

Comment 28

[David Flanagan [:djf]]

Let's ignore comment #27. It is leading me down a rabbit hole of createVideoFilepath(), and I'm guessing that the error won't be repeatable once I don't allow lots of button presses.

Comment 29

[David Flanagan [:djf]]

Turning this bug over to Justin now.

Comment 30

[David Flanagan [:djf]]

Filed bug 1073144 for the square button bug.

Comment 31

[David Flanagan [:djf]]

Filed bug 1073155 for the blue record button issue that is in the title of this bug and is visible in the video. I can't reproduce that, and it isn't something we'd block on, so spinning it off.

Comment 32

[David Flanagan [:djf]]

Assuming that we can't fix this completely in a patch that is going to be uplifted to 2.0 I have filed bug 1073161 for a proper fix in 2.2

Comment 33

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP pull-request (master)

Comment 34

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Comment 35

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v2.0)

Comment 36

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8497663 [details] [review]
pull-request (v2.0)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): none
[User impact] if declined: User can get blank dialog box error messages, app can break causing user to not be able to record video
[Testing completed]: tested on Flame KK-v180, unit tests are all green
[Risk to taking this patch] (and alternatives if risky): low -- patch was specifically made to be low-risk for v2.0
[String changes made]: none

Comment 37

[David Flanagan [:djf]]

Justin: it looks like your 2.0 patch is ready to land. I don't think that sheriffs will land it for you since it is not an uplift of a patch that went to master. 

Are you going to file a followup for 2.1 and master, or just keep this bug open and work on it here? 

It looks like your patch does not target any of the potential fixes I was considering in comment #20 and instead goes for a more global busy state to lock out events. For future reference for the followups, I think it would be worth a comment here explaining what your patch does to fix the bug and what, if anything, you plan to do differently on the other branches.  In particular, the 500ms delay after the camera indicates it is ready but before you allow events needs to be explained and justified.  I also suggest that you add some kind of FIXME comment to your patch, because the code currently reads as if it is just normal to have to wait and extra half second between photos, when in fact (at least as far as I can tell) this is a hack made out of desperation.

Since this is not a bug that is landing and being tested on master or 2.1 yet, maybe we should ask QA to try their devious best to defeat your patch before landing it?

Also: is the 500ms delay noticable to a user who wants to take a series of photos as quickly as possible? We might want to get UX signoff on the patch in that case.

Comment 38

[David Flanagan [:djf]]

Setting qawanted to ask a QA engineer to apply this patch try the camera out. I'm confident that the patch will fix the reported bug, but I worry that there might be race conditions that could cause the camera to enter a frozen state where all the UX is unresponsive.

Comment 39

[Justin D'Arcangelo [:justindarc]]

David: I have a WIP patch for master that I'm working on that I might just move to a new bug. I intentionally tried to make this patch touch the least amount of code possible since it is late for v2.0 so this is not an act of desperation as you implied. Also, the 500ms delay is hardly noticeable to the user and you can still hammer on the capture button to take photos quickly. It does, however, fix another issue we've seen where an unthrottled capture button causes the preview stream to completely freeze up and you can't even see what you're taking a picture of. This patch also resolves the issue you pointed out with the improper onRecordingError callback. The better fix for master will be to help ensure there are no race conditions with busy/ready states and they will all be coming from a single location. One of the issues on master is that multiple calls to busy() can occur before a ready() call is made which can cause the "ready" event to fire before all busy conditions are satisfied. However, the proper fix is too complex and too risky to land on v2.0 which is why this minimal patch was made.

Comment 40

[Justin D'Arcangelo [:justindarc]]

Landed on v2.0:

https://github.com/mozilla-b2g/gaia/commit/279c5ee3a2b4cfd1484196a409e05ef579de1b53

Comment 41

[Justin D'Arcangelo [:justindarc]]

Closing this since the patch for v2.0 has landed. Follow-up work will be done in Bug 1073161.

Comment 42

[Duane Dixon [:ddixon]]

(In reply to David Flanagan [:djf] from comment #38)
> Setting qawanted to ask a QA engineer to apply this patch try the camera
> out. I'm confident that the patch will fix the reported bug, but I worry
> that there might be race conditions that could cause the camera to enter a
> frozen state where all the UX is unresponsive.

Unable to reproduce issue on the latest Flame 2.0 KK build. 

Also, this issue was tested with the designated patch "bug1066885-v.20" and did NOT occur.

(319 MB memory)
Device: Flame 2.0 
Build ID: 20141001174325
Gaia: 9725d188a733a4aeebcfcf4c52d28e1ad8a2ba6f
Gecko: 05c6a4fed6bc
Version: 32.0 (2.0)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 43

[Kai-Zhen Li [:seinlin][:kli]]

https://github.com/mozilla-b2g/gaia/commit/279c5ee3a2b4cfd1484196a409e05ef579de1b53

Comment 45

[Shine(Leave from Mozilla)]

Attachment: video

This issue can't repro on woodduck 2.0
See attachment: 1066885_video.MP4
Reproducing rate: 0/20

Woodduck 2.0 biuld:
Gaia-Rev        cc690f8016b672475dc186bc7fd58aef45e684b7
Gecko-Rev       03d3ab62d5b07b915434f2d1d68495ad5915ecd2
Build-ID        20141118184148
Version         32.0

Comment 46

[Shine(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/2569/