801158 - Update ANGLE to latest HEAD

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

We want at least r1278 as it has a fix for ANGLE issue r350, which is a crash and is blocking investigation in bug 793126.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

r1314 will allow us to stop shipping copies of d3dx9_43.dll. That's a 2 MB shrink of the Firefox on-disk size on Windows. Cheers!
http://code.google.com/p/angleproject/issues/detail?id=311

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Updated gfx/angle/README.mozilla with detailed instructions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69ec59e6d61

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Detailed instructions in this file:

http://hg.mozilla.org/integration/mozilla-inbound/file/a69ec59e6d61/gfx/angle/README.mozilla

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a69ec59e6d61

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Looks like the DX11 work just landed in ANGLE! That's fantastic, but for the purpose of the present update, we'll probably want to stay on the revision just before it, for now --- until ANGLE developers tell us it's ready. So for now, let's target ANGLE r1394.

Comment 6

[daniel-bzmz]

The ANGLE DX11 work is not complete and is on a separate branch (dx11proto).  Please use HEAD on SVN to pick up.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the clarification!

Comment 8

[Kelsey Gilbert [:jgilbert]]

I'm most of the way there, but I won't be done today. I'm just pulling us to HEAD, which was r1559 when I pulled.

Comment 9

[Kelsey Gilbert [:jgilbert]]

First try run was clean:
https://tbpl.mozilla.org/?tree=Try&rev=5e37998ffe18

The next one looks to be coming back clean as well:
https://tbpl.mozilla.org/?tree=Try&rev=4950780408c4

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Nuke gfx/angle.

This should be a straight deletion.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: ANGLE r1559

This is ANGLE r1559, with SVN cruft stripped out, and our handful of files included. (Makefiles and such)

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

in today's concall it was mentioned that we absolutely want r1560. sorry :-)

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: update from ANGLE r1559 to r1561

More rubber-stamping.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Clean and update README.mozilla

This one actually needs review. We may want to further revise the language on how to do ANGLE updates. We also should maybe consider putting this on the wiki instead, since this isn't in code anyways, and so the rule-of-thumb about in-code documentation doesn't really apply.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: angle build prereq 1: stdcall alias

Carrying forward r+ for this previously applied patch.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: angle build prereq 1: stdcall alias (patch file)

The patch file and README entry for the previous patch.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5(2): angle build prereq 1: stdcall alias (patch file)

Oops, this was bitrotted.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 6: angle build prereq 2: rename debug.{cpp,h}

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 7: angle build prereq 2: rename debug (patch file)

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 8: angle build prereq 3: rename preprocessor files

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 9: angle build prereq 3: rename preproc files (patch file)

Comment 22

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 10: Fix everything so it builds!

Back to things which need review!

Comment 23

[daniel-bzmz]

(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> Created attachment 692148 [details] [diff] [review]
> patch 7: angle build prereq 2: rename debug (patch file)

BTW, I'd consider accepting patches like these in master so you don't have to redo it every time...

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 11: Prune unused dirs: samples/, tests/, third_party/

This should contain no additions.
We don't need to have samples and tests (that we don't run) mixed in here.

I could also put this step back with when we import the ANGLE rev we want, as part of the stripping process where we remove SVN files and similar.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 12: include the previous patch into angle patches

Rubber stamp adding the previous patch to the angle local patch list.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 13: spooky-hash for long-ident hashing

Back to previously-reviewed patches.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 14: add spooky-hash patch to local patches

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 15: add faceforward emulation

Comment 29

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 16: add faceforward-emu to the local patch list

Comment 30

[Kelsey Gilbert [:jgilbert]]

Alright, that's everything. Try run was clean.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692135 [details] [diff] [review]
patch 0: Nuke gfx/angle.

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

Eww. There's no way that I can meaningfully look at this. Also, this will absolutely want to be folded with the ANGLE re-import for landing.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692138 [details] [diff] [review]
patch 1: ANGLE r1559

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

Please take this occasion to remove any large unnecessary directory from our ANGLE copy, such as samples/, into which this updates adds 3 new images.

Will take this without review. (upstream code)

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692139 [details] [diff] [review]
patch 2: update from ANGLE r1559 to r1561

No need to review upstream ANGLE code.

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692141 [details] [diff] [review]
patch 3: Clean and update README.mozilla

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

Sorry, this looks a bit sketchy at this point. Need another round.

::: gfx/angle/README.mozilla
@@ +11,5 @@
>  upsteam ANGLE. Therefore, changes made to the Makefile.in files should not be stored
>  in the local .patch files.
>  
> +
> +== How to update this ANGLE copy to HEAD ==

