Closed Bug 1411472 Opened 2 years ago Closed 2 years ago

PersistentBufferProviderShared could not be used with WebRenderLayerManager

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [wr-mvp])

Attachments

(1 file, 11 obsolete files)

32.37 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
PersistentBufferProviderShared is used for canvas 2d to improve performance.  But PersistentBufferProviderShared supports only ClientLayerManager, it could not be used with WebRenderLayerManager.
I found it during looking into bug 1401609.
See Also: → 1401609
Whiteboard: [wr-mvp] [triage]
Priority: P3 → P2
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Assignee: nobody → sotaro.ikeda.g
Depends on: 1412246
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #8922732 - Attachment is obsolete: true
Rebased.
Attachment #8923258 - Attachment is obsolete: true
Update nits.
Attachment #8923649 - Attachment is obsolete: true
Attachment #8923658 - Attachment is obsolete: true
Attachment #8923684 - Flags: review?(nical.bugzilla)
Comment on attachment 8923684 [details] [diff] [review]
patch - Add support of PersistentBufferProviderShared

Review of attachment 8923684 [details] [diff] [review]:
-----------------------------------------------------------------

I have small issue with this patch but otherwise looks good.

::: gfx/layers/PersistentBufferProvider.cpp
@@ +142,5 @@
> +PersistentBufferProviderShared::GetType()
> +{
> +  if (mKnowsCompositor->GetCompositorBackendType() == LayersBackend::LAYERS_WR) {
> +    return LayersBackend::LAYERS_WR;
> +  } else if (mKnowsCompositor) {

if mKnowsCompositor is false then the if branch above segfaulted in its condition.
Attachment #8923684 - Attachment is obsolete: true
Attachment #8923684 - Flags: review?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #10)
> 
> if mKnowsCompositor is false then the if branch above segfaulted in its
> condition.

I checked the source, if mKnowsCompositor is always true in PersistentBufferProviderShared. Anyway I already updated that part in attachment 8924033 [details] [diff] [review].

  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/PersistentBufferProvider.cpp#93
Attachment #8924033 - Attachment is obsolete: true
Attachment #8924050 - Attachment is obsolete: true
Attachment #8924081 - Flags: review?(nical.bugzilla)
Attachment #8924081 - Attachment is obsolete: true
Attachment #8924081 - Flags: review?(nical.bugzilla)
attachment 8924439 [details] [diff] [review] updated PersistentBufferProviderShared::GetType(), since there is no use case of "if (!mKnowsCompositor)".
Attachment #8924439 - Flags: review?(nical.bugzilla)
Attachment #8924439 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89513063a106
Add support of PersistentBufferProviderShared r=nical
Whiteboard: [wr-mvp] → [wr-mvp][keep-open]
(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=89513063a10631897ae55d14828e965c8ebe7aab&selectedJob=1425843
> 27
> 
> test failures :(

I forgot to remove the assert.
Attachment #8924439 - Attachment is obsolete: true
Attachment #8926240 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef83816915b005a2c66d8646f7517eafcbe96114

Known test failures were addressed, but another tests were failed. It might be related to Bug 1408421.
From investigation, webrender uses more buffers than layer since Bug 1408421. And mKnowsCompositor->SyncWithCompositor() does not work with WebRenderBridgeChild.
Bug 1412249 also affect to the test failure.
(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> Bug 1412249 also affect to the test failure.

It triggered over-production when I tested locally.
Depends on: 1412249
Attachment #8926240 - Attachment is obsolete: true
Attachment #8928781 - Flags: review+
Attachment #8928781 - Attachment is obsolete: true
Attachment #8928783 - Flags: review+
But WebRenderBridgeChild::SyncWithCompositor() still has a thing that needs to improve. It is better to be handled in a new bug.

The SyncWithCompositor()  send sync message to parent side. It expects that some buffers are read unlocked. But it does not work well with WebRender, since WebRender runs as multi-threaded way.
Depends on: 1417784
(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> But WebRenderBridgeChild::SyncWithCompositor() still has a thing that needs
> to improve. It is better to be handled in a new bug.
> 
> The SyncWithCompositor()  send sync message to parent side. It expects that
> some buffers are read unlocked. But it does not work well with WebRender,
> since WebRender runs as multi-threaded way.

Created Bug 1417784 for it.
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a74a02c20ba
Add support of PersistentBufferProviderShared r=nical
Whiteboard: [wr-mvp][keep-open] → [wr-mvp]
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Depends on: 1419255
You need to log in before you can comment on or make changes to this bug.