Update ANGLE to latest HEAD

RESOLVED FIXED in mozilla21

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: jgilbert)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-next)

Attachments

(15 attachments, 4 obsolete attachments)

3.02 MB, patch
Details | Diff | Splinter Review
3.81 MB, patch
Details | Diff | Splinter Review
1.90 KB, patch
Details | Diff | Splinter Review
780 bytes, patch
Details | Diff | Splinter Review
1.69 KB, patch
Details | Diff | Splinter Review
5.40 KB, patch
Details | Diff | Splinter Review
6.63 KB, patch
Details | Diff | Splinter Review
601 bytes, patch
Details | Diff | Splinter Review
1.88 KB, patch
Details | Diff | Splinter Review
16.97 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
22.74 KB, patch
Details | Diff | Splinter Review
25.16 KB, patch
Details | Diff | Splinter Review
12.36 KB, patch
Details | Diff | Splinter Review
13.82 KB, patch
Details | Diff | Splinter Review
3.80 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
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.
(Reporter)

Comment 1

5 years ago
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
Summary: Update ANGLE to something recent → Update ANGLE to r1314
(Reporter)

Updated

5 years ago
Summary: Update ANGLE to r1314 → Update ANGLE to r1319
(Reporter)

Comment 2

5 years ago
Updated gfx/angle/README.mozilla with detailed instructions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69ec59e6d61
(Reporter)

Updated

5 years ago
Whiteboard: [leave open]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open] → [leave open] webgl-next
(Reporter)

Updated

5 years ago
Summary: Update ANGLE to r1319 → Update ANGLE to something recent
(Reporter)

Updated

5 years ago
Blocks: 808785
(Reporter)

Updated

5 years ago
Blocks: 816670
(Reporter)

Comment 5

5 years ago
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.
Summary: Update ANGLE to something recent → Update ANGLE to r1394

Comment 6

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

Comment 7

5 years ago
Thanks for the clarification!
Summary: Update ANGLE to r1394 → Update ANGLE to latest HEAD
(Reporter)

Updated

5 years ago
Blocks: 816676
(Assignee)

Comment 8

5 years ago
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.
Assignee: nobody → jgilbert
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
Created attachment 692135 [details] [diff] [review]
patch 0: Nuke gfx/angle.

This should be a straight deletion.
Attachment #692135 - Flags: review?(bjacob)
(Assignee)

Comment 11

5 years ago
Created attachment 692138 [details] [diff] [review]
patch 1: ANGLE r1559

This is ANGLE r1559, with SVN cruft stripped out, and our handful of files included. (Makefiles and such)
Attachment #692138 - Flags: review?(bjacob)
in today's concall it was mentioned that we absolutely want r1560. sorry :-)
(Assignee)

Comment 13

5 years ago
Created attachment 692139 [details] [diff] [review]
patch 2: update from ANGLE r1559 to r1561

More rubber-stamping.
Attachment #692139 - Flags: review?(bjacob)
(Assignee)

Comment 14

5 years ago
Created attachment 692141 [details] [diff] [review]
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.
Attachment #692141 - Flags: review?(bjacob)
(Assignee)

Comment 15

5 years ago
Created attachment 692143 [details] [diff] [review]
patch 4: angle build prereq 1: stdcall alias

Carrying forward r+ for this previously applied patch.
Attachment #692143 - Flags: review+
(Assignee)

Comment 16

5 years ago
Created attachment 692144 [details] [diff] [review]
patch 5: angle build prereq 1: stdcall alias (patch file)

The patch file and README entry for the previous patch.
Attachment #692144 - Flags: review+
(Assignee)

Comment 17

5 years ago
Created attachment 692146 [details] [diff] [review]
patch 5(2): angle build prereq 1: stdcall alias (patch file)

Oops, this was bitrotted.
Attachment #692144 - Attachment is obsolete: true
Attachment #692146 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 692147 [details] [diff] [review]
patch 6: angle build prereq 2: rename debug.{cpp,h}
Attachment #692147 - Flags: review+
(Assignee)

Comment 19

5 years ago
Created attachment 692148 [details] [diff] [review]
patch 7: angle build prereq 2: rename debug (patch file)
Attachment #692148 - Flags: review+
(Assignee)

