Bug 220375 – [GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequirements()

• Component: Compositing
• Status: RESOLVED FIXED

[Rini Patel]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000003ca62262e WTFCrash + 14 (Assertions.cpp:295)
1   com.apple.WebCore             	0x00000003a77efe5b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x00000003ac0e882c WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer&, WebCore::LayerOverlapMap&, WebCore::RenderLayerCompositor::CompositingState&, WebCore::RenderLayerCompositor::BackingSharingState&, bool&) + 3916 (RenderLayerCompositor.cpp:1125)
3   com.apple.WebCore             	0x00000003ac0e8437 WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer&, WebCore::LayerOverlapMap&, WebCore::RenderLayerCompositor::CompositingState&, WebCore::RenderLayerCompositor::BackingSharingState&, bool&) + 2903 (RenderLayerCompositor.cpp:1073)
4   com.apple.WebCore             	0x00000003ac0e84ea WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer&, WebCore::LayerOverlapMap&, WebCore::RenderLayerCompositor::CompositingState&, WebCore::RenderLayerCompositor::BackingSharingState&, bool&) + 3082 (RenderLayerCompositor.cpp:1076)
5   com.apple.WebCore             	0x00000003ac0b2a9e WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer*) + 3166 (RenderLayerCompositor.cpp:848)
6   com.apple.WebCore             	0x00000003ac0e6a3b WebCore::RenderLayerCompositor::didRecalcStyleWithNoPendingLayout() + 27 (RenderLayerCompositor.cpp:504)


Another crash with similar pattern 
ASSERTION FAILED: willBeComposited == needsToBeComposited(layer, queryData)
./rendering/RenderLayerCompositor.cpp(1125) : void WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer *, WebCore::RenderLayer &, WebCore::LayerOverlapMap &, WebCore::RenderLayerCompositor::CompositingState &, WebCore::RenderLayerCompositor::BackingSharingState &, bool &)
1   0x34a622629 WTFCrash
2   0x3277efe5b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x32c0e882c WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer&, WebCore::LayerOverlapMap&, WebCore::RenderLayerCompositor::CompositingState&, WebCore::RenderLayerCompositor::BackingSharingState&, bool&)
4   0x32c0e84ea WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer&, WebCore::LayerOverlapMap&, WebCore::RenderLayerCompositor::CompositingState&, WebCore::RenderLayerCompositor::BackingSharingState&, bool&)
5   0x32c0e84ea WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer&, WebCore::LayerOverlapMap&, WebCore::RenderLayerCompositor::CompositingState&, WebCore::RenderLayerCompositor::BackingSharingState&, bool&)

[Rini Patel]

Failing tests:
media/modern-media-controls/media-controller/media-controller-auto-hide.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/readyState_during_loadedmetadata.html

[Radar WebKit Bug Importer]

<rdar://problem/73012120>

[Robert Jenner]

There is another test that is failing, and appears to be part of this:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_canplaythrough.html 

HISTORY URL:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fwasm%2Fjsapi%2Ffunctions%2Fentry.html

[Rini Patel]

Another test failing 

mported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_loadedmetadata.html	

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fembedded-content%2Fmedia-elements%2Fevent_loadedmetadata.html

[Rini Patel]

Two more, adding to the TestExpectations file
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_progress.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/readyState_during_loadeddata.html

[Simon Fraser (smfr)]

The video gets into the accelerate rendering mode:

19:04:01.700 76909   Video 0x562bcb7c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 0
19:04:01.700 76909   Video 0x562bcb7c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 1

and we get into RenderLayerCompositor::computeCompositingRequirements() for the video's layer, but this does not evaluate to true:
  layer.needsPostLayoutCompositingUpdate() || compositingState.fullPaintOrderTraversalRequired || compositingState.descendantsRequireCompositingUpdate

so we don't compute willBeComposited.

So this is about no-one setting dirty bits on the RenderLayer when the video's canAccelerateVideoRendering state changes.

[Simon Fraser (smfr)]

Here's the RenderLayer tree state when the assertion fires:

