Improved WebGL architecture #17891

MortimerGoro · 2017-07-27T16:37:34Z

Info about the big picture and the goals of the WebGL refactor in this thread: https://groups.google.com/forum/#!topic/mozilla.dev.servo/0WMGz60kKzQ

I tried to reduce this PR as much as possible as requested in the thread. I'll do separate PRs for other features (e.g.: Batch messages or use shared memory to improve frame times) or fixes.

Some tips to ease the review process:

Most changes in DOM objects follow the same pattern (remove CanvasMsg wrapper and use the new sender method).
WebGLCommands are the same ones as before (moved from webrender_api). So those lines are already reviewed.
See WebGL traits in components/canvas_traits/webgl.rs
See WebGLThread and WR External Image bridge in components/canvas/webgl_thread.rs
The implementation submitted in this PR creates a single WebGLThread for all ScriptThread/Pipelines. See that in components/canvas/webgl_mode/inprocess.rs

The conformance tests will help to guarantee that we don't miss anything.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-07-27T16:37:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/pipeline.rs, components/constellation/lib.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs
@KiChjang: components/script/dom/webglframebuffer.rs, components/script/dom/webglshader.rs, components/script/dom/webgl_validations/tex_image_2d.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs and 24 more
@fitzgen: components/script/dom/webglframebuffer.rs, components/script/dom/webglshader.rs, components/script/dom/webgl_validations/tex_image_2d.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs and 24 more
@emilio: components/layout/display_list_builder.rs, components/script/dom/webglframebuffer.rs, components/script/dom/webglshader.rs, components/script/dom/webgl_validations/tex_image_2d.rs, components/layout/webrender_helpers.rs and 14 more

highfive · 2017-07-27T16:37:40Z

Warning

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

stshine · 2017-07-27T18:14:16Z

@bors-servo try

bors-servo · 2017-07-27T18:14:22Z

⌛ Trying commit 1cee878 with merge dc33138...


        Auto merge of #17891 - MortimerGoro:webgl_move, r=<try>

Improved WebGL architecture  Info about the big picture and the goals of the WebGL refactor in this thread: https://groups.google.com/forum/#!topic/mozilla.dev.servo/0WMGz60kKzQ I tried to reduce this PR as much as possible as requested in the thread. I'll do separate PRs for other features (e.g.: Batch messages or use shared memory to improve frame times) or fixes. Some tips to ease the review process: - Most changes in DOM objects follow the same pattern (remove CanvasMsg wrapper and use the new sender method). - WebGLCommands are the same ones as before (moved from webrender_api). So those lines are already reviewed. - See WebGL traits in [components/canvas_traits/webgl.rs](https://github.com/servo/servo/pull/17891/files#diff-8701045d01505418701d0631d4d45562) - See WebGLThread and WR External Image bridge in [components/canvas/webgl_thread.rs](https://github.com/servo/servo/pull/17891/files#diff-281554879f39a2a041f7a69d442a5d2e) - The implementation submitted in this PR creates a single `WebGLThread` for all ScriptThread/Pipelines. See that in [components/canvas/webgl_mode/inprocess.rs](https://github.com/servo/servo/pull/17891/files#diff-250070c6c5a38c7f9fa0f5b3c101f68b) The conformance tests will help to guarantee that we don't miss anything. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17891)

bors-servo · 2017-07-27T18:48:04Z

💔 Test failed - linux-rel-wpt

jdm · 2017-07-27T19:00:41Z

  ▶ FAIL [expected PASS] /_mozilla/mozilla/webgl/tex_image_2d_mipmap.html
  └   → /_mozilla/mozilla/webgl/tex_image_2d_mipmap.html 1017abb40a2237d2ac16e329c9754cbedcf30e1c
/_mozilla/mozilla/webgl/tex_image_2d_simple_ref.html 06711e6d8ba9520a7c0476d41130022d3e609160
Testing 1017abb40a2237d2ac16e329c9754cbedcf30e1c == 06711e6d8ba9520a7c0476d41130022d3e609160

