GraphicsContext3D::activeTexture should not be called with zero-based index

• Component: WebGL
• Status: RESOLVED FIXED

[Jinyoung Hur]

There are some misusage of GraphicsContext3D::activeTexture in WebGLRenderingContextBase::checkTextureCompleteness.
GraphicsContext3D::activeTexture is expected to be used with an argument that is greater than or equal to GraphicsContext3D::TEXTURE0.
However, WebGLRenderingContextBase::checkTextureCompleteness calls GraphicsContext3D::activeTexture with a zero-based index.

[Jinyoung Hur]

Created attachment 258989 [details]
Patch

[Alex Christensen]

Comment on attachment 258989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258989&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests because there is no behavior change.

If there is no behavior change, then what does this fix?

[Said Abou-Hallawa]

Comment on attachment 258989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258989&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests because there is no behavior change.

There should be a behavior change since there is a bug in passing the wrong parameter value to GraphicsContext3D::activeTexture(). Can not we have a layout test or even a test case which shows the rendering difference between before and after the fix?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4064
> +        m_context->activeTexture(m_activeTextureUnit + GraphicsContext3D::TEXTURE0);

Yes this looks a correct fix. This can be verified from https://www.khronos.org/opengles/sdk/docs/man/xhtml/glActiveTexture.xml and from WebGLRenderingContextBase::activeTexture() in which we set m_activeTextureUnit = texture - GraphicsContext3D::TEXTURE0.

[Jinyoung Hur]

(In reply to comment #2)
> Comment on attachment 258989 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258989&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests because there is no behavior change.
> 
> If there is no behavior change, then what does this fix?

You're right. I'll revise my patch.

[Jinyoung Hur]

(In reply to comment #3)
> Comment on attachment 258989 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258989&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests because there is no behavior change.
> 
> There should be a behavior change since there is a bug in passing the wrong
> parameter value to GraphicsContext3D::activeTexture(). Can not we have a
> layout test or even a test case which shows the rendering difference between
> before and after the fix?
> 
> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4064
> > +        m_context->activeTexture(m_activeTextureUnit + GraphicsContext3D::TEXTURE0);
> 
> Yes this looks a correct fix. This can be verified from
> https://www.khronos.org/opengles/sdk/docs/man/xhtml/glActiveTexture.xml and
> from WebGLRenderingContextBase::activeTexture() in which we set
> m_activeTextureUnit = texture - GraphicsContext3D::TEXTURE0.

Thank you for review.
I'll try to include a test case that shows the behavior difference.

[Jinyoung Hur]

Created attachment 259377 [details]
Patch

[Jinyoung Hur]

Created attachment 259379 [details]
Patch

[Build Bot]

Comment on attachment 259379 [details]
Patch

Attachment 259379 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/76152

New failing tests:
fast/canvas/webgl/texture-complete.html

[Build Bot]

Created attachment 259385 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259379 [details]
Patch

Attachment 259379 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/76302

New failing tests:
fast/canvas/webgl/texture-complete.html

[Build Bot]

Created attachment 259390 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Jinyoung Hur]

Created attachment 259416 [details]
Patch

[Alex Christensen]

Comment on attachment 259416 [details]
Patch

I confirmed that the new test fails in tip-of-tree webkit, passes in Chrome and Firefox, and is the correct behavior.  Thanks!

[Jinyoung Hur]

(In reply to comment #13)
> Comment on attachment 259416 [details]
> Patch
> 
> I confirmed that the new test fails in tip-of-tree webkit, passes in Chrome
> and Firefox, and is the correct behavior.  Thanks!

Nice!

[Radar WebKit Bug Importer]

<rdar://problem/22353988>

[WebKit Commit Bot]

Comment on attachment 259416 [details]
Patch

Clearing flags on attachment: 259416

Committed r188666: <http://trac.webkit.org/changeset/188666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

This fixed conformance/textures/texture-npot.html

[Brent Fulgham]

(In reply to comment #18)
> This fixed conformance/textures/texture-npot.html

Hey! A PROGRESSION!

Nice work Jinyoung!

[Jinyoung Hur]

(In reply to comment #19)
> (In reply to comment #18)
> > This fixed conformance/textures/texture-npot.html
> 
> Hey! A PROGRESSION!
> 
> Nice work Jinyoung!

Wow! Thanks!

[Brent Fulgham]

*** Bug 100402 has been marked as a duplicate of this bug. ***