[CoordGraphics] Revalidate need for 'coordinated update completion' in ThreadedCompositor
authormagomez@igalia.com <magomez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Apr 2019 08:53:36 +0000 (08:53 +0000)
committermagomez@igalia.com <magomez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Apr 2019 08:53:36 +0000 (08:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188839

Reviewed by Žan Doberšek.

Even if a platform layer has changed, don't wait for a main thread callback to finish the
CompositingRunLoop update. It can be finished as soon as we receive the frameComplete signal.

* Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:
(WebKit::CompositingRunLoop::scheduleUpdate):
(WebKit::CompositingRunLoop::stopUpdates):
(WebKit::CompositingRunLoop::updateCompleted):
(WebKit::CompositingRunLoop::updateTimerFired):
(WebKit::CompositingRunLoop::compositionCompleted): Deleted.
* Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.h:
* Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:
(WebKit::ThreadedCompositor::renderLayerTree):
(WebKit::ThreadedCompositor::sceneUpdateFinished):
(WebKit::ThreadedCompositor::displayRefreshMonitor):
(WebKit::ThreadedCompositor::handleDisplayRefreshMonitorUpdate): Deleted.
* Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:
* WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:
(WebKit::LayerTreeHost::handleDisplayRefreshMonitorUpdate):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243865 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp
Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.h
Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp
Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h
Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp

index 720429b..4e26112 100644 (file)
@@ -1,3 +1,29 @@
+2019-04-04  Miguel Gomez  <magomez@igalia.com>
+
+        [CoordGraphics] Revalidate need for 'coordinated update completion' in ThreadedCompositor
+        https://bugs.webkit.org/show_bug.cgi?id=188839
+
+        Reviewed by Žan Doberšek.
+
+        Even if a platform layer has changed, don't wait for a main thread callback to finish the
+        CompositingRunLoop update. It can be finished as soon as we receive the frameComplete signal.
+
+        * Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:
+        (WebKit::CompositingRunLoop::scheduleUpdate):
+        (WebKit::CompositingRunLoop::stopUpdates):
+        (WebKit::CompositingRunLoop::updateCompleted):
+        (WebKit::CompositingRunLoop::updateTimerFired):
+        (WebKit::CompositingRunLoop::compositionCompleted): Deleted.
+        * Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.h:
+        * Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:
+        (WebKit::ThreadedCompositor::renderLayerTree):
+        (WebKit::ThreadedCompositor::sceneUpdateFinished):
+        (WebKit::ThreadedCompositor::displayRefreshMonitor):
+        (WebKit::ThreadedCompositor::handleDisplayRefreshMonitorUpdate): Deleted.
+        * Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:
+        * WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:
+        (WebKit::LayerTreeHost::handleDisplayRefreshMonitorUpdate):
+
 2019-04-03  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [ATK] Cleanup WebPageAccessibilityObjectAtk
index c8084bd..715857a 100644 (file)
@@ -120,8 +120,8 @@ void CompositingRunLoop::scheduleUpdate(LockHolder& stateLocker)
     // An update was requested. Depending on the state:
     //  - if Idle, enter the Scheduled state and start the update timer,
     //  - if Scheduled, do nothing,
-    //  - if InProgress or PendingCompletion, mark an update as pending, meaning another
-    //    update will be scheduled as soon as the current one is completed.
+    //  - if InProgress mark an update as pending, meaning another update will be
+    //    scheduled as soon as the current one is completed.
 
     UNUSED_PARAM(stateLocker);
 
@@ -134,7 +134,6 @@ void CompositingRunLoop::scheduleUpdate(LockHolder& stateLocker)
     case UpdateState::Scheduled:
         return;
     case UpdateState::InProgress:
-    case UpdateState::PendingCompletion:
         m_state.pendingUpdate = true;
         return;
     }
@@ -146,48 +145,15 @@ void CompositingRunLoop::stopUpdates()
 
     LockHolder locker(m_state.lock);
     m_updateTimer.stop();
-    m_state.composition = CompositionState::Idle;
     m_state.update = UpdateState::Idle;
     m_state.pendingUpdate = false;
 }
 
