Generate shaders in build time instead of run time #2891

zakorgy · 2018-07-12T12:38:52Z

The discussion about this stared here: szeged#197 (comment)

Added a helper shaders.ron file which contains the possible feature sets for each shader.
In the build script we generate shaders using the information from this ron file, for each possible feature configuration. The generated shader file names contain the feature names in lower case separated by a _.
The shader processing code parts (get_shader_source, parse_shader_source, build_shader_strings) are moved in the build script from device/gl.rs.
In run-time we pass the base name of the shader and the used features to load the proper shader file.

There is a hacky part, because we don't always know which shader version (GL or GLES) we intend to use in build time. Replacing it in run-time, if we differ from the default #version 150 (GL), seemed the best solution.

@kvark, @gw3583 could you please take a look?

This change is

kvark · 2018-07-12T20:10:11Z

webrender/build.rs

+fn get_shader_source(shader_name: &str, shaders: &HashMap<String, String>) -> Option<String> {
+    if let Some(shader_file) = shaders.get(shader_name) {
+        let shader_file_path = Path::new(shader_file);
+        if let Ok(mut shader_source_file) = File::open(shader_file_path) {


shouldn't we panic if the shader can't be opened?

kvark · 2018-07-12T20:10:11Z

webrender/build.rs

+    let mut file = File::open("res/shaders.ron").expect("Unable to open shaders.ron");
+    let mut source = String::new();
+    file.read_to_string(&mut source).expect("Unable to read shaders.ron");
+    let shader_configs: Vec<Shader> = de::from_str(&source).expect("Unable to deserialize shaders.ron");


would probably be easier to do from_reader

kvark · 2018-07-12T20:10:11Z

webrender/res/shaders.ron

+        ],
+    ),// [18]
+    (
+    name: "pf_vector_stencil",


something weird with the indentation here

kvark · 2018-07-12T20:10:11Z

webrender/tests/angle_shader_validation.rs

@@ -16,8 +16,6 @@ struct Shader {
    features: &'static [&'static str],
 }

-const SHADER_PREFIX: &str = "#define WR_MAX_VERTEX_TEXTURE_WIDTH 1024\n";


why does the shader validator need to have a separate list of shaders and features, given that we already have them described in RON?

zakorgy · 2018-07-16T08:29:48Z

@kvark I have addressed your comments. The validator now loads the shader configs from the RON file, however I had to add a list of excluded features, because the extensions used by these features are not supported by the angle validator.

kvark · 2018-07-16T17:52:59Z

@zakorgy thank you! Please also not leave the "fixup! ..." commits and instead do a real git fixup for those, making sure that each is buildable and has a concrete purpose.
For merging, we'll need to have @gw3583 to review it.


        Genereate shaders in build script with the help of a .ron file


        Load shaders from files instead of building them

gw3583 · 2018-07-18T21:31:12Z

@kvark Looks like this is ready to go?

kvark · 2018-07-19T02:51:11Z

@gw3583 Thanks for looking (I assume you did, right?)
@bors-servo r+

bors-servo · 2018-07-19T02:51:12Z

📌 Commit 1c4f2da has been approved by kvark

bors-servo · 2018-07-19T05:58:29Z

⌛ Testing commit 1c4f2da with merge 1ca448a...


        Auto merge of #2891 - szeged:build-shaders, r=kvark

@kvark

Generate shaders in build time instead of run time The discussion about this stared here: szeged#197 (comment) 1. Added a helper `shaders.ron` file which contains the possible feature sets for each shader. 2. In the build script we generate shaders using the information from this `ron` file, for each possible feature configuration. The generated shader file names contain the feature names in lower case separated by a `_`. The shader processing code parts (get_shader_source, parse_shader_source, build_shader_strings) are moved in the build script from `device/gl.rs`. 3. In run-time we pass the base name of the shader and the used features to load the proper shader file. There is a hacky [part](master...szeged:build-shadersdiff-0ad2b4b9ac27cf5624c19208f294d671R186), because we don't always know which shader version (GL or GLES) we intend to use in build time. Replacing it in run-time, if we differ from the default `#version 150` (GL), seemed the best solution. @kvark, @gw3583 could you please take a look?  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2891)

bors-servo · 2018-07-19T07:19:28Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 1ca448a to master...

staktrace · 2018-07-19T11:12:15Z

I'm guessing this PR is the one that broke firefox CI with shader errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=059d4f6b01e13547633496cc649c76b0f8c6e282

kvark · 2018-07-19T13:03:35Z

@staktrace that is unfortunate. Reverting in #2912 until the Gecko side is clear.


        Auto merge of #2912 - kvark:revert-shaders, r=staktrace

@staktrace

Revert #2891 - shader generation at build time due to #2891 (comment) r? @staktrace cc @zakorgy  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2912)

zakorgy · 2018-07-20T13:02:01Z

@staktrace could you please point me how can I reproduce the error from the CI. I'm not familiar with gecko tests, and it would be nice to know which command/script should I use.

staktrace · 2018-07-20T14:38:52Z

@zakorgy The simplest way is to build Firefox using the [instructions] (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions) and then apply your changes to the gfx/webrender{,api} folders. Then rebuild and run to make sure it doesn't blow up. You can also run reftests using ./mach reftest . If you get the reftests in layout/reftests/reftest-sanity working that's probably a good sign and we can do a try push to see if anything else fails.

kvark reviewed Jul 12, 2018

View changes

zakorgy force-pushed the szeged:build-shaders branch 2 times, most recently from 4ebd0e8 to d969a8a Jul 16, 2018

zakorgy added 2 commits Jul 4, 2018

Genereate shaders in build script with the help of a .ron file

28bfc24

Load shaders from files instead of building them

Loading status checks…

1c4f2da

zakorgy force-pushed the szeged:build-shaders branch from d969a8a to 1c4f2da Jul 17, 2018

bors-servo merged commit 1c4f2da into servo:master Jul 19, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

kvark deleted the szeged:build-shaders branch Jul 19, 2018

kvark mentioned this pull request Jul 19, 2018

Revert #2891 - shader generation at build time #2912

Merged

zakorgy restored the szeged:build-shaders branch Jul 20, 2018

kvark deleted the szeged:build-shaders branch Jul 20, 2018

kvark mentioned this pull request Jul 20, 2018

Better inversible transformations #2913

Merged

servo / webrender

Generate shaders in build time instead of run time #2891

Generate shaders in build time instead of run time #2891

zakorgy commented Jul 12, 2018 •

edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zakorgy commented Jul 16, 2018

kvark commented Jul 16, 2018

gw3583 commented Jul 18, 2018

kvark commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

staktrace commented Jul 19, 2018

kvark commented Jul 19, 2018

zakorgy commented Jul 20, 2018

staktrace commented Jul 20, 2018

servo / webrender

Join GitHub today

Generate shaders in build time instead of run time #2891

Generate shaders in build time instead of run time #2891

Conversation

zakorgy commented Jul 12, 2018 • edited by larsbergstrom

This comment has been minimized.

kvark Jul 12, 2018

This comment has been minimized.

kvark Jul 12, 2018

This comment has been minimized.

kvark Jul 12, 2018

This comment has been minimized.

kvark Jul 12, 2018

zakorgy commented Jul 16, 2018

kvark commented Jul 16, 2018

gw3583 commented Jul 18, 2018

kvark commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

bors-servo commented Jul 19, 2018

staktrace commented Jul 19, 2018

kvark commented Jul 19, 2018

zakorgy commented Jul 20, 2018

staktrace commented Jul 20, 2018

zakorgy commented Jul 12, 2018 •

edited by larsbergstrom