Closed
Bug 1272919
Opened 8 years ago
Closed 8 years ago
Video shows the frame that was on screen when I switched from youtube to a different tab for a fraction of a second
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
platform-rel | --- | + |
firefox47 | --- | unaffected |
firefox48 | --- | unaffected |
firefox49 | + | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | verified |
People
(Reporter: alice0775, Assigned: u480271)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [platform-rel-Youtube])
Attachments
(1 file)
[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•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(dglastonbury)
Comment 2•8 years ago
|
||
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.
(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?
Flags: needinfo?(dglastonbury)
The plan is to show a blank frame in the meanwhile instead.
Updated•8 years ago
|
Priority: -- → P1
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/
Attachment #8755313 -
Flags: review?(jwwang)
Comment 8•8 years ago
|
||
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.
Attachment #8755313 -
Flags: review?(jwwang)
Comment 9•8 years ago
|
||
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?
Assignee | ||
Comment 10•8 years ago
|
||
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/
Attachment #8755313 -
Flags: review?(jwwang)
Assignee | ||
Comment 11•8 years ago
|
||
(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).
Updated•8 years ago
|
Attachment #8755313 -
Flags: review?(jwwang) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8755313 [details]
MozReview Request: Bug 1272919: Blank video frames when suspending decoding. r?jwwang
https://reviewboard.mozilla.org/r/54560/#review51406
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
(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.
Updated•8 years ago
|
platform-rel: --- → ?
Updated•8 years ago
|
Whiteboard: [platform-rel-Youtube]
Assignee | ||
Comment 15•8 years ago
|
||
This feature is enabled for nightly only at this point.
Marking as P2 for now because the problem feature isn't riding the trains.
Priority: P1 → P2
Mass change P2 -> P3
Priority: P2 → P3
Updated•8 years ago
|
status-firefox50:
--- → affected
Updated•8 years ago
|
status-firefox47:
--- → unaffected
Version: 50 Branch → Trunk
Updated•8 years ago
|
platform-rel: ? → +
Comment 18•8 years ago
|
||
What's the situation with the patch posted here? Can we land it?
Also, is this feature going to roll out in 51? 52?
status-firefox51:
--- → affected
Flags: needinfo?(dglastonbury)
Assignee | ||
Comment 19•8 years ago
|
||
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.
Flags: needinfo?(dglastonbury)
Updated•8 years ago
|
Updated•8 years ago
|
Rank: 99
Comment 20•8 years ago
|
||
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?
Flags: needinfo?(dglastonbury)
Assignee | ||
Comment 21•8 years ago
|
||
(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.)
Flags: needinfo?(dglastonbury)
Assignee | ||
Comment 22•8 years ago
|
||
This patch isn't need as it's handled by switching to blank/null decoder.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 23•8 years ago
|
||
Mark 51 fixed as bug 1274626 was fixed.
Updated•8 years ago
|
Flags: qe-verify+
Comment 24•8 years ago
|
||
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).
You need to log in
before you can comment on or make changes to this bug.
Description
•