New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize WebGL contexts with page rendering #21841

Open
wants to merge 3 commits into
base: master
from

Conversation

@jdm
Copy link
Member

jdm commented Sep 29, 2018

The code that deals with a integrating a shared GL context with the display list that Webrender receives doesn't work right now. Webrender has an API that allows locking the context, but the period between the end of layout (when script starts running again) and when WR actually processes the display list means that the GL context can be modified so that what WR sees is not what was supposed to be composited. This change marks GL contexts for WebGL canvases as locked as soon as they are processed by layout, causing any future WebGL commands that affect the to context to be buffered and replayed once WebRender unlocks the GL context.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21838
  • These changes do not require tests because it's nondeterministic behaviour.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Sep 29, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/dom/node.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/htmlcanvaselement.rs
  • @KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/node.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/htmlcanvaselement.rs
  • @emilio: components/layout/construct.rs, components/layout/context.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Sep 29, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout and script code, but no tests are modified. Please consider adding a test!
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Sep 29, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 29, 2018

⌛️ Trying commit f693335 with merge c0c8602...

bors-servo added a commit that referenced this pull request Sep 29, 2018

Auto merge of #21841 - jdm:webgl-sync, r=<try>
Synchronize WebGL contexts with page rendering

The code that deals with a integrating a shared GL context with the display list that Webrender receives doesn't work right now. Webrender has an API that allows locking the context, but the period between the end of layout (when script starts running again) and when WR actually processes the display list means that the GL context can be modified so that what WR sees is not what was supposed to be composited. This change marks GL contexts for WebGL canvases as locked as soon as they are processed by layout, causing any future WebGL commands that affect the to context to be buffered and replayed once WebRender unlocks the GL context.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21838
- [x] These changes do not require tests because it's nondeterministic behaviour.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21841)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Sep 29, 2018

r? @emilio
Let me know if this is more than you want to deal with and I can find somebody else.

@highfive highfive assigned emilio and unassigned avadacatavra Sep 29, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 29, 2018

💔 Test failed - linux-rel-wpt

@emilio
Copy link
Member

emilio left a comment

np, I think I can review this :)

Some comment below, I think it looks reasonable, though I think I need to re-page the locking / unlocking scheme is supposed to work to see if this is sound. We can probably take advantage that we're going to see us in person on Monday to double-check this :)

Thanks!

@@ -183,6 +212,13 @@ impl<VR: WebVRRenderHandler + 'static> WebGLThread<VR> {
self.webvr_compositor.as_mut().unwrap().handle(command, texture.map(|t| (t.texture_id, t.size)));
}

