Closed Bug 1365859 Opened 3 years ago Closed 3 years ago

Autogenerate D3D11 shaders as part of running mach

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files)

It's painful to have to attach and check-in a 500KB patch every time we change a line of shader code. I'm pretty sure FXC is included on all Visual Studio versions we support, probably via the Windows SDK. We can pretty easily include it as a step in the build process.
Mach can call out to external scripts to perform complex build steps, but the script must be Python. I could have just wrapped genshaders.sh in a Python script, but then the build wouldn't automatically recognize changes to included .hlsl files. So it made the most sense to just port the thing wholesale to Python.

Also, I'd like Advanced Layers to use the same script, so I separated the hardcoded shader list into a "shader.manifest" file. Since both CompositorD3D11 and Advanced Layers have multiple entry points per .hlsl file, the manifest format is designed to group entries in a similar way. For simplicity the manifest is in YAML.

Note 1: genshaders.py now wraps its output in the mozilla::layers namespace, so I had to fix up a few other files. (This is just for parity with AL's genshaders.sh file.)

Note 2: This patch provides a shell-only version of genshaders.py. Mach integration is in the next patch.
Attachment #8868908 - Flags: review?(bas)
This integrates genshaders.py into the build and deletes the pre-generated CompositorD3D11Shaders.h file currently checked into the tree. (Thus, the 450K patch.)

Luckily the shader compiler (FXC) has an option to dump #include use, so this patch makes use of that by parsing FXC's output, and returns the set of dependencies.
Attachment #8868910 - Flags: review?(mshal)
Comment on attachment 8868910 [details] [diff] [review]
part 2, integrate into mach

>diff --git a/gfx/layers/d3d11/genshaders.py b/gfx/layers/d3d11/genshaders.py
>index a5eda7ebcc52..54ef26a7b714 100644
>--- a/gfx/layers/d3d11/genshaders.py
>+++ b/gfx/layers/d3d11/genshaders.py
>+def find_dependencies(fxc_output):
>+  # Dependencies look like this:
>+  #   Resolved to [<path>]
>+  #
>+  # Microsoft likes to change output strings based on the user's language, so
>+  # instead of pattern matching on that string, we take everything in between
>+  # brackets. We filter out potentially bogus strings later.
>+  deps = set()
>+  for line in fxc_output.split('\n'):
>+    m = re.search(r"\[([^\]]+)\]", line)
>+    if m is None:
>+      continue
>+    dep_path = m.group(1)
>+    dep_path = os.path.normpath(dep_path)
>+    if os.path.isfile(dep_path):
>+      deps.add(dep_path)
>+  return deps

Is this the only way to get dependencies out of fxc? It seems to work fine, but relying on the program output to be unchanged can be problematic if they decide to change the strings. I think in that case we would just silently lose the dependencies.

The generated CompositorD3D11Shaders.h file that I got had quite a big diff from the previous version checked into the tree. Is that intended? Seems to just be lots of random numbers, so I have no idea if it's significant :)
Attachment #8868910 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #4)
> Comment on attachment 8868910 [details] [diff] [review]
> 
> Is this the only way to get dependencies out of fxc? It seems to work fine,
> but relying on the program output to be unchanged can be problematic if they
> decide to change the strings. I think in that case we would just silently
> lose the dependencies.

Yeah unfortunately it's the only way. Is it a good idea to assert if no dependencies are found?

> The generated CompositorD3D11Shaders.h file that I got had quite a big diff
> from the previous version checked into the tree. Is that intended? Seems to
> just be lots of random numbers, so I have no idea if it's significant :)

I ran part 1 locally before doing the git rm, but didn't include those changes in the first patch. The order of the blobs changed which isn't significant. And fxc can output different blobs based on the compiler version, but it doesn't really matter.
Comment on attachment 8868908 [details] [diff] [review]
part 1, rewrite genshaders in python

Review of attachment 8868908 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me. All machines we use now should use Python. Be sure to test this on VS2017.
Attachment #8868908 - Flags: review?(bas) → review+
(In reply to David Anderson [:dvander] from comment #5)
> (In reply to Michael Shal [:mshal] from comment #4)
> > Comment on attachment 8868910 [details] [diff] [review]
> > 
> > Is this the only way to get dependencies out of fxc? It seems to work fine,
> > but relying on the program output to be unchanged can be problematic if they
> > decide to change the strings. I think in that case we would just silently
> > lose the dependencies.
> 
> Yeah unfortunately it's the only way. Is it a good idea to assert if no
> dependencies are found?

Sounds reasonable to me, if you always expect fxc to have some dependencies. Right now with only the one .hlsl file it just finds the same two includes so it should be fine.

Actually since this just uses #include like C code, another option would be to run cpp to get the dependencies (eg: 'cpp -M [shader_file]'). Though on Windows we have CPP as 'cl.exe -E', so maybe that won't be so easy.
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2323c4c38c31
Rewrite genshaders.sh as a Python script that uses a manifest. (bug 1365859 part 1, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5911d850652a
Generate shader blobs as part of the build process. (bug 1365859 part 2, r=mshal)
https://hg.mozilla.org/mozilla-central/rev/2323c4c38c31
https://hg.mozilla.org/mozilla-central/rev/5911d850652a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See Also: → 1370987
You need to log in before you can comment on or make changes to this bug.