Closed Bug 1070308 Opened 5 years ago Closed 5 years ago

Add Acquire and Release semantics to SharedSurface

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file, 2 obsolete files)

The real locking/fencing/waiting semantics we want are Acquire and Release for both Producer and Consumer.

We need this architecture for using `SharedMutex`s with D3D11.
Attached patch recycle-fence (obsolete) — Splinter Review
Attachment #8492478 - Flags: review?(jmuizelaar)
I had a super quick look at this and it looks like what we want.
Comment on attachment 8492478 [details] [diff] [review]
recycle-fence

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

::: gfx/layers/composite/TextureHost.cpp
@@ +1166,5 @@
> +  MOZ_ASSERT(!mIsLocked);
> +
> +  EnsureTexSource();
> +
> +  mSurf->ConsumerAcquire();

It would work better if we did ConsumerAcquire before EnsureTexSource. I want to make it so that we only have to make it so that we only have to OpenShareHandle once per draw.
Attachment #8492478 - Flags: review?(jmuizelaar) → review+
This patch seems to be hitting the assertion in ProducerRelease() called from GLScreenBuffer::Swap()
Attached patch acquire-release.patch (obsolete) — Splinter Review
This fixes the assertion by calling Acquire in ::Resize and fixes another assertion by calling Acquire before EnsureTexSource.
Attachment #8492478 - Attachment is obsolete: true
Attachment #8505651 - Flags: review?(jgilbert)
Comment on attachment 8505651 [details] [diff] [review]
acquire-release.patch

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

Cool. It looks like we currently call Fence twice. This might explain some of the talos regressions.

::: gfx/gl/SharedSurface.h
@@ +80,5 @@
>      virtual void UnlockProdImpl() = 0;
>  
> +    virtual void ProducerAcquireImpl() {}
> +    virtual void ProducerReleaseImpl() {
> +      Fence();

This file is still 4-space.
Attachment #8505651 - Flags: review?(jgilbert) → review+
Looks like we're still failing an assertion:

https://tbpl.mozilla.org/?tree=Try&rev=002446aa01a2
This was without the double readback fix though. Trying again.
This adds a ProducerRelease() in Resize() that's called when needed.
Attachment #8505651 - Attachment is obsolete: true
Attachment #8506378 - Flags: review?(jgilbert)
Attachment #8506378 - Flags: review?(jgilbert) → review+
See Also: → 1083621
https://hg.mozilla.org/mozilla-central/rev/6e17d46ddda4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.