Bug 181396

Summary: Many CVDisplayLink threads created and destroyed while watching a YouTube video
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=216195
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Jer Noble 2018-01-08 12:49:42 PST
Many CVDisplayLink threads created and destroyed while watching YouTube video
Comment 1 Jer Noble 2018-01-08 13:01:19 PST
YouTube's media controls (and the default media controls on Mac & iOS) appear to use requestAnimationFrame() to implement layout changes during playback. The most common event which triggers layout changes is the 'timeupdate' event, which according to the HTML spec, should fire every 15 to 250ms. WebKit picks the slowest update value, 250ms.

The DisplayRefreshMonitor implements the timer which triggers rAF, and it will currently destroy the underlying platform object which handles the display refresh notification if there are no rAF clients for 10 consecutive display refresh intervals, or: 10 / 60hz = 166ms.

So a naive client that does something like: video.addEventListener('timeupdate', event => { doUpdate(event); }) will cause the platform rAF object to be created and destroyed 4x a second.

Lets update the constant in DisplayRefreshMonitor to, e.g., 22 consecutive display intervals without a client (or 366ms), so that clients of "slow" firing consistently events like 'timeupdate' and 'progress' don't inadvertently cause a lot of tear-down/bring-up overhead.
Comment 2 Jer Noble 2018-01-08 14:50:20 PST
Created attachment 330739 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-01-08 15:20:15 PST
Comment on attachment 330739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330739&action=review

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:59
> +        const int maxInactiveFireCount = 20;

I wonder if we should make this a Seconds and do it based on time, rather than fire count?

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:110
> +    LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::displayDidRefresh() - destroying monitor %p", &monitor);

You should log when the rAF fires too.
Comment 4 Jer Noble 2018-01-08 15:51:54 PST
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 330739 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330739&action=review
> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:59
> > +        const int maxInactiveFireCount = 20;
> 
> I wonder if we should make this a Seconds and do it based on time, rather
> than fire count?

That would definitely future proof us against future non-60fps monitor refresh rates. I'll file a follow-up bug.

> > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:110
> > +    LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::displayDidRefresh() - destroying monitor %p", &monitor);
> 
> You should log when the rAF fires too.
Comment 5 Jer Noble 2018-01-08 15:52:37 PST
Created attachment 330749 [details]
Patch for landing
Comment 6 Jer Noble 2018-01-08 15:55:59 PST
(In reply to Jer Noble from comment #4)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 330739 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=330739&action=review
> > 
> > > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:59
> > > +        const int maxInactiveFireCount = 20;
> > 
> > I wonder if we should make this a Seconds and do it based on time, rather
> > than fire count?
> 
> That would definitely future proof us against future non-60fps monitor
> refresh rates. I'll file a follow-up bug.

Filed bug# 181411.
Comment 7 WebKit Commit Bot 2018-01-08 16:47:09 PST
Comment on attachment 330749 [details]
Patch for landing

Rejecting attachment 330749 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 330749, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/5980111
Comment 8 Jer Noble 2018-01-09 11:11:17 PST
Created attachment 330838 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-01-09 15:47:40 PST
Comment on attachment 330838 [details]
Patch for landing

Clearing flags on attachment: 330838

Committed r226664: <https://trac.webkit.org/changeset/226664>
Comment 10 WebKit Commit Bot 2018-01-09 15:47:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-01-09 15:51:14 PST
<rdar://problem/36388728>