879952 - OES_texture_float should not allow linear filtering

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Read this email from Gregg:

  https://www.khronos.org/webgl/public-mailing-list/archives/1305/msg00022.html

Let's do the "do not let OES_texture_float allow linear filtering" part in this bug, and let's implement the new _linear extensions in a separate bug.

Comment 1

[Guillaume Abadie]

Attachment: patch

patch fixing that bug
also fix a non reported bug on webgl bindTexture with fake black status

Comment 2

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

Comment on attachment 759162 [details] [diff] [review]
patch

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +229,5 @@
> +        SetDontKnowIfNeedFakeBlack();
> +        MakeContextCurrent();
> +
> +        gl->fBindTexture(target, 0 /* == texturename */);
> +        return;

Ouch! This breaks in the case where tex is null. See how, before this patch, we then set mBound2DTextures[mActiveTexture] to tex, even if tex is null, and with this patch, we no longer do, because we return here.

In the WebGL implementation, we forward state changes to the underlying GL (the above glBindTexture call) and we also maintain our own copy of part of the state (see the below  mBound2DTextures[mActiveTexture] = tex; ). It is very important to make sure that we always perform both these two operations together, or else we get a mismatch between actual OpenGL state and our own data structures, which has been a major cause of bugs in the past ;-)

::: content/canvas/src/WebGLTexture.cpp
@@ +328,5 @@
>              }
>          }
>  
> +        if (ImageInfoAt(0).mType == LOCAL_GL_FLOAT)
> +        {

Yeah, notice that here (in this function) we are not strictly following Gecko coding style because it's awkward with these big nested if()'s.

@@ +329,5 @@
>          }
>  
> +        if (ImageInfoAt(0).mType == LOCAL_GL_FLOAT)
> +        {
> +            if (mMinFilter == LOCAL_GL_LINEAR || mMinFilter == LOCAL_GL_LINEAR_MIPMAP_LINEAR || mMinFilter == LOCAL_GL_LINEAR_MIPMAP_NEAREST)

Please write this long if condition on multiple lines.

@@ +335,5 @@
> +                mContext->GenerateWarning("%s is a texture with a linear minification filter "
> +                                          "that is not compatible with gl.FLOAT", msg_rendering_as_black);
> +                mFakeBlackStatus = DoNeedFakeBlack;
> +            }
> +            else if (mMinFilter == LOCAL_GL_NEAREST_MIPMAP_LINEAR)

Just merge this with the above if()... we don't need a separate error message for this one.

Comment 3

[Guillaume Abadie]

Attachment: patch for landing

enhancing previous patch

Comment 4

[Guillaume Abadie]

Attachment: patch for final landing

bug fix

Comment 5

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

Comment on attachment 759810 [details] [diff] [review]
patch for final landing

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +231,5 @@
>      } else {
>          return ErrorInvalidEnumInfo("bindTexture: target", target);
>      }
>  
> +    SetDontKnowIfNeedFakeBlack();

Very good catch! Please, make a mental note to write a conformance test for this soon.

Comment 6

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

Please, "patches for landing" should have a ready commit message.

Comment 7

[Guillaume Abadie]

Attachment: patch for final landing

bug 879952 fix

Comment 8

[Guillaume Abadie]

Attachment: patch for final landing

patch for landing

Comment 9

[Guillaume Abadie]

Attachment: patch for final landing

Comment 10

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/97b2763d2bae

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97b2763d2bae