Closed Bug 1124394 Opened 5 years ago Closed 5 years ago

Support running WebGL on Core Profiles on Mac

Categories

(Core :: Canvas: WebGL, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(12 files, 1 obsolete file)

4.28 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
6.44 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.77 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
18.88 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
5.62 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.45 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
11.38 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.74 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
4.30 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
570 bytes, patch
kamidphish
: review+
Details | Diff | Splinter Review
8.52 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
6.24 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
We need to use core profiles to get a number of features we require for WebGL 2.
Attachment #8552675 - Flags: review?(dglastonbury)
Attachment #8552676 - Flags: review?(dglastonbury)
Attachment #8552677 - Flags: review?(dglastonbury)
Attachment #8552678 - Flags: review?(dglastonbury)
Attachment #8552679 - Flags: review?(dglastonbury)
Attachment #8552680 - Flags: review?(dglastonbury)
Attachment #8552681 - Flags: review?(dglastonbury)
Attachment #8552682 - Flags: review?(dglastonbury)
Attached patch 0009-fixes.patchSplinter Review
Attachment #8552683 - Flags: review?(dglastonbury)
Attachment #8552675 - Flags: review?(dglastonbury) → review+
Attachment #8552676 - Flags: review?(dglastonbury) → review+
Attachment #8552677 - Flags: review?(dglastonbury) → review+
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
Attachment #8552678 - Flags: review?(dglastonbury) → review+
Attachment #8552679 - Flags: review?(dglastonbury) → review+
Attachment #8552680 - Flags: review?(dglastonbury) → review+
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.
Attachment #8552681 - Flags: review?(dglastonbury) → review+
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
Attachment #8552682 - Flags: review?(dglastonbury) → review+
Attachment #8552683 - Flags: review?(dglastonbury) → review+
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.
Attached patch 0010-fixes.patchSplinter Review
There's a terminology mismatch here: non-onscreen vs. has-offscreen-buffer.

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=29d94749844f
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/176166c0bae9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
This needs to be backed out from all trees as soon as possible for causing bug 1127304
Depends on: 1127304
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b556a1f684ed

I'll merge this around before the next b2g nightlies get scheduled.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla38 → ---
Attachment #8558756 - Flags: review?(dglastonbury)
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.
Attachment #8558756 - Flags: review?(dglastonbury) → review+
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.
Attachment #8558756 - Attachment is obsolete: true
Attachment #8559445 - Flags: review?(dglastonbury)
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.
Attachment #8559445 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/80a88a3badba
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1130086
No longer blocks: 1130086
Status: RESOLVED → REOPENED
Depends on: 1130086
Resolution: FIXED → ---
Duplicate of this bug: 1130423
How is it this passes on Try but keeps getting backed out? Does this point to a deficiency in our automated tests?
(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 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.
Attachment #8563699 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/df54006fbe9e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.