Pooja Reporter
	Description • 10 years ago

STR:
1. Open Video app
2. Start video playback
3. Suspend device and then resume it back using power button

Expected Result:
  Video frame should be shown as soon as device resumed
Actual result:
  As soon as device resumed, video playback screen remains black for few sec 
  before actually showing the video frame

Gaia and Gecko info:
Gaia "f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1"
Gecko "db7fce920e7d782d9f601384dc95924abcdaeeb8"

	Pooja Reporter
	Comment 1 • 10 years ago

[Blocking Requested - why for this release]:

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

The video app discards the currently playing video when it goes to the background and restores it when it comes back to the foreground. This is because a long time ago we had to be very careful that only one app was trying to use the hardware decoder at a time.

I think this workaround is no longer necessary, and we could probably resolve this bug by removing it.

Sotaro: can you confirm that it is now safe for multiple apps to be using video elements at the same time?

	Sotaro Ikeda [:sotaro]
	Comment 3 • 10 years ago

(In reply to David Flanagan [:djf] from comment #2)
> Sotaro: can you confirm that it is now safe for multiple apps to be using
> video elements at the same time?

General video elements' hw codec sharing mechanism is not implemented yet. Because there is no such requirement in b2g product requests. Instead simple hw video codec sharing mechanism is implemented since Bug 871485. And Bug 1029552 extends it a bit more recently.

The following is how hw video codec is shared between video elements.
- Current hw video codec user"video element" can continue to own until release it.
- video element automatically release hw video codec when document is hidden.
- video element automatically release hw video codec when the video element is detached from tree.

	No-Jun Park [:njpark]
	Comment 4 • 10 years ago

this is not reproducible in 2.0:

Gaia-Rev        c6c6116ca225c2c934220ae6867e5a3256d65e00
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/24a2aa6bf1c4
Build-ID        20141015000206
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141015.033627
FW-Date         Wed Oct 15 03:36:38 EDT 2014
Bootloader      L1TC00011840

	Russ Nicoletti [:russn] Assignee
	Comment 5 • 10 years ago

Based on my understanding of comment 3, the video app no longer needs to discard the currently playing video when it goes to the background as the code will be automatically released. I am working on implementing this behavior.

	Sotaro Ikeda [:sotaro]
	Comment 6 • 10 years ago

(In reply to Russ Nicoletti [:russn] from comment #5)
> Based on my understanding of comment 3, the video app no longer needs to
> discard the currently playing video when it goes to the background as the
> code will be automatically released.

yes.

	Sotaro Ikeda [:sotaro]
	Comment 7 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> (In reply to Russ Nicoletti [:russn] from comment #5)
> > Based on my understanding of comment 3, the video app no longer needs to
> > discard the currently playing video when it goes to the background as the
> > code will be automatically released.
> 
> yes.

hw video codec is automatically released when the app got to background.

	Andrew Overholt [:overholt]
	Comment 8 • 10 years ago

This blocks 2.1 FC and CC so 2.1+.

	Russ Nicoletti [:russn] Assignee
	Comment 9 • 10 years ago

I've created a patch where the video app does not discard the current video. However, when the app comes to the foreground, the first frame of the video id displayed. There was already code in the video app that sets the video position to the position when the app went to the background (the "restore" position) and this results in the first frame of the video being shown briefly and then switching to the "restore" frame -- not a great user experience. I am hiding the video player as soon as the video app gets the 'visibilitychange' event and then displaying the player when the app gets the 'loadedmetadata' event.

	Russ Nicoletti [:russn] Assignee
	Comment 10 • 10 years ago

I've resolved the problem of the first frame being shown briefly before the "restore" frame. I've done this by listening to the 'canplay' event and only then unhiding the player. This, however, raises some questions about what gecko is doing. 

Btw, I have removed all of the code in the video app that releases and loads the video on visibility changes. Yet on the visibility change that brings the app to the foreground, I'm seeing the following events being sent:
- loadedmetadata
- loadeddata
- canplay

Sotaro, is this expected? Is gecko essentially unloading and reloading the video when the video app is moved to the background and then the foreground?

	Sotaro Ikeda [:sotaro]
	Comment 11 • 10 years ago

(In reply to Russ Nicoletti [:russn] from comment #10)
> I've resolved the problem of the first frame being shown briefly before the
> "restore" frame. I've done this by listening to the 'canplay' event and only
> then unhiding the player. This, however, raises some questions about what
> gecko is doing. 
> 
> Btw, I have removed all of the code in the video app that releases and loads
> the video on visibility changes. Yet on the visibility change that brings
> the app to the foreground, I'm seeing the following events being sent:
> - loadedmetadata
> - loadeddata
> - canplay
> 
> Sotaro, is this expected? Is gecko essentially unloading and reloading the
> video when the video app is moved to the background and then the foreground?

It is expected. The current implementation does not filter the above events, because some file formats could emit multiple loadedmetadata events.

When application goes to background, the application becomes inactive. If video element detects it, MediaDecoder becomes dormant state and releases a part of media resources(hw video codec) if releasing is necessary. Current implementation does not release data source, audio codec and Playback state machine.

And when application becomes active again, the video element resumes from dormant state. And re-start from loadedmetadata again. During resuing, seeks to previous playback time.

	Russ Nicoletti [:russn] Assignee
	Comment 12 • 10 years ago

(In reply to No-Jun Park [:njpark] from comment #4)
> this is not reproducible in 2.0:
> 
> Gaia-Rev        c6c6116ca225c2c934220ae6867e5a3256d65e00
> Gecko-Rev      
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/24a2aa6bf1c4
> Build-ID        20141015000206
> Version         32.0
> Device-Name     flame
> FW-Release      4.4.2
> FW-Incremental  eng.cltbld.20141015.033627
> FW-Date         Wed Oct 15 03:36:38 EDT 2014
> Bootloader      L1TC00011840

No-Jun, I don't see any difference in the behavior between 2.2 and 2.0. The video of my testing with 2.0 is here: http://youtu.be/mG0tYfXE0r8. It would be odd for the behavior to be different because the code that handles when the video goes to the background and then comes to the foreground is the same for 2.0 and 2.2.

Also, I added log statements that log the time it takes to display a video when the app comes to the foreground after being in the background. For 2.2 and 2.0, I'm seeing similar results: it takes approximately 500ms. I don't see any difference between 2.0 and 2.2. Can you test again with 2.0 and confirm whether or not you see a delay?

	No-Jun Park [:njpark]
	Comment 13 • 10 years ago

a(In reply to Russ Nicoletti [:russn] from comment #12)

> Also, I added log statements that log the time it takes to display a video
> when the app comes to the foreground after being in the background. For 2.2
> and 2.0, I'm seeing similar results: it takes approximately 500ms. I don't
> see any difference between 2.0 and 2.2. Can you test again with 2.0 and
> confirm whether or not you see a delay?

I just double checked 2.0 and 2.1 and you're right, the black screen behavior is same, and the duration of the black screen seems to be about the same as well.  Sorry about the confusion.

	No-Jun Park [:njpark]
	Comment 14 • 10 years ago

Must confused with the media app, where such behavior is not seen in 2.1 either.

	Russ Nicoletti [:russn] Assignee
	Comment 15 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> When application goes to background, the application becomes inactive. If
> video element detects it, MediaDecoder becomes dormant state and releases a
> part of media resources(hw video codec) if releasing is necessary. Current
> implementation does not release data source, audio codec and Playback state
> machine.
> 
> And when application becomes active again, the video element resumes from
> dormant state. And re-start from loadedmetadata again. During resuing, seeks
> to previous playback time.

From your comment above, you seem to be saying the video should resume at the position it was at when the app went to the background, which makes sense to me in terms of user experience. However, I'm seeing that the video always resumes at the beginning. Can you comment on that? It seems like a bug to resume the video at the beginning.

	Russ Nicoletti [:russn] Assignee
	Comment 16 • 10 years ago

Clarification of comment 15:

What I'm seeing is the first frame is displayed briefly and then the video jumps to the frame it was at when the app went into the background. Video of my testing is here: http://youtu.be/smDOSU8bVz4.

I'm finding that in order to hide the brief "flash" of the first frame, I have to hide the player until the 'canplay' event. That is not ideal because the result is a brief black screen, essentially the same user experience as what is described by this bug.

Sotaro, can you comment on this behavior?

	Sotaro Ikeda [:sotaro]
	Comment 17 • 10 years ago

When I tried the following video's playback from browser, then I put the browser tab to background and put foreground again, the video was resume from the same place. Therefore, it seems to work as expected.

http://people.mozilla.org/~cpeterson/videos/H264_Baseline_Profile_Level_30_640x360p.mp4

	Sotaro Ikeda [:sotaro]
	Comment 18 • 10 years ago

(In reply to Russ Nicoletti [:russn] from comment #16)
> Clarification of comment 15:
> 
> What I'm seeing is the first frame is displayed briefly and then the video
> jumps to the frame it was at when the app went into the background. Video of
> my testing is here: http://youtu.be/smDOSU8bVz4.
> 
> I'm finding that in order to hide the brief "flash" of the first frame,

I think showing the first frame is a bug of current gecko's implementation.

	Sotaro Ikeda [:sotaro]
	Comment 19 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> (In reply to Russ Nicoletti [:russn] from comment #16)
> > Clarification of comment 15:
> > 
> > What I'm seeing is the first frame is displayed briefly and then the video
> > jumps to the frame it was at when the app went into the background. Video of
> > my testing is here: http://youtu.be/smDOSU8bVz4.
> > 
> > I'm finding that in order to hide the brief "flash" of the first frame,
> 
> I think showing the first frame is a bug of current gecko's implementation.

Bug 1084629 is created for it.

	Russ Nicoletti [:russn] Assignee
	Comment 20 • 10 years ago

This is the patch for the video app that lets gecko manage the video codec when the app is sent to the background and then back to the foreground.

	Russ Nicoletti [:russn] Assignee
	Comment 21 • 10 years ago

Updating the patch to remove the fix for bug 1082555 (which was included in the previous patch).

This patch removes the code from the video app that manages the hw video codec and instead lets gecko handle that.

	Russ Nicoletti [:russn] Assignee
	Comment 23 • 10 years ago

Comment on attachment 8508021 [details]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25342

The attached PR removes the code from the video app that manages the hw video codec and lets gecko handle that.

Note that bug 1084629 has been created against gecko to fix an issue with resuming the video where it left off when coming to the foreground after going to the background.

	Sotaro Ikeda [:sotaro]
	Comment 24 • 10 years ago

bug 1084629 becomes dup of bug 1058429.

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

Comment on attachment 8508021 [details]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25342

A couple of things noted on github that I'd like you to fix before landing this.

What did you decide to do about the flash of the first frame? Is that just deferred to the gecko bug?  Is the gecko bug going to block 2.1? If not, do we need to do anything in gaia for 2.1 to hide the first frame flash?

	Russ Nicoletti [:russn] Assignee
	Comment 26 • 10 years ago

(In reply to David Flanagan [:djf] from comment #25)
> Comment on attachment 8508021 [details]
> Github PR: https://github.com/mozilla-b2g/gaia/pull/25342
> 
> A couple of things noted on github that I'd like you to fix before landing
> this.
> 
> What did you decide to do about the flash of the first frame? Is that just
> deferred to the gecko bug?  Is the gecko bug going to block 2.1? If not, do
> we need to do anything in gaia for 2.1 to hide the first frame flash?

Yes, according to Sotaro, the flash of the first frame is a gecko bug (bug 1084629, which depends on bug 1062134 -- both have been made 'blocking-b2g: 2.1?' today.

	Russ Nicoletti [:russn] Assignee
	Comment 27 • 10 years ago

Comment on attachment 8508021 [details]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25342

Review comments have been addressed; PR has been updated.

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

Comment on attachment 8508021 [details]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25342

Looks good to me!

	Russ Nicoletti [:russn] Assignee
	Comment 29 • 10 years ago

Master:

https://github.com/mozilla-b2g/gaia/commit/4d7f051cede6544f4c83580253c743c22b0cb279

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

Please nominate this patch for Gaia v2.1 approval when you get a chance.

	Russ Nicoletti [:russn] Assignee
	Comment 31 • 10 years ago

Comment on attachment 8508021 [details]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25342

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Legacy code in the video app whereby it manages the hw video codec. 

[User impact] if declined:
Bad user experience. Without the fix, the user sees a black screen before the video when re-opening video after it has been in the background.
 
[Testing completed]:
Manual testing and unit testing

[Risk to taking this patch] (and alternatives if risky):
Low. Change is to one video app js file. The changes didn't affect the semantics of the code as the unit tests passed without changes. 

Bug 1062134 needs to be fixed as well otherwise when the video app resumes from being in the background, the first frame of the video will appear briefly before gecko seeks to the proper frame (the one showing before the app went into the background).


[String changes made]:
None

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

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

	sarsenyev
	Comment 33 • 10 years ago

The issue still reproduces on 2.2 Flame

As soon as device resumes, video playback screen remains black for a few sec 
before actually showing the video frame

It's fixed on 2.1 as soon the device resumes, the video frame is shown immediately 

Still reproduces on:

Device: Flame 2.2 Master KK
BuildID: 20141104040207
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: 54d05732f29b
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Fixed on:

Device: Flame 2.1 KK
BuildID: 20141104001202
Gaia: 8b0cf889ae0d48a9eb7ecdcb9b67590de45cc5e5
Gecko: 388b03efe92d
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

	Brogan Zumwalt [Inactive]
	Comment 34 • 10 years ago

Issue no longer reproducing on Flame 2.2. Marking as verified fixed.

Actual Results: Issue as described is no longer occurring. When following STR from comment 0 on higher quality videos the user sees a black second for a fraction of a second. Resuming video then repeating process can exacerbate issue, but not beyond half a second for an HD video. Consulted with sarsenyev and neither of us are able to reproduce original issue on 2.2 any longer.

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141112040208
Gaia: 5ae28ff11b982e2bd7d1aa097cda131536952bdc
Gecko: 688f821edcd4
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0