Closed Bug 999260 Opened 10 years ago Closed 10 years ago

Ship both d3dcompiler _43 and _46+ for use by ANGLE

Categories

(Core :: Graphics: CanvasWebGL, defect)

All
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file, 4 obsolete files)

Right now, only ship _43, though there's a build flag to try to pull in _46+. However, _46 and newer don't work on XP, so we can't strictly upgrade to a newer version.
We can, however, ship both _43 and _46+, and use the newest one the client supports.
Attached patch dual-d3dc - WIP (obsolete) — Splinter Review
Decision time: Should we:
* Require both _43 and _46+ for builds, opt-out for disabling the _46 req.
OR:
* Only require _43, and just make sure that our build slaves use the magic switch to also build _46+.
Flags: needinfo?(vladimir)
Is _43 always required? I would not like to install DXSDK again...
(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Created attachment 8409970 [details] [diff] [review]
> dual-d3dc - WIP
> 
> Decision time: Should we:
> * Require both _43 and _46+ for builds, opt-out for disabling the _46 req.
> OR:
> * Only require _43, and just make sure that our build slaves use the magic
> switch to also build _46+.

Who will care?
How about: require either for builds, make sure build slaves do both.

We want to make it easy for developers; this means really requiring only 46/47 from the standard Windows SDK.  If they want to use an old SDK, then they need to have 43 available from the old dx sdk.  It doesn't make sense to require 43, though.

For the build slaves, it should package both 43 and 46.  I would add a switch to require both to get packaged, and make the build slave scripts use that.
Flags: needinfo?(vladimir)
Attached patch dual-d3dc (obsolete) — Splinter Review
Attachment #8409970 - Attachment is obsolete: true
Attachment #8410582 - Flags: review?(mshal)
Depends on: 980697
Comment on attachment 8410582 [details] [diff] [review]
dual-d3dc

>+# Check for valid WinSDK
>+MOZ_HAS_WINSDK_WITH_D3D=
>+
>+# On mingw, check if headers are provided by toolchain.
>+if test "$OS_TARGET" = "WINNT" -a -n "$GNU_CC"; then
>+  MOZ_CHECK_HEADER(d3d10.h, MOZ_HAS_WINSDK_WITH_D3D=1)
>+fi
>+
>+if test -n "$WINDOWSSDKDIR"; then
>+  MOZ_WINSDK_TEST_PATH="$WINDOWSSDKDIR/Include/um/d3d10.h"
>+  if test -f "$MOZ_WINSDK_TEST_PATH"; then
>+    MOZ_HAS_WINSDK_WITH_D3D=1
>+  fi
>+else
>+  AC_MSG_RESULT([WINDOWSSDKDIR is missing.])
>+fi

Who is using MOZ_HAS_WINSDK_WITH_D3D after the patch?
(In reply to Masatoshi Kimura [:emk] from comment #8)
> Comment on attachment 8410582 [details] [diff] [review]
> dual-d3dc
> 
> >+# Check for valid WinSDK
> >+MOZ_HAS_WINSDK_WITH_D3D=
> >+
> >+# On mingw, check if headers are provided by toolchain.
> >+if test "$OS_TARGET" = "WINNT" -a -n "$GNU_CC"; then
> >+  MOZ_CHECK_HEADER(d3d10.h, MOZ_HAS_WINSDK_WITH_D3D=1)
> >+fi
> >+
> >+if test -n "$WINDOWSSDKDIR"; then
> >+  MOZ_WINSDK_TEST_PATH="$WINDOWSSDKDIR/Include/um/d3d10.h"
> >+  if test -f "$MOZ_WINSDK_TEST_PATH"; then
> >+    MOZ_HAS_WINSDK_WITH_D3D=1
> >+  fi
> >+else
> >+  AC_MSG_RESULT([WINDOWSSDKDIR is missing.])
> >+fi
> 
> Who is using MOZ_HAS_WINSDK_WITH_D3D after the patch?

The Gamepad API section below this one.
Comment on attachment 8410582 [details] [diff] [review]
dual-d3dc

>-MOZ_D3DCOMPILER_DLL=

This still has some references in the tree:

./browser/installer/Makefile.in:DEFINES += -DMOZ_D3DCOMPILER_DLL=$(MOZ_D3DCOMPILER_DLL)
./browser/installer/package-manifest.in:@BINPATH@/@MOZ_D3DCOMPILER_DLL@

>-MOZ_D3DCOMPILER_CAB=

This too:

./gfx/angle/angle-long-ident-hash.patch: ifdef MOZ_D3DCOMPILER_CAB

Any idea if the ValueError in the try log is related to one of these?

The rest looks good to me, but you'll need someone else to review the GLLibraryEGL.cpp changes.
Attachment #8410582 - Flags: review?(mshal) → feedback+
Attached patch dual-d3dc (obsolete) — Splinter Review
Attachment #8410582 - Attachment is obsolete: true
Attachment #8413952 - Flags: review?(bjacob)
With this change, the test slaves should pick up both versions automatically, but we can enforce this with "ac_add_options --enable-require-all-d3dc-versions". I don't know where to put this, though. :mshal?
Flags: needinfo?(mshal)
Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

I don't know if Ted is the right person, but I touch some of the Gamepad API configure.in section here, so let's get sign-off on that change, too.
Attachment #8413952 - Flags: review?(ted)
Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

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

Meh, I can't see a problem, so r+, but I also don't recognize this code: it seems to have received patches since I last looked at it, and I didn't review these patches, so now I don't feel 100% competent to do this review. Glad that Ted is also flagged.
Attachment #8413952 - Flags: review?(bjacob) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> With this change, the test slaves should pick up both versions
> automatically, but we can enforce this with "ac_add_options
> --enable-require-all-d3dc-versions". I don't know where to put this, though.
> :mshal?

I'm not sure exactly what you're asking for here. Why would a test slave care about a configure flag?
Flags: needinfo?(mshal)
(In reply to Michael Shal [:mshal] from comment #17)
> (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > With this change, the test slaves should pick up both versions
> > automatically, but we can enforce this with "ac_add_options
> > --enable-require-all-d3dc-versions". I don't know where to put this, though.
> > :mshal?
> 
> I'm not sure exactly what you're asking for here. Why would a test slave
> care about a configure flag?
Sorry, I meant build slaves. We want to have our build slaves build with this flag, so they fail to build if they don't have both DLLs we want them to have.
Flags: needinfo?(mshal)
Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

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

::: build/mozconfig.common
@@ +15,5 @@
>  ac_add_options --enable-crashreporter
>  
>  ac_add_options --enable-release
> +
> +ac_add_options --enable-require-all-d3dc-versions

I don't think you want this here, this mozconfig is for all platforms. You probably want:
http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/win32/common-opt
(and if you want it in debug:
http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/win32/debug )
Attachment #8413952 - Flags: review?(ted) → review+
I think #c19 addresses the question - if not let me know!
Flags: needinfo?(mshal)
(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> The Gamepad API section below this one.

Not needed anymore due to bug 996078. (Also, it should have checked dinput.h rather than d3d10.h.)
Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

MOZ_HAS_WINSDK_WITH_D3D is never AC_SUBST'd with this patch, even though it's referenced in gfx/angle/src/libEGL/Makefile.in and libGLESv2.

We actually *want* to take that path as much as possible, because including the DirectX SDK's include files causes conflicts and a bunch of warnings due to duplicate definitions in the Windows SDK.  Basically, the only time we want to use the DirectX SDK's includes is if you're building with VS2010 and the Windows 7 SDK.  If someone is using the Windows 8/8.1 SDKs, we shouldn't be touching the DX SDK at all, except perhaps to pull out d3dcompiler_43.dll.

This can be fixed in a followup patch though, but it should be done soon.
Attachment #8413952 - Flags: feedback-
As a side question: remind me why we don't just put the DXSDK DLLs that we need in MozillaBuild?
Putting them in MozillaBuild is not a bad idea. I don't think we went down that route.

However, after this patch, developers should never need the DXSDK any more, unless they're using VS2010/Win7 SDK and want ANGLE.  Only the build machines will need to have somewhere to get d3dcompiler_43.dll from.
Attached patch dual-d3dc (obsolete) — Splinter Review
Alright, one more time. Green on try, and build slaves are pulling both DLLs. (verified both by checking the build log, where we claim to find both, and the built ZIP, which contains both _43 and _46.

At some point, we should try to move this to _47.
Attachment #8413952 - Attachment is obsolete: true
Attachment #8416701 - Flags: review?(mshal)
Comment on attachment 8416701 [details] [diff] [review]
dual-d3dc

>-MOZ_DIRECTX_SDK_CPU_SUFFIX=

1) How come this doesn't break this flag in libxul.mk?

EXTRA_DSO_LDOPTS += \
  -LIBPATH:'$(MOZ_DIRECTX_SDK_PATH)/lib/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)' \
  $(NULL)

2) And I'm not sure if you saw this from my earlier comment, or if we just don't care to update the patch:

./gfx/angle/angle-long-ident-hash.patch: ifdef MOZ_D3DCOMPILER_CAB
Attachment #8416701 - Flags: review?(mshal) → feedback+
(In reply to Michael Shal [:mshal] from comment #27)
> Comment on attachment 8416701 [details] [diff] [review]
> dual-d3dc
> 
> >-MOZ_DIRECTX_SDK_CPU_SUFFIX=
> 
> 1) How come this doesn't break this flag in libxul.mk?
> 
> EXTRA_DSO_LDOPTS += \
>   -LIBPATH:'$(MOZ_DIRECTX_SDK_PATH)/lib/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)' \
>   $(NULL)
It probably does break that, but I think this line is redundant with at least what we do in gfx/angle.
> 
> 2) And I'm not sure if you saw this from my earlier comment, or if we just
> don't care to update the patch:
> 
> ./gfx/angle/angle-long-ident-hash.patch: ifdef MOZ_D3DCOMPILER_CAB
I'll try fixing this, but I'm not going to guarantee the patch survives this migration without scratches.
Attached patch dual-d3dcSplinter Review
Fixed the libxul.mk instance, and decided to ignore the angle patch. It'll just mean it doesn't apply cleanly, but that's fine. (It probably doesn't already. We don't keep them up-to-date)
Attachment #8416701 - Attachment is obsolete: true
Attachment #8416726 - Flags: review?(mshal)
Comment on attachment 8416726 [details] [diff] [review]
dual-d3dc

Sounds fine - thanks!
Attachment #8416726 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/8b48386fc226
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1005456
Depends on: 1005484
You need to log in before you can comment on or make changes to this bug.