Also, all of our multiprocess tests panicked with internal error: entered unreachable code (thread Constellation, at /home/servo/buildbot/slave/linux-rel-wpt/build/components/canvas_traits/webgl_channel/mpsc.rs:76). We run them like so: ./mach test-wpt --release --binary-arg=--multiprocess --processes 24 eventsource

asajeffrey · 2017-07-27T19:56:29Z

Yay, our multiprocess tests worked!

MortimerGoro · 2017-07-28T16:18:44Z

▶ FAIL [expected PASS] /_mozilla/mozilla/webgl/tex_image_2d_mipmap.html
└ → /_mozilla/mozilla/webgl/tex_image_2d_mipmap.html

Fixed. The webrender_api::ImageDescriptor.is_opaque property was not correctly set, leading to a black background instead of a transparent one when the webgl canvas is composited. To fix this I used the WebGLContextAttributes::alpha attribute to correctly set the webrender_api::ImageDescriptor.is_opaque property.

Also, all of our multiprocess tests panicked with internal error: entered unreachable code (thread Constellation, at /home/servo/buildbot/slave/linux-rel-wpt/build/components/canvas_traits/webgl_channel/mpsc.rs:76). We run them like so: ./mach test-wpt --release --binary-arg=--multiprocess --processes 24 eventsource

Thanks for the info! I fixed the unreachable code crash and all multiprocess tests pass now.

bors-servo · 2017-07-28T19:33:48Z

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

MortimerGoro · 2017-07-28T20:05:42Z

Rebased. The latest WR update includes servo/webrender#1524 which was required to fix a failing webgl test.

All tests should pass now.

jdm · 2017-07-28T21:30:44Z

@bors-servo: try

bors-servo · 2017-07-28T21:30:49Z

⌛ Trying commit 931826b with merge 1f2d551...


        Auto merge of #17891 - MortimerGoro:webgl_move, r=<try>

Improved WebGL architecture  Info about the big picture and the goals of the WebGL refactor in this thread: https://groups.google.com/forum/#!topic/mozilla.dev.servo/0WMGz60kKzQ I tried to reduce this PR as much as possible as requested in the thread. I'll do separate PRs for other features (e.g.: Batch messages or use shared memory to improve frame times) or fixes. Some tips to ease the review process: - Most changes in DOM objects follow the same pattern (remove CanvasMsg wrapper and use the new sender method). - WebGLCommands are the same ones as before (moved from webrender_api). So those lines are already reviewed. - See WebGL traits in [components/canvas_traits/webgl.rs](https://github.com/servo/servo/pull/17891/files#diff-8701045d01505418701d0631d4d45562) - See WebGLThread and WR External Image bridge in [components/canvas/webgl_thread.rs](https://github.com/servo/servo/pull/17891/files#diff-281554879f39a2a041f7a69d442a5d2e) - The implementation submitted in this PR creates a single `WebGLThread` for all ScriptThread/Pipelines. See that in [components/canvas/webgl_mode/inprocess.rs](https://github.com/servo/servo/pull/17891/files#diff-250070c6c5a38c7f9fa0f5b3c101f68b) The conformance tests will help to guarantee that we don't miss anything. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17891)

bors-servo · 2017-07-28T22:06:08Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2017-07-28T22:06:12Z

⌛ Trying commit 931826b with merge e0645d0...


        Auto merge of #17891 - MortimerGoro:webgl_move, r=<try>

Improved WebGL architecture  Info about the big picture and the goals of the WebGL refactor in this thread: https://groups.google.com/forum/#!topic/mozilla.dev.servo/0WMGz60kKzQ I tried to reduce this PR as much as possible as requested in the thread. I'll do separate PRs for other features (e.g.: Batch messages or use shared memory to improve frame times) or fixes. Some tips to ease the review process: - Most changes in DOM objects follow the same pattern (remove CanvasMsg wrapper and use the new sender method). - WebGLCommands are the same ones as before (moved from webrender_api). So those lines are already reviewed. - See WebGL traits in [components/canvas_traits/webgl.rs](https://github.com/servo/servo/pull/17891/files#diff-8701045d01505418701d0631d4d45562) - See WebGLThread and WR External Image bridge in [components/canvas/webgl_thread.rs](https://github.com/servo/servo/pull/17891/files#diff-281554879f39a2a041f7a69d442a5d2e) - The implementation submitted in this PR creates a single `WebGLThread` for all ScriptThread/Pipelines. See that in [components/canvas/webgl_mode/inprocess.rs](https://github.com/servo/servo/pull/17891/files#diff-250070c6c5a38c7f9fa0f5b3c101f68b) The conformance tests will help to guarantee that we don't miss anything. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17891)

