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

Switch the default renderer to WebRender #11136

Closed

Conversation

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 11, 2016

Just testing for now to see if we even pass the tests... planning to try this once the queue dies down a bit. DO NOT MERGE.


This change is Reviewable

@metajack
Copy link
Contributor

metajack commented May 11, 2016

Consider that we've regressed -c (and probably -g), I think it makes sense to flip this switch.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented May 11, 2016

2:06 PM <cbrewster> jdm: A while back we discovered that the WebGL bindings don't work on mac with webrender because webrender uses OpenGL 3.x and the webgl 1.0 bindings are based on legacy opengl
2:06 PM <~jdm> orly
2:07 PM <cbrewster> We had discussed some possible solutions, but I am not sure if anything was decided yet. I just noticed we are considering going to webrender by default and thought of that

More discussion in #11138

@paulrouget
Copy link
Contributor

paulrouget commented May 12, 2016

So -w becomes useless. We should remove this option. Also, you want to change gfx.webrender.enabled in prefs.js.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented May 12, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 12, 2016

Trying commit c78eeb8 with merge 33c0ba6...

bors-servo added a commit that referenced this pull request May 12, 2016
Switch the default renderer to WebRender

Just testing for now to see if we even pass the tests... planning to `try` this once the queue dies down a bit. DO NOT MERGE.

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

bors-servo commented May 12, 2016

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented May 12, 2016

@metajack Should I also remove -g entirely? Pretty sure that's an even more broken code path. So, this would have just one optional -c flag, which we would eventually replace with whatever the LLVMPIPE/etc. solution will be in the future.

cc @glennw @pcwalton

@emilio
Copy link
Member

emilio commented May 12, 2016

I'm actually totally surprised about this passing given WebGL+webrender on mac is apparently broken, and that we only test WebGL on mac because of linux resource exhaustion.

@ConnorGBrewster, maybe this depends on the OSX version?

Could someone with the same version of OSX as the buildbots check that ./mach run -d -w tests/html/test_webgl_triangle.html works for them (note you'll have to move the mouse due to #11068)?

@metajack
Copy link
Contributor

metajack commented May 12, 2016

I can confirm that this is broken in 10.11.4. @larsbergstrom what version are the builders on?

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented May 12, 2016

@metajack The OSX builders are all on 10.10. We haven't yet updated to 10.11.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented May 12, 2016

@glennw Should I remove the show-parallel-paint and show-parallel-layout options, as they appear not to be implemented, or are those things we should bring back in the future?

@metajack
Copy link
Contributor

metajack commented May 12, 2016

I would really like to keep those if it's not a lot of work. If we do decide to remove them, let's open bugs to bring them back.

@metajack
Copy link
Contributor

metajack commented May 12, 2016

Well, show-parallel-layout anyway. show-parallel-paint doesn't really make sense with webrender, and webrender's debug overlay is the replacement.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented May 14, 2016

@bors-servo clean retry try

bors-servo added a commit that referenced this pull request May 14, 2016
Switch the default renderer to WebRender

Just testing for now to see if we even pass the tests... planning to `try` this once the queue dies down a bit. DO NOT MERGE.

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

bors-servo commented May 14, 2016

Trying commit c78eeb8 with merge 688e076...

@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2016

@SimonSapin
Copy link
Member

SimonSapin commented Jun 7, 2016

What’s blocking this from landing?

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 7, 2016

Supposedly, the WebGL tests do not pass on OSX 10.11. The successful run above is on OSX 10.10. We're still upgrading the machines to 10.11.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 7, 2016

Indeed, here's the log:

[larsberg@larsberg servo2]$ ./mach run -r -w tests/html/test_webgl_triangle.html
ERROR:script::dom::webglrenderingcontext: Couldn't create WebGLRenderingContext: CGLCreateContext
ERROR:script::dom::webglrenderingcontext: Couldn't create WebGLRenderingContext: CGLCreateContext
vertex source: 

precision mediump float;

attribute vec2 aVertexPosition;
attribute vec4 aColour;
varying vec4 aVertexColor;

void main() {
  aVertexColor = aColour;
  gl_Position = vec4(aVertexPosition, 0.0, 1.0);
}

fragment source: 

precision mediump float;

varying vec4 aVertexColor;

void main() {
  gl_FragColor = aVertexColor;
}

ERROR:js::rust: Error at file:///Users/larsberg/servo2/tests/html/test_webgl_triangle.html:20:8: gl is null

[larsberg@larsberg servo2]$ 
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 7, 2016

@glennw @pcwalton The error above is blocking WR-as-default.

@emilio
Copy link
Member

emilio commented Jun 7, 2016

The proper fix for this is a large-ish refactor (though the linux code would benefit from it too).

As a short-term solution, I think webrender can run over a compatibility context, in which case I think OSX shouldn't complain when creating a shared context. I can't test it since I have no mac, but @glennw or @pcwalton should be able to confirm if WR could use a compat profile.

@emilio
Copy link
Member

emilio commented Jun 7, 2016

Also, we could fall back to readback if we fail to create a shared context, that should be easy to integrate too (will have to convert WebGL display items to contain either a context id or an "image", and in the second case the path followed would be the same as 2d canvas).

If a compat context is not viable, maybe we can do this as a workaround to keep WebGL tests running.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 28, 2016

@bors-servo retry

  • just testing infra changes
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 28, 2016

@bors-servo retry clean

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 28, 2016

@bors-servo try retry clean

bors-servo added a commit that referenced this pull request Jun 28, 2016
Switch the default renderer to WebRender

Just testing for now to see if we even pass the tests... planning to `try` this once the queue dies down a bit. DO NOT MERGE.

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

bors-servo commented Jun 28, 2016

Trying commit c78eeb8 with merge 8211c98...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 28, 2016

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 28, 2016

@bors-servo try retry clean

  • more infra testing
@bors-servo
Copy link
Contributor

bors-servo commented Jun 28, 2016

Trying commit c78eeb8 with merge fd7f46d...

bors-servo added a commit that referenced this pull request Jun 28, 2016
Switch the default renderer to WebRender

Just testing for now to see if we even pass the tests... planning to `try` this once the queue dies down a bit. DO NOT MERGE.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11136)
<!-- Reviewable:end -->
@larsbergstrom larsbergstrom force-pushed the larsbergstrom:default_to_webrender branch from c78eeb8 to 8f75d4a Jun 28, 2016
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 28, 2016