19:04:01.700 76909   (S)tacking Context/(F)orced SC/O(P)portunistic SC, (N)ormal flow only, (O)verflow clip, (A)lpha (opacity or mask), has (B)lend mode, (I)solates blending, (T)ransform-ish, (F)ilter, Fi(X)ed position, Behaves as fi(x)ed, (C)omposited, (P)rovides backing/uses (p)rovided backing/paints to (a)ncestor, (c)omposited descendant, (s)scrolling ancestor, (t)transformed ancestor
19:04:01.700 76909   Dirty (z)-lists, Dirty (n)ormal flow lists
19:04:01.700 76909   Traversal needs: requirements (t)raversal on descendants, (b)acking or hierarchy traversal on descendants, (r)equirements traversal on all descendants, requirements traversal on all (s)ubsequent layers, (h)ierarchy traversal on all descendants, update of paint (o)rder children
19:04:01.700 76909   Update needs:    post-(l)ayout requirements, (g)eometry, (k)ids geometry, (c)onfig, layer conne(x)ion, (s)crolling tree
19:04:01.700 76909   Scrolling scope: box contents
19:04:01.700 76909   
19:04:01.700 76909   S---------C---- -- tb---- ------ 29 29 0x5630a3210 (0,0) width=800 height=600 [SA 0x562f78dc0] (layerID 230) {sc 7} RenderView 0x563439ec0
19:04:01.700 76909   S-------------- -- tb---- ------ 29 29   + 0x562f67000 (0,0) width=800 height=212 [SA 0x5630d6d20] RenderBlock 0x5636e4360 HTML 0x5621e7ca0
19:04:01.700 76909   SN------------- -- tb---- ------ 29 29     n 0x55db62a50 (262,50) width=300 height=150 RenderVideo 0x562bcb7c0 VIDEO 0x562bca2e0 id='v'
19:04:01.700 76909   S-----------c-- -- tb---- ------ 29 29       + 0x55db62b58 (0,0) width=300 height=150 RenderBlock (relative positioned) 0x562bdb4e0 DIV 0x5629b9200 class='media-controls-container'
19:04:01.700 76909   --------------- -- ------ ------ 29 29         + 0x55db62c60 (0,0) width=300 height=150 RenderBlock (positioned) 0x562bdb3c0 DIV 0x5629b9100 class='media-controls inline mac uses-ltr-user-interface-layout-direction'
19:04:01.700 76909   S----I----C-c-- -- ------ ------ 29 29         + 0x56337a738 (6,6) width=47 height=31 (layerID 257) graphical effect RenderBlock (positioned) 0x562bdb2a0 DIV 0x562bdda80 class='controls-bar top-left'
19:04:01.700 76909   --------------- -- ------ ------ 29 29           + 0x5633c8420 (0,0) width=47 height=31 RenderBlock (positioned) 0x562bdb180 DIV 0x562bdd980 class='background-tint'
...

Note the 'tb' dirty bits. There should be more.

[Simon Fraser (smfr)]

This is caused a race between HTMLMediaElement::scheduleMediaEngineWasUpdated() and some other compositing update.

From RenderLayerCompositor's perspective, RenderLayerBacking::contentChanged(VideoChanged) needs to get called before the next compositing update where canAccelerateVideoRendering() returns true.

canAccelerateVideoRendering() depends on the creation of the MediaPlayerPrivateAVFoundationObjC in the GPU Process, which IPCs back via MediaPlayerPrivateRemote::engineUpdated() which gets to HTMLMediaElement::mediaPlayerEngineUpdated(), which does a scheduleTask() to call mediaEngineWasUpdated().

If that task hasn't run before we RenderLayerCompositor::updateCompositingLayers() then we'll assert.

Media people, why does mediaPlayerEngineUpdated() have to schedule a task when underlying state (the answer to supportsAcceleratedRendering()) has already changed?

[Simon Fraser (smfr)]

From logging:

No assertion:

