Closed Bug 980178 Opened 7 years ago Closed 7 years ago

Cleanup context loss handling

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Whiteboard: webgl-internal)

Attachments

(2 files, 4 obsolete files)

The context-loss handling code is not that clear. We should make it clear, since it's really important.
Depends on: 978418
Attached patch fixup-context-loss (obsolete) — Splinter Review
Attachment #8386543 - Flags: review?(dglastonbury)
Attached patch fixup-context-loss (obsolete) — Splinter Review
Forgot to refresh...
Attachment #8386543 - Attachment is obsolete: true
Attachment #8386543 - Flags: review?(dglastonbury)
Attachment #8386549 - Flags: review?(dglastonbury)
Comment on attachment 8386549 [details] [diff] [review]
fixup-context-loss

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

Minor nits.

::: content/canvas/src/WebGLContext.cpp
@@ +86,1 @@
>                              MOZ_UTF16("heap-minimize")))

Does this fix on one line?

@@ +88,3 @@
>          wantToLoseContext = mContext->mLoseContextOnHeapMinimize;
> +    }
> +    if (wantToLoseContext) {

\n before if

@@ +95,1 @@
>  WebGLContextOptions::WebGLContextOptions()

\n before

@@ +1225,5 @@
> +    } else if (isEGL) {
> +        // Simulate a ARB_robustness guilty context loss for when we
> +        // get an EGL_CONTEXT_LOST error. It may not actually be guilty,
> +        // but we can't make any distinction.
> +        if (!gl->MakeCurrent(true) && gl->IsContextLost()) {

What does true mean? I think we should make another bug to replace bool param of MakeCurrent with an enum.

@@ +1269,5 @@
> +
> +bool
> +WebGLContext::TryToRestoreContext()
> +{
> +    if (NS_FAILED(SetDimensions(mWidth, mHeight)))

return !NS_FAILED(SetDimensions(...));
Is there a NS_SUCCEEDED?
Attachment #8386549 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> Comment on attachment 8386549 [details] [diff] [review]
> fixup-context-loss
> 
> Review of attachment 8386549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minor nits.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +86,1 @@
> >                              MOZ_UTF16("heap-minimize")))
> 
> Does this fix on one line?
Yeah, I can put this on one line.
> 
> @@ +88,3 @@
> >          wantToLoseContext = mContext->mLoseContextOnHeapMinimize;
> > +    }
> > +    if (wantToLoseContext) {
> 
> \n before if
Yep.
> 
> @@ +95,1 @@
> >  WebGLContextOptions::WebGLContextOptions()
> 
> \n before
Ok.
> 
> @@ +1225,5 @@
> > +    } else if (isEGL) {
> > +        // Simulate a ARB_robustness guilty context loss for when we
> > +        // get an EGL_CONTEXT_LOST error. It may not actually be guilty,
> > +        // but we can't make any distinction.
> > +        if (!gl->MakeCurrent(true) && gl->IsContextLost()) {
> 
> What does true mean? I think we should make another bug to replace bool
> param of MakeCurrent with an enum.
Maybe. 'what does true mean' is sorta supposed to be a heads up that we're going something a little unusual here. 'true' means 'force', so we'll always MakeCurrent, instead of skipping it if we're already current. I'm still not convinced that we should s/bool/enums/ for these seldom used paths.
> 
> @@ +1269,5 @@
> > +
> > +bool
> > +WebGLContext::TryToRestoreContext()
> > +{
> > +    if (NS_FAILED(SetDimensions(mWidth, mHeight)))
> 
> return !NS_FAILED(SetDimensions(...));
> Is there a NS_SUCCEEDED?
I wanted to leave this function open so that we could add more criteria later. I don't *think* we should need to add anything here, but formatted like this implies that we could.

I also don't like squashing all that logic down. I think it's more readable this way.
(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > @@ +1225,5 @@
> > > +    } else if (isEGL) {
> > > +        // Simulate a ARB_robustness guilty context loss for when we
> > > +        // get an EGL_CONTEXT_LOST error. It may not actually be guilty,
> > > +        // but we can't make any distinction.
> > > +        if (!gl->MakeCurrent(true) && gl->IsContextLost()) {
> > 
> > What does true mean? I think we should make another bug to replace bool
> > param of MakeCurrent with an enum.
> Maybe. 'what does true mean' is sorta supposed to be a heads up that we're
> going something a little unusual here. 'true' means 'force', so we'll always
> MakeCurrent, instead of skipping it if we're already current. I'm still not
> convinced that we should s/bool/enums/ for these seldom used paths.

if it said if (!gl->MakeCurrent(FORCE_CURRENT) && ... then it would be clear at that point that:

a) Something unusual is going on, and
b) That that is forcing the context.

with out having to go look at the implementation of MakeCurrent to see what the bool value does.
Whiteboard: webgl-internal
(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > > @@ +1225,5 @@
> > > > +    } else if (isEGL) {
> > > > +        // Simulate a ARB_robustness guilty context loss for when we
> > > > +        // get an EGL_CONTEXT_LOST error. It may not actually be guilty,
> > > > +        // but we can't make any distinction.
> > > > +        if (!gl->MakeCurrent(true) && gl->IsContextLost()) {
> > > 
> > > What does true mean? I think we should make another bug to replace bool
> > > param of MakeCurrent with an enum.
> > Maybe. 'what does true mean' is sorta supposed to be a heads up that we're
> > going something a little unusual here. 'true' means 'force', so we'll always
> > MakeCurrent, instead of skipping it if we're already current. I'm still not
> > convinced that we should s/bool/enums/ for these seldom used paths.
> 
> if it said if (!gl->MakeCurrent(FORCE_CURRENT) && ... then it would be clear
> at that point that:
> 
> a) Something unusual is going on, and
> b) That that is forcing the context.
> 
> with out having to go look at the implementation of MakeCurrent to see what
> the bool value does.

True, that's fair and better.
Having issues on some platforms:
https://tbpl.mozilla.org/?tree=Try&rev=001be2430ce0
Alright, so I think the context-loss test in the conformance suite is in conflict with the spec.
In the spec, there are two ways this can work:
* Without calling event.preventDefault(), we get the default behavior: Context restoration is automatic at the end of the context-lost callback.
* With event.preventDefault(), we go into 'manual mode', where we do not automatically restore the context. 

Notably, without WEBGL_lose_context.restoreContext(), we can never restore the context from 'manual mode'. It seems clear that we should allow restoreContext() to request a context be restored, or this preventDefault option is relatively useless. (unless we're requiring apps to do the "poor man's context reset" of recreating the canvas element, and getting a new rendering context from it)
(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> Alright, so I think the context-loss test in the conformance suite is in
> conflict with the spec.
> In the spec, there are two ways this can work:
> * Without calling event.preventDefault(), we get the default behavior:
> Context restoration is automatic at the end of the context-lost callback.
> * With event.preventDefault(), we go into 'manual mode', where we do not
> automatically restore the context. 
> 

I'm confused by "event.preventDefault()".
If we don't call event.preventDefault(), I think the default behavior is: do not restore context.
See the setp 6.2 in spec.
So, without event.preventDefault(), the context will not restore.

http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2
    1.Let canvas be the context's canvas.
    2.If context's webgl context lost flag is set, abort these steps.
    3.Set context's webgl context lost flag.
    4.Set the invalidated flag of each WebGLObject instance created by this context.
    5.Disable all extensions except "WEBGL_lose_context".
    6.Queue a task to perform the following steps:
        6.1.Fire a WebGL context event named "webglcontextlost" at canvas....
        6.2.If the event's canceled flag is not set, abort these steps.   <= check preventDefault() here.
        6.3.Perform the following steps asynchronously.
        6.4.Await a restorable drawing buffer.
        6.5.Queue a task to restore the drawing buffer for context.
(In reply to Jerry Shih[:jerry] from comment #10)
> (In reply to Jeff Gilbert [:jgilbert] from comment #9)
> > Alright, so I think the context-loss test in the conformance suite is in
> > conflict with the spec.
> > In the spec, there are two ways this can work:
> > * Without calling event.preventDefault(), we get the default behavior:
> > Context restoration is automatic at the end of the context-lost callback.
> > * With event.preventDefault(), we go into 'manual mode', where we do not
> > automatically restore the context. 
> > 
> 
> I'm confused by "event.preventDefault()".
> If we don't call event.preventDefault(), I think the default behavior is: do
> not restore context.
> See the setp 6.2 in spec.
> So, without event.preventDefault(), the context will not restore.
> 
> http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2
>     1.Let canvas be the context's canvas.
>     2.If context's webgl context lost flag is set, abort these steps.
>     3.Set context's webgl context lost flag.
>     4.Set the invalidated flag of each WebGLObject instance created by this
> context.
>     5.Disable all extensions except "WEBGL_lose_context".
>     6.Queue a task to perform the following steps:
>         6.1.Fire a WebGL context event named "webglcontextlost" at canvas....
>         6.2.If the event's canceled flag is not set, abort these steps.   <=
> check preventDefault() here.
>         6.3.Perform the following steps asynchronously.
>         6.4.Await a restorable drawing buffer.
>         6.5.Queue a task to restore the drawing buffer for context.

Until 20 hours ago, 6.2 read "flag is not set". There was a bug in the spec, so my understanding in this bug here was based on the incorrect wording in the spec.

See here: https://github.com/KhronosGroup/WebGL/pull/550
Rather, it used to read "flag is set". I added the "not" in the PR.
Attached patch fixup-context-loss (obsolete) — Splinter Review
Two spec fixes later, and with a better understanding, we need to review this new version.
Attachment #8386549 - Attachment is obsolete: true
Attachment #8419783 - Flags: review?(dglastonbury)
Attached patch fixup-context-loss (obsolete) — Splinter Review
Newest patch, green on Try.
Attachment #8419783 - Attachment is obsolete: true
Attachment #8419783 - Flags: review?(dglastonbury)
Attachment #8420200 - Flags: review?(dglastonbury)
Comment on attachment 8420200 [details] [diff] [review]
fixup-context-loss

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

r+ conditional on fixing up the WS issues caused by your editor.

::: content/canvas/src/WebGLContext.cpp
@@ -77,5 @@
>                                       const char16_t* aSomeData)
>  {
>      if (strcmp(aTopic, "memory-pressure"))
>          return NS_OK;
> -

What happened to all this vertical WS in this function?

@@ +996,5 @@
>          // reset out state to what we *think* it is, we'll get it wrong.
>  
>          // Dither shouldn't matter when we're clearing to {0,0,0,0}.
> +        printf_stderr("actual/shadow: %d/%d\n", (int)gl->fIsEnabled(LOCAL_GL_SCISSOR_TEST),
> +                                                (int)mScissorTestEnabled);

Is this from the shadow state patch? (Bug 1004309)

@@ +1370,5 @@
>  
>  void
>  WebGLContext::ForceLoseContext()
>  {
> +    printf_stderr("WebGL(%p)::ForceLoseContext\n", this);

Is this debugging or desired?

@@ +1386,5 @@
>  
>  void
>  WebGLContext::ForceRestoreContext()
>  {
> +    printf_stderr("WebGL(%p)::ForceRestoreContext\n", this);

Ditto

::: content/canvas/src/WebGLContextLossTimer.cpp
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "WebGLContext.h"
> +using namespace mozilla;
> +/* static */ void
> +WebGLContext::ContextLossCallbackStatic(nsITimer* timer, void* thisPointer)

Editor ate all the vertical WS?

::: content/canvas/src/WebGLContextValidate.cpp
@@ +1649,5 @@
>  
> +    mDitherEnabled = false;
> +    mRasterizerDiscardEnabled = false;
> +    mScissorTestEnabled = false;
> +

Is this a bonus fix? Doesn't seem related to context lost.
Attachment #8420200 - Flags: review?(dglastonbury) → review+
Try is clean:
https://tbpl.mozilla.org/?tree=Try&rev=d09799e7bd76
Attachment #8420200 - Attachment is obsolete: true
Attachment #8429499 - Flags: review?(dglastonbury)
Flags: needinfo?(jgilbert)
Attachment #8429499 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/441137505200
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1016899
Depends on: 1016884
Depends on: 1017068
Backed out for being the very likely cause of multiple TBPL crash bugs.
https://hg.mozilla.org/mozilla-central/rev/518117e8fc9a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8431274 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/mozilla-central/rev/3ef52ec5569d
https://hg.mozilla.org/mozilla-central/rev/325caa2c0279
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 1057061
You need to log in before you can comment on or make changes to this bug.