Bug 41381 – Need to track texture completeness

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

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.

[Zhenyao Mo]

Created attachment 60490 [details]
patch

[Oliver Hunt]

Comment on attachment 60490 [details]
patch

What is meant by texture completeness? -- there should be some description in the changelog

[WebKit Review Bot]

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.

[Zhenyao Mo]

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

[Adam Barth]

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?)

[Zhenyao Mo]

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

[Zhenyao Mo]

(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

[Kenneth Russell]

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?

[Kenneth Russell]

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

[Kenneth Russell]

Comment on attachment 60490 [details]
patch

WebCore/html/canvas/WebGLTexture.cpp:291
 +                  height = std::max(1, height >> 1);
Oops; I meant here.

[Zhenyao Mo]

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

[Kenneth Russell]

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

[Darin Fisher (:fishd, Google)]

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

[Zhenyao Mo]

(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

[Zhenyao Mo]

Committed r63253: <http://trac.webkit.org/changeset/63253>