18.83 KB, patch
|Details | Diff | Splinter Review|
64.45 KB, patch
|Details | Diff | Splinter Review|
2.27 KB, patch
|Details | Diff | Splinter Review|
The WEBGL_EXT_lose_context extension's semantics are no longer valid with the updated spec. See: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/specs/latest/index-context-lost.html#5.15 and...: http://people.mozilla.org/~dsherk/webgl/sdk/tests/conformance/context/context-lost-restored.html This is a patched version of the Khronos trunk version, which can be found at: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/context/context-lost-restored.html
Created attachment 581568 [details] [diff] [review] Patch v1.0, fix EXT_context_loss semantics I believe that the new context-loss.html test may be impossible to pass in its current state. I sent out an email to public_webgl: "I don't believe right now that the Khronos version of context-lost.html can pass with the semantics described in this thread. The following check seems to me impossible to pass: shouldGenerateGLError(gl, gl.NO_ERROR, "extension.loseContext()"); I'm assuming the following: * extension.loseContext() should trigger a context loss immediately * as long as a context is lost, the first getError() call should return CONTEXT_LOST_WEBGL Note that shouldGenerateGLError eval()'s the statement then calls getError() immediately afterwards if no exception was thrown. This should always trigger either an INVALID_OPERATION or CONTEXT_LOST_WEBGL. Perhaps I'm missing something?" I "fixed" this test (the change was trivial) and also fixed context-lost-restored.html, which had some typos (but otherwise I didn't change any functionality). The tests can be found here: http://people.mozilla.org/~dsherk/webgl/sdk/tests/conformance/context/context-lost.html http://people.mozilla.org/~dsherk/webgl/sdk/tests/conformance/context/context-lost-restored.html This patch fixes our EXT_context_loss implementation to pass these two tests. It also renames WEBGL_EXT_lose_context to the new standard, MOZ_WEBGL_lose_context. This shouldn't be landed until we update our conformance suite.
Assignee: nobody → dsherk
Attachment #581568 - Flags: review?(bjacob)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=ee416a3c7174 Note that there are M1 failures. These are completely expected as this shouldn't be landed until the conformance suite is updated; that is, the tests that we're performing are invalid.
Also, I tested the GLX, WGL and new EGL robustness implementations and they seemed to all work as we would expect them to.
Created attachment 582157 [details] [diff] [review] Patch v1.1, fix EXT_context_loss semantics I broke this up into two separate patches because it was impossible to distinguish boilerplate mContextLost->!IsContextStable() changes from actual logic.
Created attachment 582158 [details] [diff] [review] (part 2) Patch v1.1, fix EXT_context_loss semantics. Second part, just tons of replacements of "mContextLost" with "!IsContextStable()". There should be no signficant logic changes here.
Created attachment 582159 [details] [diff] [review] Patch v1.1, fix EXT_context_loss semantics I suck at bugzilla. Please refer to previous comments.
To answer the age-old question, "oh god what is this", I'm writing up some docs on what happened here. The previous iteration of the context loss testing extension was very poorly defined and didn't actually really help with testing that scripts indeed handled context loss correctly. The WebGL WG decided to update the spec, as you can see here: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/specs/latest/index-context-lost.html#5.15.2 The changes are largely in how a context loss should be handled asynchronously rather than synchronously. We used to send a webglcontextlost event immediately when we detected a context loss, or when extension.loseContext() was called. Now, we use the robustness timer to send it on the next Notify(). The script then has the option of handling it, just as it did before. However, one change is that a script must handle the webglcontextlost event if it triggered loseContext(), otherwise it can't use restoreContext(). This change was done to more closely mirror real functionality. Real functionality, i.e. when a DoS happens, is identical in this regard. After the script has handled it, on the next Notify, we then try to restore the context. This is in contrast to again doing this synchronously. It used to be that a context loss detection, the webglcontextlost event, the actual restore, and the webglcontextrestored event were fired all at the same time, but now they happen over 3 Notify iterations. This required a big change in how we mark a context as lost. Instead of just being "lost" or "not lost", we now have states. These states are as follows (also described in the patch/code): 1) Stable, i.e. we don't know of any problems or there just aren't any. 2) We know it's lost, but haven't sent out the asynchronous webglcontextlost event. 3) It is lost and we have sent out the event; the script has either not handled the event or we aren't going to restore it for some other reason. 4) It is lost and waiting to be restored on the next Notify. Another small change I made was that I noticed it was flipping the height/width dimensions on context restore (no idea how I missed this before, probably because I was working entirely working with square canvases and I've never seen a dimension specified as width x height rather than height x width). I swapped these so it should restore correctly now.
Status: NEW → ASSIGNED
Created attachment 582163 [details] [diff] [review] Patch v1.1, fix EXT_context_loss semantics
Created attachment 582164 [details] [diff] [review] (part 2) Patch v1.1, fix EXT_context_loss semantics.
Comment on attachment 582163 [details] [diff] [review] Patch v1.1, fix EXT_context_loss semantics Review of attachment 582163 [details] [diff] [review]: ----------------------------------------------------------------- Looks wonderful!
Attachment #582163 - Flags: review?(bjacob) → review+
Attachment #582164 - Flags: review?(bjacob) → review+
Created attachment 582756 [details] [diff] [review] (part 3) Patch v1.1, fix EXT_lose_context semantics. With this patch and bug 711592's, this entire patch stack should now work. Unfortunately, I can't push it to try or inbound because my moco account has been deleted and my permissions seem to be tied to that.
Attachment #582756 - Flags: review?(bjacob)
Attachment #582756 - Flags: review?(bjacob) → review+
(In reply to Doug Sherk (:dRdR) from comment #11) > Created attachment 582756 [details] [diff] [review] > (part 3) Patch v1.1, fix EXT_lose_context semantics. > > With this patch and bug 711592's, this entire patch stack should now work. > Unfortunately, I can't push it to try or inbound because my moco account has > been deleted and my permissions seem to be tied to that. Tying your hg permissions to your employment status seems unlikely. Should be just an accident. File an IT bug (like when you got access) to explain the problem, there should be an easy solution.
Target Milestone: --- → mozilla12
This cset was empty: > https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd5039ca2e7
https://hg.mozilla.org/mozilla-central/rev/7dd5039ca2e7 https://hg.mozilla.org/mozilla-central/rev/23f3b97f655a https://hg.mozilla.org/mozilla-central/rev/8e3d4ad9e413 leaving open for the empty changeset to be fixed.
Please close; empty cset was ok, the changes made in that page were made by someone else, so it bitrotted and I accidentally pushed it as empty anyways.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.