Comment 20

5 years ago
Created attachment 692149 [details] [diff] [review]
patch 8: angle build prereq 3: rename preprocessor files
Attachment #692149 - Flags: review+
(Assignee)

Comment 21

5 years ago
Created attachment 692150 [details] [diff] [review]
patch 9: angle build prereq 3: rename preproc files (patch file)
Attachment #692150 - Flags: review+
(Assignee)

Comment 22

5 years ago
Created attachment 692151 [details] [diff] [review]
patch 10: Fix everything so it builds!

Back to things which need review!
Attachment #692151 - Flags: review?(bjacob)

Comment 23

5 years ago
(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...
(Assignee)

Comment 24

5 years ago
Created attachment 692153 [details] [diff] [review]
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.
Attachment #692153 - Flags: review?(bjacob)
(Assignee)

Comment 25

5 years ago
Created attachment 692154 [details] [diff] [review]
patch 12: include the previous patch into angle patches

Rubber stamp adding the previous patch to the angle local patch list.
Attachment #692154 - Flags: review?(bjacob)
(Assignee)

Comment 26

5 years ago
Created attachment 692155 [details] [diff] [review]
patch 13: spooky-hash for long-ident hashing

Back to previously-reviewed patches.
Attachment #692155 - Flags: review+
(Assignee)

Comment 27

5 years ago
Created attachment 692156 [details] [diff] [review]
patch 14: add spooky-hash patch to local patches
Attachment #692156 - Flags: review+
(Assignee)

Comment 28

5 years ago
Created attachment 692157 [details] [diff] [review]
patch 15: add faceforward emulation
Attachment #692157 - Flags: review+
(Assignee)

Comment 29

5 years ago
Created attachment 692158 [details] [diff] [review]
patch 16: add faceforward-emu to the local patch list
Attachment #692158 - Flags: review+
(Assignee)

Comment 30

5 years ago
Alright, that's everything. Try run was clean.
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.
Attachment #692135 - Flags: review?(bjacob)
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)
Attachment #692138 - Flags: review?(bjacob)
Comment on attachment 692139 [details] [diff] [review]
patch 2: update from ANGLE r1559 to r1561

No need to review upstream ANGLE code.
Attachment #692139 - Flags: review?(bjacob)
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.
Attachment #692141 - Flags: review?(bjacob) → review-
Comment on attachment 692143 [details] [diff] [review]
patch 4: angle build prereq 1: stdcall alias

Let's keep unreviewed stuff free of any "review+".
Attachment #692143 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692146 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692147 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692148 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692149 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692150 - Flags: review+
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?
Attachment #692151 - Flags: review?(bjacob) → review+
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.
Attachment #692153 - Flags: review?(bjacob)
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.
Attachment #692154 - Flags: review?(bjacob) → review-
(Reporter)

Updated

5 years ago
Attachment #692155 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692156 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692157 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #692158 - Flags: review+
In conclusion: please update patch 3, answer my questions on patch 10, and remove patch 12.
(Assignee)

Comment 40

5 years ago
(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.
(Assignee)

Comment 41

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #692154 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #692153 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 700695 [details] [diff] [review]
patch 3: Clean and update README.mozilla

How's this look?
Attachment #692141 - Attachment is obsolete: true
Attachment #700695 - Flags: review?(bjacob)
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
Attachment #700695 - Flags: review?(bjacob) → review+
(Assignee)

Comment 44

5 years ago
(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)".
(Reporter)

Updated

5 years ago
Blocks: 830386
(Reporter)

Updated

5 years ago
Blocks: 827106
Fixed unexpected-pass on WinXP (yay, this fixes texture-mips!)

https://hg.mozilla.org/integration/mozilla-inbound/rev/5421b6685f3f
(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).
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.

Updated

5 years ago
Depends on: 834845
(Reporter)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open] webgl-next → webgl-next

Updated

5 years ago
Target Milestone: --- → mozilla21
No longer depends on: 834845
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 :-(
(Assignee)

Updated

5 years ago
Depends on: 883478
You need to log in before you can comment on or make changes to this bug.