1124394 - Support running WebGL on Core Profiles on Mac

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We need to use core profiles to get a number of features we require for WebGL 2.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Don-t-share-CGL-GLContexts.patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Reorg-CGL-context-creation.patch

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Use-core-profiles.patch

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Fix-GL3.0-extension-parsing.patch

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Remove-bit-depth-queries.patch

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: 0006-Don-t-query-stencil-bits.patch

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: 0007-Allow-requests-for-compat-profiles.patch

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: 0008-Only-use-core-for-WebGL2.patch

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: 0009-fixes.patch

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b777bceef70
Clean.

Comment 11

[u480271]

Comment on attachment 8552678 [details] [diff] [review]
0004-Fix-GL3.0-extension-parsing.patch

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

::: gfx/gl/GLContext.h
@@ +3686,5 @@
> +                 std::vector<nsACString*>* out);
> +
> +template<size_t N>
> +bool
> +MarkBitfieldByString(const nsACString& str, const char* markStrList[N],

These don't compile on MSVC but I see changes in Patch 9

Comment 12

[u480271]

Comment on attachment 8552681 [details] [diff] [review]
0007-Allow-requests-for-compat-profiles.patch

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

Follow up patch to switch bool param out for enums after this lands.

::: dom/canvas/WebGLContext.cpp
@@ +515,4 @@
>          return nullptr;
>      }
>  
> +    nsRefPtr<GLContext> gl = gl::GLContextProvider::CreateHeadless(false);

I think this should be an enum instead of a bool param. I have patches to do that which we can land after this.

Comment 13

[u480271]

Comment on attachment 8552682 [details] [diff] [review]
0008-Only-use-core-for-WebGL2.patch

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

::: gfx/gl/GLContextProviderCGL.mm
@@ -259,4 @@
>  
>      if (!requireCompatProfile) {
>          profile = ContextProfile::OpenGLCore;
> -        context == CreateWithFormat(kAttribs_offscreen_coreProfile);

Oops

Comment 14

[u480271]

Phew, review done. There's a couple of patches I have to get WebGL 2 working on OSX that I'll send your way once this lands.

Comment 15

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/783d0fe2770b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for test bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/cffc8f5b7068

https://treeherder.mozilla.org/logviewer.html#?job_id=5941086&repo=mozilla-inbound

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: 0010-fixes.patch

There's a terminology mismatch here: non-onscreen vs. has-offscreen-buffer.

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=29d94749844f

Comment 18

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/176166c0bae9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/176166c0bae9

Comment 20

[[:fabrice] Fabrice Desré]

This needs to be backed out from all trees as soon as possible for causing bug 1127304

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b556a1f684ed

I'll merge this around before the next b2g nightlies get scheduled.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: 0011-Make-copies-of-extension-strings.patch

Comment 24

[u480271]

Comment on attachment 8558756 [details] [diff] [review]
0011-Make-copies-of-extension-strings.patch

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

Strings!!!! *shakes fist*

::: gfx/gl/GLContext.cpp
@@ +1612,5 @@
>                                                            i);
> +            // We CANNOT use nsDependentCString here, because the spec doesn't guarantee
> +            // that the pointers returned are different, only that their contents are.
> +            // On Flame, each of these index string queries returns the same address.
> +            mDriverExtensionList.push_back(nsCString(rawExt));

Oh, joy.

Comment 25

[Kelsey Gilbert [:jgilbert]]

So when holding onto a std::vector<nsCString>, we get leakage of nsStringBuffers:
https://tbpl.mozilla.org/?tree=Try&rev=7a8309fddbae

Without persisting these, we appear green.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: 0011-Make-copies-of-extension-strings.patch

Comment 27

[u480271]

Comment on attachment 8559445 [details] [diff] [review]
0011-Make-copies-of-extension-strings.patch

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

I swear I've reviewed these changes already.

Comment 28

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/80a88a3badba

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80a88a3badba

Comment 30

[Kelsey Gilbert [:jgilbert]]

Backed out:
  https://hg.mozilla.org/mozilla-central/rev/aa5f8d47a0ba

Comment 32

[u480271]

How is it this passes on Try but keeps getting backed out? Does this point to a deficiency in our automated tests?

Comment 33

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #32)
> How is it this passes on Try but keeps getting backed out? Does this point
> to a deficiency in our automated tests?

The two post-Try backouts appear to be different categories of gaps, at least. The first one was driver-related, so there's not a ton we can do there with our current Try configuration. The second (present) one has a good chance of being missed by our tests. I'm still figuring out the actual cause.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: 0012-Attach-Readback-func-to-GLContext-not-GLScreenBuffer.patch

Comment 35

[u480271]

Comment on attachment 8563699 [details] [diff] [review]
0012-Attach-Readback-func-to-GLContext-not-GLScreenBuffer.patch

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

From what I can tell, ScopeBindFramebuffer was replaced with more detailed version and the code was moved. LTGM.

Comment 36

[Kelsey Gilbert [:jgilbert]]

Let's try again:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/df54006fbe9e

Comment 37

[Carsten Book [:Tomcat]]

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