732441 - Delay WebGLContext::SetDimensions until a WebGL call is made

Status: NEW

(Core :: Graphics: CanvasWebGL, defect, P5)

Description

[Benoit Jacob [:bjacob] (mostly away)]

There are two problems that I'm trying to fix here.

1. Many websites are creating WebGL contexts without making any WebGL call on them. Typically, sites using Modernizr. Examples: GitHub, The Economist. The OpenGL context creation typically takes 100 ms. So, avoiding it would make page load faster, and would save memory too.

2. When a script resizes a canvas, setting width and then setting height, that results in SetDimensions being called twice, which is wasteful.

I would repurpose IsContextStable() into a generic "pre-webgl-call" hook and do the work there. Adding one if() per WebGL call is negligible overhead.

Comment 1

[Kelsey Gilbert [:jgilbert]]

This is a really neat idea. I would be a little concerned because this lazy initialization is not the expected behavior, which could leave webgl devs scratching their heads. Adding to the documentation about context creation would alleviate this.

Comment 2

[Cedric Vivier [:cedricv]]

It would be great for (2).

I'm not sure to understand how this would be possible for (1).
How would we deal with context creation failure (iirc the underlying GL context is created in SetDimensions) ?

According to the spec, getContext returns null if context creation failed. If context creation cannot be offloaded to another thread, even asynchronous context creation (as discussed for the next revision of the spec) wouldn't help here as the goal of Modernizr is to find out if WebGL can be used at all.


In fact, I think Modernizr should update its WebGL code to do soft detection testing presence of window.WebGLRenderingContext only, and removing webgl.exts, as the current code [1] is indeed 'wasteful' and doesn't seem to guarantee much more :
- creating a context on an offscreen 0x0 (~1x1) canvas, does not guarantee the application can later create a WebGL on, say, a 2000x1000 canvas.
- available extensions might change depending on canvas size and other factors (ie. a browser might prefer initially creating a software emulation renderer context for small canvases and/or background tabs [possibly after context loss]).

What do you think Paul?

[1] : https://github.com/Modernizr/Modernizr/blob/master/feature-detects/webgl-extensions.js

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Cedric Vivier [:cedricv] from comment #2)
> I'm not sure to understand how this would be possible for (1).
> How would we deal with context creation failure (iirc the underlying GL
> context is created in SetDimensions) ?

Darn, I completely overlooked that problem. I guess that kills (1) then.

Cedric, there is a Modernizr bug to discuss this:
https://github.com/Modernizr/Modernizr/issues/511

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Anyway, we still want this for (2), yes.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Cedric's objection about (1) is another thing that would be fixed by having asynchronous context creation creating initially lost contexts: we could delay the OpenGL context creation until the webglcontextrestored event.

Comment 6

[Paul Irish]

