⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 274264 in webkit


Ignore:
Timestamp:
Mar 10, 2021, 6:40:35 PM (6 years ago)
Author:
Peng Liu
Message:

[GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequirements()
​https://bugs.webkit.org/show_bug.cgi?id=220375

Reviewed by Eric Carlson.

Source/WebCore:

MediaPlayer calls HTMLMediaElement::mediaEngineWasUpdated() when a media element
loads a new URL and MediaPlayer::supportsAcceleratedRendering() may change from
false to true at the same time. However, HTMLMediaElement::mediaEngineWasUpdated()
does not notify the renderer about the change in the same run loop. Instead, it
schedules a task to do that. This leads to a race condition that
RenderLayerBacking::contentChanged(VideoChanged) gets called after the compositing
update where RenderLayerCompositor::canAccelerateVideoRendering() returns true.
This happens because renderer checks MediaPlayer::supportsAcceleratedRendering()
in RenderLayerCompositor::canAccelerateVideoRendering(), which changes its value
when MediaPlayer calls HTMLMediaElement::mediaEngineWasUpdated().

To fix this race condition, HTMLMediaElement needs to notify RenderVideo and
changes the supportsAcceleratedRendering property seen from renderer's perspective
in the same run loop. With this patch, HTMLMediaElement keeps a cached value of
MediaPlayer::supportsAcceleratedRendering(), and only updates its value when
HTMLMediaElement notifies the renderer. In addition, RenderVideo checks
HTMLMediaElement::supportsAcceleratedRendering() instead of
MediaPlayer::supportsAcceleratedRendering(), so that the renderer will
see the new value of supportsAcceleratedRendering and receive the content
change notification in the same run loop.

No new tests. Fix assertion failures in tests.

  • html/HTMLMediaElement.cpp:

(WebCore::HTMLMediaElement::mediaEngineWasUpdated):
(WebCore::HTMLMediaElement::clearMediaPlayer):

  • html/HTMLMediaElement.h:

(WebCore::HTMLMediaElement::supportsAcceleratedRendering const):

  • rendering/RenderVideo.cpp:

(WebCore::RenderVideo::paintReplaced):
(WebCore::RenderVideo::supportsAcceleratedRendering const):

Source/WebKit:

When "GPU Process: Media" is enabled, MediaPlayer will call mediaPlayerEngineUpdated()
three times when a media element tries to load a URL. The three calls happen at the
following time points:
1) MediaPlayer creates a MediaPlayerPrivateRemote.
2) MediaPlayerPrivateRemote receives the response of a "createMediaPlayer" message
from the GPU process and gets the initial configurations of the "remote" media player.
3) The RemoteMediaPlayerProxy creates a MediaPlayerPrivate in the GPU process and
notify the MediaPlayerPrivateRemote in the WebContent process.

The second call of mediaPlayerEngineUpdated() is unnecessary because the player's
configuration at that time is similar to NullMediaPlayerPrivate. This patch removes it.

For the third call of mediaPlayerEngineUpdated(), we have to make sure that
the MediaPlayerPrivateRemote has received the configuration update from the GPU
process before the call. Therefore, this patch removes the message
MediaPlayerPrivateRemote::EngineUpdated, and let MediaPlayerPrivateRemote
decide when to call mediaPlayerEngineUpdated().

  • GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:

(WebKit::RemoteMediaPlayerManagerProxy::createMediaPlayer):

  • GPUProcess/media/RemoteMediaPlayerManagerProxy.h:
  • GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in:

The playerConfiguration is meaningless in this step so we can remove it.

  • GPUProcess/media/RemoteMediaPlayerProxy.cpp:

(WebKit::RemoteMediaPlayerProxy::mediaPlayerEngineUpdated): Deleted.

  • GPUProcess/media/RemoteMediaPlayerProxy.h:
  • GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:

(WebKit::RemoteMediaPlayerProxy::prepareForPlayback):

  • WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:

(WebKit::MediaPlayerPrivateRemote::MediaPlayerPrivateRemote):
(WebKit::MediaPlayerPrivateRemote::load):
(WebKit::MediaPlayerPrivateRemote::setConfiguration): Deleted.
(WebKit::MediaPlayerPrivateRemote::engineUpdated): Deleted.

  • WebProcess/GPU/media/MediaPlayerPrivateRemote.h:
  • WebProcess/GPU/media/MediaPlayerPrivateRemote.messages.in:
  • WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:

