Closed Bug 1370007 Opened 7 years ago Closed 7 years ago

Autogenerating shaders using fxc does not work on Linux (MinGW Build)

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file)

We don't have the Windows SDK, or are able to run Windows Binaries on Linux as part of the build chain we're working on in Bug 1330608. We've been able to work around it so far without much problem, but I'm not very familiar with Graphics to know what our options are for fxc and Bug 1365859.

Is there an open source alternative to fxc? Is it something that can be created by someone who doesn't know anything about Graphics?  

Failing those, how do you feel about a partial revert where the generated file is still present in the source tree? I'm not sure if the patch was done solely for the purpose of avoiding an ugly diff in hg, or if it was also to help an edit-run-checkin workflow. If it's the latter, we can detect if fxc is present, and if so, regenerate the file for easy checkin - that would solve the workflow problem while making the program optional...
Flags: needinfo?(dvander)
There's no open source alternative that I know of that can generate Shader Model 4 instructions. The patch was done for both reasons you mentioned.
Flags: needinfo?(dvander)
Can I write a patch that behaves the way I described? (Have the generated file in-tree, detect if fxc is present, and if so, on modification regenerate the file.) Would you accept it?

I'm open to other options, but I'm not sure if there are others.
After talking with David, there is a strong desire to not do a partial revert and put the file back in tree (even with autogenerating), as it slows down the developer workflow.

Shaders can be compiled at run time, so the tentative plan (and potentially only option?) is to add and conditionally compile in the shader compiling code if fxc cannot be found. An alternative would involve storing the latest version of the generated file somewhere and downloading it out of band or something very ugly.
Updating this with latest approach: I'm writing a 'fxc2' program that will call D3DCompileFromFile that exists in d3dcompiler_47.dll. This isn't ideal as it's not open source but it's the only option. https://github.com/wine-mirror/wine/tree/master/dlls/d3dcompiler_43 is not feature-complete enough to use.
Attachment #8880565 - Flags: review?(jmuizelaar)
I put in a try job to make sure this still runs on Windows, I should submit another to confirm artifact job still works after the first completes successfully. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3268cdfeb9da
Comment on attachment 8880565 [details]
Bug 1370007 Generate Shaders on a MinGW Cross Compile on Linux

dvander's probably more appropriate to review and maybe a build peer...
Attachment #8880565 - Flags: review?(jmuizelaar) → review?(dvander)
Attachment #8880565 - Flags: review?(jmuizelaar) → review?(dvander)
Attachment #8880565 - Flags: review?(ted)
Ted, could you review this from a build perspective?
Comment on attachment 8880565 [details]
Bug 1370007 Generate Shaders on a MinGW Cross Compile on Linux

https://reviewboard.mozilla.org/r/151898/#review160034

Kind of a bummer that these fxc tools rely on a non-open-source DLL provided by Microsoft. :-(

::: gfx/layers/d3d11/genshaders.py:85
(Diff revision 7)
>      shader_file,
>      '-E{0}'.format(shader_name),
>      '-Vn{0}'.format(shader_name),
>      '-Vi',
>    ]
> +  if 'fxc2' in buildconfig.substs['FXC']:

I think this would probably be better if you just explicitly checked if you were building on Linux here.

::: moz.configure:450
(Diff revision 7)
>      return ''
>  
>  set_config('MAKENSISU_FLAGS', nsis_flags)
>  
> +fxc = check_prog('FXC', ('fxc.exe', 'fxc2.exe'), when=depends(target)(lambda t: t.kernel == 'WINNT'))
> +wine = check_prog('WINE', ['wine'], when=depends(fxc)(lambda f: f and 'fxc2' in f))

Similarly here I would make this only run when `target.kernel == 'WINNT' and host.kernel == 'Linux'`.
Attachment #8880565 - Flags: review?(ted) → review+
Comment on attachment 8880565 [details]
Bug 1370007 Generate Shaders on a MinGW Cross Compile on Linux

https://reviewboard.mozilla.org/r/151898/#review160048

The whole operating system is closed source. It doesn't bother me that the build tools are too.

This seems fine as long as it doesn't affect normal Windows builds.
Attachment #8880565 - Flags: review?(dvander) → review+
nits fixed, thanks all!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/018e683b25cd
Generate Shaders on a MinGW Cross Compile on Linux r=dvander,ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/018e683b25cd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
have to backout this change for issues like https://treeherder.mozilla.org/logviewer.html#?job_id=112552463&repo=mozilla-central
Status: RESOLVED → REOPENED
Flags: needinfo?(tom)
Resolution: FIXED → ---
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/173533a31006
Backed out changeset 018e683b25cd for breaking tier 2 nightly builds on windows
We'd like to have these checks in in toolchain.config to avoid an extra depends(), but that is broken by Bug 1374727. If that is not resolved when I'm near completing Bug 1330608, we'll land these checks in the top level moz.config.
Depends on: 1374727
Flags: needinfo?(tom)
Okay, Bug 1374727 landed and the updated version gives me green try runs (except the L10N repack, but that error occurs elsewhere and not on the fxc detection like it did the first time.)  I'm going to mark this checkin-needed, and if there's any errors, feel free to back out without acknowledgement.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aeb9b61cd780
Generate Shaders on a MinGW Cross Compile on Linux r=dvander,ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aeb9b61cd780
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.