Changeset 277774 in webkit

Timestamp:

May 19, 2021, 10:05:19 PM (4 years ago)

Author:

Chris Dumez

Message:

[GPUProcess] It is not safe to call GraphicsContext::paintFrameForMedia() off the main thread
​https://bugs.webkit.org/show_bug.cgi?id=225996

Reviewed by Simon Fraser.

It is not safe to call GraphicsContext::paintFrameForMedia() off the main thread because it
relies on the MediaPlayer / MediaPlayerPrivate objects, which are main-thread object. Making
this function thread-safe would be a significant amount of work. As a result, I am simply
calling callOnMainThreadAndWait() in RemoteRenderingBackend::applyMediaItem(). Note that this
code path is only used when painting a video to a canvas.

• GPUProcess/graphics/RemoteRenderingBackend.cpp:

(WebKit::RemoteRenderingBackend::applyMediaItem):
Make sure we call paintFrameForMedia() on the main thread.

• GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:

(WebKit::RemoteMediaPlayerManagerProxy::createMediaPlayer):
(WebKit::RemoteMediaPlayerManagerProxy::deleteMediaPlayer):
(WebKit::RemoteMediaPlayerManagerProxy::mediaPlayer):

• GPUProcess/media/RemoteMediaPlayerManagerProxy.h:

Drop lock that is no longer needed now that mediaPlayer() is always called on the main thread.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• GPUProcess/graphics/RemoteRenderingBackend.cpp (modified) (1 diff)
• GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp (modified) (3 diffs)
• GPUProcess/media/RemoteMediaPlayerManagerProxy.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-05-19  Chris Dumez  <cdumez@apple.com>
+
+        [GPUProcess] It is not safe to call GraphicsContext::paintFrameForMedia() off the main thread
+        https://bugs.webkit.org/show_bug.cgi?id=225996
+
+        Reviewed by Simon Fraser.
+
+        It is not safe to call GraphicsContext::paintFrameForMedia() off the main thread because it
+        relies on the MediaPlayer / MediaPlayerPrivate objects, which are main-thread object. Making
+        this function thread-safe would be a significant amount of work. As a result, I am simply
+        calling callOnMainThreadAndWait() in RemoteRenderingBackend::applyMediaItem(). Note that this
+        code path is only used when painting a video to a canvas.
+
+        * GPUProcess/graphics/RemoteRenderingBackend.cpp:
+        (WebKit::RemoteRenderingBackend::applyMediaItem):
+        Make sure we call paintFrameForMedia() on the main thread.
+
+        * GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:
+        (WebKit::RemoteMediaPlayerManagerProxy::createMediaPlayer):
+        (WebKit::RemoteMediaPlayerManagerProxy::deleteMediaPlayer):
+        (WebKit::RemoteMediaPlayerManagerProxy::mediaPlayer):
+        * GPUProcess/media/RemoteMediaPlayerManagerProxy.h:
+        Drop lock that is no longer needed now that mediaPlayer() is always called on the main thread.
+
 2021-05-19  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp

@@ -133 +133 @@
 
     auto& mediaItem = item.get<DisplayList::PaintFrameForMedia>();
-    auto player = m_gpuConnectionToWebProcess->remoteMediaPlayerManagerProxy().mediaPlayer(mediaItem.identifier());
-    if (!player)
-        return true;
-
-    context.paintFrameForMedia(*player, mediaItem.destination());
+    callOnMainRunLoopAndWait([&, gpuConnectionToWebProcess = m_gpuConnectionToWebProcess, mediaPlayerIdentifier = mediaItem.identifier()] {
+        auto player = gpuConnectionToWebProcess->remoteMediaPlayerManagerProxy().mediaPlayer(mediaPlayerIdentifier);
+        if (!player)
+            return;
+        // It is currently not safe to call paintFrameForMedia() off the main thread.
+        context.paintFrameForMedia(*player, mediaItem.destination());
+    });
     return true;
 }

trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp

@@ -62 +62 @@
     ASSERT(RunLoop::isMain());
     ASSERT(m_gpuConnectionToWebProcess);
-
-    auto locker = holdLock(m_proxiesLock);
     ASSERT(!m_proxies.contains(identifier));
 
@@ -73 +71 @@
 {
     ASSERT(RunLoop::isMain());
-    {
-        auto locker = holdLock(m_proxiesLock);
-        if (auto proxy = m_proxies.take(identifier))
-            proxy->invalidate();
-    }
+
+    if (auto proxy = m_proxies.take(identifier))
+        proxy->invalidate();
+
     if (m_gpuConnectionToWebProcess && allowsExitUnderMemoryPressure())
         m_gpuConnectionToWebProcess->gpuProcess().tryExitIfUnusedAndUnderMemoryPressure();
@@ -174 +171 @@
 }
 
-// May get called on a background thread.
 RefPtr<MediaPlayer> RemoteMediaPlayerManagerProxy::mediaPlayer(const MediaPlayerIdentifier& identifier)
 {
-    auto locker = holdLock(m_proxiesLock);
+    ASSERT(RunLoop::isMain());
     auto results = m_proxies.find(identifier);
     if (results != m_proxies.end())

trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h

@@ -84 +84 @@
     void supportsKeySystem(WebCore::MediaPlayerEnums::MediaEngineIdentifier, const String&&, const String&&, CompletionHandler<void(bool)>&&);
 
-    Lock m_proxiesLock;
     HashMap<WebCore::MediaPlayerIdentifier, std::unique_ptr<RemoteMediaPlayerProxy>> m_proxies;
     WeakPtr<GPUConnectionToWebProcess> m_gpuConnectionToWebProcess;