Closed
Bug 879952
Opened 11 years ago
Closed 11 years ago
OES_texture_float should not allow linear filtering
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bjacob, Assigned: guillaume.abadie)
References
Details
(Whiteboard: webgl-test-needed)
Attachments
(1 file, 5 obsolete files)
4.42 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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•11 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•11 years ago
|
||
enhancing previous patch
Attachment #759162 -
Attachment is obsolete: true
Attachment #759722 -
Flags: review?(bjacob)
Assignee | ||
Comment 4•11 years ago
|
||
bug fix
Attachment #759722 -
Attachment is obsolete: true
Attachment #759722 -
Flags: review?(bjacob)
Attachment #759810 -
Flags: review?(bjacob)
Reporter | ||
Comment 5•11 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•11 years ago
|
Whiteboard: webgl-test-needed
Reporter | ||
Comment 6•11 years ago
|
||
Please, "patches for landing" should have a ready commit message.
Assignee | ||
Comment 8•11 years ago
|
||
patch for landing
Attachment #759819 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #759826 -
Attachment is obsolete: true
Attachment #759827 -
Flags: review?(bjacob)
Reporter | ||
Updated•11 years ago
|
Attachment #759827 -
Flags: review?(bjacob) → review+
Reporter | ||
Comment 10•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/97b2763d2bae
Target Milestone: --- → mozilla24
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/97b2763d2bae
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•