The default bug view has changed. See this FAQ.

OES_texture_float should not allow linear filtering

RESOLVED FIXED in mozilla24

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: Guillaume Abadie)

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-test-needed)

Attachments

(1 attachment, 5 obsolete attachments)

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.
(Reporter)

Updated

4 years ago
Blocks: 879954
(Assignee)

Comment 1

4 years ago
Created attachment 759162 [details] [diff] [review]
patch

patch fixing that bug
also fix a non reported bug on webgl bindTexture with fake black status
Attachment #759162 - Flags: review?(bjacob)
(Reporter)

Comment 2

4 years ago
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-
(Assignee)

Comment 3

4 years ago
Created attachment 759722 [details] [diff] [review]
patch for landing

enhancing previous patch
Attachment #759162 - Attachment is obsolete: true
Attachment #759722 - Flags: review?(bjacob)
(Assignee)

Comment 4

4 years ago
Created attachment 759810 [details] [diff] [review]
patch for final landing

bug fix
Attachment #759722 - Attachment is obsolete: true
Attachment #759722 - Flags: review?(bjacob)
Attachment #759810 - Flags: review?(bjacob)
(Reporter)

Comment 5

4 years ago
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+
(Reporter)

Updated

4 years ago
Whiteboard: webgl-test-needed
(Reporter)

Comment 6

4 years ago
Please, "patches for landing" should have a ready commit message.
(Assignee)

Comment 7

4 years ago
Created attachment 759819 [details] [diff] [review]
patch for final landing

bug 879952 fix
Attachment #759810 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 759826 [details] [diff] [review]
patch for final landing

patch for landing
Attachment #759819 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 759827 [details] [diff] [review]
patch for final landing
Attachment #759826 - Attachment is obsolete: true
Attachment #759827 - Flags: review?(bjacob)
(Reporter)

Updated

4 years ago
Attachment #759827 - Flags: review?(bjacob) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/97b2763d2bae
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/97b2763d2bae
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.