(In reply to Cedric Vivier [:cedricv] from comment #2)
> In fact, I think Modernizr should update its WebGL code to do soft detection

Sounds good. https://github.com/Modernizr/Modernizr/commit/b0c7dfc7
Will the global be present on blacklisted GPUs?

I guess the answer to this determines if the context creation failure case matters. If the global isn't cognizant of blacklisted GPUs then a lazy creation that may fail later on is no better, detection wise.. but much better perf-wise.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Paul Irish from comment #6)
> (In reply to Cedric Vivier [:cedricv] from comment #2)
> > In fact, I think Modernizr should update its WebGL code to do soft detection
> 
> Sounds good. https://github.com/Modernizr/Modernizr/commit/b0c7dfc7
> Will the global be present on blacklisted GPUs?

Yes, it is unaffected by blacklisting.

Comment 8

[Paul Irish]

I'll file that bug on you guys then. :) 
Bug 736232

And thus, from the web developer perspective, this ticket's proposed change is Good. :)

Comment 9

[Nicolas Silva [:nical]]

Attachment: Delay the effect of setdimensions to the next webgl call

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 680037 [details] [diff] [review]
Delay the effect of setdimensions to the next webgl call

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

In WebGLContext.cpp, in GetInputStream and in Render, we only check if(!gl), we don't call IsContextStable(). So, doing View Image on a canvas that is pending a resize could lead to problems.

I think we want to change this if(!gl) to if(!IsContextStable()) like elsewhere. To avoid issues in the case of a newly created canvas, where gl==nullptr, we could:
 - either have IsContextStable check if gl==nullptr
 - or let mContextStatus default to a new enum value, say ContextPendingCreation

Also, I wondered if instead of mDimensionsChanged you would do mRequestedWidth / mRequestedHeight, so that we would do nothing if people undo a resizing before we have acted on it. Probably not a very important case though. Up to you.

::: content/canvas/src/WebGLContext.h
@@ +510,5 @@
>      NS_DECL_NSIDOMWEBGLRENDERINGCONTEXT
>  
>      // nsICanvasRenderingContextInternal
>      NS_IMETHOD SetDimensions(int32_t width, int32_t height);
> +    bool MaybeUpdateDimensions();

Move this declaration outside of the // nsICanvasRenderingContextInternal block, and into a protected section.

@@ +653,5 @@
>          
>      void GetContextAttributes(dom::WebGLContextAttributes& retval);
> +    bool IsContextLost() const {
> +        return mContextStatus != ContextStable;
> +    }

No need for that change. That only saves an if(), but this function is not that performance-sensitive.

Moreover, I wanted to replace all the if(!IsContextStable()) in all the WebGL functions by If(IsContextLost()) to avoid the double negation and to remove that concept of "context stable" to have fewer concepts around.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Jacob [:bjacob] from comment #10)
> Comment on attachment 680037 [details] [diff] [review]
> Delay the effect of setdimensions to the next webgl call
> 
> Review of attachment 680037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In WebGLContext.cpp, in GetInputStream and in Render, we only check if(!gl),
> we don't call IsContextStable(). So, doing View Image on a canvas that is
> pending a resize could lead to problems.

Problems would also occur with OMTC+basic layers as Render is what basic layers use.

Comment 12

[Nicolas Silva [:nical]]

Attachment: Delay the effect of setdimensions to the next webgl call

here is a new version that replaces IsContextStable with IsContextLost and handles the resizing in IsContextLost.
since the resize is there, IsContextLost lost its constness.
I added a "GetContextState() const" method for the few places where we need to check the status in a const method or on a const context like in here:

http://dxr.mozilla.org/mozilla-central/content/canvas/src/WebGLContext.cpp#l658
where contexts[i] is const.

I see that Render returns NS_OK when gl is nil, should it also return true when an error occurs in the resize code within IsContextStable?

I don't know enough about webgl yet to be 100% sure about all the possible cases where I should have added IsContextLost in a method where we did not check before (to handle the resize), so I basically kept IsContextLost wherever we had "!IsContextStable" before, plus the two methods you pointed in a comment of this bug.

I left a couple a TODOs to discuss about whether we should handle resize in some places like DrawingBufferWidth/Height (in which case they'd lose constness as well).

Comment 13

[Nicolas Silva [:nical]]

Attachment: Delay the effect of setdimensions to the next webgl call

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 681083 [details] [diff] [review]
Delay the effect of setdimensions to the next webgl call

Waiting for non-busted patch

Comment 15

[Nicolas Silva [:nical]]

I don't have time to work on that these days so if anyone feels like working on it, the bug is yours :)

Comment 16

[Nick Lebedev [:nl]]

Attachment: Delayed webgl context recreation due to the dimension update until the next webl call is made.

Slightly updated the previous patch and also rebased it.

I have a few questions about the webgl logic:
1. Do the dimension changes affect WebGLContextAsyncQueries methods e.g. [1]? I'm not currently update webgl context/dimensions when the mentioned methods are called.

2. In the previous patch we didn't touch the IsContextLost method in the WebGLContextValidate file, e.g. [2]. Did we do it for some purpose?

3. When the dimensions are set i've added the following check:
if the gl variable is null we instantly update the dimensions and therefore create the context. I might be wrong but it seems like that if i do not create gl context here then the script's logic like canvas.getContext('webgl') may return good value in spite of that the context which is created in UpdateDimensions method wasn't created yet.

[1] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextAsyncQueries.cpp#114
[2] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextValidate.cpp#1465

I've also triggered build/tests on the try server: https://tbpl.mozilla.org/?tree=Try&rev=884c31086bda.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8424728 [details] [diff] [review]
Delayed webgl context recreation due to the dimension update until the next webl call is made.

As time passes, the right reviewer for a given bug/patch changes. At present, for this one, your first choice should be :jgilbert.

Comment 18

[Nick Lebedev [:nl]]

Attachment: Delay webgl context recreation due to the dimension update until the next webl call is made. v2.

Fixed mochitest failure (related to not updating dimensions when too big size is set).

Try results: https://tbpl.mozilla.org/?tree=Try&rev=6a6a2e61d622 (no regression is mentioned).

Comment 19

[Nick Lebedev [:nl]]

jgilbert, could you take a look at my patch please ?

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8425295 [details] [diff] [review]
Delay webgl context recreation due to the dimension update until the next webl call is made. v2.

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

First off, thanks for taking a swing at this bug.

You don't need to call Prepare() everywhere. We only need to ensure we have a backbuffer before calls that require a backbuffer to function.
This includes, when FB zero is bound:
draw*()
readPixels()
CopyTexImage*
getParameter() for:
  *_BITS
  SAMPLE_BUFFERS
  SAMPLES

These functions should EnsureBackbuffer() after checking for ContextLost, and if they return false, should cause the context to become lost. (failure to create backbuffer)

::: content/canvas/src/WebGLContext.cpp
@@ +384,5 @@
>  
> +    mWidth = width;
> +    mHeight = height;
> +
> +	if (!gl || mHeight > mGLMaxViewportVerticalDimension || mWidth > mGLMaxViewportHorizontalDimension) {

Bad indent. Also we need to check min(MaxTextureSize,MaxViewportDims), not just viewport dims.

@@ +388,5 @@
> +	if (!gl || mHeight > mGLMaxViewportVerticalDimension || mWidth > mGLMaxViewportHorizontalDimension) {
> +        // In case there is no context created yet we immediately update the dimensions and
> +        // create the context to let know to the client about the result of creation.
> +        // If the provided size of bigger then available we perform the same thing since setting too
> +        // big dimensions can cause failure on context (re)creation which we should notify about.

It shouldn't, but we do right now. If the size is too big, we are supposed to resize it until it's a reasonable size. We don't do this properly right now, but we should.

We don't have to fix that here. (it's a minefield of driver issues) We do want to write the comment correctly, though.

@@ +398,5 @@
> +
> +    return NS_OK;
> +}
> +
> +bool WebGLContext::UpdateDimensions() {

type
class::name()
{
    code
}