1307546 - memory burst and stop responding when playing specific video

Status: RESOLVED FIXED

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

Description

[YF (Yang)]

Caused by bug 1258870, tested by mozregression.

I can reproduce it on Fx50b4 ~ Nightly 52, whatever e10s and 32/64bit.


Steps to reproduce:
Install the https://greasyfork.org/scripts/22441-bilibili-html5%E6%92%AD%E6%94%BE%E5%99%A8 to activating HTML5 mode for bilibili.com. You can turn off it after the first visit.
Open the http://www.bilibili.com/video/av6535800/ (I only found this example).
Click the video to play, wait at least ten seconds, as well as observe the Firefox's process footprint.


Actual results:
Within 10 seconds of the start of the playback, the content process occupies many CPU resources, and the UI stops responding, the memory usage of the content process is growing.


Expected results:
Smooth playback, low CPU and memory usage.

Comment 1

[YF (Yang)]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8146a675a477c67534886dcf10937e62d0c1da68&tochange=4f39f2e1324b8c631dc700f86500f49b9680458d

Comment 2

[Jet Villegas (inactive)]

You can also get to this HTML5 site by spoofing the Safari user agent. I posted the video file to help isolate the issue:

http://media.junglecode.net/test/1307546/10632694-1hd.mp4

Yang: does it still happen for you if you view the URL above in Firefox?

Comment 3

[YF (Yang)]

Yes, I can reproduce it with this url in Firefox 50.0b4 with clean profile.

Thanks for posting this video, I did not get a working sample before.

Comment 4

[Jet Villegas (inactive)]

Chris: can you fire this up in a debugger and see what comes back? Thx!

Comment 5

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

Can't see CPU spikes on Linux by running
`./mach run http://media.junglecode.net/test/1307546/10632694-1hd.mp4`.

Hi Yang,
Can you repro the issue on Mac or Linux?

Comment 6

[YF (Yang)]

Please try to reproduce it using Firefox Beta on Windows.


I see the 100% CPU during playback and no playback started, buffer and adjust the time is normal, using http://ftp.mozilla.org/pub/firefox/releases/50.0b6/linux-x86_64/en-US/ on Lubuntu 14.04 x64, I'm not see the memory burst on the Linux.

Comment 7

[Andrew Overholt [:overholt]]

JW, can you try comment 6?

Comment 8

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

I can't repro the issue with http://ftp.mozilla.org/pub/firefox/releases/50.0b6/linux-x86_64/en-US/. There is not CPU spikes nor memory burst.

Comment 9

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

Hi Alastor,
Can you try to repro this issue on Windows?

Comment 10

[Alastor Wu [:alwu]]

I didn't see the memory burst in beta 50.0b10 on Windows 7, the video playback is smooth and I can still interact with UI interface.

But I observed the CPU usage when using HTML5 player is indeed higher than using flash player.
When using the HTML5 player, the CPU usage is 56~60%. 
When using flash player, the CPU usage is 15~20%.

Maybe the problem is their HTML player doesn't be implemented well causing the high CPU usage rate?

Comment 11

[YF (Yang)]

It seems to have been alleviated in 50.0b10, I can still reproduce it with https://ftp.mozilla.org/pub/firefox/releases/50.0b4/win32/.

Can you try the 50.0b4 to make sure this is not my environmental problems?


In 50.0b10, the memory GC looks normal, but CPU usage is still full while playing.

Comment 12

[Alastor Wu [:alwu]]

