Bug 170599 – [GTK] Use the DisplayRefreshMonitor facilities

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Zan Dobersek]

Instead of relying on a dumb timer in ThreadedCompositor, we should follow the vsync updates as reported by EGL/GLX, and coordinate frame completions with a DisplayRefreshMonitor object instance that lives on the main thread.

[Zan Dobersek]

Created attachment 306494 [details]
WIP patch

[Zan Dobersek]

(In reply to Zan Dobersek from comment #1)
> Created attachment 306494 [details]
> WIP patch

This still needs more work, but it gives the idea.

[Zan Dobersek]

Created attachment 306666 [details]
WIP

[Carlos Garcia Campos]

Comment on attachment 306494 [details]
WIP patch

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

Thanks for working on this. I see this is enabling refresh monitor but only implementing it in the threaded compositor, what happens in case of RAF animation with accelerated compositing disabled?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:50
> +#if PLATFORM(WPE)

I guess you mean GTK here.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:125
> +    if (currentRootLayer->descendantsOrSelfHaveRunningAnimations() && m_client)
> +        m_client->updateViewport();

Why? is it because you know this is always happening in the compositing thread and then we don't need to go through dispatchOnClientRunLoop? It looks unrelated to this patch in any case.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:173
> -    updateViewport();
> +    if (m_client)
> +        m_client->updateViewport();

Ditto. Also, these are the only callers of updateViewport, so if we do these changes we should remove updateViewport(). Is onNewBufferAvailable guaranteed to be called from the compositing thread? I would add an asert there just in case.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:27
> +#include "TCDisplayRefreshMonitor.h"

I think this file should be called DisplayRefreshMonitorTC or even better DisplayRefreshMonitorThreadedCompositor, since it's the DisplayRefreshMonitor implementation for the threaded compositor.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:33
> +#include <glib.h>

I don't think we need glib here.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:44
> +    m_displayRefreshTimer.setPriority(RunLoopSourcePriority::LayerFlushTimer);

I think we should give a name to this timer priority.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:53
> +    if (isPreviousFrameDone() && m_compositor) {

m_compositor being nullptr means the monitor has been invalidated, do we want to set is scheduled in that case too? Wouldn't it be better to early return in case m_compositor is nullptr?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:56
> +        m_compositor->m_coordinateUpdateCompletionWithClient.store(true);
> +        if (!m_compositor->m_compositingRunLoop->isActive())
> +            m_compositor->m_compositingRunLoop->scheduleUpdate();

Maybe we could add a method to ThreadedCompositor coordinateUpdateCompletionWithClient() that does this, since this is also duplicated below. I also don't like this friendship, I prefer to add public methods to the threaded compositor.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:70
> +    m_displayRefreshTimer.startOneShot(0);

So, this timer is only to do the displayRefreshCallback in the main thread? and the high priority to do that asap?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:75
> +    m_compositor = nullptr;

I think we should probably stop the timer here too.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:94
> +        if (m_compositor->m_clientRendersNextFrame.compareExchangeStrong(true, false))
> +            m_compositor->m_scene->renderNextFrame();

ThreadedCompositor::renderNextFrameIfNeeded()?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:100
> +                m_compositor->m_compositingRunLoop->scheduleUpdate();

In the case of the GTK+ port we shouldn't schedule this update immediately, we should check the time since the last frame to ensure we don not render at more than 60fps, because we are not actually using the display refresh in the web process.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.h:27
> +#ifndef ThreadedCompositor_DisplayRefreshMonitor_h
> +#define ThreadedCompositor_DisplayRefreshMonitor_h

This should be DisplayRefreshMonitorThreadedCompositor

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.h:32
> +

Extra line here.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:61
> +    , m_displayRefreshMonitor(adoptRef(new WebKit::DisplayRefreshMonitor(*this)))

We are inside WebKit namespace here. I would add a create method that returns a Ref instead of doing the adop + new here, though.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:220
> +    // For the GTK+ port, {egl,glx}SwapBuffers() does the frame sync.
> +    sceneUpdateFinished();

This is not true. In the GTK+ port the frame sync is done by GTK+ in the UI process. EGL/GLX frame sync is disabled in the web process because we are not rendering to the screen.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:255
> +RefPtr<WebCore::DisplayRefreshMonitor> ThreadedCompositor::createDisplayRefreshMonitor(PlatformDisplayID)

I would call this displayRefreshMonitor(), since this is not creating anything.

[Carlos Garcia Campos]

Comment on attachment 306494 [details]
WIP patch

I was reviewing this while the new patch was submitted.

[Zan Dobersek]

Comment on attachment 306494 [details]
WIP patch

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

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:125
>> +        m_client->updateViewport();
> 
> Why? is it because you know this is always happening in the compositing thread and then we don't need to go through dispatchOnClientRunLoop? It looks unrelated to this patch in any case.

We don't need dispatched on the composition thread anymore because we're now updating an atomic variable. It is related to this patch.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:27
>> +#include "TCDisplayRefreshMonitor.h"
> 
> I think this file should be called DisplayRefreshMonitorTC or even better DisplayRefreshMonitorThreadedCompositor, since it's the DisplayRefreshMonitor implementation for the threaded compositor.

I'll be using ThreadedDisplayRefreshMonitor.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:44
>> +    m_displayRefreshTimer.setPriority(RunLoopSourcePriority::LayerFlushTimer);
> 
> I think we should give a name to this timer priority.

It should have the same value as the LayerFlushTimer priority, so I don't know whether it makes sense to add a new one.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:53
>> +    if (isPreviousFrameDone() && m_compositor) {
> 
> m_compositor being nullptr means the monitor has been invalidated, do we want to set is scheduled in that case too? Wouldn't it be better to early return in case m_compositor is nullptr?

OK.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:70
>> +    m_displayRefreshTimer.startOneShot(0);
> 
> So, this timer is only to do the displayRefreshCallback in the main thread? and the high priority to do that asap?

Priority is as high as it is for the layer flush timer.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:75
>> +    m_compositor = nullptr;
> 
> I think we should probably stop the timer here too.

OK.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:100
>> +                m_compositor->m_compositingRunLoop->scheduleUpdate();
> 
> In the case of the GTK+ port we shouldn't schedule this update immediately, we should check the time since the last frame to ensure we don not render at more than 60fps, because we are not actually using the display refresh in the web process.

Figures.

>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:61
>> +    , m_displayRefreshMonitor(adoptRef(new WebKit::DisplayRefreshMonitor(*this)))
> 
> We are inside WebKit namespace here. I would add a create method that returns a Ref instead of doing the adop + new here, though.

The problem was that it clashed with WebCore::DisplayRefreshMonitor.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #6)
> Comment on attachment 306494 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306494&action=review
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:125
> >> +        m_client->updateViewport();
> > 
> > Why? is it because you know this is always happening in the compositing thread and then we don't need to go through dispatchOnClientRunLoop? It looks unrelated to this patch in any case.
> 
> We don't need dispatched on the composition thread anymore because we're now
> updating an atomic variable. It is related to this patch.

In that case, since updateViewport is now thread safe, I would change the updateViewport impl to not schedule to the compositing thread, instead of modifying the callers and leaving updateViewport unused.

> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:27
> >> +#include "TCDisplayRefreshMonitor.h"
> > 
> > I think this file should be called DisplayRefreshMonitorTC or even better DisplayRefreshMonitorThreadedCompositor, since it's the DisplayRefreshMonitor implementation for the threaded compositor.
> 
> I'll be using ThreadedDisplayRefreshMonitor.
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:44
> >> +    m_displayRefreshTimer.setPriority(RunLoopSourcePriority::LayerFlushTimer);
> > 
> > I think we should give a name to this timer priority.
> 
> It should have the same value as the LayerFlushTimer priority, so I don't
> know whether it makes sense to add a new one.

We decided to choose names based on what the timer does instead of generic names like urgent, high, medium, low, etc. That's what would allow us to use different values for the same timer in different ports. We have several priorities with the same value already.

> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:53
> >> +    if (isPreviousFrameDone() && m_compositor) {
> > 
> > m_compositor being nullptr means the monitor has been invalidated, do we want to set is scheduled in that case too? Wouldn't it be better to early return in case m_compositor is nullptr?
> 
> OK.
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:70
> >> +    m_displayRefreshTimer.startOneShot(0);
> > 
> > So, this timer is only to do the displayRefreshCallback in the main thread? and the high priority to do that asap?
> 
> Priority is as high as it is for the layer flush timer.
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:75
> >> +    m_compositor = nullptr;
> > 
> > I think we should probably stop the timer here too.
> 
> OK.
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/TCDisplayRefreshMonitor.cpp:100
> >> +                m_compositor->m_compositingRunLoop->scheduleUpdate();
> > 
> > In the case of the GTK+ port we shouldn't schedule this update immediately, we should check the time since the last frame to ensure we don not render at more than 60fps, because we are not actually using the display refresh in the web process.
> 
> Figures.
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:61
> >> +    , m_displayRefreshMonitor(adoptRef(new WebKit::DisplayRefreshMonitor(*this)))
> > 
> > We are inside WebKit namespace here. I would add a create method that returns a Ref instead of doing the adop + new here, though.
> 
> The problem was that it clashed with WebCore::DisplayRefreshMonitor.

[Zan Dobersek]

Created attachment 306671 [details]
WIP

Still needs the helper methods, at least.

[Zan Dobersek]

Created attachment 306678 [details]
Patch

[Zan Dobersek]

Created attachment 306682 [details]
Patch

[Build Bot]

Attachment 306682 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:61:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 306682 [details]
Patch

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

I've been doing tests in both X11 and Wayland enabling again the sync frame and not using the timer introduced in this patch, and things seem to work fine. So, the performance of resizes in wayland might have been fixed somehow, not sure if it's mesa, wayland or WebKit. So, to make things easier let's forget about the timer for now and enable frame sync.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:-135
> -    if (!m_client)
> -        return;
> -    dispatchOnClientRunLoop([this] {
> -        if (m_client)
> -            m_client->updateViewport();
> -    });

Please add an assert here to ensure it's called from the compositing thread.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:197
> +#ifndef NDEBUG
> +bool CompositingRunLoop::isCurrent()
> +{
> +    return &RunLoop::current() == &m_runLoop;
> +}
> +#endif

This doesn't seem to be used.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:61
> +    , m_displayRefreshMonitor(adoptRef(new ThreadedDisplayRefreshMonitor(*this)))

Could this be a Ref instead of RefPtr?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:75
> +
> +        m_frameCompletion.timer = std::make_unique<RunLoop::Timer<ThreadedCompositor>>(RunLoop::current(), this, &ThreadedCompositor::performFrameCompletion);

Let's remove this.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:186
>  void ThreadedCompositor::updateViewport()
>  {
> -    m_compositingRunLoop->startUpdateTimer(CompositingRunLoop::WaitUntilNextFrame);
> +    m_compositingRunLoop->scheduleUpdate();
>  }
>  
>  void ThreadedCompositor::scheduleDisplayImmediately()
>  {
> -    m_compositingRunLoop->startUpdateTimer(CompositingRunLoop::Immediate);
> +    m_compositingRunLoop->scheduleUpdate();
>  }

I think it's clearer if updateViewport() calls scheduleDisplayImmediately() instead of having two methods with the same implementation. I wonder if if we could get rid of scheduleDisplayImmediately() now and use m_compositingRunLoop->scheduleUpdate(); or if we keep it, we can rename it as scheduleDisplay(), because now it's always immediately and there's no other way to schedule a display.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:224
> +
> +    const static double targetFPS = 60;
> +    double nextUpdateTime = std::max((1 / targetFPS) - (monotonicallyIncreasingTime() - m_frameCompletion.lastTime), 0.0);
> +    m_frameCompletion.timer->startOneShot(1_s * nextUpdateTime);

So, do not remove the m_doFrameSync yet, and simply use it here to decide whether to call sceneUpdateFinished() or not. Remember to update the layer tree host to pass ShouldDoFrameSync::Yes. We can remove the option entirely in the future if we don't use it at all, but for now, it helps to leave for testing both ways.

> Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:229
> +    if (!m_layerTreeHost)

if (!m_layerTreeHost || m_wantsToExitAcceleratedCompositingMode || exitAcceleratedCompositingModePending())

[Zan Dobersek]

Created attachment 306816 [details]
Patch for landing

Cleaned up, addressed review comments. Still needs updated ChangeLogs.

[Carlos Garcia Campos]

Comment on attachment 306816 [details]
Patch for landing

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

LGTM, thanks!

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:-135
> -    if (!m_client)
> -        return;
> -    dispatchOnClientRunLoop([this] {
> -        if (m_client)
> -            m_client->updateViewport();
> -    });

Please add an assert here to ensure it's called from the compositing thread.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:214
> +    sceneUpdateFinished();

My idea was to do this here only when doing frame sync, but then it will not work when not doing it unless we add the timer back, so let's leave it this way.

[Zan Dobersek]

Comment on attachment 306816 [details]
Patch for landing

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

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:-135
>> -    });
> 
> Please add an assert here to ensure it's called from the compositing thread.

It's not necessarily called on the composition thread, but we don't need to dispatch a new task anymore on that thread for the call to have an effect.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #15)
> Comment on attachment 306816 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306816&action=review
> 
> >> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:-135
> >> -    });
> > 
> > Please add an assert here to ensure it's called from the compositing thread.
> 
> It's not necessarily called on the composition thread, but we don't need to
> dispatch a new task anymore on that thread for the call to have an effect.

Ah, right, it was because now it only updates an atomic, sorry for the noise.

[Zan Dobersek]

Comment on attachment 306816 [details]
Patch for landing

Clearing flags on attachment: 306816

Committed r215259: <http://trac.webkit.org/changeset/215259>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.