Last Comment Bug 879952 - OES_texture_float should not allow linear filtering
: OES_texture_float should not allow linear filtering
Status: RESOLVED FIXED
webgl-test-needed
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Guillaume Abadie
:
Mentors:
Depends on:
Blocks: 879954
  Show dependency treegraph
 
Reported: 2013-06-05 13:11 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2013-06-11 14:47 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.44 KB, patch)
2013-06-06 08:45 PDT, Guillaume Abadie
jacob.benoit.1: review-
Details | Diff | Review
patch for landing (5.23 KB, patch)
2013-06-07 06:09 PDT, Guillaume Abadie
no flags Details | Diff | Review
patch for final landing (4.38 KB, patch)
2013-06-07 10:26 PDT, Guillaume Abadie
jacob.benoit.1: review+
Details | Diff | Review
patch for final landing (4.38 KB, patch)
2013-06-07 10:51 PDT, Guillaume Abadie
no flags Details | Diff | Review
patch for final landing (4.42 KB, patch)
2013-06-07 10:58 PDT, Guillaume Abadie
no flags Details | Diff | Review
patch for final landing (4.42 KB, patch)
2013-06-07 10:59 PDT, Guillaume Abadie
jacob.benoit.1: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2013-06-05 13:11:31 PDT
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 2013-06-06 08:45:06 PDT
Created attachment 759162 [details] [diff] [review]
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) 2013-06-06 19:29:31 PDT
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 2013-06-07 06:09:12 PDT
Created attachment 759722 [details] [diff] [review]
patch for landing

enhancing previous patch
Comment 4 Guillaume Abadie 2013-06-07 10:26:55 PDT
Created attachment 759810 [details] [diff] [review]
patch for final landing

bug fix
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2013-06-07 10:38:51 PDT
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) 2013-06-07 10:40:03 PDT
Please, "patches for landing" should have a ready commit message.
Comment 7 Guillaume Abadie 2013-06-07 10:51:39 PDT
Created attachment 759819 [details] [diff] [review]
patch for final landing

bug 879952 fix
Comment 8 Guillaume Abadie 2013-06-07 10:58:52 PDT
Created attachment 759826 [details] [diff] [review]
patch for final landing

patch for landing
Comment 9 Guillaume Abadie 2013-06-07 10:59:31 PDT
Created attachment 759827 [details] [diff] [review]
patch for final landing
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2013-06-10 13:49:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/97b2763d2bae
Comment 11 Ed Morley [:emorley] 2013-06-11 01:44:25 PDT
https://hg.mozilla.org/mozilla-central/rev/97b2763d2bae

Note You need to log in before you can comment on or make changes to this bug.