894007 - WebGL regression on conformance test 1.0.2

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

This WebGL conformance test 1.0.2 fail https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/limits/gl-max-texture-dimensions.html because GLContext::GetIntegerv can return a non power of two value for MAX_TEXTURE_SIZE.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Neat. When you have a fix, please make a PR for the webgl github repo.

Comment 2

[Guillaume Abadie]

Attachment: patch revision 1

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776062 [details] [diff] [review]
patch revision 1

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

::: content/canvas/src/WebGLContextValidate.cpp
@@ +873,5 @@
>  }
>  
> +static int32_t nearestPowerOfTwo(int32_t x)
> +{
> +    return int32_t(pow(2.0, floor(log(double(x))/log(2.0))));

Correct in theory, but I'd rather have an int-only solution to this.
Evidently this is Coming Soon(tm) to mfbt/MathAlgo.h, but let's not hold ourselves back on that. Just steal the POT func from gfxUtils for now, which is int-only.

@@ +966,5 @@
>  
> +    mGLMaxTextureSize = nearestPowerOfTwo(mGLMaxTextureSize);
> +    mGLMaxRenderbufferSize = nearestPowerOfTwo(mGLMaxRenderbufferSize);
> +    mGLMaxViewportDims[0] = std::min(mGLMaxViewportDims[0], std::max(mGLMaxTextureSize, mGLMaxRenderbufferSize));
> +    mGLMaxViewportDims[1] = std::min(mGLMaxViewportDims[1], std::max(mGLMaxTextureSize, mGLMaxRenderbufferSize));

I don't think we should have this limitation. MAX_VIEWPORT_DIMS is a hard limit on the size of the viewport, whereas MAX_TEXTURE_SIZE is a softer limit, and strictly-speaking only gives a hint about the largest square texture that the driver is willing to handle. (under my latest reading of the spec, at least)

Comment 4

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> Comment on attachment 776062 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 776062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +873,5 @@
> >  }
> >  
> > +static int32_t nearestPowerOfTwo(int32_t x)
> > +{
> > +    return int32_t(pow(2.0, floor(log(double(x))/log(2.0))));
> 
> Correct in theory, but I'd rather have an int-only solution to this.
> Evidently this is Coming Soon(tm) to mfbt/MathAlgo.h, but let's not hold
> ourselves back on that. Just steal the POT func from gfxUtils for now, which
> is int-only.
Noted
> 
> @@ +966,5 @@
> >  
> > +    mGLMaxTextureSize = nearestPowerOfTwo(mGLMaxTextureSize);
> > +    mGLMaxRenderbufferSize = nearestPowerOfTwo(mGLMaxRenderbufferSize);
> > +    mGLMaxViewportDims[0] = std::min(mGLMaxViewportDims[0], std::max(mGLMaxTextureSize, mGLMaxRenderbufferSize));
> > +    mGLMaxViewportDims[1] = std::min(mGLMaxViewportDims[1], std::max(mGLMaxTextureSize, mGLMaxRenderbufferSize));
> 
> I don't think we should have this limitation. MAX_VIEWPORT_DIMS is a hard
> limit on the size of the viewport, whereas MAX_TEXTURE_SIZE is a softer
> limit, and strictly-speaking only gives a hint about the largest square
> texture that the driver is willing to handle. (under my latest reading of
> the spec, at least)
I disagree.
Assuming MAX = max(MAX_TEXTURE_SIZE, NAX_RENDERBUFFER_SIZE) : The assertion MAX_VIEWPORT_DIMS <= MAX is not a limitation, because : 
 - if MAX_VIEWPORT_DIMS > MAX, therefore we can set a gl.viewport(0, 0, MAX, MAX) a draw once the entire frame. A even bigger viewport would be useless because it would be bigger than the rendering size, therefore samples outside the framebuffer/texture would be just ignored.
 - if MAX_VIEWPORT_DIMS == MAX, therefore we can also set a gl.viewport(0, 0, MAX, MAX) a draw once the entire frame. No limitation
 - if MAX_VIEWPORT_DIMS < MAX, therefore we can't set a gl.viewport(0, 0, MAX, MAX) and need to draw by several pass because of the driver limitation.
Therefore this assertion doesn't bring any limitations.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Guillaume, Jeff, remind me why trunk and aurora don't fail this test with a non-power-of-two max on OSX?

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #4)
> I disagree.
> Assuming MAX = max(MAX_TEXTURE_SIZE, NAX_RENDERBUFFER_SIZE) : The assertion
> MAX_VIEWPORT_DIMS <= MAX is not a limitation, because : 
>  - if MAX_VIEWPORT_DIMS > MAX, therefore we can set a gl.viewport(0, 0, MAX,
> MAX) a draw once the entire frame. A even bigger viewport would be useless
> because it would be bigger than the rendering size, therefore samples
> outside the framebuffer/texture would be just ignored.
>  - if MAX_VIEWPORT_DIMS == MAX, therefore we can also set a gl.viewport(0,
> 0, MAX, MAX) a draw once the entire frame. No limitation
>  - if MAX_VIEWPORT_DIMS < MAX, therefore we can't set a gl.viewport(0, 0,
> MAX, MAX) and need to draw by several pass because of the driver limitation.
> Therefore this assertion doesn't bring any limitations.

