Closed Bug 1191042 Opened 5 years ago Closed 4 years ago

Trying to use PBuffers with different depth values with ANGLE

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jrmuizel, Assigned: jgilbert)

References

Details

Attachments

(17 files, 2 obsolete files)

21.18 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
18.93 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
11.31 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
6.21 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.58 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
789 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.25 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.15 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.35 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.55 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.84 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.14 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.66 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.95 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
6.98 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.25 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
9.22 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Updated ANGLE is perhaps more strict and complains when we try to EGLMakeCurrent from a surface that has no depth buffer to one that has a depth buffer.
Blocks: 1179280
This causes MakeCurrent to fail and sadness follows.
jgilbert, why are we making PBuffers with different configs and trying to use them with same context?
Flags: needinfo?(jgilbert)
How does it fail? What error does it give? Tracing into ANGLE, where is it failing?

This doesn't seem to be restricted by the spec.

We make PBuffers with different configs because we initially ask for just any old effectively-headless GL context. Once we create an accelerated layer for it, we ask it to start using something more sophisticated. (EGLSurface PBuffers, in ANGLE's case)
Flags: needinfo?(jgilbert)
Ugh, this is a ludicrous restriction if true.
The restriction appears to match the spec.

While I question why this was added at all, it's here for now.

Oh well. We know at context-creation time whether we need depth/stencil/alpha/antialias, so we can pipe those through when requesting a pseudo-headless context.

I can do this tomorrow.

It would be more ideal if we could just get a D3D texture for a GL texture, or access a D3D texture via a GL texture. (EGLImage?)
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Any progress on othis?
Flags: needinfo?(jgilbert)
I'll post the patch in a few.
Works on my machine. Just sent it to Try.
Flags: needinfo?(jgilbert)
Attachment #8644732 - Flags: review?(jmuizelaar)
(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Created attachment 8644732 [details] [diff] [review]
> 0001-Establish-correct-EGLConfig-at-GLContext-creation.patch
> 
> Works on my machine. Just sent it to Try.

This seems to work with the ANGLE update. Thanks very much!
Attachment #8644732 - Flags: review?(jmuizelaar) → review+
See Also: → 1193015
Any plans to land this soon? If not, I'd like to rip out the context creation flags and make a GLX equivalent for bug 1193015. Thanks!
Jeff said to go ahead and land this. Seems okay with Windows on try (Linux failures are unrelated); https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c4a4993f7b
Updated patch with correct bool casting from a typeable enum, and using mozilla::Vector (as C++11 is not used in gfx/gl, so we couldn't use std::vector::data()).
Attachment #8644732 - Attachment is obsolete: true
Attachment 8654229 [details] [diff] should be ready to land now- a try push against EGL targets might be a good idea. Can't seem to get hg.mozilla.org to accept my SSH public key for try pushes atm, will take a look at it later.
We're going to go all in here, and actually refactor WebGL context creation to match the reality of not being able to use CreateHeadless.
Attachment #8654229 - Attachment is obsolete: true
Attachment #8662734 - Flags: review?(jmuizelaar)
Attachment #8662735 - Flags: review?(jmuizelaar)
Can you write a description of your overall approach here?
Flags: needinfo?(jgilbert)
Sure:

Currently, we create a headless context, and then try to make different offscreen buffer combinations until we successfully create and attach a combo that works for our requirements.
The problem with this is that for EGL using PBuffers (and GLX using Pixmaps), the surface creation configs *must match (nearly) exactly* the config chosen at context creation time. Since we currently create headless contexts, we basically pick a config at random. This context creation config is generally not going to match the config we need to use for surface creation.

The solution is to know what we need at context creation time. I revive the older CreateOffscreen(SurfaceCaps) entrypoint, so that the context we create matches the config we'll need for the surfaces we'll create.

This involves redoing some of the config selection code, as well as refactoring the WebGL GLContext creation and initialization code, because of the differences between CreateHeadless and CreateOffscreen.
Flags: needinfo?(jgilbert)
Attachment #8662737 - Flags: review?(jmuizelaar) → review+
Attachment #8662738 - Flags: review?(jmuizelaar) → review+
Attachment #8662739 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8662736 [details] [diff] [review]
0003-Use-mConfig-from-GLContextEGL-in-ShSurfANGLE.patch

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

::: gfx/gl/GLContextEGL.h
@@ +120,3 @@
>      EGLSurface mSurface;
> +public:
> +    const EGLContext mContext;

It might be better to organize these by visibility unless there's a better reason to keep the order and swap visibility
Attachment #8662736 - Flags: review?(jmuizelaar) → review+
Attachment #8662734 - Flags: review?(jmuizelaar) → review+
Attachment #8662735 - Flags: review?(jmuizelaar) → review+
Attachment #8663975 - Flags: review?(jmuizelaar)
Attachment #8663976 - Flags: review?(jmuizelaar)
Attachment #8663978 - Flags: review?(jmuizelaar)
Attachment #8663979 - Flags: review?(jmuizelaar)
Attachment #8663980 - Flags: review?(jmuizelaar)
Attachment #8663981 - Flags: review?(jmuizelaar)
Attachment #8663985 - Flags: review?(dglastonbury)
Attachment #8663981 - Flags: review?(jmuizelaar) → review+
Attachment #8663980 - Flags: review?(jmuizelaar) → review+
Attachment #8663975 - Flags: review?(jmuizelaar) → review+
Attachment #8663976 - Flags: review?(jmuizelaar) → review+
Attachment #8663982 - Flags: review?(jmuizelaar) → review+
Attachment #8663978 - Flags: review?(jmuizelaar) → review+
Attachment #8663979 - Flags: review?(jmuizelaar) → review+
Attachment #8663984 - Flags: review?(jmuizelaar) → review+
Dan's on PTO, but these are not too webgl-related.
Can you review these two patches as well, jrmuizel?
Attachment #8664420 - Flags: review?(jmuizelaar)
Attachment #8663985 - Flags: review?(dglastonbury) → review?(jmuizelaar)
Attachment #8663985 - Flags: review?(jmuizelaar) → review+
Attachment #8664420 - Flags: review?(jmuizelaar) → review+
Attachment #8665207 - Flags: review?(jmuizelaar)
Comment on attachment 8665207 [details] [diff] [review]
0017-Fix-B2G-CreateForWindow.patch

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

It would be better to not have cosmetic changes mixed in with functional ones.
Attachment #8665207 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/0cecdec0f6e1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
So this is causing a ton of unexpected talos improvements for webgl-terrain on Windows:
(Improvement) Mozilla-Inbound-Non-PGO - WEBGL Terrain - WINNT 5.1 (ix) - 6.53%
(Improvement) Mozilla-Inbound-Non-PGO - WEBGL Terrain - WINNT 5.1 (e10s) - 6.46%
(Improvement) Mozilla-Inbound-Non-PGO - WEBGL Terrain - WINNT 6.1 (ix) - 16.9%
(Improvement) Mozilla-Inbound-Non-PGO - WEBGL Terrain - WINNT 6.1 (ix) (e10s) - 18.8%
(Improvement) Mozilla-Inbound-Non-PGO - WEBGL Terrain - WINNT 6.2 x64 - 23.2%
(Improvement) Mozilla-Inbound-Non-PGO - WEBGL Terrain - WINNT 6.2 x64 (e10s) - 24.8%
Scary
(In reply to Jeff Muizelaar [:jrmuizel] from comment #46)
> Scary

Agreed. Particularly the 15-25% ones.
Depends on: 1209022
You need to log in before you can comment on or make changes to this bug.