1272919 - Video shows the frame that was on screen when I switched from youtube to a different tab for a fraction of a second

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Bad behavior due to regression

Original report: http://forums.mozillazine.org/viewtopic.php?p=14603429#p14603429

Reproducible : always

Steps To Reproduce:
1. Open youtube video in tab[A], (e.g., https://www.youtube.com/watch?v=BH0BNbwqNK8 ) 
   Change 720P or 1080P (this might not be necessary)
    ---- Remember what is displayed
2. Open any web page in tab[B]

Actual Results:
it shows the frame that was on screen when I switched from youtube to a different tab for a fraction of a second.

Expected Results:
Not so, Video should be displayed current frame.


Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=13e4cb81d4eb1c4996daf34c68301ca8a8ab0183&tochange=2fe467985f04512869ca7daa1a2db286642ce127

Regressed by: Bug 1224973

Comment 1

[Jean-Yves Avenard [:jya]]

Not sure I'm reading this properly as there's some conflict between the title and the description.

Are you saying that you're seeing the old video frame from prior the switch.

If so, I'm not sure there's much we can do about it as we have to seek to the new position to resume video playback.

Maybe we should only suspend the video after a few seconds rather than immediately once the video goes into the backround.

FWIW, it's also the same behaviour in Chrome when it suspend the video from playing when in the background.

This much lower the CPU usage

Comment 2

[randomreporter]

Hello!
Sorry for my poor English, I'll try to explain what's the problem:
Open tab [A] with any website
Open tab [B] with youtube 30 fps video
Let's say you keep watching video for 10 seconds and then switch back to tab [A]. That means that at the moment of switching there is frame number 30*10=300 on screen
Keep tab [A] active for 5 seconds and then switch to tab [B]. That means there should be frame number 300+5*30=450 on screen when tab [B] becomes active but instead you see frame number [300] for a fraction of a second.

Comment 3

[u480271]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> If so, I'm not sure there's much we can do about it as we have to seek to
> the new position to resume video playback.

Yes, this is due to suspending the video decoding when the tab is in the background to save energy.

> Maybe we should only suspend the video after a few seconds rather than
> immediately once the video goes into the backround.

We've talked about doing this but the frame will still be wrong when the video resumes from being suspended. Jean-yves, you said that Chrome has the same behaviour but could we instead blank the last frame so the video is black before the seek takes place?

Comment 4

[Marcia Knous [:marcia]]

Tracking for 49 - good to keep on eye on this one.

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The plan is to show a blank frame in the meanwhile instead.

Comment 7

[u480271]

Attachment: MozReview Request: Bug 1272919: Blank video frames when suspending decoding. r?jwwang

Blank out the video frame when suspending video decoding, so a black frame
is seen when resuming and not the frame previous to being suspended.

Review commit: https://reviewboard.mozilla.org/r/54560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54560/

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8755313 [details]
MozReview Request: Bug 1272919: Blank video frames when suspending decoding. r?jwwang

https://reviewboard.mozilla.org/r/54560/#review51144

::: dom/media/mediasink/MediaSink.h:122
(Diff revision 1)
>    // Called on the state machine thread to shut down the sink. All resources
>    // allocated by this sink should be released.
>    // Must be called after playback stopped.
>    virtual void Shutdown() {}
>  
> +  virtual void ResetVideo() {}

Document what this function do and its preconditions.

::: dom/media/mediasink/VideoSink.cpp:243
(Diff revision 1)
>  
>  void
> +VideoSink::ResetVideo()
> +{
> +  AssertOwnerThread();
> +  mVideoQueue.Reset();

It is bad to change a MediaQueue from "finished" to "unfinished". Please just pop all frames from the queue.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Can you check what will happen when playback reaches the last video frame? We would like to display the last frame instead of a blank one when switching back to the tab, right?

Comment 10

[u480271]

Comment on attachment 8755313 [details]
MozReview Request: Bug 1272919: Blank video frames when suspending decoding. r?jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54560/diff/1-2/

Comment 11

[u480271]

(In reply to JW Wang [:jwwang] from comment #9)
> Can you check what will happen when playback reaches the last video frame?
> We would like to display the last frame instead of a blank one when
> switching back to the tab, right?

I've been testing with a MP4 in <video> and YouTube, allowing the playback to end whilst in the back is in background and when switching back the last frame of the video is visible. (Not black, nor any visible seeking).

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8755313 [details]
MozReview Request: Bug 1272919: Blank video frames when suspending decoding. r?jwwang

https://reviewboard.mozilla.org/r/54560/#review51406

Comment 13

[u480271]

https://reviewboard.mozilla.org/r/54560/#review51144

> It is bad to change a MediaQueue from "finished" to "unfinished". Please just pop all frames from the queue.

I can't just call PopFront() since it notifies mPopEvent, so I'll extract the nsDeque::PopFront into a separate function.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Dan Glastonbury :kamidphish from comment #13)
> I can't just call PopFront() since it notifies mPopEvent, 
Is that a problem? The video frames are consumed by the VideoSink in the same sense anyway.

> so I'll extract the nsDeque::PopFront into a separate function.
It is a good idea to document that Drain() will not fire pop events.

Comment 15

[u480271]

This feature is enabled for nightly only at this point.

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Marking as P2 for now because the problem feature isn't riding the trains.

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 18

[Jim Mathies [:jimm]]

What's the situation with the patch posted here? Can we land it?

Also, is this feature going to roll out in 51? 52?

Comment 19

[u480271]

Aaah, thought this has landed. I'll take a look at what went wrong.

The feature is actively being worked on and at the moment we're keeping it enabled only on nightly. The plan is to limit video decode suspending to a very limited set of videos, mainly focused on large, banner videos such as at http://www.gyg.com.au. The feature will initially be disabled for Youtube.

Comment 20

[Tzuhao Kuo [:kaku]]

Reviewing the patch again, I think what the patch wanted to do is already be achieved by switching to blank decoder (Bug 1274626). Maybe we can just close this bug?

Comment 21

[u480271]

(In reply to Tzuhao Kuo [:kaku] (PTO until 10/2) from comment #20)
> Reviewing the patch again, I think what the patch wanted to do is already be
> achieved by switching to blank decoder (Bug 1274626). Maybe we can just
> close this bug?

The intention of the patch attached to this bug was to clear the VideoQueue when suspending video decoding. If that happens when we switch decoders than this is no longer needed. (From the work I'm testing in bug 1295921 it certainly appears so.)

Comment 22

[u480271]

This patch isn't need as it's handled by switching to blank/null decoder.

Comment 23

[Gerry Chang [:gchang]]

Mark 51 fixed as bug 1274626 was fixed.

Comment 24

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using old Nightly from 2016-05-14. Verified that the issue does not reproduce anymore using Fx 51 beta 10 across platforms (Windows 7 64-bit, Mac OS X 10.12.1 and Ubuntu 16.04 64-bit).