My current reading of the spec is that MAX_TEXTURE_SIZE is only the largest size the driver guarantees that it won't emit INVALID_VALUE for, not the largest size that it will allow. Thus, with a 4k MAX_TEXTURE_SIZE, we might still succeed at allocation an 8k*1k texture, though it's not guaranteed. If we're given an 8k*1k texture, we'd want MAX_VIEWPORT_DIMS to at least support an 8k horizontal. Capping MAX_VIEWPORT_DIMS to MAX_TEXTURE_SIZE would disallow this.

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> Guillaume, Jeff, remind me why trunk and aurora don't fail this test with a
> non-power-of-two max on OSX?

We use the 1.0.1 conformance suite on all branches so far.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Hmm.  Trying to figure out how 8k-1 max landed fine on central and aurora, but got bounced out of beta with the not-power-of-two error...

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Doing a quick diff between beta's and central's version of content/canvas/test/webgl/test_webgl_conformance_test_suite.html shows significant differences between the two.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

This test is blocking the uplift of the work in bug 879656 to FF23. We're already going to build with our first beta of the week today, our second-to-last Beta is going to build on Thursday - what is the status here and will this be ready in the next couple of days?

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to [e-mail] [:lsblakk] from comment #10)
> This test is blocking the uplift of the work in bug 879656 to FF23. We're
> already going to build with our first beta of the week today, our
> second-to-last Beta is going to build on Thursday - what is the status here
> and will this be ready in the next couple of days?

Yes, the fix for that bug no longer requires this bug.

Comment 12

[Guillaume Abadie]

We still have to decide if we are keeping or not the assertion MAX_VIEWPORT_DIMS <= MAX_TEXTURE_SIZE

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Untracking based on comment 11

Comment 14

[Guillaume Abadie]

Attachment: patch revision 2

After discussion on the public webgl mailing list, here is the patch.
https://tbpl.mozilla.org/?tree=Try&rev=57561ca03e38

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 782655 [details] [diff] [review]
patch revision 2

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

::: content/canvas/src/WebGLContextValidate.cpp
@@ +888,5 @@
> +    x |= (x >> 2);
> +    x |= (x >> 4);
> +    x |= (x >> 8);
> +    x |= (x >> 16);
> +    return (x + 1) / 2;

This is far more tricky than we need. Can we just use a while loop instead?

Comment 16

[Guillaume Abadie]

Attachment: patch revision 3

I feel so sad to let this that way, but here is :

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 782768 [details] [diff] [review]
patch revision 3

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

::: content/canvas/src/WebGLContextValidate.cpp
@@ +885,5 @@
> +{
> +    MOZ_ASSERT(x > 0);
> +    int32_t POT = 1;
> +    while (POT * 2 <= x) {
> +        POT *= 2;

This looks like it will break for x=INT_MAX, or more generally, and x>=0x40000000.

Comment 18

[Guillaume Abadie]

Attachment: patch revision 4

Oups I coded to fast. Still thinking that my first solution was the best, but here is the patch :

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 782875 [details] [diff] [review]
patch revision 4

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

R+, but I think it's cleaner as:
while (pot < 0x40000000) {
    if (x < pot*2)
        break;
    pot *= 2;
}
(breaking up the conditionals, since they have different purposes)

Comment 20

[Guillaume Abadie]

I did your tip.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda34262d3b8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Were you going to request approval to uplift this to Fx24/esr17?

Comment 23

[Guillaume Abadie]

Attachment: patch aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): security bug 879656
User impact if declined: WebGL MAX_TEXTURE_SIZE must be a power of two
Testing completed (on m-c, etc.): Landed (revision dda34262d3b8)
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: No.

Comment 24

[Guillaume Abadie]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7203c65864c2

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> Were you going to request approval to uplift this to Fx24/esr17?

esr17?

Comment 26

[Guillaume Abadie]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> > Were you going to request approval to uplift this to Fx24/esr17?
> 
> esr17?
Oups sorry... I have forgotten to say that I tried to apply the patch on esr17, but the WebGL code is old. It can't be applied. Even we would like to apply it, we would have to changes to many things.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

It will be in ESR24, so wontfixing for ESR17 - it doesn't meet the branch landing criteria anyway.

Comment 28

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0

Verified fixed on Firefox 24 beta 10 (buildID: 20130909203154) and latest Nightly (buildID: 20130909030204).

Comment 29

[u279076]

Please verify this is fixed in Firefox 25.

Comment 30

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0


Also verified fixed on Firefox 25.0a2 (buildID: 20130910004002).