Bug 181396 – Many CVDisplayLink threads created and destroyed while watching a YouTube video

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Many CVDisplayLink threads created and destroyed while watching YouTube video

[Jer Noble]

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.

[Jer Noble]

Created attachment 330739 [details]
Patch

[Simon Fraser (smfr)]

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.

[Jer Noble]

(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.

[Jer Noble]

Created attachment 330749 [details]
Patch for landing

[Jer Noble]

(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.

[WebKit Commit Bot]

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

[Jer Noble]

Created attachment 330838 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 330838 [details]
Patch for landing

Clearing flags on attachment: 330838

Committed r226664: <https://trac.webkit.org/changeset/226664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36388728>