980178 - Cleanup context loss handling

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

The context-loss handling code is not that clear. We should make it clear, since it's really important.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: fixup-context-loss

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: fixup-context-loss

Forgot to refresh...

Comment 3

[u480271]

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?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 5

[u480271]

(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.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Having issues on some platforms:
https://tbpl.mozilla.org/?tree=Try&rev=001be2430ce0

Comment 8

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=e00d1c818c79

Comment 9

[Kelsey Gilbert [:jgilbert]]

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)

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(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.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(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

Comment 12

[Kelsey Gilbert [:jgilbert]]

Rather, it used to read "flag is set". I added the "not" in the PR.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: fixup-context-loss

Two spec fixes later, and with a better understanding, we need to review this new version.

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=5e4cfd7a6c72

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: fixup-context-loss

Newest patch, green on Try.

Comment 16

[u480271]

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.

Comment 17

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/350f36f463d5

Comment 18

[Kelsey Gilbert [:jgilbert]]

Remove the printfs: (oops!)
https://hg.mozilla.org/integration/mozilla-inbound/rev/475168143f2e

Comment 19

[Kelsey Gilbert [:jgilbert]]

Try to patch the bustage:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/247f04ad3201

Comment 20

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

I backed these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e1476873e8e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/74dbaf5ff1fc and https://hg.mozilla.org/integration/mozilla-inbound/rev/d839669e740c for the bustage (and I had to back out 1004309 for other bustage and these looked related).

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: fixup-context-loss

Try is clean:
https://tbpl.mozilla.org/?tree=Try&rev=d09799e7bd76

Comment 22

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/441137505200

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for being the very likely cause of multiple TBPL crash bugs.
https://hg.mozilla.org/mozilla-central/rev/518117e8fc9a

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Hold a strong ref in the task

https://tbpl.mozilla.org/?tree=Try&rev=6105ac5a3690

Comment 26

[Kelsey Gilbert [:jgilbert]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef52ec5569d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/325caa2c0279

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ef52ec5569d
https://hg.mozilla.org/mozilla-central/rev/325caa2c0279