Steps:- 1. Launch Camera and switch to camcorder 2. Press home key when it reaches 10 seconds 3. Go to Gallery and play the recorded video and observe that video will be played for 15 seconds. Actual behavior:- Recording does not stop immediately when we press home key while recording is going on. If we press home key at 10th second video will be recorded for 15 seconds when we go back and check the video in Gallery. Expected behavior:- Recording should stop immediately when we press home key

[Blocking Requested - why for this release]:

What environment are you running on? Device? Gonk/OEM build? Gecko? Gaia?

Are you running JB or KK?

(In reply to Diego Marcos [:dmarcos] from comment #3) > Are you running JB or KK? Its on v2.0 with KK

(In reply to bhargavg1 from comment #4) > (In reply to Diego Marcos [:dmarcos] from comment #3) > > Are you running JB or KK? > And device is same as Flame (8x10)

It seems camera stop recording when the app receives "visibilitychange" event from system. However this app is not prioritized so the event is not received immediately and that's the reason why a few seconds are added to the recording. could it be a system app problem that takes a long time to notify other apps?

(In reply to Manuel Casas Barrado [:mancas] from comment #6) > It seems camera stop recording when the app receives "visibilitychange" > event from system. However this app is not prioritized so the event is not > received immediately and that's the reason why a few seconds are added to > the recording. > > could it be a system app problem that takes a long time to notify other apps? We cannot do that. The app will have the visibilitychange event once the closing transition terminated and usually it takes 300ms. And it you are talking about this 300ms: If we send the active app to background immediately, you will see an empty app doing transition. It's because sending app to background also drops the rendering. This kind of issue might be resolved if https://bugzilla.mozilla.org/show_bug.cgi?id=1034001 is implemented.

QA Wanted for branch checks.

This bug bit us already in the past. When the camera is recording video and and there's an incoming call the ringtone gets registered and if the user takes the call fast a little bit of the conversation might be recorded as well. I had filed bug 1006200.

(In reply to Diego Marcos [:dmarcos] from comment #9) > This bug bit us already in the past. When the camera is recording video and > and there's an incoming call the ringtone gets registered and if the user > takes the call fast a little bit of the conversation might be recorded as > well. I had filed bug 1006200. > > *** This bug has been marked as a duplicate of bug 1006200 *** Diego, I remember discussing 1006200 and we landing the hack to avoid the issue in the camera app, is the hack not working ?

(In reply to bhavana bajaj [:bajaj] from comment #10) > (In reply to Diego Marcos [:dmarcos] from comment #9) > > This bug bit us already in the past. When the camera is recording video and > > and there's an incoming call the ringtone gets registered and if the user > > takes the call fast a little bit of the conversation might be recorded as > > well. I had filed bug 1006200. > > > > *** This bug has been marked as a duplicate of bug 1006200 *** > > Diego, I remember discussing 1006200 and we landing the hack to avoid the > issue in the camera app, is the hack not working ? The original issue was bug 995540. We were not able to stop recording on an incoming call before the ringtone starts playing. The fix is still in place and it's specific to incoming calls. We use the settings API and listen for 'private.broadcast.attention_screen_opening' that it's set by the attention screen before an alarm or a ringtone plays. The hack does not apply to this bug.

Diego, is it possible for another workaround to fix this bug for 2.0 instead of fixing bug 1006200? The concern is that fixing bug 1006200 is too risky for how far along the 2.0 release is. Thanks!

I would like to listen about workarounds by someone in the system front end team. A good temporary solution would be adding an event visibilitywillchange so applications can stop operations before leaving the screen. That additional event would not affect any of the current functionality and should be easy to implement. Kevin, Do you know who can help with it? Do you have any other ideas about workarounds?

Tim/Alive, This is biting us again in a different form, we have been putting in band-aid patches. While I agree that is too risky to fix the root cause for 2.0 (I believe this bug is tracking it: https://bugzilla.mozilla.org/show_bug.cgi?id=1006200), can we please help prioritize bug 1006200 for 2.1 so we can get rid of workarounds. Also I would appreciate your input on a workaround solution for this particular issue. It is a p2 raised by CAF, so your input here is much needed. https://bugzilla.mozilla.org/show_bug.cgi?id=1051172#c13 - Diego has a workaround suggestion here. Please review. I am reopening this bug so we can use it for another workaround for 2.0 (but I am hoping that we can fix the root of the problem once for all in 2.1) Thanks so much! Hema

We can't let long and drawn out processes slow us down. Either we continue to make a ton of hacks in the system app, or we do this the right way and simply expose some kind of event to the app. This could be in the form of visibilitywillchange, or firing it earlier and having a better API than document.hidden to check on. It seems like we should draw up a proposal, sent it off to dev-webapi, and stop spinning our wheels here. Needinfo on Jonas in case he wants to weigh in.

Moving to the same component as the tracking bug.

(In reply to Kevin Grandon :kgrandon from comment #15) > We can't let long and drawn out processes slow us down. Either we continue > to make a ton of hacks in the system app, or we do this the right way and > simply expose some kind of event to the app. This could be in the form of > visibilitywillchange, or firing it earlier and having a better API than > document.hidden to check on. > > It seems like we should draw up a proposal, sent it off to dev-webapi, and > stop spinning our wheels here. Needinfo on Jonas in case he wants to weigh > in. visibilitywillchange proposal is an API change, so this is not a window management bug. System app doesn't do anything wrong here. IMO the proposal and bug 1034001 are two different ways to go. We should not invent some API because 'this is urgent' but deprecate it later. But just let Jonas judge.

The *right* way is bug 1034001 and accompany window management fixes, probably bug 1034001. We are not looking at any application-facing changes, since this should be solved internally between Gecko and Gaia System. The currently API design leaves window management in dilemma when the UI need to be kept but the app need to respond to visibilitychange right away. As Diago points out, the scenario depicted in this particular CAF blocker is distinct from bug 995540. To remove that workaround (with a long, 3 sec delay) a fix is being worked on by applying screenshot again. However this bug is talking about 0.5sec transition out from app to home, where screenshot in Gaia scope will not help. We have also previously exhibit this behavior since 1.0 -- I am not sure why this is only raised by CAF today and why that makes this a blocker. If this is really becomes a blocker because of some other reasons, I would recommend we again workaround it by fake a visibilitywillchange event with IAC.

Adding qawanted to check the exact STR in 1.3 and 2.0 ? IF this is reproduced in 1.3 then we will not block 2.0 on it, but if this is a regression we will have to fix this.

This bug repro's using Flame 2.0 set to 319mem. Using Flame 2.0 v123 in 319mem mode, I'm consistently getting a 10 second video to balloon up to 28-32 seconds before recording stops. However in Flame base 1.3 v123 in 319mem OR 512mem, I'm seeing normal 10 second videos be 10-12 seconds. Nothing really strange there. Using the Reporters steps here is the break down of the tests. [319mem] Flame V1.3 Base - 10 second recordings create 10-12 second videos. Flame v2.0 - 10 second recordings create 28-32 second videos. [512mem] Flame V1.3 Base - 10 second recordings create 10-12 second videos. Flame v2.0 - 10 second recordings create 10-12 second videos. Environmental Variables: Device: Flame 1.3 Build ID: 20140627162151 Gaia: 5c43d012a9351e8aaeacda0b87b0886b7f67d33d Gecko: e181a36ebafaa24e5390db9f597313406edfc794 Version: 28.0 (1.3) Firmware Version: v123 ------------------------------------------------ Environmental Variables: Device: Flame 2.0 BuildID: 20140818095113 Gaia: 1a215917df01bb815811f33665bd3fdca4130708 Gecko: ceb9b2aa1f14 Version: 32.0 (2.0) Firmware Version: v123

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #18) > The *right* way is bug 1034001 and accompany window management fixes, > probably bug 1034001. We are not looking at any application-facing changes, > since this should be solved internally between Gecko and Gaia System. > > The currently API design leaves window management in dilemma when the UI > need to be kept but the app need to respond to visibilitychange right away. > > As Diago points out, the scenario depicted in this particular CAF blocker is > distinct from bug 995540. To remove that workaround (with a long, 3 sec > delay) a fix is being worked on by applying screenshot again. However this > bug is talking about 0.5sec transition out from app to home, where > screenshot in Gaia scope will not help. We have also previously exhibit this > behavior since 1.0 -- I am not sure why this is only raised by CAF today and > why that makes this a blocker. > > If this is really becomes a blocker because of some other reasons, I would > recommend we again workaround it by fake a visibilitywillchange event with > IAC. Tim, Could you please have someone in your team work on the hack you are suggesting. So it is easy for you to take it off and replace with the right way as you suggested. We are seeing this on camera for this bug but can manifest itself in any app that needs to do an operation on a home button press. I hope we prioritize the right fix for this as soon as we can and get that landed in the next release. Thanks Hema

(In reply to Cody Roesch [:croesch] from comment #20) > This bug repro's using Flame 2.0 set to 319mem. > > Using Flame 2.0 v123 in 319mem mode, I'm consistently getting a 10 second > video to balloon up to 28-32 seconds before recording stops. > > However in Flame base 1.3 v123 in 319mem OR 512mem, I'm seeing normal 10 > second videos be 10-12 seconds. Nothing really strange there. > This is strange and the regression should not be related to anything we are discussing. I don't think our discussion applies to the behavior here. Let's identify the regression and fix it to the original behavior, without any hacks or API changes. (Move it to Gaia until we could find out if this is a Window Mgmt regression or Video app regression)

BTW the discussion does not apply because we were talking about the delay of visibilitychange event -- it's only sent after the close app transition is over. However from comment 20 we either is not sending the event on that timing (a regression on window management) or the Video app did not stop at the time of receiving the event (a regression on video app). Fixing neither of them does not require API changes nor hack.

Wait -- (In reply to Cody Roesch [:croesch] from comment #20) > [319mem] > Flame V1.3 Base - 10 second recordings create 10-12 second videos. > Flame v2.0 - 10 second recordings create 28-32 second videos. > > [512mem] > Flame V1.3 Base - 10 second recordings create 10-12 second videos. > Flame v2.0 - 10 second recordings create 10-12 second videos. This bug is only reproduced on Flame 2.0 319mem ONLY. Flame 2.0 512mem behaves correctly as expected. This is not a Gaia issue and we need Gecko help to identify the real cause. I am throwing this to Performance component for further investigation. (Thanks Alive for point that out)

There's no performance team anymore. Also the fact that this only appears on the low memory device doesn't mean this is necessarily a Gecko issue. This could also be a race condition or something in Gaia, like bug 1047945.

Hey, I had tried 2.0 with 319MB flame, and the recording time is 11-13sec. What's the missing step? BTW, if you could repro it, could you check adb shell b2g-info to see if camera app is going to background right away after it is closed?

I suspect comment 22 is because we don't exclude 720p in camera app in pvt gaia build so the UI reaction become slow. https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/config/config.js#L205-L213 I am verifying, but I had to say it's REALLY hard to repro comment 22.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #27) > I suspect comment 22 is because we don't exclude 720p in camera app in pvt > gaia build so the UI reaction become slow. > https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/config/config. > js#L205-L213 > > I am verifying, but I had to say it's REALLY hard to repro comment 22. Without excluding 720p: "[Dump: AppWindow][OperatorVariant][AppWindow_0][425.898] publishing external event: destroyedundefined" app_window.js:958 "[Dump: AppWindow][OperatorVariant][AppWindow_0][425.899] publishing external event: terminatedundefined" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][482.931]portrait-primary" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.010]close with reduce" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.010]opened,closing,::,close" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.012]timer to ensure closed does occur." app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.015] publishing internal event: closing" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.015] publishing external event: closingundefined" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.019] publishing internal event: willclose" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][483.027] publishing external event: willcloseundefined" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.702] publishing internal event: closingtimeout" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.710]closing,closed,::,timeout" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.720] publishing internal event: closed" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.723]set visibility -> ,false" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.745]screenshot state -> ,screenshot" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.757] publishing external event: closedundefined" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.758] publishing internal event: close" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][484.763] publishing external event: closeundefined" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.086]getScreenshot succeed!" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.148]setVisible on browser element:false" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.954] Handling mozbrowservisibilitychange event..." app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.955] Handling mozbrowservisibilitychange event..." app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.966] publishing internal event: background" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.968] publishing external event: backgroundundefined" app_window.js:958 Looking closer, "[Dump: AppWindow][Camera][AppWindow_1][484.723]set visibility -> ,false" app_window.js:958 "[Dump: AppWindow][Camera][AppWindow_1][502.955] Handling mozbrowservisibilitychange event..." app_window.js:958 We are sending the app to background in 484s, but the gecko reflects the change in 502s after booting. If we don't exclude 720p, the whole phone just become nonfunctional and slowly when recording. It doesn't scare me if visibilitychange is not invoked.

BTW the expectation of 720p should be removed comes from https://intranet.mozilla.org/QA/B2G_Tips_and_Tricks#Changing_the_Camera_Resolution Is this bug a dup to bug 1050011?

the expectation of 720p "support" should be "excluded" comes from

Flame in 319 Mb cannot record 720p video. It's not a valid configuration to reproduce any bug. There's no phones in the market with 256 MB capable of recording 720p video.

Bhargav, Are you seeing the issue with 480p recording on 8x10 with KK build - 256MB? Or is this a 720p or on higher resolutions only? (as described in the original desc). Thanks Hema

I'm a little uncertain what the status of the bug is at this point, so take the following with a large grain of salt. I agree with Tim that the right fix here is bug 1034001 (and probably some followup work that will be needed after that). But that won't happen for 2.0 or even 2.1. In the meantime, it seems fine to me to add a temporary API to the browser API specifically for "turn off any camera recording". Then we can remove this API once we get the right fix in place. Is something like that needed to fix the problem? Or would simply fixing the 18 second delay in comment 28 fix the problem here?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #26) > Hey, I had tried 2.0 with 319MB flame, and the recording time is 11-13sec. > What's the missing step? > > BTW, if you could repro it, could you check adb shell b2g-info to see if > camera app is going to background right away after it is closed? Alive, There was no special steps here. This was done with the latest engineering build. STR 1. Flash Flame device to latest 2.0 319mem Engineering build. 2. Launch the Camera app and switch to Video mode. 3. Begin recording and wait till the timer hits 10 seconds. 4. Tap the Home button. Actual Results: - Camera app is slow to respond to step 4 and takes a few seconds to minimize and show the home screen. - The home screen icons and status bar at the top also appears slow to populate, sometimes taking 20+ seconds or until the recording eventually stops. - Recording won't shut down for 20-30 seconds. Here is the [adb shell b2g-info] results taken 2 seconds after step 4. | megabytes | NAME PID PPID CPU(s) NICE USS PSS RSS VSIZE OOM_ADJ USER b2g 306 1 146.7 0 22.9 23.2 24.1 233.4 0 root (Nuwa) 893 306 2.9 0 0.0 0.1 0.4 53.8 0 root Homescreen 1117 893 21.0 1 8.0 8.5 9.5 85.8 2 u0_a1117 Camera 2256 893 9.2 1 2.8 3.5 4.8 107.0 2 u0_a2256 (Preallocated a 2655 893 1.0 18 2.8 3.2 4.0 59.8 1 u0_a2655 System memory info: Total 213.4 MB Used - cache 193.5 MB B2G procs (PSS) 38.4 MB Non-B2G procs 155.1 MB Free + cache 20.0 MB Free 6.9 MB Cache 13.1 MB Low-memory killer parameters: notify_trigger 14336 KB oom_adj min_free 0 4096 KB 58 5120 KB 117 6144 KB 352 7168 KB 470 8192 KB 588 20480 KB flash@FlashStation57:~$

CAF CC blocker, so marking 2.0+

From the descriptions, I'm almost sure that the issue here is not really related to the resolution of the video, though larger videos require more memory and will probably make the issue worse. This sound to me like a zmem thrashing issue. We saw it all the time on Tarako, and it has come up again many times on the 256mb QRD device. When we are close to the limit of memory, things that ordinarily happen quickly (like delivery of visibility change events) start taking a very long time. In this situation, when the user is recording a video and presses the home button, think about what has to happen... We swap out part of the camera app, swap in the part of the system app that handles the home button, then swap in (or relaunch) the homescreen app. All this time, we're swapping out more and more of the camera app to make room for the homescreen app. Then, once the homescreen transition is complete, gecko sends the visibilitychange events and we have to swap in the part of the camera app that handles that event. When we are at the limits of available memory, it should surprise no one that ordinary operations can take multiple seconds to perform. I've argued before that we need something like the MacOS beachball on the screen to indicate when zmem is thrashing. I think it would be helpful for developers and users to see that and understand that things are going to be sluggish. So if we're going to fix this (and I'll note that I don't see anyone explaining here why this should be a blocker) then we're going to need to send a signal from the system app to the camera app before the homescreen starts swapping itself in. I know there has been discussion of allowing apps to listen to hardware button events. And I know that the homescreen app itself is notified when the home button is pressed via a hashchange event. Is there some way that the camera app could just listen to the home button? That would be the most direct way to solve the issue, I think. But failing that it seems to me that the quickest solution would be to generalize the 'private.broadcast.attention_screen_opening' settings hack to a 'private.broadcast.app_will_be_hidden' hack that applies to the home button as well as the call screen. No one like it, but we could probably implement this quickly. Since I am the author of the original hack, I'm willing to have this bug assigned to me if we want to try generalizing the settigns-based hack.

I was hoping to just generalize the private.broadcast.attention_screen_opening setting hack to cover both the callscreen and the homescreen case. But it turns out that the FM radio app now relies on the callscreen hack. The intent of the hack was to work around the latency involved in the visiblitychange event. But for the FM radio app it appears that the hack is being used to work around problems with the audio competition policy code. And it means that I can't just generalize the one hack to cover both cases, but need to add a separate hack.

Alive and Tim: could you take a look at the system app and shared/js changes in this patch? Justin: would you review the camera app and shared/js changes? I'm calling this a WIP and not asking for final review because I have not written tests and also because I'm thinking that just handling the home button case is not enough. Presumably we'll have the same issue needing to stop recording right away if the user goes to the task manager or does an edge swipe to switch apps. So I think I need to generalize the system app settings hack so that it is sent any time the foreground app is going to change and not just on the home button.

The attached patch is a work in progress, but it is suitable for testing by those who can reproduce the bug. Bhargavg1 and Cody, could you see if this patch makes a difference for you, please?

Is anyone reading comment 24 and onward except Diego in comment 31? Yesterday we have already identified this issue is due to the fact we have a mis-configured 319MB Flame, which is a mid-mem device but with 720p camcorder enabled. We should not be spending time writing and try to land a piece of code that serve no purpose outside of the mis-configured platform. I am very sad that :djf is being asked to spend time on this, when we have already spend time yesterday in Taipei hours to figure out the real cause.

Comment on attachment 8475538 [details] work in progress patch Not taking the patch. See previous comment.

I meant to find :djf on irc but he is not avail. Commenting below to further explain why this patch is not valid. (In reply to David Flanagan [:djf] from comment #36) > From the descriptions, I'm almost sure that the issue here is not really > related to the resolution of the video, though larger videos require more > memory and will probably make the issue worse. This sound to me like a zmem > thrashing issue. We saw it all the time on Tarako, and it has come up again > many times on the 256mb QRD device. When we are close to the limit of > memory, things that ordinarily happen quickly (like delivery of visibility > change events) start taking a very long time. > AFAIK we are not using zram in any phone >=512M, and there are no phone <512M equip with 720p camcorder. So the patch is essentially trying to workaround a particular case which only happen on our dearly 319M Flame, which bug 1050011 exists (not me) to say, clearly, that, it's mis-configured. If there are really such device, I would argue the platform is responsible of workaround the problem. (.. removing technical analysis ..) > I know there has been discussion of allowing apps to listen to hardware > button events. And I know that the homescreen app itself is notified when > the home button is pressed via a hashchange event. Is there some way that > the camera app could just listen to the home button? That would be the most > direct way to solve the issue, I think. Underlining the Browser API we are still using IPC messages for the hash event and the future key event passing. You might find they are "faster" than visibilitychange event, but in reality if the platform is mis-configured and slow, there is really nothing we can do in Gaia to make any message deliver faster to content process, than IPC. (Maybe we should patch Gecko to make sure visibilitychange event delivers as fast as other IPC messages?) (In reply to David Flanagan [:djf] from comment #38) > I'm calling this a WIP and not asking for final review because I have not > written tests and also because I'm thinking that just handling the home > button case is not enough. Presumably we'll have the same issue needing to > stop recording right away if the user goes to the task manager or does an > edge swipe to switch apps. So I think I need to generalize the system app > settings hack so that it is sent any time the foreground app is going to > change and not just on the home button. The complexity for this patch to cover all cases actually is self-evident to the fact we should not be trying to workaround it in Gaia at first place.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #42) > (Maybe we should patch Gecko to make sure visibilitychange event delivers as > fast as other IPC messages?) If that is not the case then that indeed sounds like a bug that we should fix. Please file and cc me.

Bhargavg: what is the resolution of the video you record when you experience this bug, and how much memory is on your device? If you have this bug with 480p video and the patch I just attached helps, then perhaps Tim will consider allowing that patch to land. Otherwise, if you only experience the bug when recording 720p video on a 256mb device, then I think we should just disable 720p video. Tim: the mis-configuration of the Flame is not relevant since this bug was filed for a QRD device. And on a limited-memory device, I've got to assume that sending a signal to the foreground app while it still is the foreground app and is swapped in is going to be much faster than swapping another app in and then trying to send a signal to the app that was just swapped out.

Unassigning myself from this bug since Tim is not even willing to consider reviewing the approach I took. I don't have any other suggestions to offer. I would still be interested to know whether my patch helps on the QRD device, though.

Comment on attachment 8475538 [details] work in progress patch Clearing the feedback request for Justin. No point spending time to offer feedback on the Camera and shared parts of the patch if Tim won't even consider the system app part.

This is a CAF CS blocker bug with a fix required by August 21st, and is too high priority to leave unassigned. Based on Tim's comment #40 that people are working on this in Taipei, I'm changing the component from Camera to window management and reassigning it to Tim, with the expectation that Tim will assign to whoever is doing the work.

Tim: Note that one of the important pieces of information that we still need here is the video resolution used by the QRD device. Hema and I (in comment #32 and comment #44) have asked Bhargav about this, and separately I have asked Bhavana to ping CAF about this question. Also note that I'm not sure that the speed at which the visiblitychange event is delivered (per comment #43) matters so much. On memory limited devices, the transition from one app to another is going to cause a lot of swapping. Since visibilitychange is not sent until after the transition completes, the event delivery and/or the respone to that event will often be delayed by thrashing zmem, even if it is sent over a fast channel. I think that we need to send a "willhide" event (of some sort) before the app transition so it can be received and acted upon before all of the swapping begins, and that is what my patch was attempting to do.

(In reply to David Flanagan [:djf] from comment #44) > Bhargavg: what is the resolution of the video you record when you experience > this bug, and how much memory is on your device? If you have this bug with > 480p video and the patch I just attached helps, then perhaps Tim will > consider allowing that patch to land. Otherwise, if you only experience the > bug when recording 720p video on a 256mb device, then I think we should just > disable 720p video. > David - this issue was on 8x10 with 256MB config, we will try out the patch in comment 46 and provide feedback. Pooja -- can you please try out the patch and see if it helps.

> David - this issue was on 8x10 with 256MB config, we will try out the patch > in comment 46 and provide feedback. > Pooja -- can you please try out the patch and see if it helps. Missed to mention, with 480p encoding resolution.

(In reply to Inder from comment #49) > (In reply to David Flanagan [:djf] from comment #44) > > Bhargavg: what is the resolution of the video you record when you experience > > this bug, and how much memory is on your device? If you have this bug with > > 480p video and the patch I just attached helps, then perhaps Tim will > > consider allowing that patch to land. Otherwise, if you only experience the > > bug when recording 720p video on a 256mb device, then I think we should just > > disable 720p video. > > > David - this issue was on 8x10 with 256MB config, we will try out the patch > in comment 46 and provide feedback. > Pooja -- can you please try out the patch and see if it helps. Can we have a patch compatible with v2.0?

Bhargav, Sorry I didn't think to create a v2.0 patch in the first place. Here's a version that works with 2.0. On a 512mb flame, I see the new 'willhide' event delivered 2 to 3 seconds sooner than the visibilitychange event and it makes a noticeable difference in how long it takes to hear the stop recording sound. As Tim points out, however, this might just be because the Flame is misconfigured. So your feedback on whether this patch works on the QRD will be helpful.

Just realized that maybe I should have flaged Pooja for testing this patch...

David, I realized there is an important information missing from the discussion, i.e. what Inder will test for in comment 49/50. However, even if Inder can reproduce the bug on 480p resolution on a 2.0 QRD, this is still a QRD-only bug that does not reproduce on our reference device (Flame 319M) -- in that case maybe we should land, on 2.0 branch, or even 2.1 branch while finding the real cause on master, while keep questioning the validity of our reference platform. All I want is not making Gaia System and other apps become a dump for platform workarounds, no matter how fast we could reproduce Gaia workaround to replace platform fixes. Having workaround APIs between Gaia apps and Gaia System also creates disadvantage to 3rd-party apps, which is something we should avoid. Your patch is somewhat a fake impl of bug 1034001 + Gaia fix (sending visibilitychange event *before* app is switched away from memory priority), having that in the tree to take the schedule pressure off the real impl is a valid point too (but again, I am not sure this bug is a valid reason for landing this patch). I will be keeping myself as the assignee before CAF report back their finding.

See previous comment.

(In reply to David Flanagan [:djf] from comment #52) > Created attachment 8476392 [details] > work in progress patch for v2.0 > > Bhargav, > > Sorry I didn't think to create a v2.0 patch in the first place. Here's a > version that works with 2.0. > Hi David, Looks patch works. Before going to home screen it stops recording.

Per irc discussion and previous comments with :djf, we concluded that we are hitting a bug that is only reproducible on QRD with 480p, and 319MB Flame but with 720p only. Since the visibilitychange event is only delivered *after* the app process is being pushed to background, per :djf it always comes with a delay of video recording in the STR of this bug (~2 sec). It's just worse on low-men devices. Under this assumption, I agree with :djf that the patch in this bug can be taken as a workaround before bug 1034001 and friends is done. After that patch we can send visibilitychange *before* the transition so the app will be responsive as it should be. Redirect assignee to :djf for him to continue work on it. Thanks.

Comment on attachment 8475538 [details] work in progress patch I've read the patch this morning and this patch can land when it's ready and reviewed, per previous comment.

The debug log in Alive's comment #28 points to what appear to be useful events that would provide a more general way to do this patch rather than tying it explicitly to the home button. If I add logging to AppWindow.prototype.close() I see that method gets called when returning to the homescreen or when going to the task switcher. Unfortunately, it does not get called when switching from one app to another with a left or right swipe. close() also is not called when the attention screen pops up, so it can't replace the existing attention screen hack.

Ah, but AppWindow.prototype._handle__swipeout() is called for edge gesture app transitions.

Tested the 2.1 patch with 2.1 Flame 319mb. Here are my results. -Tapping the home button after 10 seconds creates a 12-13 second video. I did not see an issue with 2.1 prior to patch either. -I'm noticing that tapping home does not provide a smooth transition anymore. There is not an immediate vibration when tapping home button and there is a stuttery beep for ending the recording. Almost sounds like 2 beeps back to back. I didn't notice this behavior without the patch. Environmental Variables: Device: Flame Master BuildID: 20140821060216 Gaia: 7d7001531e99215cb1ef064670345f4ce0127e06 Gecko: c14e5feadc61 Version: 34.0a1 (Master) Firmware Version: v123 ------------------------------------------------------------ ------------------------------------------------------------ Tested the 2.0 patch with 2.0 Flame 319mb. Here are my results. -Tapping the Home button after 10 seconds, will usually drop out of camera and stop recording within 2-3 seconds which is a big improvement over prepatch. -However when tapping the Home button, Often the button won't respond to my tap and I cannot get out of camera or stop the recording with the stop button. Recording will continue for 10-25 seconds before the camera app can close and recording can stop. So the bug as it's written up seems to have been addressed about video times running longer then expected but there are some new bugs that are causing big issues when leaving camera. Environmental Variables: Device: Flame 2.0 BuildID: 20140821084115 Gaia: ed257456c512c3b345f5a89e9211581e7ae0f1c0 Gecko: b0e90fdbb257 Version: 32.0 (2.0) Firmware Version: v123

The repro rate for the Home button not working when trying to quit out of camera is about 2/10 times.

Test case added to moztrap: https://moztrap.mozilla.org/manage/case/14447/

Comment on attachment 8476392 [details] link to v2.0 patch on github I've updated the v2.0 version of the patch. It should now stop the recording quickly not just for the home button, but also for edge gestures and when doing a long press on the home button to bring up the task switcher. Kevin: would you review the system app and shared portions of this patch? Justin: would you review the camera and shared portions of this patch?

(In reply to Cody Roesch [:croesch] from comment #62) > The repro rate for the Home button not working when trying to quit out of > camera is about 2/10 times. Cody: I just finished a new version of the patch. The code has changed significantly and I'm hoping that the bugs you were seeing with the old version are no longer there. Could you test again, please?

Cody: also note that the two beeps back to back for video recording is a weird flame bug. Both Gaia and the camera driver play sounds when we record and stop. That is unrelated to this patch.

Comment on attachment 8476392 [details] link to v2.0 patch on github Please add unit tests for `shared/js/will_hide_event.js`. Also, I had one other question in the GitHub PR.

Comment on attachment 8476392 [details] link to v2.0 patch on github I'm generally fine with what I've seen and will leave an R+ since you've checked with Tim on this. My biggest concern is the regression introduced with the current approach when displaying the homescreen after pressing the home button. I would strongly recommend having some way of apps "opting into" this functionality as stated on github. I also understand that this would probably double the size of the patch, take more time, and add extra complexity - so I'll leave the decision up to you on what you want to do. I can't wait for the day when this is all happy due to proper API support.

Kevin had the good idea of only sending these events (and only slowing down the home button) when there is an app that is currently recording. So I'll modify the patch to do that and modify the shared will_hide_event.js module to be a stop_recording_event.js module. After discussing with Justin on IRC, he's willing to land this to 2.0 without tests for the new shared module, but we will need to add those before landing on master. We're right up against CAF's deadline here, so I hope I can get this all wrapped up soon.

I've updated the PR again to change from a will_hide event to a stop_recording event. This is ready for testing.

Looks like I've broken at least one system app unit test with my patch. Will be AFK for a while, but will try to get back to this tonight.

v2.0 pull request updated with fixes for the failing camera and system app unit tests.

(In reply to David Flanagan [:djf] from comment #60) > Ah, but AppWindow.prototype._handle__swipeout() is called for edge gesture > app transitions. Exactly you were right here: Send fake stop event in close() and stackManager is the best bet. The problem that we have two entry point of close function is because stackManager is invented before the new window mgmt codes. It should be integrated but not yet.

Pull request updated again with unit tests for shared/js/stop_recording_event.js

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #73) > (In reply to David Flanagan [:djf] from comment #60) > > Ah, but AppWindow.prototype._handle__swipeout() is called for edge gesture > > app transitions. > > Exactly you were right here: > Send fake stop event in close() and stackManager is the best bet. > The problem that we have two entry point of close function is because > stackManager is invented before the new window mgmt codes. It should be > integrated but not yet. Thanks, Alive. I started to do this in AppWindow.close(), but moved it into AppWindowManager because it wasn't actually specific to individual app windows. At Kevin's suggestion I made it check mediaRecording.isRecording, so most of the time the call to sendStopRecordingRequest() just calls its callback synchronously and doesn't do anything with the settings. I think that I'm already past the CAF deadline for this bug, so I hope you're okay with the patch as it stands here, for 2.0 at least. For master we can change it as needed.

Comment on attachment 8476392 [details] link to v2.0 patch on github David: Updated patch looks good! Thank you for adding the unit tests :-)

(In reply to David Flanagan [:djf] from comment #75) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #73) > > (In reply to David Flanagan [:djf] from comment #60) > > > Ah, but AppWindow.prototype._handle__swipeout() is called for edge gesture > > > app transitions. > > > > Exactly you were right here: > > Send fake stop event in close() and stackManager is the best bet. > > The problem that we have two entry point of close function is because > > stackManager is invented before the new window mgmt codes. It should be > > integrated but not yet. > > Thanks, Alive. I started to do this in AppWindow.close(), but moved it into > AppWindowManager because it wasn't actually specific to individual app > windows. At Kevin's suggestion I made it check mediaRecording.isRecording, > so most of the time the call to sendStopRecordingRequest() just calls its > callback synchronously and doesn't do anything with the settings. > > I think that I'm already past the CAF deadline for this bug, so I hope > you're okay with the patch as it stands here, for 2.0 at least. For master > we can change it as needed. Thanks a lot for the help on this David ! I agree on doing the changes on master and leave this on 2.0. CAF will pick this up for tomorrow's build on their side.

Testing on a 319mb flame without the patch: record 5s video and push home. Resulting video length: 18s record 5s and long press home. Resulting length: 20s record 5s and edge gesture. Resulting length: 20s Now the same tests with the patch: record 5s video and push home. Resulting video length: 7s record 5s and long press home. Resulting length: 11s record 5s and edge gesture. Resulting length:12s The times with the patch are much better. The 319mb Flame is a really messed up device when recording video, so the times are still not ideal when going to the task manager or doing an edge gesture, but that's probably in part because the Flame is recording such big videos.

I'm not seeing the issues that Cody was reporting in comment #61 with the earlier version of this patch.

Landed on v2.0: https://github.com/mozilla-b2g/gaia/commit/4ea69f01f52f472331d12b001ec1e37631bff575 We needed to get this landed in 2.0 to night for CAF's 2.0 testing. Leaving the bug open so we can get this patch into the master branch.

Tim and Alive, This patch has evolved since we discussed it last. At Kevin's suggestion, I only do the settings hack now when we know that an app is recording something, so it should have no performance impact in other cases. Are you okay with landing this on master, or would you like changes to the patch, or would you like to continue looking for a better solution.

FWIW we're one week away from 2.1 FL. I don't really see a point avoiding landing on master just to have this same fire drill in a few weeks. My opinion is that we should land and have a bug filed to remove the hack upon getting the proper visibility API.

I am fine if we have it in master and refine it in 2.2, David, please land to master as well.

Comment on attachment 8476392 [details] link to v2.0 patch on github Confirming on pooja's comment

Patch updated for master, with a bunch of camera merge conflicts resolved. Waiting on tbpl tests to run.

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e424c85eda87a40c0fa64d6a779c3fa368bf770b

David, Sorry I was unable to text your patch in comment 65 due to me being Out of the office, but it looks like you got the tests done in comment 78. Thanks for doing that and I'm glad you got useful data. Removing NI for me.

This issue has been verified successfully on Flame2.0&2.1 Verify video:"verify_1051172.mp4". **From the conversation, I can see it's normal that the video you actually recorded for 10s will be displayed as 12s. Flame2.0 build Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3 Build-ID 20141130000204 Version 32.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141130.032432 FW-Date Sun Nov 30 03:24:44 EST 2014 Bootloader L1TC00011880 Flame2.1 build: Gaia-Rev ccb49abe412c978a4045f0c75abff534372716c4 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22 Build-ID 20141130001203 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141130.034738 FW-Date Sun Nov 30 03:47:49 EST 2014 Bootloader L1TC00011880