Instead of "update to HEAD" vs "update from rev A to rev B" I think this is rather "how to do a clean-slate update" vs "how to do an incremental update"

@@ +22,5 @@
> +
> +2.  $ rm -rf gfx/angle
> +
> +3.  Copy the folder containing the angle rev you want onto gfx/angle.
> +3a. Delete all any leftover .svn folders.

You should tell people to use 'svn export' for this rather than manually deleting .svn files.

@@ +29,5 @@
> +
> +5.  Clear out the "Applied Local Patches" section above, since we're going to
> +    repopulate it.
> +
> +6.  Re-apply the angle-build-*.patch files and record them above.

it is not clear to me what we get from singling out certain patches as 'build' patches.

@@ +31,5 @@
> +    repopulate it.
> +
> +6.  Re-apply the angle-build-*.patch files and record them above.
> +
> +7.  Update the Makefile.in files with the current deps from the .gpy files.

s/gpy/gyp/  and I also think that you want to tell people to look specifically at the diff on the gyp files between the old and new ANGLE revision. And also, you can be more specific than "the .gyp files", I think the only one is src/build_angle.gyp.

@@ +35,5 @@
> +7.  Update the Makefile.in files with the current deps from the .gpy files.
> +
> +9.  Build. Fix things until it builds.
> +
> +8.  Apply angle-build-prune.patch. Remove cruft like angle/samples/.

OK, good.

At this point I don't know what angle-build-prune.patch is?

@@ +50,5 @@
> +In general:
> +$ patch -p1 -R < gfx/angle/angle-some-bug-fix.patch
> +
> +SVN diffs however can be iffy. They don't seem to be completely compatible
> +with `patch`. Be aware that you may get some weird rejects.

They are very compatible with 'patch', you just have to cd gfx/angle/ before you apply them and pass the right -pN. Also, you can try with --dry-run so you don't end up with weird rejects from experimenting.

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692143 [details] [diff] [review]
patch 4: angle build prereq 1: stdcall alias

Let's keep unreviewed stuff free of any "review+".

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692151 [details] [diff] [review]
patch 10: Fix everything so it builds!

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

r=me but a significant concern:

::: gfx/angle/Makefile.in
@@ +25,4 @@
>  
> +# The below is a rough translation of build_angle.gypi:
> +DEFINES += -DANGLE_DISABLE_TRACE
> +#DEFINES += -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL0

This this intentionally commented out? We used to have it in the previous version. Also, what about ANGLE_USE_NEW_PREPROCESSOR?

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692153 [details] [diff] [review]
patch 11: Prune unused dirs: samples/, tests/, third_party/

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

Hard to review this. If this is plainly removing dirs which we don't use, go for it.

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 692154 [details] [diff] [review]
patch 12: include the previous patch into angle patches

Let's not add a patch file for that. Instead, keep the description in the Makefile of what you are removing.

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

In conclusion: please update patch 3, answer my questions on patch 10, and remove patch 12.

Comment 40

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #34)
> Comment on attachment 692141 [details] [diff] [review]
> patch 3: Clean and update README.mozilla
> 
> Review of attachment 692141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, this looks a bit sketchy at this point. Need another round.
> 
> ::: gfx/angle/README.mozilla
> @@ +11,5 @@
> >  upsteam ANGLE. Therefore, changes made to the Makefile.in files should not be stored
> >  in the local .patch files.
> >  
> > +
> > +== How to update this ANGLE copy to HEAD ==
> 
> Instead of "update to HEAD" vs "update from rev A to rev B" I think this is
> rather "how to do a clean-slate update" vs "how to do an incremental update"
Alright.
> 
> @@ +22,5 @@
> > +
> > +2.  $ rm -rf gfx/angle
> > +
> > +3.  Copy the folder containing the angle rev you want onto gfx/angle.
> > +3a. Delete all any leftover .svn folders.
> 
> You should tell people to use 'svn export' for this rather than manually
> deleting .svn files.
Done. Didn't know about that.
> 
> @@ +29,5 @@
> > +
> > +5.  Clear out the "Applied Local Patches" section above, since we're going to
> > +    repopulate it.
> > +
> > +6.  Re-apply the angle-build-*.patch files and record them above.
> 
> it is not clear to me what we get from singling out certain patches as
> 'build' patches.
This way you can do a test build to make sure nothing is messed up before applying all the patches.
> 
> @@ +31,5 @@
> > +    repopulate it.
> > +
> > +6.  Re-apply the angle-build-*.patch files and record them above.
> > +
> > +7.  Update the Makefile.in files with the current deps from the .gpy files.
> 
> s/gpy/gyp/  and I also think that you want to tell people to look
> specifically at the diff on the gyp files between the old and new ANGLE
> revision. And also, you can be more specific than "the .gyp files", I think
> the only one is src/build_angle.gyp.
I didn't look at the diff myself. I just translated from gyp to make. For the incremental update, looking at the diff is more important though.
> 
> @@ +35,5 @@
> > +7.  Update the Makefile.in files with the current deps from the .gpy files.
> > +
> > +9.  Build. Fix things until it builds.
> > +
> > +8.  Apply angle-build-prune.patch. Remove cruft like angle/samples/.
> 
> OK, good.
> 
> At this point I don't know what angle-build-prune.patch is?
This is part of the initial import now, so ignore this.
> 
> @@ +50,5 @@
> > +In general:
> > +$ patch -p1 -R < gfx/angle/angle-some-bug-fix.patch
> > +
> > +SVN diffs however can be iffy. They don't seem to be completely compatible
> > +with `patch`. Be aware that you may get some weird rejects.
> 
> They are very compatible with 'patch', you just have to cd gfx/angle/ before
> you apply them and pass the right -pN. Also, you can try with --dry-run so
> you don't end up with weird rejects from experimenting.
I know I was getting rejections whenever it deleted a file, and possibly for renames. Since all of the old preprocessor is gone now, that was a lot of rejects to sort though.