bors-servo · 2017-07-28T23:15:11Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

MortimerGoro · 2017-08-02T19:37:40Z

Rebased to fix the conflict. Why does it still show needs-rebase label?


         Improve WebGL architecture.

MortimerGoro · 2017-08-15T20:19:54Z

Those tests failed because I accidentally removed a byte_swap() call in one of the review commits MortimerGoro@5cf6ac3

Let's try again!

@bors-servo r=glennw,emilio

MortimerGoro · 2017-08-15T21:00:06Z

@bors-servo r=glennw,emilio

bors-servo · 2017-08-15T21:00:07Z

📌 Commit 703962f has been approved by glennw,emilio

bors-servo · 2017-08-15T21:00:11Z

⌛ Testing commit 703962f with merge 90f55ea...


        Auto merge of #17891 - MortimerGoro:webgl_move, r=glennw,emilio

Improved WebGL architecture  Info about the big picture and the goals of the WebGL refactor in this thread: https://groups.google.com/forum/#!topic/mozilla.dev.servo/0WMGz60kKzQ I tried to reduce this PR as much as possible as requested in the thread. I'll do separate PRs for other features (e.g.: Batch messages or use shared memory to improve frame times) or fixes. Some tips to ease the review process: - Most changes in DOM objects follow the same pattern (remove CanvasMsg wrapper and use the new sender method). - WebGLCommands are the same ones as before (moved from webrender_api). So those lines are already reviewed. - See WebGL traits in [components/canvas_traits/webgl.rs](https://github.com/servo/servo/pull/17891/files#diff-8701045d01505418701d0631d4d45562) - See WebGLThread and WR External Image bridge in [components/canvas/webgl_thread.rs](https://github.com/servo/servo/pull/17891/files#diff-281554879f39a2a041f7a69d442a5d2e) - The implementation submitted in this PR creates a single `WebGLThread` for all ScriptThread/Pipelines. See that in [components/canvas/webgl_mode/inprocess.rs](https://github.com/servo/servo/pull/17891/files#diff-250070c6c5a38c7f9fa0f5b3c101f68b) The conformance tests will help to guarantee that we don't miss anything. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17891)

bors-servo · 2017-08-15T22:07:16Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: glennw,emilio
Pushing 90f55ea to master...


        Revert "Auto merge of servo#17891 - MortimerGoro:webgl_move, r=glennw…

…,emilio" This reverts commit 90f55ea, reversing changes made to 2e60b27.


        Auto merge of #18114 - emilio:revert-webgl-refactor, r=nox

Revert "Auto merge of #17891 - MortimerGoro:webgl_move, r=glennw,emilio" This reverts commit 90f55ea, reversing changes made to 2e60b27. Doing that per Josh's request, since it's causing very frequent intermittent OOMs on the android builders.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18114)


        Auto merge of #18221 - MortimerGoro:webgl_fnv, r=jdm

Use FnvHashMap in WebGL implementation.  Follow up from the WebGL redesign: #17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18221)


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : ae0f3f7bedbe4a6d2313cc3e0120a347b597d91c


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0 UltraBlame original commit: 5ebdac555524c105e7ad2514a415d4c32c4a76c1


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0 UltraBlame original commit: 5ebdac555524c105e7ad2514a415d4c32c4a76c1


        servo: Merge #18221 - Use FnvHashMap in WebGL implementation (from Mo…