20:14:15.409 80924   Video 0x4eabb77c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 0
20:14:15.409 80924   HTMLMediaElement 0x4ea4f4880 scheduleMediaEngineWasUpdated()
20:14:15.409 80924   HTMLMediaElement 0x4eabb62e0 scheduleMediaEngineWasUpdated()
20:14:15.409 80924   RenderVideo 0x4eabb77c0 updatePlayer()
20:14:15.409 80924   Video 0x4eabb77c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 0
20:14:15.410 80924   HTMLMediaElement 0x4ea4f4880 mediaEngineWasUpdated()
20:14:15.410 80924   HTMLMediaElement 0x4eabb62e0 mediaEngineWasUpdated()
20:14:15.410 80924   RenderVideo 0x4eabb77c0 updatePlayer()
20:14:15.410 80924   RenderVideo 0x4eabb77c0 updatePlayer()
20:14:15.410 80924   Video 0x4eabb77c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 1


Assertion:

20:15:16.990 80906   Video 0x4b2bcb7c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 0
20:15:16.990 80906   HTMLMediaElement 0x4b21dcd40 scheduleMediaEngineWasUpdated()
20:15:16.990 80906   HTMLMediaElement 0x4b21dd6c0 scheduleMediaEngineWasUpdated()
20:15:16.990 80906   Video 0x4b2bcb7c0 requiresImmediateCompositing 0, shouldDisplayVideo 1 canAccelerateVideoRendering 1

[Peng Liu]

Created attachment 422794 [details]
WIP patch

[Eric Carlson]

(In reply to Simon Fraser (smfr) from comment #8)
> Media people, why does mediaPlayerEngineUpdated() have to schedule a task
> when underlying state (the answer to supportsAcceleratedRendering()) has
> already changed?

Because mediaPlayerEngineUpdated() calls mediaEngineWasUpdated(), which can trigger arbitrary DOM mutations. This can happen when the call comes through many methods in HTMLMediaElement, so it is much safer to do it in a task.

Why does rendering code call directly into MediaPlayer but also depend on state that can only be set by HTMLMediaElement/HTMLVideoElement?

[Simon Fraser (smfr)]

(In reply to Eric Carlson from comment #11)
> Why does rendering code call directly into MediaPlayer but also depend on
> state that can only be set by HTMLMediaElement/HTMLVideoElement?

Rendering/compositing code needs to know whether the video can be accelerated (RenderVideo::supportsAcceleratedRendering()). It just wants a consistent state to be presented, such that state changes are matched with calls to contentChanged().

[Eric Carlson]

(In reply to Simon Fraser (smfr) from comment #12)
> Rendering/compositing code needs to know whether the video can be
> accelerated (RenderVideo::supportsAcceleratedRendering()). 

  Understood.

> It just wants a
> consistent state to be presented, such that state changes are matched with
> calls to contentChanged().

  My point was that getting some state directly from `mediaPlayer()` (which should probably be a protected method as the player is an implementation detail), and some from from the element is a recipe for this kind of problem.

[Peng Liu]

Created attachment 422879 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 422879 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:1021
> +    bool m_chacedSupportsAcceleratedRendering { false };

"m_chacedSupportsAcceleratedRendering"

[Peng Liu]

Comment on attachment 422879 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.h:1021
>> +    bool m_chacedSupportsAcceleratedRendering { false };
> 
> "m_chacedSupportsAcceleratedRendering"

Sorry, should be "cached". Copy&paste spreads errors. :-(

[Peng Liu]

Created attachment 422884 [details]
Patch

[Eric Carlson]

Comment on attachment 422884 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:149
> +    bool supportsAcceleratedRendering() const { return m_player && m_cachedSupportsAcceleratedRendering; }

Nit: if you cleared m_cachedSupportsAcceleratedRendering in `clearMediaPlayer()`, this null-check won't be necessary.

[Peng Liu]

Comment on attachment 422884 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.h:149
>> +    bool supportsAcceleratedRendering() const { return m_player && m_cachedSupportsAcceleratedRendering; }
> 
> Nit: if you cleared m_cachedSupportsAcceleratedRendering in `clearMediaPlayer()`, this null-check won't be necessary.

Good idea!

[Peng Liu]

Created attachment 422885 [details]
Patch for landing

[EWS]

Committed r274264: <https://commits.webkit.org/r274264>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422885 [details].