/// Handles a request to prevent a GL context from being modified until further notice.
fn handle_prevent_modification(&mut self, context_id: WebGLContextId) {
if !self.buffered_contexts.contains_key(&context_id) {

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

nit: This can use entry instead to avoid two hash table lookups.

let target_id = match msg {
WebGLMsg::CreateContext(..) |
WebGLMsg::Lock(..) |
WebGLMsg::Unlock(..) |

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

We should probably assert that we don't receive lock messages while it's unlocked, or similar. Otherwise the code below in handle_unlock won't work. Though I think this is a pre-existing issue.

{
let data = Self::make_current_if_needed(context_id, &self.contexts, &mut self.bound_context_id)
.expect("WebGLContext not found in a WebGLMsg::Unlock message");
let info = self.cached_context_info.get_mut(&context_id).unwrap();

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

nit: while you're touching this, using the indexing operator may give a nicer error message if it fails.

@@ -64,6 +65,9 @@ pub struct WebGLThread<VR: WebVRRenderHandler + 'static> {
webvr_compositor: Option<VR>,
/// Texture ids and sizes used in DOM to texture outputs.
dom_outputs: FnvHashMap<webrender_api::PipelineId, DOMToTextureData>,
/// Tracking GL contexts that must not be modified but can still be
/// targeted by WebGL commands.

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

This should document that this map only has keys for contexts that are currently locked, and for which a PreventModification message has been sent.

guards.clone(),
true,
data.reflow_goal.needs_display(),
&map);

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

nit: paren on the next line.

guards,
false,
goal.needs_display(),
&snapshots

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

nit: I think rustfmt would add a trailing comma here.

@@ -183,6 +212,13 @@ impl<VR: WebVRRenderHandler + 'static> WebGLThread<VR> {
self.webvr_compositor.as_mut().unwrap().handle(command, texture.map(|t| (t.texture_id, t.size)));
}

/// Handles a request to prevent a GL context from being modified until further notice.
fn handle_prevent_modification(&mut self, context_id: WebGLContextId) {

This comment has been minimized.

@emilio

emilio Sep 29, 2018

Member

Should this assert that the context is locked? Otherwise there's no guarantee that the context would be unlocked, right?

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Oct 1, 2018

Another thing to check is whether WebRender is guaranteed to give us an unlock() message. I'd expect it to not do so if the image is culled (because the element is off-screen for example).

@MortimerGoro

This comment has been minimized.

Copy link
Contributor

MortimerGoro commented Oct 2, 2018

As an optimization we could start the msg buffering only when a Draw operation arrives (e.g. glDrawElements, glDrawArrays or glClear). It should be safe to run other methods.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Oct 9, 2018

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

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Oct 23, 2018

The concern about webrender only calling lock/unlock when the canvas is rendered onscreen is real. Firefox uses a queue of shared textures (instead of one shared context) to ensure that it can continue performing webgl operations while webrender will always see consistent texture contents. To avoid completely rewriting our webgl setup, I'm going to try delaying all webgl command execution until the webrender image handler lock is called; the webgl thread can buffer all commands and group them by frames. For a canvas with preserveDrawingBuffer: false, we can discard all buffered frames except the most recent complete one, otherwise we will need to execute all of them.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 9, 2018

This runs into problems where JS will block on compositing. I'm going to try using textures like Gecko instead of a single offscreen framebuffer so we can just tell webrender "this is the frame; draw it" and keep working on a separate texture.

@MortimerGoro

This comment has been minimized.

Copy link
Contributor

MortimerGoro commented Nov 9, 2018

That's a good idea. We are also going to need render to texture (SurfaceTexture) for WebVR integration with FxR.

jdm added some commits Nov 14, 2018

webgl: Swap the current draw buffer every time a webgl canvas is mark…
…ed dirty.

Refactors the WebGL thread and canvas rendering context to reflect the fact that
WebRender image keys are immutable. Any readback that is necessary occurs right
before the draw buffer is swapped.

@jdm jdm force-pushed the jdm:webgl-sync branch from cf78efe to 3c229f1 Nov 22, 2018

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 22, 2018

Huh. In a release build on my pixel 2, there is way more flickering with these changes than without them. That's... unexpected.

@jdm jdm removed the S-needs-rebase label Nov 22, 2018

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 23, 2018

This is definitely a huge improvement in debug builds, at least.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 26, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 26, 2018

⌛️ Trying commit 3c229f1 with merge 40dec74...

bors-servo added a commit that referenced this pull request Nov 26, 2018

Auto merge of #21841 - jdm:webgl-sync, r=<try>
Synchronize WebGL contexts with page rendering

The code that deals with a integrating a shared GL context with the display list that Webrender receives doesn't work right now. Webrender has an API that allows locking the context, but the period between the end of layout (when script starts running again) and when WR actually processes the display list means that the GL context can be modified so that what WR sees is not what was supposed to be composited. This change marks GL contexts for WebGL canvases as locked as soon as they are processed by layout, causing any future WebGL commands that affect the to context to be buffered and replayed once WebRender unlocks the GL context.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21838
- [x] These changes do not require tests because it's nondeterministic behaviour.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21841)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 26, 2018

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 26, 2018

Current theory about the pixel 2 brokenness:

  1. WR locks the external image and fetches the texture id for the complete frame
  2. the canvas thread processes the lock message and returns the current texture id for the complete frame
  3. the canvas thread receives a message to present the current frame and swaps the textures, resetting the previously complete texture to black
  4. WR draws the texture id that has now been reset to black

More specifically, the locking mechanism currently implemented is broken by the switch to use multiple textures. The swapping code should only run when the texture is unlocked; otherwise we should just reset the current frame and skip displaying it, I suppose.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Nov 29, 2018

The flickering is not helped by dealing with the issue described in the previous comment. It is helped by commenting out the call to reset_draw_buffer_contents in servo/rust-offscreen-rendering-context#131.

@jdm jdm added this to In progress in WebGL content Nov 30, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 31, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment