1010371 - Update ANGLE, 2014-06

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We should upgrade ANGLE to something recent.  Our source should come from http://github.com/mozilla/angle 's mozilla branch, which is a clone of the angle repo with mozilla patches applied as commits.  There's also a gyp-to-mozbuild generator there, see README.mozilla for more details.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixes needed for new ANGLE

Fixes needed for ANGLE shader translator API changes.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Full angle update (.gz)

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note that this also needs the patch from bug 1009965.

Try run is at https://tbpl.mozilla.org/?tree=Try&rev=35b962e29110 -- results are that things pass on Windows.  On Linux and 10.6, we have some issues in glsl-long-variable-names.html and uniform-location-length-limits.html, but I can't reproduce those with native-gl on Windows.  On 10.8, we have a crash.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: put back in identifier hashing

New ANGLE includes a generic mechanism for specifying a hash function for long identifier hashing, but it doesn't use one by default.  It includes MurmurHash even, but they use it only within the d3d11 backend for some specific stuff.

This just adds murmurhash to webgl and specifies the hash function in our code, so we don't have to modify ANGLE.  I thought about having a way to hash it directly with ANGLE, but there's really no easy way short of exposing a hash function from the API.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Try run with all above patches: https://tbpl.mozilla.org/?tree=Try&rev=9a24827f684e

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What blocks moving forward here?  There's a cset from March that fixes a threadsafety bug we're hitting in webgl on workers.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nothing -- time.  I had to fix up a few things, but I'm hoping to get this up for review again tomorrow (all the issues here have been worked around).  Latest run is at https://tbpl.mozilla.org/?tree=Try&rev=41ddf8c186a8 .  I just need to figure out what's up on XP, and then we're good to go.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

XP failures now:

https://tbpl.mozilla.org/?tree=Try&rev=274ced4fef28

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The OSX crashes are due to bug 771406; I dropped that patch by accident.  I'll work to get it upstream as well.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

New try run going:

https://tbpl.mozilla.org/?tree=Try&rev=5ad8fc25d1e6

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixes for new ANGLE

Flagging jrmuizel for review on this, to try to land it today (holiday).

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fixes for new ANGLE (same patch, diff -w)

There are some whitespace changes -- all of CompileShader was indented by an extra amount because it was all wrapped inside an if().  I nuked that.

Comment 13

[Robert Longson [:longsonr]]

This seems to have brought bug 914547 back. I'm getting fatal error C1083: Cannot open include file: 'd3dcompiler.h' on my build. I do have d3dcompiler.h, the build just can't find it.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/72a70862ee28
https://hg.mozilla.org/mozilla-central/rev/401bc7471c24
https://hg.mozilla.org/mozilla-central/rev/3f60457cbb7b

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Robert Longson from comment #13)
> This seems to have brought bug 914547 back. I'm getting fatal error C1083:
> Cannot open include file: 'd3dcompiler.h' on my build. I do have
> d3dcompiler.h, the build just can't find it.

I'll add the -I for the dxsdk to the generated moz.build for the next update (should be very soon, it's a much smaller update)... though arguably configure should be adding it to the include path if a separate DX SDK is being used.  The expectation is that, ideally, you'd be building with the Win 7/8 SDK that has d3d headers as part of the SDK.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8450979 [details] [diff] [review]
fixes for new ANGLE

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +138,5 @@
>      if (!ValidateAttribIndex(location, "bindAttribLocation"))
>          return;
>  
> +    if (StringBeginsWith(name, NS_LITERAL_STRING("gl_")))
> +        return ErrorInvalidOperation("bindAttribLocation: can't set the location of a name that starts with 'gl_'");

Is this an existing bug? We also want to prevent "webgl_", I believe.

@@ +3001,5 @@
> +    ShBuiltInResources resources;
> +
> +    memset(&resources, 0, sizeof(ShBuiltInResources));
> +
> +    ShInitBuiltInResources(&resources);

Try to keep the initialization steps together.

ShHandle...

ShBuiltInRes foo;
memset()
ShInit(foo)

//etc.