(WebKit::RemoteMediaPlayerManager::createRemoteMediaPlayer):

LayoutTests:

  • platform/wk2/TestExpectations:
Location:
trunk
Files:
17 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r274244 r274264  
     12021-03-10  Peng Liu  <peng.liu6@apple.com>
     2
     3        [GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequirements()
     4        https://bugs.webkit.org/show_bug.cgi?id=220375
     5
     6        Reviewed by Eric Carlson.
     7
     8        * platform/wk2/TestExpectations:
     9
    1102021-03-10  Chris Gambrell  <cgambrell@apple.com>
    211

  • trunk/LayoutTests/platform/wk2/TestExpectations

    r274244 r274264  
    255255
    256256webkit.org/b/221783 [ Debug ] loader/change-src-during-iframe-load-crash.html [ Skip ]
    257 
    258 # webkit.org/b/220375
    259 [ Debug ] imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_loadedmetadata.html [ Crash Pass ]
    260 [ Debug ] imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_progress.html [ Crash Pass ]
    261 [ Debug ] imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/readyState_during_loadeddata.html [ Crash Pass ]
    262257
    263258webkit.org/b/222569 fast/mediastream/media-stream-track-interrupted.html [ Crash Pass ]

  • trunk/Source/WebCore/ChangeLog

    r274258 r274264  
     12021-03-10  Peng Liu  <peng.liu6@apple.com>
     2
     3        [GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequirements()
     4        https://bugs.webkit.org/show_bug.cgi?id=220375
     5
     6        Reviewed by Eric Carlson.
     7
     8        `MediaPlayer` calls `HTMLMediaElement::mediaEngineWasUpdated()` when a media element
     9        loads a new URL and `MediaPlayer::supportsAcceleratedRendering()` may change from
     10        false to true at the same time. However, `HTMLMediaElement::mediaEngineWasUpdated()`
     11        does not notify the renderer about the change in the same run loop. Instead, it
     12        schedules a task to do that. This leads to a race condition that
     13        `RenderLayerBacking::contentChanged(VideoChanged)` gets called after the compositing
     14        update where `RenderLayerCompositor::canAccelerateVideoRendering()` returns true.
     15        This happens because renderer checks `MediaPlayer::supportsAcceleratedRendering()`
     16        in `RenderLayerCompositor::canAccelerateVideoRendering()`, which changes its value
     17        when `MediaPlayer` calls `HTMLMediaElement::mediaEngineWasUpdated()`.
     18
     19        To fix this race condition, `HTMLMediaElement` needs to notify `RenderVideo` and
     20        changes the `supportsAcceleratedRendering` property seen from renderer's perspective
     21        in the same run loop. With this patch, `HTMLMediaElement` keeps a cached value of
     22        `MediaPlayer::supportsAcceleratedRendering()`, and only updates its value when
     23        `HTMLMediaElement` notifies the renderer. In addition, `RenderVideo` checks
     24        `HTMLMediaElement::supportsAcceleratedRendering()` instead of
     25        `MediaPlayer::supportsAcceleratedRendering()`, so that the renderer will
     26        see the new value of `supportsAcceleratedRendering` and receive the content
     27        change notification in the same run loop.
     28
     29        No new tests. Fix assertion failures in tests.
     30
     31        * html/HTMLMediaElement.cpp:
     32        (WebCore::HTMLMediaElement::mediaEngineWasUpdated):
     33        (WebCore::HTMLMediaElement::clearMediaPlayer):
     34        * html/HTMLMediaElement.h:
     35        (WebCore::HTMLMediaElement::supportsAcceleratedRendering const):
     36        * rendering/RenderVideo.cpp:
     37        (WebCore::RenderVideo::paintReplaced):
     38        (WebCore::RenderVideo::supportsAcceleratedRendering const):
     39
    1402021-03-10  Chris Dumez  <cdumez@apple.com>
    241

  • trunk/Source/WebCore/html/HTMLMediaElement.cpp

    r274175 r274264  
    50065006
    50075007    beginProcessingMediaPlayerCallback();
     5008    m_cachedSupportsAcceleratedRendering = m_player && m_player->supportsAcceleratedRendering();
    50085009    updateRenderer();
    50095010    endProcessingMediaPlayerCallback();
    … …  
    55165517        m_player->invalidate();
    55175518        m_player = nullptr;
     5519        m_cachedSupportsAcceleratedRendering = false;
    55185520    }
    55195521    schedulePlaybackControlsManagerUpdate();

  • trunk/Source/WebCore/html/HTMLMediaElement.h

    r274175 r274264  
    147147
    148148    RefPtr<MediaPlayer> player() const { return m_player; }
     149    bool supportsAcceleratedRendering() const { return m_cachedSupportsAcceleratedRendering; }
    149150
    150151    virtual bool isVideo() const { return false; }
    … …  
    10181019
    10191020    RefPtr<MediaPlayer> m_player;
     1021    bool m_cachedSupportsAcceleratedRendering { false };
    10201022
    10211023    MediaPlayer::Preload m_preload { Preload::Auto };

  • trunk/Source/WebCore/rendering/RenderVideo.cpp

    r269407 r274264  
    230230    if (displayingPoster)
    231231        paintIntoRect(paintInfo, rect);
    232     else if (!videoElement().isFullscreen() || !mediaPlayer->supportsAcceleratedRendering()) {
     232    else if (!videoElement().isFullscreen() || !videoElement().supportsAcceleratedRendering()) {
    233233        if (paintInfo.paintBehavior.contains(PaintBehavior::FlattenCompositingLayers))
    234234            context.paintFrameForMedia(*mediaPlayer, rect);
    … …  
    295295bool RenderVideo::supportsAcceleratedRendering() const
    296296{
    297     if (auto player = videoElement().player())
    298         return player->supportsAcceleratedRendering();
    299     return false;
     297    return videoElement().supportsAcceleratedRendering();
    300298}
    301299

  • trunk/Source/WebKit/ChangeLog

    r274252 r274264  
     12021-03-10  Peng Liu  <peng.liu6@apple.com>
     2
     3        [GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequirements()
     4        https://bugs.webkit.org/show_bug.cgi?id=220375
     5
     6        Reviewed by Eric Carlson.
     7
     8        When "GPU Process: Media" is enabled, `MediaPlayer` will call `mediaPlayerEngineUpdated()`
     9        three times when a media element tries to load a URL. The three calls happen at the
     10        following time points:
     11        1) `MediaPlayer` creates a `MediaPlayerPrivateRemote`.
     12        2) `MediaPlayerPrivateRemote` receives the response of a "createMediaPlayer" message
     13        from the GPU process and gets the initial configurations of the "remote" media player.
     14        3) The `RemoteMediaPlayerProxy` creates a `MediaPlayerPrivate` in the GPU process and
     15        notify the `MediaPlayerPrivateRemote` in the WebContent process.
     16
     17        The second call of `mediaPlayerEngineUpdated()` is unnecessary because the player's
     18        configuration at that time is similar to `NullMediaPlayerPrivate`. This patch removes it.
     19
     20        For the third call of `mediaPlayerEngineUpdated()`, we have to make sure that
     21        the `MediaPlayerPrivateRemote` has received the configuration update from the GPU
     22        process before the call. Therefore, this patch removes the message
     23        `MediaPlayerPrivateRemote::EngineUpdated`, and let `MediaPlayerPrivateRemote`
     24        decide when to call `mediaPlayerEngineUpdated()`.
     25
     26        * GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:
     27        (WebKit::RemoteMediaPlayerManagerProxy::createMediaPlayer):
     28        * GPUProcess/media/RemoteMediaPlayerManagerProxy.h:
     29        * GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in:
     30        The `playerConfiguration` is meaningless in this step so we can remove it.
     31
     32        * GPUProcess/media/RemoteMediaPlayerProxy.cpp:
     33        (WebKit::RemoteMediaPlayerProxy::mediaPlayerEngineUpdated): Deleted.
     34        * GPUProcess/media/RemoteMediaPlayerProxy.h:
     35        * GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:
     36        (WebKit::RemoteMediaPlayerProxy::prepareForPlayback):
     37
     38        * WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:
     39        (WebKit::MediaPlayerPrivateRemote::MediaPlayerPrivateRemote):
     40        (WebKit::MediaPlayerPrivateRemote::load):
     41        (WebKit::MediaPlayerPrivateRemote::setConfiguration): Deleted.
     42        (WebKit::MediaPlayerPrivateRemote::engineUpdated): Deleted.
     43        * WebProcess/GPU/media/MediaPlayerPrivateRemote.h:
     44        * WebProcess/GPU/media/MediaPlayerPrivateRemote.messages.in:
     45
     46        * WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:
     47        (WebKit::RemoteMediaPlayerManager::createRemoteMediaPlayer):
     48
    1492021-03-10  Chris Dumez  <cdumez@apple.com>
    250

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

    r274189 r274264  
    5858}
    5959
    60 void RemoteMediaPlayerManagerProxy::createMediaPlayer(MediaPlayerIdentifier identifier, MediaPlayerEnums::MediaEngineIdentifier engineIdentifier, RemoteMediaPlayerProxyConfiguration&& proxyConfiguration, CompletionHandler<void(RemoteMediaPlayerConfiguration&)>&& completionHandler)
     60void RemoteMediaPlayerManagerProxy::createMediaPlayer(MediaPlayerIdentifier identifier, MediaPlayerEnums::MediaEngineIdentifier engineIdentifier, RemoteMediaPlayerProxyConfiguration&& proxyConfiguration)
    6161{
    6262    ASSERT(RunLoop::isMain());
    … …  
    6666    ASSERT(!m_proxies.contains(identifier));
    6767
    68     RemoteMediaPlayerConfiguration playerConfiguration;
    69 
    7068    auto proxy = makeUnique<RemoteMediaPlayerProxy>(*this, identifier, m_gpuConnectionToWebProcess->connection(), engineIdentifier, WTFMove(proxyConfiguration));
    71     proxy->getConfiguration(playerConfiguration);
    7269    m_proxies.add(identifier, WTFMove(proxy));
    73 
    74     completionHandler(playerConfiguration);
    7570}
    7671

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

    r274189 r274264  
    7272    bool didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, UniqueRef<IPC::Encoder>&) final;
    7373
    74     void createMediaPlayer(WebCore::MediaPlayerIdentifier, WebCore::MediaPlayerEnums::MediaEngineIdentifier, RemoteMediaPlayerProxyConfiguration&&, CompletionHandler<void(RemoteMediaPlayerConfiguration&)>&&);
     74    void createMediaPlayer(WebCore::MediaPlayerIdentifier, WebCore::MediaPlayerEnums::MediaEngineIdentifier, RemoteMediaPlayerProxyConfiguration&&);
    7575    void deleteMediaPlayer(WebCore::MediaPlayerIdentifier);
    7676

  • trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in

    r274021 r274264  
    2525
    2626messages -> RemoteMediaPlayerManagerProxy NotRefCounted {
    27     CreateMediaPlayer(WebCore::MediaPlayerIdentifier identifier, enum:uint8_t WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier, struct WebKit::RemoteMediaPlayerProxyConfiguration proxyConfiguration) -> (struct WebKit::RemoteMediaPlayerConfiguration playerConfiguration) Async
     27    CreateMediaPlayer(WebCore::MediaPlayerIdentifier identifier, enum:uint8_t WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier, struct WebKit::RemoteMediaPlayerProxyConfiguration proxyConfiguration)
    2828    DeleteMediaPlayer(WebCore::MediaPlayerIdentifier identifier)
    2929

  • trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp

    r273526 r274264  
    631631}
    632632
    633 void RemoteMediaPlayerProxy::mediaPlayerEngineUpdated()
    634 {
    635     m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::EngineUpdated(), m_id);
    636 }
    637 
    638633void RemoteMediaPlayerProxy::mediaPlayerActiveSourceBuffersChanged()
    639634{

  • trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h

    r274189 r274264  
    214214    void mediaPlayerResourceNotSupported() final;
    215215    void mediaPlayerEngineFailedToLoad() const final;
    216     void mediaPlayerEngineUpdated() final;
    217216    void mediaPlayerActiveSourceBuffersChanged() final;
    218217    void mediaPlayerBufferedTimeRangesChanged() final;

  • trunk/Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm

    r273568 r274264  
    5555    m_player->setPreload(preload);
    5656    m_player->setPreservesPitch(preservesPitch);
    57     m_player->prepareForRendering();
     57    if (prepareForRendering)
     58        m_player->prepareForRendering();
    5859    m_videoContentScale = videoContentScale;
    5960    if (!m_inlineLayerHostingContext)

  • trunk/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp

    r274174 r274264  
    112112    , m_remoteEngineIdentifier(engineIdentifier)
    113113    , m_id(playerIdentifier)
     114    , m_documentSecurityOrigin(player->documentSecurityOrigin())
    114115{
    115116    INFO_LOG(LOGIDENTIFIER);
    … …  
    129130        m_audioSourceProvider->close();
    130131#endif
    131 }
    132 
    133 void MediaPlayerPrivateRemote::setConfiguration(RemoteMediaPlayerConfiguration&& configuration, WebCore::SecurityOriginData&& documentSecurityOrigin)
    134 {
    135     m_configuration = WTFMove(configuration);
    136     m_documentSecurityOrigin = WTFMove(documentSecurityOrigin);
    137     m_player->mediaEngineUpdated();
    138132}
    139133
    … …  
    182176    }
    183177
    184     connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::Load(url, sandboxExtensionHandle, contentType, keySystem), [weakThis = makeWeakPtr(*this)](auto&& configuration) {
    185         if (weakThis)
    186             weakThis->m_configuration = WTFMove(configuration);
     178    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::Load(url, sandboxExtensionHandle, contentType, keySystem), [weakThis = makeWeakPtr(*this), this](auto&& configuration) {
     179        if (!weakThis)
     180            return;
     181
     182        m_configuration = WTFMove(configuration);
     183        m_player->mediaEngineUpdated();
    187184    }, m_id);
    188185}
    … …  
    662659    if (m_remoteEngineIdentifier == MediaPlayerEnums::MediaEngineIdentifier::AVFoundationMSE) {
    663660        auto identifier = RemoteMediaSourceIdentifier::generate();
    664         connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::LoadMediaSource(url, contentType, RuntimeEnabledFeatures::sharedFeatures().webMParserEnabled(), identifier), [weakThis = makeWeakPtr(*this)](auto&& configuration) {
    665             if (weakThis)
    666                 weakThis->m_configuration = WTFMove(configuration);
     661        connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::LoadMediaSource(url, contentType, RuntimeEnabledFeatures::sharedFeatures().webMParserEnabled(), identifier), [weakThis = makeWeakPtr(*this), this](auto&& configuration) {
     662            if (!weakThis)
     663                return;
     664
     665            m_configuration = WTFMove(configuration);
     666            m_player->mediaEngineUpdated();
    667667        }, m_id);
    668668        m_mediaSourcePrivate = MediaSourcePrivateRemote::create(m_manager.gpuProcessConnection(), identifier, m_manager.typeCache(m_remoteEngineIdentifier), *this, client);
    … …  
    12221222}
    12231223
    1224 void MediaPlayerPrivateRemote::engineUpdated()
    1225 {
    1226     m_player->mediaEngineUpdated();
    1227 }
    1228 
    12291224void MediaPlayerPrivateRemote::activeSourceBuffersChanged()
    12301225{

  • trunk/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h

    r274172 r274264  
    8282    ~MediaPlayerPrivateRemote();
    8383
    84     void setConfiguration(RemoteMediaPlayerConfiguration&&, WebCore::SecurityOriginData&&);
    85 
    8684    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
    8785
    … …  
    144142    void resourceNotSupported();
    145143
    146     void engineUpdated();
    147 
    148144    void activeSourceBuffersChanged();
    149145

  • trunk/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.messages.in

    r270563 r274264  
    7272    ResourceNotSupported()
    7373
    74     EngineUpdated()
    75 
    7674    ActiveSourceBuffersChanged()
    7775

  • trunk/Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp

    r274021 r274264  
    166166
    167167    auto identifier = MediaPlayerIdentifier::generate();
    168     RemoteMediaPlayerConfiguration playerConfiguration;
    169     auto completionHandler = [this, weakThis = makeWeakPtr(this), identifier, documentSecurityOrigin = WTFMove(documentSecurityOrigin)](auto&& playerConfiguration) mutable {
    170         if (!weakThis)
    171             return;
    172 
    173         if (const auto& player = m_players.get(identifier))
    174             player->setConfiguration(WTFMove(playerConfiguration), WTFMove(documentSecurityOrigin));
    175     };
    176 
    177     gpuProcessConnection().connection().sendWithAsyncReply(Messages::RemoteMediaPlayerManagerProxy::CreateMediaPlayer(identifier, remoteEngineIdentifier, proxyConfiguration), completionHandler, 0);
     168    gpuProcessConnection().connection().send(Messages::RemoteMediaPlayerManagerProxy::CreateMediaPlayer(identifier, remoteEngineIdentifier, proxyConfiguration), 0);
    178169
    179170    auto remotePlayer = MediaPlayerPrivateRemote::create(player, remoteEngineIdentifier, identifier, *this);
Note: See TracChangeset for help on using the changeset viewer.