In theory we don't have to (GL and GLES defines the same behavior), but seems like there is a driver bug in Mac, instead of falling back to (0, 0, 0, 1) texture, it falls back to (0, 0, 0, 0). Thus, we have to do full tracking as in Chrome command buffer implementation. Partial tracking (as in NPOT patch, whcih only deals with the difference between GL and GLES) won't work.
Created attachment 60490 [details] patch
Comment on attachment 60490 [details] patch What is meant by texture completeness? -- there should be some description in the changelog
Attachment 60490 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/html/canvas/WebGLTexture.cpp:200: WebGLTexture::_deleteObject is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > Attachment 60490 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 > WebCore/html/canvas/WebGLTexture.cpp:200: WebGLTexture::_deleteObject is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > Total errors found: 1 in 8 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. I didn't touch the _deleteObject() function, besides, it's in all WebGL object classes. So I won't fix this style issue in this patch.
Can you fix the names in a follow up patch? This is new code. It shouldn't be out of style. (How did it get past the Not the first time?)
(In reply to comment #2) > (From update of attachment 60490 [details]) > What is meant by texture completeness? -- there should be some description in the changelog It's well defined in ES spec 2.0.24 Section 3.7.l0. I could add this reference to the log.
(In reply to comment #4) > (In reply to comment #3) > > Attachment 60490 [details] [details] did not pass style-queue: > > > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 > > WebCore/html/canvas/WebGLTexture.cpp:200: WebGLTexture::_deleteObject is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > > Total errors found: 1 in 8 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > I didn't touch the _deleteObject() function, besides, it's in all WebGL object classes. So I won't fix this style issue in this patch. (In reply to comment #5) > Can you fix the names in a follow up patch? This is new code. It shouldn't be out of style. (How did it get past the Not the first time?) The bug has already been created by kbr: https://bugs.webkit.org/show_bug.cgi?id=38761
Comment on attachment 60490 [details] patch Looks good overall. I didn't compare the logic side-by-side with the command buffer implementation so my review is relying on the fact that this is more or less a straight port. If this test isn't part of the conformance suite, it should be added. One comment: WebCore/html/canvas/WebGLTexture.cpp:162 + height = std::max(1, height >> 1); These look wrong. Should these be "width >> level" and "height >> level"? (Actually, I just checked the command buffer code and the same issue is there.) If so, and these errors weren't caught by a test, can we add a test for them?
Comment on attachment 60490 [details] patch WebCore/html/canvas/WebGLTexture.cpp:162 + height = std::max(1, height >> 1); From what I can see, the same error is here (and in the command buffer code as well).
Comment on attachment 60490 [details] patch WebCore/html/canvas/WebGLTexture.cpp:291 + height = std::max(1, height >> 1); Oops; I meant here.
(In reply to comment #8) > (From update of attachment 60490 [details]) > Looks good overall. I didn't compare the logic side-by-side with the command buffer implementation so my review is relying on the fact that this is more or less a straight port. If this test isn't part of the conformance suite, it should be added. One comment: > > WebCore/html/canvas/WebGLTexture.cpp:162 > + height = std::max(1, height >> 1); > These look wrong. Should these be "width >> level" and "height >> level"? (Actually, I just checked the command buffer code and the same issue is there.) If so, and these errors weren't caught by a test, can we add a test for them? Actually the original code is correct. This is because we update the width/height at each level, so at level k+1, the width/height is already the width/height for level k, not level 0, so shifting 1 bit right is the correct thing to do.
(In reply to comment #11) > (In reply to comment #8) > > (From update of attachment 60490 [details] [details]) > > Looks good overall. I didn't compare the logic side-by-side with the command buffer implementation so my review is relying on the fact that this is more or less a straight port. If this test isn't part of the conformance suite, it should be added. One comment: > > > > WebCore/html/canvas/WebGLTexture.cpp:162 > > + height = std::max(1, height >> 1); > > These look wrong. Should these be "width >> level" and "height >> level"? (Actually, I just checked the command buffer code and the same issue is there.) If so, and these errors weren't caught by a test, can we add a test for them? > > Actually the original code is correct. This is because we update the width/height at each level, so at level k+1, the width/height is already the width/height for level k, not level 0, so shifting 1 bit right is the correct thing to do. I see. Sorry for not realizing that. Looks good to me.
Comment on attachment 60490 [details] patch WebCore/html/canvas/WebGLRenderingContext.cpp:1072 + RefPtr<WebGLTexture> tex = 0; nit: no need to explicitly initialize a RefPtr. the default ctor initializes it to a null value. WebCore/html/canvas/WebGLTexture.h:63 + static bool isNPOT(unsigned, unsigned); note: the NPOT acronym was a mystery to me. webkit encourages minimizing the use of acronyms unless they are commonly known or pertaining to the name of a protocol, etc. That said, "site:khronos.org NPOT" told me what I need to know. isNotPowerOfTwo would probably be a better function name. R=me
(In reply to comment #13) > (From update of attachment 60490 [details]) > WebCore/html/canvas/WebGLRenderingContext.cpp:1072 > + RefPtr<WebGLTexture> tex = 0; > nit: no need to explicitly initialize a RefPtr. the default ctor > initializes it to a null value. Will fix in the landing patch. > > WebCore/html/canvas/WebGLTexture.h:63 > + static bool isNPOT(unsigned, unsigned); > note: the NPOT acronym was a mystery to me. webkit encourages > minimizing the use of acronyms unless they are commonly known > or pertaining to the name of a protocol, etc. That said, > "site:khronos.org NPOT" told me what I need to know. > isNotPowerOfTwo would probably be a better function name. I'll add a comment in the function declaration to explain NPOT. > > R=me
Committed r63253: <http://trac.webkit.org/changeset/63253>