::: content/canvas/src/WebGLProgram.cpp
@@ +239,5 @@
>      return info;
>  }
>  
> +/* static */ uint64_t
> +WebGLProgram::IdentifierHashFunction(const char *ident, size_t size)

Star to left.

@@ +243,5 @@
> +WebGLProgram::IdentifierHashFunction(const char *ident, size_t size)
> +{
> +    uint64_t outhash[2];
> +    // NB: we use the x86 function everywhere, even though it's suboptimal perf
> +    // on x64.  They return different results; not sure if that's a requirement.

FWIW, it's not a requirement. If we see different behavior, it would only be because of hash collisions, which are 'impossible'.

@@ +253,5 @@
> +WebGLProgram::HashMapIdentifier(const nsACString& name, nsCString *hashedName)
> +{
> +    uint64_t hash = IdentifierHashFunction(name.BeginReading(), name.Length());
> +    hashedName->Truncate();
> +    // This MUST MATCH angle/src/compiler/translator/HashNames.h HASHED_NAME_PREFIX

Gross. Can we not pull this from that source?

::: content/canvas/src/WebGLProgram.h
@@ +114,5 @@
>      // public post-link data
>      std::map<GLint, nsCString> mActiveAttribMap;
>  
> +    static uint64_t IdentifierHashFunction(const char *ident, size_t size);
> +    static void HashMapIdentifier(const nsACString& name, nsCString *hashedName);

Stars to left.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Whoops, my comment got lost -- we should do another mini angle update using the new git repo and stuff shortly; ideally you or Dan would do it to make sure there aren't any issues in the process.  We can fix up the above issues as part of that.

(the HASHED_NAME_PREFIX, I don't think we can pull from source unless we explicitly #include that, but there might be a better way to get it)

Comment 18

[Robert Longson [:longsonr]]

ac_add_options --disable-webgl

doesn't seem to work any more either post this update.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, let's just kill that.

Comment 20

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #19)
> Yeah, let's just kill that.

I think we have a bug for that. Really, we should never have offered --disable-webgl, but instead have a --disable-angle.

Comment 21

[Bob Clary [:bc] (inactive)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #15)

> 
> I'll add the -I for the dxsdk to the generated moz.build for the next update
> (should be very soon, it's a much smaller update)... though arguably
> configure should be adding it to the include path if a separate DX SDK is
> being used.  The expectation is that, ideally, you'd be building with the
> Win 7/8 SDK that has d3d headers as part of the SDK.

On Win7 with VS 2010, mozilla-build gives me the VS 2010 SDK without the DirectX SDK and not the installed Win 8/8.1 SDK. How soon is 'very soon'?

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Very soon is days, early next week.  But I don't know why you're getting the 2010 SDK instead of the 8/8.1 SDK; do you have the latest mozilla-build installed?

Comment 23

[Robert Longson [:longsonr]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #22)
> Very soon is days, early next week.  But I don't know why you're getting the
> 2010 SDK instead of the 8/8.1 SDK; do you have the latest mozilla-build
> installed?

I know you didn't ask me but I'm in the same situation and yes I have the latest mozilla-build installed.

Comment 24

[Bob Clary [:bc] (inactive)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #22)
> Very soon is days, early next week.  But I don't know why you're getting the
> 2010 SDK instead of the 8/8.1 SDK; do you have the latest mozilla-build
> installed?

Yes. I also removed the mozilla-build directory completely and reinstalled and still see the same issue. --disable-webgl doesn't work around the issue either.

Vlad, can you post a simple patch that provides the change to moz.build so I can keep crash testing 32bit builds?

Comment 25

[Bob Clary [:bc] (inactive)]

fwiw, I tried to compile by defining

export CXXFLAGS=-I/c/PROGRA~1/MI5E29~1/Include

to include the C:\Program Files\Microsoft DirectX SDK (2010)/Include but ran into an issue with being unable to include DXGI1_2.h which appears to be part of the Windows 8 SDK. This appears to be due to http://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libGLESv2/precompiled.h#46 Other occurences of the header are protected by ifdef MOZ_METRO.

Ted, is there a known issue with the latest mozilla-build for VS 2010 using the VS 2010 SDK and not picking up the Windows 8 SDK?

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm, ok, so looks like there's a couple of things that need fixing for that config.  One, we would need to disable building the d3d11 backend in ANGLE (we're not using it yet, but we want to soon if it's available).  Two, we need to add the old DX SDK -I to the includes path.

For a quick fix, try:

in gfx/angle/moz.build:
  - remove the ANGLE_ENABLE_D3D11 define.
in gfx/angle/src/libGLESv2/moz.build: 
  - remove the ANGLE_ENABLE_D3D11 define
  - delete all entries in SOURCES that have renderer/d3d11 in them

I can't test this right now since I'm on the road and don't have a 2010 setup available.  But instead of making the above fixes permanent, I'd rather we fix mozilla-build and figure out why the Win7/Win8 SDK isn't being picked up -- it's required anyway, and that way we can drop needing to install the old DirectX SDK (which has installation issues on modern windows even!).

Comment 27

[(not currently active) Ted Mielczarek]

We *ought* to pick up the Win8 SDK when you're using VS2010, since it's newer. I don't know about the specifics though, RyanVM has taken over ownership of MozillaBuild, so I'm punting your needinfo to him.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Is comment 26 just bug 1014633?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I chatted with bc on IRC a bit. He's going to try out the updated start-msvc2010* batch files from the repo and will file a new bug if he can still reproduce.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

OK, we've confirmed that bc's problem is bug 990983. There's a patch in the bug if anybody wants to try it out.

Comment 31

[:Paolo Amadini]

I'm building on Windows 7 x64 with MozillaBuild 1.9.0 and incurred in the "Cannot open include file: 'd3dcompiler.h'" bug.

I'm using Visual C++ 2010 Express and I only installed the Windows 7 SDK because this is what is required by the Express edition according to the build prerequisites page:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites#Install_build_prerequisites

Neither the workaround in comment 26 nor the --disable-webgl build configuration switch worked for me.

I wonder if the promised fix from comment 22 will fix this configuration. If you know of any workaround I could try in the meantime, or you need help with testing a new patch later, please let me know!

I reverted to the older revision 0c408ede302a but I get other build errors there.

I'm considering updating my environment to Visual C++ 2012 Express, but I'm wary of doing that since the official mozilla-central builds are compiled with Visual C++ 2010.

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

filed bug 1037667 for the outstanding build/dxsdk issues

Comment 33

[Kestrel]

Switching to the Win 8.1 SDK fixed it for me, it is also a simpler setup as it comes with DX SDK. The build instructions page should probably recommend the 8.1 SDK over the 7 SDK.

Comment 34

[Robert Longson [:longsonr]]

The 8.1 SDK won't install on Vista though.

Comment 35

[Mike Hommey [:glandium]]

Is the "Please note this file is autogenerated from generate_mozbuild.py, so do not modify it directly" comment this landing added true? Because if it is, then bug 1037667 happily ignored it.

Comment 36

[Mike Hommey [:glandium]]

Okay, I see generate_mozbuild.py was updated in https://github.com/mozilla/angle. Why is that file not in mozilla-central as well, like for skia?

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Mike Hommey [:glandium] from comment #35)
> Is the "Please note this file is autogenerated from generate_mozbuild.py, so
> do not modify it directly" comment this landing added true? Because if it
> is, then bug 1037667 happily ignored it.

I thought so too, but what just happened in that bug is that a newer set of generated moz.build files got uplifed to beta.  They were still generated by the generator.

The generator isn't in mozilla-central because to update angle we need the patches we have applied on top of angle , all the original gyp machinery, and generate_mozbuild.py (and another thing that I forget).  Putting it in m-c would have meant that anyone doing the update would need to transplant those things from m-c to a angle repo, run the commands, and copy the results back.  Or they'd live in both places, increasing the chances of them getting out of sync.  Instead I chose to just keep everything in the mozilla/angle github repo.  We can certainly check in the bits as part of copying stuff over though, but it won't have the patches because maintaining in-tree .patch files is a giant pain (though I guess those can be recreated via a diff).