Pooja Reporter
	Description • 10 years ago

It happens only with 480p recording profile,with other lower recording profile like cif, low, qif playback is fast.

Issue was not present on v1.3  and v1.4
Target: 8x10 QRD target with 256MB  memory.

STR:
1. Open camera app
2. Switch to recording. start recording say for 10-15sec min then stop recording
3. Tap on generated filmstrip
4. Play recorded video

Actual behavior:
Grey screen stays on the display for 4-5 sec and then playback starts.


Expected behavior:
Videoplayback should start without any noticeable delay.

	Jason Smith [:jsmith]
	Comment 1 • 10 years ago

QA Wanted - Can this be reproduced on Flame?

	Pi Wei Cheng [:piwei] (inactive)
	Comment 2 • 10 years ago

(In reply to Jason Smith [:jsmith] from comment #1)
> QA Wanted - Can this be reproduced on Flame?

On Flame with 273mb mem, recording a video immediately resulted in:
FF OS rebooted itself x 2
Camera app exited itself x3
(total of 5 attempts)

Therefore we can't fully follow the STR.

Tested on:
Device: Flame 2.0
Build ID: 20140711112114
Gaia: c24d115bb51e40d5cb28c49324102b893966944b
Gecko: 1016a9d4fa70
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 3 • 10 years ago

(In reply to Pi Wei Cheng [:piwei] from comment #2)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > QA Wanted - Can this be reproduced on Flame?
> 
> On Flame with 273mb mem, recording a video immediately resulted in:
> FF OS rebooted itself x 2
> Camera app exited itself x3
> (total of 5 attempts)

273MB mem is nowhere near enough to record video on Flame. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1027592#c15

	bhargavg1
	Comment 4 • 10 years ago

(In reply to Mike Habicher [:mikeh] from comment #3)
> 273MB mem is nowhere near enough to record video on Flame. See:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1027592#c15

What is the default resolution that's getting selected on flame?

	bhargavg1
	Comment 5 • 10 years ago

(In reply to bhargavg1 from comment #4)
> (In reply to Mike Habicher [:mikeh] from comment #3)
> > 273MB mem is nowhere near enough to record video on Flame. See:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1027592#c15
> 
> What is the default resolution that's getting selected on flame?

did we try forcing that to 480p.

1 quick way can be to edit /system/etc/media_profiles.xml and remove all references to 720p (height=720) for both camera1/camera2

Basically where ever Encoderprofile quality is high or 720p

	bhavana bajaj [:bajaj]
	Comment 6 • 10 years ago

Ni :mkieh, to see if we can pursue options listed in comment #5 , can we reduce the recording to 480px for 256 MB devices running 2.0. May be something similar to what we are doing here(https://bugzilla.mozilla.org/show_bug.cgi?id=1036637). CAF is defaulted their testing 480px on these low memory devices.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 7 • 10 years ago

Based on bug 1027592, it doesn't sound like 480p mode would alleviate this issue.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 8 • 10 years ago

Pooja, a couple of questions: when this happens, what is the output from the kernel log (captured ~10 seconds after you press the record button)?

# adb shell cat dmesg | grep sigkill

If you're seeing processes getting killed, please try increasing the amount of memory on the QRD device until nothing gets killed anymore. At that threshold, does this 4-6s issue reproduce? (One way of doing this is by making sure that everything works with 512MB, and doing a bisection to locate the thresholds where the kernel log starts to show processes getting killed.)

	Hema Koka [:hema]
	Comment 9 • 10 years ago

Mike will work with Kyle (Performance/Mem team) to further analyze this issue.

	Pooja Reporter
	Comment 11 • 10 years ago

(In reply to Mike Habicher [:mikeh] from comment #8)
> Pooja, a couple of questions: when this happens, what is the output from the
> kernel log (captured ~10 seconds after you press the record button)?
> 
> # adb shell cat dmesg | grep sigkill
> 
> If you're seeing processes getting killed, please try increasing the amount
> of memory on the QRD device until nothing gets killed anymore. At that
> threshold, does this 4-6s issue reproduce? (One way of doing this is by
> making sure that everything works with 512MB, and doing a bisection to
> locate the thresholds where the kernel log starts to show processes getting
> killed.)

Recording duration 30sec

At 271Mb - received sigkill to Homescreen -Delay was 4-6 sec 

At 286 MB - No sigkill - delay reduced to 1-2 sec (Attached video with 286MB).

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 12 • 10 years ago

(In reply to Pooja from comment #11)

> At 286 MB - No sigkill - delay reduced to 1-2 sec (Attached video with
> 286MB).

Are you referring to the grey screen before the video player appears? I see the same thing on Flame w/ 512MB of RAM, but there is nothing missing from the video.

David, is the grey screen part of the UI?

	Pooja Reporter
	Comment 13 • 10 years ago

Yes Mike. Its about how long that grey screen stays on UI.
With 271MB it was staying quite longer i.e 4-6 sec
But with increased memory it stays only fr 1-2 sec.

Also On other platform we don't see such grey or any screen before video starts. Need confirmation if or not its a part of UI.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 14 • 10 years ago

(In reply to Pooja from comment #13)

> Yes Mike. Its about how long that grey screen stays on UI.
> With 271MB it was staying quite longer i.e 4-6 sec
> But with increased memory it stays only fr 1-2 sec.

That makes sense: the tighter memory constraint would certainly make bringing up new UI components take longer. djf, is there anything we can do to speed up the switch-to-preview time.

	David Flanagan [:djf]
	Comment 15 • 10 years ago

The gray screen is not an intentional part of the API. I think that it is just the background that becomes visible while we hide the poster screen and display the actual video element. I imagine zmem is swapping or thrashing and this process just takes a really long time.

If it would help to resolve this bug, it might be possible to change the code (in shared/js/media/media_frame.js, probably) that does that swapping so that it first displays the video element and then removes the poster image...  But then instead of having a blank gray screen, we'd have a static poster image and the user would feel like nothing had happened when they tapped to play the video.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 16 • 10 years ago

(In reply to David Flanagan [:djf] from comment #15)

> If it would help to resolve this bug, it might be possible to change the
> code (in shared/js/media/media_frame.js, probably) that does that swapping
> so that it first displays the video element and then removes the poster
> image...  But then instead of having a blank gray screen, we'd have a static
> poster image and the user would feel like nothing had happened when they
> tapped to play the video.

It pains me to suggest this: but perhaps this is a job for a spinner?

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 17 • 10 years ago

Stealing this from Mike to implement the dreaded spinner :-/

	Hema Koka [:hema]
	Comment 18 • 10 years ago

Justin is working on adding a visual indicator (spinner) in place of the gray screen. Lets keep this bug open and re-evaluate when the homescreen optimizations land. 

The other worst case option is to disable 480p recording. 

Thanks
Hema

	David Flanagan [:djf]
	Comment 19 • 10 years ago

Rather than adding ad-hoc spinners whenever this happens, it would be nice if we had a system-level spinner, like the MacOS beachball.  I wonder if there is some way that the gonk code that handles zmem swapping could draw some kind of beachball thing to the screen. Even if we can fix this particular bug, we are always going to have slowdown issues when memory is close to full and swapping happens.

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 20 • 10 years ago

Diego: Please review. Note, I renamed our `camera:busy`/`camera:ready` events at the app level to simply be `busy`/`ready` because we now use the loading screen for more things than just the camera. Also, the `busy` event already takes an optional parameter to indicate the reason for the busy-ness (we have thresholds for showing the spinner in config.js based on this `type`).

David: Added you for feedback since I made a minor change to video_player.js. We already had an `onplaying` callback that was called after the video begins to play. So, I simply added an `onplay` callback that gets called as soon as video playback is requested for the first time (when the poster is still visible). I decided to take this route because it has minimal impact for breakage for other apps that depend on video_player.js. Also, we already had most of the mechanisms in-place within the Camera app to show our spinner, so I figured it would be less invasive to have video_player.js show its own spinner.

	David Flanagan [:djf]
	Comment 21 • 10 years ago

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

I'm giving feedback- because the new play event from video player is emitted every time the video starts playing, but is only different than the playing event for the first play, when the poster is hidden and the video shown.  If you're going to define a new event, I'd like it to be meaningful. So a 'loading' event emitted just on that first play would be fine.

But see my other comments on github as well: I think there might be other problems with this... Playing a video might re-enable the viewfinder while we're still in preview gallery, for example.

	David Flanagan [:djf]
	Comment 22 • 10 years ago

What a pain in the butt this bug is... 

Have you tried solving it without a spinner by just swapping the lines hidePoster() and showPlayer() so that we show the player first?  Maybe doing that and animating the play button until playback actually begins would be good enough.  Instead of hiding it the button, pulse it by dimming and brightening until you get the actual playing event from the video element and then hide it.

	David Flanagan [:djf]
	Comment 23 • 10 years ago

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

Thanks for addressing my concerns, Justin. Changing f- to f+.

	Diego Marcos [:dmarcos]
	Comment 24 • 10 years ago

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

Made a few suggestions on GH to rename things

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 25 • 10 years ago

Landed on v2.0:

https://github.com/mozilla-b2g/gaia/commit/b9d19011123487009c80d1200937652d58c434a0

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 26 • 10 years ago

Pull request for master -- Carrying R+ forward

	Justin D'Arcangelo [:justindarc] Assignee
	Comment 27 • 10 years ago

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/265bcc8bd54f9da013ff67e1f5826ccfb84df1de

	Yeojin Chung [:YeojinC]
	Comment 28 • 10 years ago

Found Test Case: https://moztrap.mozilla.org/manage/case/13753/

The step 1 through 5 covers the issue.

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 29 • 10 years ago

Test case has been updated to ensure there is not a significant delay when playing the video:

https://moztrap.mozilla.org/manage/case/13753/