…rtimerGoro:webgl_fnv); r=jdm  Follow up from the WebGL redesign: servo/servo#17891 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   Source-Repo: https://github.com/servo/servo Source-Revision: 4598ddb0bcf62aa2ec54d896a67eae20db4588c0 UltraBlame original commit: 5ebdac555524c105e7ad2514a415d4c32c4a76c1

highfive assigned wafflespeanut Jul 27, 2017

highfive added the S-awaiting-review label Jul 27, 2017

MortimerGoro force-pushed the MortimerGoro:webgl_move branch from 6822295 to 1cee878 Jul 27, 2017

MortimerGoro force-pushed the MortimerGoro:webgl_move branch from 1cee878 to 2ab4bc7 Jul 28, 2017

MortimerGoro mentioned this pull request Jul 28, 2017

WebGL encapsulation servo/webrender#1530

Merged

MortimerGoro force-pushed the MortimerGoro:webgl_move branch from 2ab4bc7 to 1b13a69 Jul 28, 2017

highfive added the S-needs-rebase label Jul 28, 2017

MortimerGoro force-pushed the MortimerGoro:webgl_move branch from 1b13a69 to 931826b Jul 28, 2017

jdm assigned jdm and unassigned wafflespeanut Aug 1, 2017

MortimerGoro force-pushed the MortimerGoro:webgl_move branch from 931826b to 009f3df Aug 2, 2017

Improve WebGL architecture.

Loading status checks…

703962f

MortimerGoro force-pushed the MortimerGoro:webgl_move branch from fb7152f to 703962f Aug 15, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Aug 15, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 15, 2017

jdm mentioned this pull request Aug 15, 2017

Optimize WebGL function calls #16679

Closed

3 of 5 tasks complete

bors-servo merged commit 703962f into servo:master Aug 15, 2017
2 checks passed

2 checks passed

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 15, 2017

This was referenced Aug 15, 2017

Implement CanvasRenderingContext2d.fillText's "unimplemented" message #18020

Merged

Expose the source map URL on a style sheet #18073

Merged

jdm mentioned this pull request Aug 16, 2017

Android builder runs out of memory #18116

Closed

MortimerGoro mentioned this pull request Aug 16, 2017

Improved WebGL architecture. #18119

Closed

3 of 5 tasks complete

MortimerGoro mentioned this pull request Aug 24, 2017

Use FnvHashMap in WebGL implementation. #18221

Merged

3 of 5 tasks complete

servo / servo

Improved WebGL architecture #17891

Improved WebGL architecture #17891

MortimerGoro commented Jul 27, 2017 •

edited

highfive commented Jul 27, 2017

highfive commented Jul 27, 2017

stshine commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 27, 2017

jdm commented Jul 27, 2017 •

edited

asajeffrey commented Jul 27, 2017

MortimerGoro commented Jul 28, 2017 •

edited

bors-servo commented Jul 28, 2017

MortimerGoro commented Jul 28, 2017

jdm commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

MortimerGoro commented Aug 2, 2017

MortimerGoro commented Aug 15, 2017

MortimerGoro commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

servo / servo

Join GitHub today

Improved WebGL architecture #17891

Improved WebGL architecture #17891

Conversation

MortimerGoro commented Jul 27, 2017 • edited

highfive commented Jul 27, 2017

highfive commented Jul 27, 2017

stshine commented Jul 27, 2017

bors-servo commented Jul 27, 2017

bors-servo commented Jul 27, 2017

jdm commented Jul 27, 2017 • edited

asajeffrey commented Jul 27, 2017

MortimerGoro commented Jul 28, 2017 • edited

bors-servo commented Jul 28, 2017

MortimerGoro commented Jul 28, 2017

jdm commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

bors-servo commented Jul 28, 2017

MortimerGoro commented Aug 2, 2017

MortimerGoro commented Aug 15, 2017

MortimerGoro commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

MortimerGoro commented Jul 27, 2017 •

edited

jdm commented Jul 27, 2017 •

edited

MortimerGoro commented Jul 28, 2017 •

edited