Skip to content
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

Restructure compositor layers to work with iframes #2943

Closed
wants to merge 1 commit into from

Conversation

@bjwbell
Copy link
Contributor

bjwbell commented Jul 28, 2014

When a frame is selected via set_ids, a tree of root compositor
layers is also created, matching the tree of pipelines in the frame.
This decouples the chronological ordering dependency for parent frames
and child iframes sending CreateOrUpdateRootLayer &
CreateOrUpdateDescendentLayer messages.

Change the Compositor ready and render states to per pipeline.
This ensures the compositor doesn't composite for an epoch until
every pipeline in the epoch is finished rendering.

For iframes it fixes a bug where the compositor didnt wait on the
child pipeline ready state before compositing the window.

Split SetLayerClipRect into two messages, SetLayerOrigin and
SetLayerSize. This is because only the parent layout task knows
the iframe position and only the iframe layout task knows the iframe
size. The parent layout task sets the iframe origin and the iframe
layout task sets the iframe size.

Gotchas:

  • layout task or script task failure on exit ("task '' failed at sending
    on a closed channel"), this happens if the child iframe shares the
    same script task as the parent and can be avoided by adding the
    sandbox attribute to the iframe.

Second time around...
@zwarich , CC @mrobinson

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 28, 2014

Critic review: https://critic.hoppipolla.co.uk/r/2173

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@zwarich
Copy link

zwarich commented Jul 28, 2014

@bjwbell These changes make scrolling really choppy for me.

@zwarich
Copy link

zwarich commented Jul 28, 2014

This patch fixes it:

diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs
index 00a83df..c996c3e 100644
--- a/src/components/compositing/compositor.rs
+++ b/src/components/compositing/compositor.rs
@@ -389,7 +389,7 @@ impl IOCompositor {

         let new_render_state = self.get_render_state();
         self.window.set_render_state(new_render_state);
-        self.composite_ready = new_render_state == IdleRenderState;
+        self.composite_ready |= new_render_state == IdleRenderState;
     }

     fn get_render_state(&self) -> RenderState {

If we want to synchronize changes from different pipelines, we need to do that in a way that doesn't block compositing with any outstanding changes, since there will always be a pipeline of new tiles coming.

@zwarich
Copy link

zwarich commented Jul 28, 2014

Of course, that change breaks the new iframe test you added.

@bjwbell
Copy link
Contributor Author

bjwbell commented Jul 28, 2014

If it's not one thing than it's another... I'll patch it to not break the test.

@zwarich
Copy link

zwarich commented Jul 28, 2014

@bjwbell At the end of the day, any synchronization based on RenderState is going to be insufficient, both because it is pessimistically synchronous and inherently racey. We need a transaction mechanism to correlate changes to distinct pipelines in a 'single turn of the runloop' in the script task, whatever that ends up meaning. Hopefully we can avoid synchronizing iframes that don't share a script task. We might need to do something special for testing; I'm not sure.

@bjwbell
Copy link
Contributor Author

bjwbell commented Jul 29, 2014

@zwarich Added change to only synchronously wait when outputting the result to a file. The ref tests now pass and scrolling shouldn't be jerky. This doesn't solve the inherently racey issue... That should probably be addressed with a follow up PR. Like you said we need a transaction mechanism, which I don't think will be completely trivial.

When a frame is selected via set_ids, a tree of root compositor
layers is also created, matching the tree of pipelines in the frame.
This decouples the chronological ordering dependency for parent frames
and child iframes sending CreateOrUpdateRootLayer &
CreateOrUpdateDescendentLayer messages.

Change the Compositor ready and render states to per pipeline.
This ensures the compositor doesn't composite for an epoch until
every *pipeline* in the epoch is finished rendering.

For iframes it fixes a bug where the compositor didnt wait on the
child pipeline ready state before compositing the window.

Split SetLayerClipRect into two messages, SetLayerOrigin and
SetLayerSize. This is because only the parent layout task knows
the iframe position and only the iframe layout task knows the iframe
size. The parent layout task sets the iframe origin and the iframe
layout task sets the iframe size.

Gotchas:
* layout task or script task failure on exit ("task '' failed at sending
  on a closed channel"), this happens if the child iframe shares the
  same script task as the parent and can be avoided by adding the
  sandbox attribute to the iframe.

* The iframe_summit ref test fails on the travis-ci linux build.
  It passes on the travis-ci osx build and also my local linux build.

* Synchronously wait for render only for tests
  Dont wait for all pipelines to finish rendering to composite unless
  outputting the result to a file. Note Compositing is racy with rendering.
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 8, 2014

@zwarich, @bjwbell: what is this waiting for?

@@ -397,67 +507,128 @@ impl IOCompositor {
}
}

fn create_or_update_root_layer(&mut self, layer_properties: LayerProperties) {
fn create_or_update_root_layer(&mut self,

This comment has been minimized.

Copy link
@mrobinson

mrobinson Aug 19, 2014

Member

This method is a little strange now and I'm not sure I understand what's going on. It used to update the layers if they existed or create them otherwise, thus the name create_or_update_root_layer. Now it seems to update the root layers in either case. Shouldn't the method be renamed to update_root_layer and the need_new_root_layer case removed?

@mrobinson
Copy link
Member

mrobinson commented Aug 20, 2014

This patch is pretty big, but I think it has some parts that are useful independently. For instance, perhaps it makes sense to split out a persistent root layer into another commit. This would simplify a lot of the compositor. Hopefully it is not a problem if I do that. I hope it will help move this patch forward, by allowing it to be simpler.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 27, 2014

@mrobinson @zwarich Where is this patch at? I know it was rebased by @mrobinson and working (I used it for the demo), but can we open/land it? This is required to fix our iframes-related demos :-)

@mrobinson
Copy link
Member

mrobinson commented Aug 28, 2014

The first part of this PR split off here: #3179

@mrobinson
Copy link
Member

mrobinson commented Sep 2, 2014

The rest is here: #3197.

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 3, 2014

Closing in favor of #3197. (Please reopen if this was incorrect.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.