Bug 220375 - [GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequirements()
Summary: [GPU Process] Assertion under RenderLayerCompositor::computeCompositingRequir...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks: 221830
  Show dependency treegraph
 
Reported: 2021-01-06 11:30 PST by Rini Patel
Modified: 2022-04-27 20:43 PDT (History)
17 users (show)

See Also:


Attachments
WIP patch (18.62 KB, patch)
2021-03-09 20:44 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (23.10 KB, patch)
2021-03-10 15:22 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (23.10 KB, patch)
2021-03-10 15:40 PST, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (23.38 KB, patch)
2021-03-10 16:04 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rini Patel 2021-01-06 11:30:47 PST
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&)
Comment 1 Rini Patel 2021-01-06 11:33:44 PST
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
Comment 2 Radar WebKit Bug Importer 2021-01-11 13:45:11 PST
<rdar://problem/73012120>
Comment 3 Robert Jenner 2021-02-16 12:46:51 PST
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
Comment 4 Rini Patel 2021-02-18 16:26:08 PST
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
Comment 5 Rini Patel 2021-02-18 17:20:25 PST
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
Comment 6 Simon Fraser (smfr) 2021-03-08 19:16:14 PST
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.
Comment 7 Simon Fraser (smfr) 2021-03-08 19:17:16 PST
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.
Comment 8 Simon Fraser (smfr) 2021-03-08 20:20:39 PST
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?
Comment 9 Simon Fraser (smfr) 2021-03-08 20:23:14 PST
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
Comment 10 Peng Liu 2021-03-09 20:44:13 PST
Created attachment 422794 [details]
WIP patch
Comment 11 Eric Carlson 2021-03-10 08:32:54 PST
(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?
Comment 12 Simon Fraser (smfr) 2021-03-10 09:12:38 PST
(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().
Comment 13 Eric Carlson 2021-03-10 09:32:00 PST
(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.
Comment 14 Peng Liu 2021-03-10 15:22:20 PST
Created attachment 422879 [details]
Patch
Comment 15 Simon Fraser (smfr) 2021-03-10 15:26:36 PST
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"
Comment 16 Peng Liu 2021-03-10 15:29:33 PST
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. :-(
Comment 17 Peng Liu 2021-03-10 15:40:57 PST
Created attachment 422884 [details]
Patch
Comment 18 Eric Carlson 2021-03-10 15:52:45 PST
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.
Comment 19 Peng Liu 2021-03-10 16:02:56 PST
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!
Comment 20 Peng Liu 2021-03-10 16:04:05 PST
Created attachment 422885 [details]
Patch for landing
Comment 21 EWS 2021-03-10 18:40:40 PST
Committed r274264: <https://commits.webkit.org/r274264>

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