Use EGL Sync Objects to ensure that shared WebGL textures are ready to be used #1407

MortimerGoro · 2017-06-20T16:22:00Z

Currently a glFlush call is issued at the start of the frame to guarantee that shared WebGL texture data is valid. But glFlush is not enough in some GPUs because it doesn't guarantee the completion of the GL commands when the shared texture is sampled. This leads to some graphic glitches on some demos or even nothing being rendered at all (GPU Mali-T880). glFinish guarantees the completion of the commands but it may hurt performance a lot.

Sync objects are the recommended way to ensure that textures are ready in OpenGL 3.0+. They are more performant than glFinish and guarantee the completion of the GL commands.

I added dirty checking so the sync is only performed when WebGL contexts are being used. This PR might be improved if we move the glClientWaitSync to the WR render thread (before the shared texture is used). The parallelism should be better that way. I don know where is the best place to add the sync wait in WR. If you can guide me on that I'll update the PR.

This change is

kvark · 2017-06-20T16:44:24Z

webrender/src/render_backend.rs

+            if self.dirty_webgl_contexts.contains(&id) {
+                webgl_context.make_current();
+                // Call FenceSync and ClientWaitSync to ensure that textures are ready.
+                let sync = webgl_context.gl().fence_sync(gl::SYNC_GPU_COMMANDS_COMPLETE, 0);


we shouldn't be calling any GL functions directly from render_backend

Do you mean that they should be called using WebGLCommand enum? (as it was done with the existing glFlush call)

I suppose, yes. Just don't assume there is GL under the hood.

kvark · 2017-06-20T16:44:24Z

webrender/src/render_backend.rs

+        // Sync objects are the recommended way to ensure that textures are ready in OpenGL 3.0+.
+        // They are more performant than glFinish and guarantee the completion of the GL commands.
+        for (id, webgl_context) in &self.webgl_contexts {
+            if self.dirty_webgl_contexts.contains(&id) {


could be if self.dirty_webgl_context.remove(&id) {..}

glennw · 2017-06-20T20:03:30Z

glClientWaitSync is only available on desktop GL 3.2+, I believe. Although we currently do require 3.2, we're meant to be dropping required desktop GL to 3.1...

MortimerGoro · 2017-06-20T21:01:49Z

glClientWaitSync is only available on desktop GL 3.2+, I believe. Although we currently do require 3.2, we're meant to be dropping required desktop GL to 3.1...

Yes, glClientWaitSync is available in GL 3.2+ and GL ES 3.0+. We could check if GL_ARB_sync extension is available in GL 3.1 and fallback to glFlush/glFinish if not. Do you want to do that in this PR or create an issue or handle this in the future WebGL separate crate?

glennw · 2017-06-20T21:50:30Z

That's fine to do it as a follow up, so long as we have a valid fallback plan (which it sounds like we do).

Thanks!

bors-servo · 2017-06-22T23:16:49Z

☔ The latest upstream changes (presumably #1409) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2017-07-05T21:18:25Z

@MortimerGoro This needs a rebase.

MortimerGoro · 2017-07-05T21:45:22Z

Done!

glennw

One question, otherwise looks good.

glennw · 2017-07-05T22:06:34Z

webrender_api/src/webgl.rs

+        // Call FenceSync and ClientWaitSync to ensure that textures are ready.
+        let sync = gl.fence_sync(gl::SYNC_GPU_COMMANDS_COMPLETE, 0);
+        // SYNC_FLUSH_COMMANDS_BIT is used to automatically generate a glFlush before blocking on the sync object.
+        gl.client_wait_sync(sync, gl::SYNC_FLUSH_COMMANDS_BIT, gl::TIMEOUT_IGNORED);


Why does this need to be a glClientWaitSync ? Wouldn't a glWaitSync be more appropriate in this case, so that the CPU isn't blocked? I would have though a glWaitSync would be enough to sync between the two command streams?

Yes, glWaitSync is enough. Just tried and it works ok.


        Use EGL Sync Objects to ensure that shared WebGL textures are ready t…

…o be used

glennw · 2017-07-05T23:05:40Z

Looks good, thanks!

@bors-servo r+

bors-servo · 2017-07-05T23:05:40Z

📌 Commit 969a0d6 has been approved by glennw

bors-servo · 2017-07-05T23:05:43Z

⌛ Testing commit 969a0d6 with merge 0fc57c6...


        Auto merge of #1407 - MortimerGoro:glsync, r=glennw

Use EGL Sync Objects to ensure that shared WebGL textures are ready to be used Currently a glFlush call is issued at the start of the frame to guarantee that shared WebGL texture data is valid. But glFlush is not enough in some GPUs because it doesn't guarantee the completion of the GL commands when the shared texture is sampled. This leads to some graphic glitches on some demos or even nothing being rendered at all (GPU Mali-T880). glFinish guarantees the completion of the commands but it may hurt performance a lot. Sync objects are the recommended way to ensure that textures are ready in OpenGL 3.0+. They are more performant than glFinish and guarantee the completion of the GL commands. I added dirty checking so the sync is only performed when WebGL contexts are being used. This PR might be improved if we move the glClientWaitSync to the WR render thread (before the shared texture is used). The parallelism should be better that way. I don know where is the best place to add the sync wait in WR. If you can guide me on that I'll update the PR.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1407)

bors-servo · 2017-07-05T23:53:49Z

☀️ Test successful - status-travis
Approved by: glennw
Pushing 0fc57c6 to master...


        Partially revert servo#1407

This change caused many WebGL tests to fail in Servo, which is blocking the WebRender update. Partially revert the change in order to reland it later with the issue properly fixed.

mrobinson · 2017-07-12T12:52:07Z

This change is causing many Servo WebGL tests to fail, so I posted a PR to partially revert this (temporarily) in order to move the WebRender update forward. Sorry!

#1471

MortimerGoro · 2017-07-12T14:58:51Z

Ok, no problem. I'm working on #1353 so the WebGL code will be refactored to a new component. I'll check all the WebGL tests after the refactor.


        Auto merge of #1471 - mrobinson:partial-revert-of-1407, r=kvark

Partially revert #1407 This change caused many WebGL tests to fail in Servo, which is blocking the WebRender update. Partially revert the change in order to reland it later with the issue properly fixed.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1471)

kvark reviewed Jun 20, 2017

View changes

MortimerGoro force-pushed the MortimerGoro:glsync branch from 57bfec3 to 939400e Jun 20, 2017

MortimerGoro force-pushed the MortimerGoro:glsync branch from 939400e to 9dd0cee Jun 20, 2017

MortimerGoro force-pushed the MortimerGoro:glsync branch from 9dd0cee to 5881d33 Jul 5, 2017

glennw reviewed Jul 5, 2017

View changes

Use EGL Sync Objects to ensure that shared WebGL textures are ready t…

Loading status checks…

969a0d6

…o be used

MortimerGoro force-pushed the MortimerGoro:glsync branch from 5881d33 to 969a0d6 Jul 5, 2017

glennw approved these changes Jul 5, 2017

View changes

bors-servo merged commit 969a0d6 into servo:master Jul 5, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / webrender

Use EGL Sync Objects to ensure that shared WebGL textures are ready to be used #1407

Use EGL Sync Objects to ensure that shared WebGL textures are ready to be used #1407

MortimerGoro commented Jun 20, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Jun 20, 2017

MortimerGoro commented Jun 20, 2017 •

edited

glennw commented Jun 20, 2017

bors-servo commented Jun 22, 2017

glennw commented Jul 5, 2017

MortimerGoro commented Jul 5, 2017

glennw left a comment

This comment has been minimized.

This comment has been minimized.

glennw commented Jul 5, 2017

bors-servo commented Jul 5, 2017

bors-servo commented Jul 5, 2017

bors-servo commented Jul 5, 2017

mrobinson commented Jul 12, 2017

MortimerGoro commented Jul 12, 2017 •

edited

servo / webrender

Join GitHub today

Use EGL Sync Objects to ensure that shared WebGL textures are ready to be used #1407

Use EGL Sync Objects to ensure that shared WebGL textures are ready to be used #1407

Conversation

MortimerGoro commented Jun 20, 2017 • edited

This comment has been minimized.

kvark Jun 20, 2017

This comment has been minimized.

MortimerGoro Jun 20, 2017

This comment has been minimized.

kvark Jun 20, 2017

This comment has been minimized.

kvark Jun 20, 2017

glennw commented Jun 20, 2017

MortimerGoro commented Jun 20, 2017 • edited

glennw commented Jun 20, 2017

bors-servo commented Jun 22, 2017

glennw commented Jul 5, 2017

MortimerGoro commented Jul 5, 2017

glennw left a comment

This comment has been minimized.

glennw Jul 5, 2017

This comment has been minimized.

MortimerGoro Jul 5, 2017

glennw commented Jul 5, 2017

bors-servo commented Jul 5, 2017

bors-servo commented Jul 5, 2017

bors-servo commented Jul 5, 2017

mrobinson commented Jul 12, 2017

MortimerGoro commented Jul 12, 2017 • edited

MortimerGoro commented Jun 20, 2017 •

edited

MortimerGoro commented Jun 20, 2017 •

edited

MortimerGoro commented Jul 12, 2017 •

edited