@bors-servo try retry clean

more infra testing
@bors-servo
Copy link
Contributor

bors-servo commented Jun 28, 2016

Trying commit 8f75d4a with merge cad3afd...

bors-servo added a commit that referenced this pull request Jun 28, 2016
Switch the default renderer to WebRender

Just testing for now to see if we even pass the tests... planning to `try` this once the queue dies down a bit. DO NOT MERGE.

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

bors-servo commented Jun 28, 2016

💔 Test failed - linux-rel

@emilio
Copy link
Member

emilio commented Jun 28, 2016

That test failure is #11703

On 06/28/2016 02:30 PM, bors-servo wrote:

💔 Test failed - linux-rel
http://build.servo.org/builders/linux-rel/builds/1839


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11136 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/ABQwugRR4FMEaVlwetoK143MksMtlyGgks5qQTAVgaJpZM4IcYga.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jun 28, 2016

lol -

let use_webrender =
(prefs::get_pref("gfx.webrender.enabled").as_boolean().unwrap() || opt_match.opt_present("w")) &&
!opt_match.opt_present("z");

@glennw
Copy link
Member

glennw commented Jun 29, 2016

@larsbergstrom I have enabled WR as the default and run through all the CSS and WPT tests. Comments below:

test-css:

Ran 9977 tests finished in 2669.0 seconds.
• 9902 ran as expected. 131 tests skipped.
• 34 tests failed unexpectedly
• 7 tests timed out unexpectedly
• 34 tests passed unexpectedly

attachment-local-clipping-* - CRASH - Around 10 crashes - some panic in WR that should be relatively simple.
/css-backgrounds-3_dev/html4/ - About 10 genuine new passes.
/css-transforms-1_dev/html/ - Several new passes, around 25 new fails - transform math / clip issues.
/filters-1_dev/html/ - Around 8 new passes, 2 new fails.

test-wpt:

• 6197 ran as expected. 2135 tests skipped.
• 10 tests failed unexpectedly
• 10 tests timed out unexpectedly
• 29 tests had unexpected subtest results

/cssom-view/elementScroll.html - Several DOM scroll related fails - I think some of this is disabled / missing under WR.
/2dcontext/compositing/ - Most of the subtest unexpected results - probably genuine fails / issues that need to be fixed.
/mozilla/css - Around 10 failures - 5 of them need test expectations (images) updated, the rest are genuine failures that need to be fixed at some point.

Given those results above, should we update test expectations and make the switch (perhaps filing bugs for the remaining issues as follow ups) or leave WR as non-default until we fix up some of those areas?

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 29, 2016

I would be fine with updating the expectations, as long as we're convinced we will fix the issues soon.

@pcwalton
Copy link
Contributor

pcwalton commented Jul 25, 2016

I'm fine with updating the expectations too.

@jdm
Copy link
Member

jdm commented Jul 26, 2016

I'm still a bit worried about the regression in script-initiated scrolling that turning on webrender by default means. We currently support APIs like setting scrollTop and using window.scroll(), and those aren't supported by webrender yet.

@glennw
Copy link
Member

glennw commented Jul 26, 2016

@jdm Hmmm, I thought that had been resolved with some work @pcwalton did a month or so ago, but I'm having trouble finding the commit. @pcwalton Do you know what the status of that is?

@pcwalton
Copy link
Contributor

pcwalton commented Jul 26, 2016

Uh, I don't recall. Let me look through my git branches.

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

You can’t perform that action at this time.