Closed Bug 861039 Opened 7 years ago Closed 6 years ago

Update ANGLE to r2042

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox23 - wontfix
firefox24 + fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(10 files, 1 obsolete file)

393.48 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
13.59 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
955 bytes, patch
bjacob
: review+
Details | Diff | Splinter Review
2.08 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
634 bytes, patch
bjacob
: review+
Details | Diff | Splinter Review
1.73 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
72.41 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.98 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
3.16 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
12.21 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
Here we go again.
r=existing-code
Attachment #736612 - Flags: review+
I was having trouble getting this to work using the `-Dfoo=bar` semantics in the makefile, and it's only ever used in one place, anyways.
Attachment #736613 - Flags: review?(bjacob)
Attachment #736614 - Flags: review?(bjacob)
Attachment #736616 - Flags: review?(bjacob)
Attached patch patch 4: Get it building! (obsolete) — Splinter Review
Attachment #736617 - Flags: review?(bjacob)
r=existing-code
Attachment #736618 - Flags: review+
This is my fix to this ANGLE issue:
http://code.google.com/p/angleproject/issues/detail?id=418
Attachment #736619 - Flags: review?(bjacob)
That should be it!

Here's a sloppily abbreviated Try run:
https://tbpl.mozilla.org/?tree=Try&rev=756b4db56f94

Previous Try run with broken Windows clobber builds:
https://tbpl.mozilla.org/?tree=Try&rev=53deda5d4bcc

These Try runs also include the fix to bug 837213.
Attachment #736613 - Flags: review?(bjacob) → review+
Attachment #736614 - Flags: review?(bjacob) → review+
Attachment #736615 - Flags: review?(bjacob) → review+
Comment on attachment 736616 [details] [diff] [review]
patch 3b: Patch file and record.

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

These two local patches here really look like they should be upstreamed.
Attachment #736616 - Flags: review?(bjacob) → review+
Comment on attachment 736617 [details] [diff] [review]
patch 4: Get it building!

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

::: gfx/angle/Makefile.in
@@ +19,5 @@
> +STL_FLAGS =
> +
> +# ANGLE uses exceptions internally, so we need to have exception handling
> +# support
> +ENABLE_CXX_EXCEPTIONS = 1

This is r-: this is the part of ANGLE that's built into either libxul or libgkmedias, and we don't do C++ exceptions there.

@@ +23,5 @@
> +ENABLE_CXX_EXCEPTIONS = 1
> +
> +ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> +# Enable unwind semantics for exception handlers in response to warning C4530.
> +OS_CPPFLAGS += -EHsc

Same.

::: gfx/angle/src/libGLESv2/Makefile.in
@@ -196,5 @@
>  
>  else
>  
> -EXTRA_DSO_LDOPTS = "$(MOZ_DIRECTX_SDK_PATH)/lib/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)/d3d9.lib" \
> -                   "$(MOZ_DIRECTX_SDK_PATH)/lib/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)/D3DCompiler.lib"

Why don't we need this anymore?
Attachment #736617 - Flags: review?(bjacob) → review-
Comment on attachment 736619 [details] [diff] [review]
patch 6a: Fix default clamp strategy

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

Please explain this patch. Are you upstreaming it?
Attachment #736619 - Flags: review?(bjacob)
Attachment #736621 - Flags: review?(bjacob)
Comment on attachment 736619 [details] [diff] [review]
patch 6a: Fix default clamp strategy

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

Hadn't seen the ANGLE bug. OK.
Attachment #736619 - Flags: review+
Removed the exception stuff. I'm pretty sure we don't need to statically link to the libs since we ship the dll as well. Clobber-builds work fine this way, and the Try run seemed to come back clean. Checking about:support shows us using ANGLE as we should expect.
Attachment #736617 - Attachment is obsolete: true
Attachment #738808 - Flags: review?(bjacob)
Comment on attachment 738808 [details] [diff] [review]
patch 4: Get it building!

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

Yep, if it works without linking to the .lib and it's actually using ANGLE then that's fine.
Attachment #738808 - Flags: review?(bjacob) → review+
Are you going to land this? Or switch to a newer rev (upstream is at r2195 now)?
I should have some blank time Wednesday and Thursday to do r2195 instead. The only thing that took time for r2042 was the build stuff.
Try run with no build errors, and OSX M1 green: (at time of posting)
https://tbpl.mozilla.org/?tree=Try&rev=00604829c427

Tested Win7 WebGL test suite locally, so that should be fine too.

Inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fe104053f4f2
https://hg.mozilla.org/mozilla-central/rev/e9b37726c020
https://hg.mozilla.org/mozilla-central/rev/a16f28a76fcc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
This actually missed uplift, so we should get either this uplifted, or do another update and uplift that.
If you want to uplift anything to aurora, your best chance is to request approval for what already landed. An ANGLE update is going to be very difficult to get approval for, but right after uplift you have a fair chance.
Depends on: 872303
Depends on: 872307
Blocks: 837213
Duplicate of this bug: 837214
Blocks: 883478
So it seems like this missed being nominated for uplift the entire time 23 was on Aurora and now we're in week 2 of the Beta cycle. As with bug 837213 you can make a nomination case with clear risk/reward but I'm inclined to let this level of change ride the train and ship with FF24 unless there's a good user win here.
Flags: needinfo?(jgilbert)
Nope, let's just wait until 24.
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.