-void CompositingRunLoop::compositionCompleted(LockHolder& stateLocker)
-{
-    // Composition has been signaled as completed, pushing the state into Idle.
-    // Depending on the state of the scene update:
-    //  - if Idle, Scheduled or InProgress, do nothing,
-    //  - if PendingCompletion, schedule a new update in case a pending update was marked,
-    //    or push the scene update state into Idle otherwise.
-
-    UNUSED_PARAM(stateLocker);
-
-    m_state.composition = CompositionState::Idle;
-
-    switch (m_state.update) {
-    case UpdateState::Idle:
-    case UpdateState::Scheduled:
-    case UpdateState::InProgress:
-        return;
-    case UpdateState::PendingCompletion:
-        if (m_state.pendingUpdate) {
-            m_state.pendingUpdate = false;
-            m_state.update = UpdateState::Scheduled;
-            if (!m_state.isSuspended)
-                m_updateTimer.startOneShot(0_s);
-            return;
-        }
-
-        m_state.update = UpdateState::Idle;
-        return;
-    }
-}
-
 void CompositingRunLoop::updateCompleted(LockHolder& stateLocker)
 {
     // Scene update has been signaled as completed. Depending on the state:
     //  - if Idle, Scheduled or InProgress, do nothing,
-    //  - if InProgress, push the state into PendingCompletion if the composition state is
-    //    InProgress, otherwise schedule a new update in case a pending update was marked,
+    //  - if InProgress, schedule a new update in case a pending update was marked,
     //    otherwise push the scene update state into Idle.
 
     UNUSED_PARAM(stateLocker);
@@ -197,11 +163,6 @@ void CompositingRunLoop::updateCompleted(LockHolder& stateLocker)
     case UpdateState::Scheduled:
         return;
     case UpdateState::InProgress:
-        if (m_state.composition == CompositionState::InProgress) {
-            m_state.update = UpdateState::PendingCompletion;
-            return;
-        }
-
         if (m_state.pendingUpdate) {
             m_state.pendingUpdate = false;
             m_state.update = UpdateState::Scheduled;
@@ -212,19 +173,16 @@ void CompositingRunLoop::updateCompleted(LockHolder& stateLocker)
 
         m_state.update = UpdateState::Idle;
         return;
-    case UpdateState::PendingCompletion:
-        return;
     }
 }
 
 void CompositingRunLoop::updateTimerFired()
 {
     {
-        // Both composition and scene update are now in progress.
+        // Scene update is now in progress.
         LockHolder locker(m_state.lock);
         if (m_state.isSuspended)
             return;
-        m_state.composition = CompositionState::InProgress;
         m_state.update = UpdateState::InProgress;
     }
     m_updateFunction();
index 4916282..59400cb 100644 (file)
@@ -56,19 +56,13 @@ public:
     void scheduleUpdate(LockHolder&);
     void stopUpdates();
 
-    void compositionCompleted(LockHolder&);
     void updateCompleted(LockHolder&);
 
 private:
-    enum class CompositionState {
-        Idle,
-        InProgress,
-    };
     enum class UpdateState {
         Idle,
         Scheduled,
         InProgress,
-        PendingCompletion,
     };
 
     void updateTimerFired();
@@ -81,7 +75,6 @@ private:
 
     struct {
         Lock lock;
-        CompositionState composition { CompositionState::Idle };
         UpdateState update { UpdateState::Idle };
         bool pendingUpdate { false };
         bool isSuspended { false };
index 7dd7f6d..e0cdeed 100644 (file)
@@ -226,27 +226,6 @@ void ThreadedCompositor::renderLayerTree()
         if (!states.isEmpty()) {
             // Client has to be notified upon finishing this scene update.
             m_attributes.clientRendersNextFrame = true;
-
-            // Coordinate scene update completion with the client in case of changed or updated platform layers.
-            // But do not change coordinateUpdateCompletionWithClient while in force repaint because that
-            // demands immediate scene update completion regardless of platform layers.
-            // FIXME: Check whether we still need all this coordinateUpdateCompletionWithClient logic.
-            // https://bugs.webkit.org/show_bug.cgi?id=188839
-            // Relatedly, we should only ever operate with a single Nicosia::Scene object, not with a Vector
-            // of CoordinatedGraphicsState instances (which at this time will all contain RefPtr to the same
-            // Nicosia::State object anyway).
-            if (!m_inForceRepaint) {
-                bool coordinateUpdate = false;
-                for (auto& state : states) {
-                    state.nicosia.scene->accessState(
-                        [&coordinateUpdate](Nicosia::Scene::State& state)
-                        {
-                            coordinateUpdate |= state.platformLayerUpdated;
-                            state.platformLayerUpdated = false;
-                        });
-                }
-                m_attributes.coordinateUpdateCompletionWithClient = coordinateUpdate;
-            }
         }
 
         // Reset the needsResize attribute to false.
@@ -284,10 +263,6 @@ void ThreadedCompositor::sceneUpdateFinished()
     //  - a DisplayRefreshMonitor callback was requested from the Web engine
     bool shouldDispatchDisplayRefreshCallback { false };
 
-    // If coordinateUpdateCompletionWithClient is true, the scene update completion has to be
-    // delayed until the DisplayRefreshMonitor callback.
-    bool shouldCoordinateUpdateCompletionWithClient { false };
-
     {
         LockHolder locker(m_attributes.lock);
         shouldDispatchDisplayRefreshCallback = m_attributes.clientRendersNextFrame
@@ -296,7 +271,6 @@ void ThreadedCompositor::sceneUpdateFinished()
 #else
             ;
 #endif
-        shouldCoordinateUpdateCompletionWithClient = m_attributes.coordinateUpdateCompletionWithClient;
     }
 
     LockHolder stateLocker(m_compositingRunLoop->stateLock());
@@ -307,12 +281,8 @@ void ThreadedCompositor::sceneUpdateFinished()
         m_displayRefreshMonitor->dispatchDisplayRefreshCallback();
 #endif
 
-    // Mark the scene update as completed if no coordination is required and if not in a forced repaint.
-    if (!shouldCoordinateUpdateCompletionWithClient && !m_inForceRepaint)
-        m_compositingRunLoop->updateCompleted(stateLocker);
-
-    // Independent of the scene update, the composition itself is now completed.
-    m_compositingRunLoop->compositionCompleted(stateLocker);
+    // Mark the scene update as completed.
+    m_compositingRunLoop->updateCompleted(stateLocker);
 }
 
 void ThreadedCompositor::updateSceneState(const CoordinatedGraphicsState& state)
@@ -327,24 +297,6 @@ RefPtr<WebCore::DisplayRefreshMonitor> ThreadedCompositor::displayRefreshMonitor
 {
     return m_displayRefreshMonitor.copyRef();
 }
-
-void ThreadedCompositor::handleDisplayRefreshMonitorUpdate()
-{
-    // Retrieve coordinateUpdateCompletionWithClient.
-    bool coordinateUpdateCompletionWithClient { false };
-    {
-        LockHolder locker(m_attributes.lock);
-        coordinateUpdateCompletionWithClient = std::exchange(m_attributes.coordinateUpdateCompletionWithClient, false);
-    }
-
-    LockHolder stateLocker(m_compositingRunLoop->stateLock());
-
-    // If required, mark the current scene update as completed. CompositingRunLoop will take care of
-    // scheduling a new update in case an update was marked as pending due to previous layer flushes
-    // or DisplayRefreshMonitor notifications.
-    if (coordinateUpdateCompletionWithClient)
-        m_compositingRunLoop->updateCompleted(stateLocker);
-}
 #endif
 
 void ThreadedCompositor::frameComplete()
index d2f0931..7e16738 100644 (file)
@@ -79,7 +79,6 @@ public:
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
     RefPtr<WebCore::DisplayRefreshMonitor> displayRefreshMonitor(WebCore::PlatformDisplayID);
-    void handleDisplayRefreshMonitorUpdate();
 #endif
 
     void frameComplete();
@@ -119,7 +118,6 @@ private:
         Vector<WebCore::CoordinatedGraphicsState> states;
 
         bool clientRendersNextFrame { false };
-        bool coordinateUpdateCompletionWithClient { false };
     } m_attributes;
 
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
index ad16b6c..76b0770 100644 (file)
@@ -435,7 +435,6 @@ void LayerTreeHost::handleDisplayRefreshMonitorUpdate(bool hasBeenRescheduled)
     // Call renderNextFrame. If hasBeenRescheduled is true, the layer flush will force a repaint
     // that will cause the display refresh notification to come.
     renderNextFrame(hasBeenRescheduled);
-    m_compositor->handleDisplayRefreshMonitorUpdate();
 }
 
 void LayerTreeHost::renderNextFrame(bool forceRepaint)