Closed Bug 879952 Opened 6 years ago Closed 6 years ago

OES_texture_float should not allow linear filtering

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bjacob, Assigned: guillaume.abadie)

References

Details

(Whiteboard: webgl-test-needed)

Attachments

(1 file, 5 obsolete files)

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.
Blocks: 879954
Attached patch patch (obsolete) — Splinter Review
patch fixing that bug
also fix a non reported bug on webgl bindTexture with fake black status
Attachment #759162 - Flags: review?(bjacob)
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.
Attachment #759162 - Flags: review?(bjacob) → review-
Attached patch patch for landing (obsolete) — Splinter Review
enhancing previous patch
Attachment #759162 - Attachment is obsolete: true
Attachment #759722 - Flags: review?(bjacob)
Attached patch patch for final landing (obsolete) — Splinter Review
bug fix
Attachment #759722 - Attachment is obsolete: true
Attachment #759722 - Flags: review?(bjacob)
Attachment #759810 - Flags: review?(bjacob)
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.
Attachment #759810 - Flags: review?(bjacob) → review+
Whiteboard: webgl-test-needed
Please, "patches for landing" should have a ready commit message.
Attached patch patch for final landing (obsolete) — Splinter Review
bug 879952 fix
Attachment #759810 - Attachment is obsolete: true
Attached patch patch for final landing (obsolete) — Splinter Review
patch for landing
Attachment #759819 - Attachment is obsolete: true
Attachment #759826 - Attachment is obsolete: true
Attachment #759827 - Flags: review?(bjacob)
Attachment #759827 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/mozilla-central/rev/97b2763d2bae
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.