Closed Bug 894007 Opened 7 years ago Closed 7 years ago

WebGL regression on conformance test 1.0.2

Categories

(Core :: Canvas: WebGL, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox23 - wontfix
firefox24 --- verified
firefox25 --- verified
firefox-esr17 --- wontfix
b2g18 --- unaffected

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Assignee: nobody → gabadie
Neat. When you have a fix, please make a PR for the webgl github repo.
Attached patch patch revision 1 (obsolete) — Splinter Review
Attachment #776062 - Flags: review?(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)
Attachment #776062 - Flags: review?(jgilbert) → review-
(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.
Guillaume, Jeff, remind me why trunk and aurora don't fail this test with a non-power-of-two max on OSX?
Flags: needinfo?(jgilbert)
Flags: needinfo?(gabadie)
(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.
Flags: needinfo?(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.
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...
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.
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?
(In reply to lsblakk@mozilla.com [: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.
We still have to decide if we are keeping or not the assertion MAX_VIEWPORT_DIMS <= MAX_TEXTURE_SIZE
Flags: needinfo?(gabadie)
Attached patch patch revision 2 (obsolete) — Splinter Review
After discussion on the public webgl mailing list, here is the patch.
https://tbpl.mozilla.org/?tree=Try&rev=57561ca03e38
Attachment #776062 - Attachment is obsolete: true
Attachment #782655 - Flags: review?(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?
Attached patch patch revision 3 (obsolete) — Splinter Review
I feel so sad to let this that way, but here is :
Attachment #782655 - Attachment is obsolete: true
Attachment #782655 - Flags: review?(jgilbert)
Attachment #782768 - Flags: review?(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.
Attachment #782768 - Flags: review?(jgilbert) → review-
Attached patch patch revision 4Splinter Review
Oups I coded to fast. Still thinking that my first solution was the best, but here is the patch :
Attachment #782768 - Attachment is obsolete: true
Attachment #782875 - Flags: review?(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)
Attachment #782875 - Flags: review?(jgilbert) → review+
I did your tip.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda34262d3b8
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/dda34262d3b8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Were you going to request approval to uplift this to Fx24/esr17?
Flags: needinfo?(gabadie)
Attached patch patch auroraSplinter Review
[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.
Attachment #784601 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gabadie)
Attachment #784601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> Were you going to request approval to uplift this to Fx24/esr17?

esr17?
(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.
It will be in ESR24, so wontfixing for ESR17 - it doesn't meet the branch landing criteria anyway.
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).
Please verify this is fixed in Firefox 25.
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.