Closed Bug 1303443 Opened 4 years ago Closed 4 years ago

Update to ANGLE/2862

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jrmuizel, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

There have been some bug fixes in ANGLE that we should probably pick up.

    D3D11: Release SRV cache in Buffer11 destructor.
    D3D11: Detect driver version before workaround init.
    Add workaround for B5G6R5 format in Intel driver
Assignee: nobody → mtseng
Blocks: 1212878
(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> There have been some bug fixes in ANGLE that we should probably pick up.
> 
>     D3D11: Release SRV cache in Buffer11 destructor.
>     D3D11: Detect driver version before workaround init.
>     Add workaround for B5G6R5 format in Intel driver

Jeff, I assume we need to include this for FF51 and FF50.
(In reply to Peter Chang[:pchang] from comment #1)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> > There have been some bug fixes in ANGLE that we should probably pick up.
> > 
> >     D3D11: Release SRV cache in Buffer11 destructor.
> >     D3D11: Detect driver version before workaround init.
> >     Add workaround for B5G6R5 format in Intel driver
> 
> Jeff, I assume we need to include this for FF51 and FF50.

It's not needed for sure, but it seems like we're better of taking these fixes in FF51 and FF50
Ethan will upgrade the new ANGLE.
Assignee: mtseng → ethlin
I just finished angle update in my local. There are 2 changes in the angle/2862 that may effect gecko. :jrmuizel, do you have any idea?

1. SH_INIT_VARYINGS_WITHOUT_STATIC_USE was removed in this commit: https://github.com/google/angle/commit/7ebb97fc9a51aad770e07ee10de420b5fbc2a278, and gecko uses the parameter here: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLShaderValidator.cpp#37
   I'm not sure if we still need this.

2. SH_EMULATE_BUILT_IN_FUNCTIONS was removed in this commit: https://github.com/google/angle/commit/e5bb72ff28773c42aae1a8e744882d194049b0a3, and gecko uses the parameter to do some workaround for driver bugs(https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLShaderValidator.cpp#79). Though the parameter might be not needed now.
Flags: needinfo?(jmuizelaar)
(In reply to Ethan Lin[:ethlin] from comment #4)
> I just finished angle update in my local. There are 2 changes in the
> angle/2862 that may effect gecko. :jrmuizel, do you have any idea?
> 
> 1. SH_INIT_VARYINGS_WITHOUT_STATIC_USE was removed in this commit:
> https://github.com/google/angle/commit/
> 7ebb97fc9a51aad770e07ee10de420b5fbc2a278, and gecko uses the parameter here:
> https://dxr.mozilla.org/mozilla-central/source/dom/canvas/
> WebGLShaderValidator.cpp#37
>    I'm not sure if we still need this.
> 

In the current ANGLE/2845 in gecko , ANGLE doesn't use SH_INIT_VARYINGS_WITHOUT_STATIC_USE to do any checking. So I think we don't need to set this parameter.
(In reply to Ethan Lin[:ethlin] from comment #4)
> 2. SH_EMULATE_BUILT_IN_FUNCTIONS was removed in this commit:
> https://github.com/google/angle/commit/
> e5bb72ff28773c42aae1a8e744882d194049b0a3, and gecko uses the parameter to do
> some workaround for driver
> bugs(https://dxr.mozilla.org/mozilla-central/source/dom/canvas/
> WebGLShaderValidator.cpp#79). Though the parameter might be not needed now.

Here's the bug for that removal: https://bugs.chromium.org/p/chromium/issues/detail?id=642227.
I don't really understand how removing this was safe. Perhaps you can ask the ANGLE people about it?
Flags: needinfo?(jmuizelaar)
(In reply to Ethan Lin[:ethlin] from comment #7)
> try looks good.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a855f92cc45

OSX 10.10 has a failure: 
dom/canvas/test/webgl-conf/generated/test_2_conformance__glsl__misc__shaders-with-varyings.html | [unexpected link status] vertex shader with unused varying and fragment shader with used varying must succeed 

It may related to SH_INIT_VARYINGS_WITHOUT_STATIC_USE. I'll see how to fix it.
(In reply to Ethan Lin[:ethlin] from comment #8)
> OSX 10.10 has a failure: 
> dom/canvas/test/webgl-conf/generated/test_2_conformance__glsl__misc__shaders-
> with-varyings.html | [unexpected link status] vertex shader with unused
> varying and fragment shader with used varying must succeed 
> 
> It may related to SH_INIT_VARYINGS_WITHOUT_STATIC_USE. I'll see how to fix
> it.

From my local test, gecko needs to add SH_INIT_OUTPUT_VARIABLES for MacOS to initialize variable. I'll send a try to double check
Attached patch ANGLE/2862 (obsolete) — Splinter Review
Update to ANGLE/2862.
Attachment #8797971 - Flags: review?(jgilbert)
I removed SH_INIT_VARYINGS_WITHOUT_STATIC_USE and SH_EMULATE_BUILT_IN_FUNCTIONS from gecko. SH_INIT_VARYINGS_WITHOUT_STATIC_USE is unused in the original ANGLE. SH_EMULATE_BUILT_IN_FUNCTIONS is for some Mac deives with OSX 10.6~10.8 driver which opengl version is 3.2 or before, but Nightly can only install on OSX 10.9 or later which opengl version is 3.3 or later.
Attachment #8797974 - Flags: review?(jgilbert)
Attachment #8797974 - Flags: review?(jgilbert) → review+
Comment on attachment 8797971 [details] [diff] [review]
ANGLE/2862

I'm not sure why it's changing all the file modes, but ok. I mean, they technically shouldn't be executable, so this is 'better', but it's still weird.
Attachment #8797971 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> Comment on attachment 8797971 [details] [diff] [review]
> ANGLE/2862
> 
> I'm not sure why it's changing all the file modes, but ok. I mean, they
> technically shouldn't be executable, so this is 'better', but it's still
> weird.

I didn't notice that! Per discussion with Morris, this may be because I did gyp_mozbuild on Mac and Morris did gyp_mozbuild on Windows.
Attached patch ANGLE/2862 (carry r+: jgilbert) (obsolete) — Splinter Review
Rebase the patch and remove the changes of file mode.
Attachment #8797971 - Attachment is obsolete: true
Update the correct patch.
Attachment #8804524 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/042d532e3d9e
Update ANGLE to chromium/2862. r=jgilbert
Keywords: checkin-needed
Backed out for build bustage in WebGLShaderValidator.cpp after SH_INIT_VARYINGS_WITHOUT_STATIC_USE etc. got removed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd14dd39440eeef1e29dfc4d4427ad176828a88

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=042d532e3d9e553c89062ba169940602558755ff
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38300962&repo=mozilla-inbound

 /builds/slave/m-in-lx-0000000000000000000000/build/src/dom/canvas/WebGLShaderValidator.cpp:37:19: error: 'SH_INIT_VARYINGS_WITHOUT_STATIC_USE' was not declared in this scope
/builds/slave/m-in-lx-0000000000000000000000/build/src/dom/canvas/WebGLShaderValidator.cpp:57:16: error: 'SH_EMULATE_BUILT_IN_FUNCTIONS' was not declared in this scope

These got removed here: https://hg.mozilla.org/integration/mozilla-inbound/rev/042d532e3d9e553c89062ba169940602558755ff#l2.137
Flags: needinfo?(ethlin)
Please push attachment 8804528 [details] [diff] [review] and attachment 8797974 [details] [diff] [review] together.
Flags: needinfo?(ethlin)
Keywords: checkin-needed
I'm going to push this as one folded-up patch because we generally try to ensure that everything that lands is individually buildable and passing tests so as to not break bisection.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c5a60badc7
Update ANGLE to chromium/2862. r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/57c5a60badc7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1313875
Comment on attachment 8804528 [details] [diff] [review]
ANGLE/2862 (carry r+: jgilbert)

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8804528 - Flags: approval-mozilla-aurora?
Comment on attachment 8804528 [details] [diff] [review]
ANGLE/2862 (carry r+: jgilbert)

WebGL 2 is the feature we want to ship in 51. Aurora51+.
Attachment #8804528 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Iris Hsiao [:ihsiao] from comment #26)
> Sorry had to back out for bustages like
> https://treeherder.mozilla.org/logviewer.html#?job_id=4151297&repo=mozilla-
> aurora#L12814

That's because the other patch should be uplifted too. I'll set the request comments.
Flags: needinfo?(ethlin)
Comment on attachment 8797974 [details] [diff] [review]
Change option parameters for angle update

pproval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:tested locally and try server
[Risks and why]: Low. The patch just update the parameter for the new ANGLE.
[String/UUID change made/needed]: None.
Attachment #8797974 - Flags: approval-mozilla-aurora?
(In reply to Ethan Lin[:ethlin] from comment #28)
> Comment on attachment 8797974 [details] [diff] [review]
> Change option parameters for angle update
> 
> pproval Request Comment
> [Feature/regressing bug #]: webgl2
> [User impact if declined]:
> [Describe test coverage new/current, TreeHerder]:tested locally and try
> server
> [Risks and why]: Low. The patch just update the parameter for the new ANGLE.
> [String/UUID change made/needed]: None.

Please uplift attachment 8804528 [details] [diff] [review] and attachment 8797974 [details] [diff] [review] together.
Attachment #8797974 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.