Comment 41

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #36)
> Comment on attachment 692151 [details] [diff] [review]
> patch 10: Fix everything so it builds!
> 
> Review of attachment 692151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but a significant concern:
> 
> ::: gfx/angle/Makefile.in
> @@ +25,4 @@
> >  
> > +# The below is a rough translation of build_angle.gypi:
> > +DEFINES += -DANGLE_DISABLE_TRACE
> > +#DEFINES += -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL0
> 
> This this intentionally commented out? We used to have it in the previous
> version. Also, what about ANGLE_USE_NEW_PREPROCESSOR?

This was absent in r1560, so I removed it. Apparently it was re-added in r1562, though:
http://code.google.com/p/angleproject/source/detail?r=1562

I'll uncomment it accordingly.

Comment 42

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Clean and update README.mozilla

How's this look?

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 700695 [details] [diff] [review]
patch 3: Clean and update README.mozilla

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

Good! one typo:

::: gfx/angle/README.mozilla
@@ +32,5 @@
> +    repopulate it.
> +
> +6.  Re-apply the angle-build-*.patch files and record them above.
> +
> +7.  Update the Makefile.in files with the current deps from the .gpy files.

.gyp

Comment 44

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #43)
> Comment on attachment 700695 [details] [diff] [review]
> patch 3: Clean and update README.mozilla
> 
> Review of attachment 700695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good! one typo:
> 
> ::: gfx/angle/README.mozilla
> @@ +32,5 @@
> > +    repopulate it.
> > +
> > +6.  Re-apply the angle-build-*.patch files and record them above.
> > +
> > +7.  Update the Makefile.in files with the current deps from the .gpy files.
> 
> .gyp

Changed to ".gyp(i)".

Comment 45

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a941576624be

Comment 46

[Benoit Jacob [:bjacob] (mostly away)]

Fixed unexpected-pass on WinXP (yay, this fixes texture-mips!)

https://hg.mozilla.org/integration/mozilla-inbound/rev/5421b6685f3f

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a941576624be
https://hg.mozilla.org/mozilla-central/rev/5421b6685f3f

Comment 48

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ryan VanderMeulen from comment #47)
> https://hg.mozilla.org/mozilla-central/rev/a941576624be

   5.243 +# We have to filter out -pedantic, because of
   5.244 +# comma-at-end-of-enumerator list failures.  We can try to get this fixed
   5.245 +# upstream at some point.
   5.246 +CXXFLAGS := $(filter-out -pedantic,$(CXXFLAGS))
   5.247 +CFLAGS   := $(filter-out -pedantic,$(CFLAGS))

is not actually true; unfortunately, we stopped using -pedantic (bug 394311).

Comment 49

[Benoit Jacob [:bjacob] (mostly away)]

Good, I'd r+ a patch taking out this stuff. If it were needed again in the future, the better approach would be to patch ANGLE.

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

Argh... what landed is r1561 from December 6 --- the update took so long, it's old upon arrival. Not recent enough to have array index clamping (r1638 from Dec. 21).

Need another ANGLE update now :-(