[webkit-reviews] review granted: [Bug 170599] [GTK] Use the DisplayRefreshMonitor facilities : [Attachment 306682] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 170599: [GTK] Use the DisplayRefreshMonitor facilities
https://bugs.webkit.org/show_bug.cgi?id=170599

Attachment 306682: Patch

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




--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 306682
  --> https://bugs.webkit.org/attachment.cgi?id=306682
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())