Closed Bug 1381393 Opened 3 years ago Closed 3 years ago

Fix TextureClient assertion with OMTP

Categories

(Core :: Graphics: Layers, enhancement)

40 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When OMTP is enabled we lock TextureClients, record draw commands, then unlock them. This breaks an assertion that the borrowed DrawTarget is not used after unlocking [1]. This assertion is important in that doing so violates the API contract of Lock/Unlock. It's hard to fix the expected refcount since the ref could be added or dropped asynchronously.

Looking at the API, it seems kind of pointless these days. It was designed for supporting different backends that might have various locking requirements, but (1) none of the backends we use have granular locks anymore, (2) we do not want more backends, ever, and (3) OMTP fundamentally breaks the way the API is supposed to work anyway, since we'd have to lock on one thread and unlock on another.

I'd like to see if we can't rework how the locking/unlocking happens just enough to bypass all the non-OMTP code. If it looks too difficult we can just flag the TC (maybe via OpenMode) to silence the assertion.

[1] http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/gfx/layers/client/TextureClient.cpp#561
Attached patch patchSplinter Review
All the classes involved here are super intertwined, so this patch just tries to bypass the assertion when async painting.
Attachment #8888647 - Flags: review?(matt.woodrow)
Comment on attachment 8888647 [details] [diff] [review]
patch

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

::: gfx/layers/CompositorTypes.h
@@ +256,5 @@
> +
> +  // This is only used in conjunction with OMTP to indicate that the DrawTarget
> +  // that is being borrowed will be painted asynchronously, and so will outlive
> +  // the write lock.
> +  OPEN_ASYNC_WRITE = 0x08

Can just use 0x4 here?
Attachment #8888647 - Flags: review?(matt.woodrow) → review+
Yup, will fix that.
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd3ff1e99a2
Don't assert about borrowed DT refcounts when async painting. (bug 1381393, r=mattwoodrow)
https://hg.mozilla.org/mozilla-central/rev/4cd3ff1e99a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.