(In reply to YF (Yang) from comment #11)
> It seems to have been alleviated in 50.0b10, I can still reproduce it with
> https://ftp.mozilla.org/pub/firefox/releases/50.0b4/win32/.
> 
> Can you try the 50.0b4 to make sure this is not my environmental problems?
> 
> 
> In 50.0b10, the memory GC looks normal, but CPU usage is still full while
> playing.

Same result as comment10, I didn't see the memory burst in 50.0b4.

Comment 13

[YF (Yang)]

So, what should I do to get more debugging information?

Added, I can still reproduce it in latest Aurora and Nightly with clear profile and comment 2, it seems be relieved in the latest Beta only.

Comment 14

[Andrew Overholt [:overholt]]

Hmm. Did something recently change in this area in beta?

Comment 15

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

(In reply to Andrew Overholt [:overholt] from comment #14)
> Hmm. Did something recently change in this area in beta?

Not that I'm aware of.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

I cannot reproduce this issue with 50b10.

Yang: can you please open about:support and "Copy text to clipboard" and post that here?

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

We are definitely using more CPU in 50b10 than in 49 release on my machine. I will run a mozregression.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Mozregression points to bug 1258870, as reported by Yang. Bummer.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

[Tracking Requested - why for this release]:
This causes a spike in CPU usage when playing back video. It's a regression from bug 1258870. I propose we back out bug 1258870 from beta and rebuild the RC, and fix bug 1258870 in 51.

That's a shame, as bug 1258870 fixed a nasty A/V sync issue affecting Amazon Prime video.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

To help understand the severity, on my i7 4670K desktop, I'm seeing about 20% CPU utilization with the regression, and about 1% without. The reporter of this bug says Firefox is unusable when playing HD video with the regression on his machine.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

To back this out cleanly, we would need to backout the following changesets (in top to bottom order) from the 'release' branch:


changeset:   350216:7852ac7e74fa
user:        Chris Pearce <[e-mail]>
date:        Fri Aug 19 21:34:42 2016 +1200
summary:     Bug 1295630 - Provide a way for A/V sync to be ruined, so talos can test compositor throughput by painting expired frames. r=mattwoodrow, a=ritu

changeset:   350150:f04b44d05fcc
user:        Jean-Yves Avenard <[e-mail]>
date:        Mon Aug 29 15:46:56 2016 +1000
summary:     Bug 1298594 - P4. Pop the frame when current time is past the end of the current frame. r=jwwang, a=ritu

changeset:   349946:21fd3ea62d8c
user:        Chris Pearce <[e-mail]>
date:        Mon Aug 15 13:35:52 2016 +1200
summary:     Bug 1258870 - Don't push late video frames to the compositor, drop them. r=jwwang,a=ritu

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem is in VideoSink::UpdateRenderedVideoFrames(), here:

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/mediasink/VideoSink.cpp#445

The expression (nextFrameTime - clockTime) is negative. This results in us setting a timeout to run UpdateRenderedVideoFrames() again immediately.

The expression is negative because the file is badly muxed. When we first go bad, we hit two consecutive frames with these time ranges: [100000,200000] [67000,133000], that is the frame cover mutually ovelapping time ranges, and don't have monotonically increasing start times. In this example, if clockTime was 100001, clockTime would be greater than the start of the first frame (100000) but the start of the next frame (67000) minus clockTime is negative. So we end up hot-looping.

This is fortunate, since it means we only perform badly on badly muxed files. I have no idea how common files with this particular variation of bad muxing are.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1307546 - Ensure we don't set timers with negative intervals in to update A/V sync.

Our logic to do A/V sync sets a timer to drop expired frames based on the
start time of the next frame in the queue. If the frames in the queue are
badly muxed and don't have monotonically increasing start times, we can
end up setting a timer with a negative interval. This causes us to reevaluate
the frames in the VideoSink's queue immediately, set the same timer again,
and so we end up hot-looping.

This is a simple low-risk fix that detects when we're about to set a negative
interval timer, and instead sets the timer 1/30th of a second in the future.

This fix is deliberately low risk, such that it's suitable for uplift. I have
an idea how to do this better, but the lower risk this is most suitable for
uplift.

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

Comment 24

[Jean-Yves Avenard [:jya]]

I've seen a lot of files with out of order pts. All the sample files used in wpt to start with. 

Few amazon files are like this too

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807414 [details]
Bug 1307546 - Ensure we don't set timers with negative intervals in to update A/V sync.

https://reviewboard.mozilla.org/r/90550/#review90284

Comment 26

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

Comment on attachment 8807414 [details]
Bug 1307546 - Ensure we don't set timers with negative intervals in to update A/V sync.

https://reviewboard.mozilla.org/r/90550/#review90316

::: dom/media/mediasink/VideoSink.cpp:444
(Diff revision 1)
>    if (frames.Length() < 2) {
>      return;
>    }
>  
>    int64_t nextFrameTime = frames[1]->mTime;
> +  int64_t delta = (nextFrameTime > clockTime) ? (nextFrameTime - clockTime)

For small timeouts, the timer might fire immediately without the audio clock being progressed at all (depending on the resolution of the audio clock). In this situation the timer will hog the CPU for a short time and cause CPU usage peaks.

We might want to do
delta = std::max(nextFrameTime - clockTime, SOME_THRESHOLD);
to avoid the situation.

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807414 [details]
Bug 1307546 - Ensure we don't set timers with negative intervals in to update A/V sync.

https://reviewboard.mozilla.org/r/90550/#review90316

> For small timeouts, the timer might fire immediately without the audio clock being progressed at all (depending on the resolution of the audio clock). In this situation the timer will hog the CPU for a short time and cause CPU usage peaks.
> 
> We might want to do
> delta = std::max(nextFrameTime - clockTime, SOME_THRESHOLD);
> to avoid the situation.

if doing so, can't use the 30fps constant defined here!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5a00a0b689c
Ensure we don't set timers with negative intervals in to update A/V sync. r=jya

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5a00a0b689c

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Yang, could you please verify this issue is fixed as expected on a latest (11/05) Nightly build? Thanks!

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8807414 [details]
Bug 1307546 - Ensure we don't set timers with negative intervals in to update A/V sync.

Requesting uplift for Firefox 50 and 51.

Approval Request Comment
[Feature/regressing bug #]: Bug 1307546, regression from fixing A/V sync when the computer is too slow to playback video in real time.

[User impact if declined]: Some users on lower end hardware will find that video playback on some videos which are incorrectly encoded will cause unnecessarily high CPU usage, making Firefox not respond well or even lock up the UI.

[Describe test coverage new/current, TreeHerder]: It's very hard to write automated tests for a/v sync.

[Risks and why]: Low; this patch detects the degenerate case, and backs off to make it not hurt. The usual case of a video which is correctly encoded will not have its behaviour changed.

[String/UUID change made/needed]: None.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8807414 [details]
Bug 1307546 - Ensure we don't set timers with negative intervals in to update A/V sync.

We discussed this over email, this is a low risk fix in that it does not alter playback for correctly encoded video files. this fixes the case when we encounter a badly encoded file which leads to CPU usage spike. Aurora51+, Beta50+

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3888f1a5702b

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-release/rev/8e32fc90a6aa

Comment 35

[YF (Yang)]

With https://ftp.mozilla.org/pub/firefox/nightly/2016/11/2016-11-05-03-02-11-mozilla-central/firefox-52.0a1.en-US.win32.installer.exe and comment 2, smooth playback.

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from central (and autoland and inbound) for increased mda test_BufferingWait_mp4.html failure on Windows 7:

https://hg.mozilla.org/mozilla-central/rev/143ac19c32251f3cdbef47c7d1998fc870762b19

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d5a00a0b689c385afd53b7b93f41e244976e613f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6198971&repo=autoland

07:00:49     INFO - TEST-START | dom/media/mediasource/test/test_BufferingWait_mp4.html
07:06:07     INFO - TEST-INFO | started process screenshot
07:06:07     INFO - TEST-INFO | screenshot: exit 0
07:06:07     INFO - Buffered messages logged at 07:00:49
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | Receive a sourceopen event 
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | Should only receive one sourceopen for this test 
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | Create a SourceBuffer 
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | fetchWithXHR load uri='bipbop/bipbopinit.mp4' status=200 
07:06:07     INFO - Loading buffer: [0, 1395)
07:06:07     INFO - SourceBuffer buffered ranges grew from TimeRanges:  to TimeRanges: 
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | fetchWithXHR load uri='bipbop/bipbop1.m4s' status=200 
07:06:07     INFO - Loading buffer: [0, 24424)
07:06:07     INFO - SourceBuffer buffered ranges grew from TimeRanges:  to TimeRanges: [0.095, 0.835918)
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | fetchWithXHR load uri='bipbop/bipbop2.m4s' status=200 
07:06:07     INFO - Loading buffer: [0, 22205)
07:06:07     INFO - SourceBuffer buffered ranges grew from TimeRanges: [0.095, 0.835918) to TimeRanges: [0.095, 1.625396)
07:06:07     INFO - Buffered messages logged at 07:00:50
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | fetchWithXHR load uri='bipbop/bipbop5.m4s' status=200 
07:06:07     INFO - Loading buffer: [0, 18367)
07:06:07     INFO - SourceBuffer buffered ranges grew from TimeRanges: [0.095, 1.625396) to TimeRanges: [0.095, 1.625396)[3.298333, 4.040272)
07:06:07     INFO - Playing video. It should play for a bit, then fire 'waiting'
07:06:07     INFO - Got a waiting event at 0
07:06:07     INFO - Buffered messages logged at 07:00:51
07:06:07     INFO - Got a waiting event at 1.623945
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | Reached target time of: 1.57895 
07:06:07     INFO - TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | fetchWithXHR load uri='bipbop/bipbop3.m4s' status=200 
07:06:07     INFO - Loading buffer: [0, 24013)
07:06:07     INFO - SourceBuffer buffered ranges grew from TimeRanges: [0.095, 1.625396)[3.298333, 4.040272) to TimeRanges: [0.095, 2.414875)[3.298333, 4.040272)
07:06:07     INFO - Buffered messages logged at 07:00:52
07:06:07     INFO - Got a waiting event at 2.168117
07:06:07     INFO - Buffered messages finished
07:06:07     INFO - TEST-UNEXPECTED-FAIL | dom/media/mediasource/test/test_BufferingWait_mp4.html | Test timed out. 
07:06:07     INFO -     reportError@SimpleTest/TestRunner.js:114:7
07:06:07     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7
07:06:07     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:156:5

List of failures on OrangeFactor: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1293170&startday=2016-10-01&endday=2016-11-05&tree=all

Comment 37

[Phil Ringnalda (:philor)]

Despite that status flag change, it's not backed out on either mozilla-release or mozilla-aurora.

firefox50's status in particular is unchanged: I did 50 retriggers, opt and debug e10s and not, in https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=8e32fc90a6aadb62ab464ef56a53da8a5d66091a and I don't see anything there other than the slight possibility of an increase in the rate of bug 1282260, which probably isn't real (50 didn't get whatever magic caused that to go away on 52, and 50 runs is almost three weeks worth of mozilla-beta - in that time on beta we got 2 failures, and I triggered up 7, maybe something, maybe nothing).

firefox51 is actually worse than 52, since there it's a timeout in test_BufferingWait_mp4.html and a hang in test_DrainOnMissingData_mp4.html killing the rest of the run, so we frequently aren't actually running all the media tests on 51 anymore.

Comment 38

[Jean-Yves Avenard [:jya]]

I wouldn't disable the test. It's unreliable due to skip to next keyframe logic being randomly activated. This occurs when a machine is too slow to keep up A/V sync and video frames will be dropped. 

We have a pending bug opened to disable skip to next keyframe during testing

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

Given that this is failing on m-c but not on m-a or m-r, it must be interacting with something in m-c.

I'll do a regression range bisection search over m-c. On Try, as of course this doesn't fail locally.

I think we should proceed to ship this fix in 50 and 51, and we'll fix whatever's regressed it in 52.

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be404ef87ac

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2316974a92d3

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f067eeb7d07b

Comment 43

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=008ea4a8b38a

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69739c89f161

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40603f920d13

Comment 46

[Phil Ringnalda (:philor)]

(In reply to Chris Pearce (:cpearce) from comment #39)
> Given that this is failing on m-c but not on m-a or m-r, it must be
> interacting with something in m-c.

It is failing on m-a, in fact failing worse since either test_DrainOnMissingData_mp4.html or test_BufferingWait_mp4.html itself hangs for 330 seconds without output after the timeout in test_BufferingWait_mp4.html.

Comment 47

[Chris Pearce [:cpearce (Not reading bugmail)]]

But it's not failing on m-r? I see lots of retriggers of mda-e10s there, but no failures.

Comment 48

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98090384549e

Comment 49

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cbe7df71bb6

Comment 50

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5518d1db0223

Comment 51

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf7f89e88dec

Comment 52

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54837936c52a

Comment 53

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b71c02a7f3

Comment 54

[Phil Ringnalda (:philor)]

m-r? comment 37. m-a? comment 37.

Comment 55

[Chris Pearce [:cpearce (Not reading bugmail)]]

Changing FAILOVER_UPDATE_INTERVAL_US to 5ms makes us not fail, however I'm seeing a lot of timeouts in dom/media/test/test_bug1113600.html, which I'm not sure are related:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98090384549e&selectedJob=30529041

This seems to have regressed in 51 in between 23 Aug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f528ba3076535612efa6d6d8601691df0c373563
.. and 32 Aug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30dffb464ab0c5da396bc30f105f0020990cf70a&selectedJob=30528927

Comment 56

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

(In reply to Chris Pearce (:cpearce) from comment #55)
> Changing FAILOVER_UPDATE_INTERVAL_US to 5ms makes us not fail, however I'm
> seeing a lot of timeouts in dom/media/test/test_bug1113600.html, which I'm
> not sure are related:

Bug 1315586 should fix the timeout of test_bug1113600.html.

Comment 57

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andrei, could you please provide specific details on how much of a CPU spike are you noticing on OS X even with CPearce's latest fix? Thanks!

Comment 58

[Andrei Vaida [:avaida]]

(In reply to Ritu Kothari (:ritu) from comment #57)
> Hi Andrei, could you please provide specific details on how much of a CPU
> spike are you noticing on OS X even with CPearce's latest fix? Thanks!

We noticed spikes of around 40% on 50.0-build2, using two different machines with Mac OS X 10.11.6 and the video sample from Comment 2.

As a side note, on 49.0.2-build2 with the same machines/platforms, we noticed spikes of about 44%.

Comment 59

[Andrei Vaida [:avaida]]

Also, this is verified fixed on Windows 7 x64 and Windows 10 x64. We were unable to replicate this bug on Linux, so I assume that's unaffected across branches.

I'm not sure exactly what's the best way to point out these results in this bug. Is this fix specifically for Windows? If so, we can mark it verified fixed and just file a follow up bug for Mac, if need be.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Setting the 51 status back to fixed to reflect that it wasn't ever backed out from Aurora.

Comment 61

[Chris Pearce [:cpearce (Not reading bugmail)]]

Looks like what jwwang suggested in comment 26 is working, provided Bug 1315586 is included as an ancestor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5c2f54e435f&selectedJob=30534990

Comment 62

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #58)
> (In reply to Ritu Kothari (:ritu) from comment #57)
> > Hi Andrei, could you please provide specific details on how much of a CPU
> > spike are you noticing on OS X even with CPearce's latest fix? Thanks!
> 
> We noticed spikes of around 40% on 50.0-build2, using two different machines
> with Mac OS X 10.11.6 and the video sample from Comment 2.
> 
> As a side note, on 49.0.2-build2 with the same machines/platforms, we
> noticed spikes of about 44%.

Given that is not the same regressions as what was introduced in 50, we can afford to not hold the release on this.

We've been aware that playing video on MacOS often results in the CPU fan spinning up, which indicates higher CPU usage, so I expect you're seeing that.

Comment 63

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

[Tracking Requested - why for this release]: Let's try to get this fixed in 51.

Comment 64

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1307546 - Give VideoSink a minimum update interval.

Without this, we can end up with very short, or even negative, update
intervals, meaning we unnecessarily use CPU when we can't actually
advance the playback position.

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

Comment 65

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

Comment on attachment 8808347 [details]
Bug 1307546 - Give VideoSink a minimum update interval.

https://reviewboard.mozilla.org/r/91166/#review91062

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9b7b51d1416
Give VideoSink a minimum update interval. r=jwwang

Comment 67

[Liz Henry (:lizzard) (relman/hg->git project)]

cpearce: does some of this still need to land in aurora? It's unclear here if these patches are for uplift or if you need to uplift from bug 1315586. As long as you have it on your radar and know what to request uplift for, great.

Comment 68

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes. We'll need the new patch in this bug, and bug 1315586. I'll request here and in bug 1315586. I was waiting to make sure I wasn't backed out again.

Comment 69

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9b7b51d1416

Comment 70

[Gerry Chang [:gchang]]

Track 51- as this was fixed in 51.

Comment 71

[Gerry Chang [:gchang]]

Hi :cpearce,
The first patch in m-c was backed out in comment #36 but was not backed out in aurora & beta. Now, you have a new patch and re-land in m-c in comment #69, do we need to back out codes in aurora and re-land your new patch in aurora?

Comment 72

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've rebased my second patch on top of the first, it looks green in Aurora:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98574795b210e5ecdd4f050fe8be1706ae4f2474

bug 1315586 doesn't apply to Aurora, and the above Try push is on Aurora without bug 1315586, so I think we can avoid uplifting bug 1315586 as well and just take my second patch here.

Comment 73

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Give VideoSink a minimum update interval

Patch 2; rebased on Aurora

Comment 74

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8809516 [details] [diff] [review]
Give VideoSink a minimum update interval

Approval Request Comment
[Feature/regressing bug #]: Bug 1307546, and earlier patch in this bug; regression from fixing A/V sync when the computer is too slow to playback video in real time.

[User impact if declined]: Intermittent test failures on Aurora.

[Describe test coverage new/current, TreeHerder]: This fixes test failures on Aurora.

[Risks and why]: Low; this is a further back-off in the degenerate case. So it should be safe.

[String/UUID change made/needed]: None.

Comment 75

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8809516 [details] [diff] [review]
Give VideoSink a minimum update interval

Fix for performance issues with video syncing. OK to uplift to aurora.

Comment 76

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes or Tomcat, this may not show up in your queries for 51/aurora uplift because the status flag is already set to fixed. Can you please merge the patch in comment 74 to aurora? thanks!

Comment 77

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea8a661310c1

Comment 78

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Aurora 52.0a2 2016-11-22 under Win 10 64-bit.
Considering this and the above comments, I'm removing the qe-verify+ flag.