Bug 148020 - GraphicsContext3D::activeTexture should not be called with zero-based index
Summary: GraphicsContext3D::activeTexture should not be called with zero-based index
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jinyoung Hur
URL:
Keywords: InRadar
: 100402 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-08-13 23:20 PDT by Jinyoung Hur
Modified: 2016-03-21 17:11 PDT (History)
12 users (show)

See Also:


Attachments
Patch (2.41 KB, patch)
2015-08-13 23:34 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff
Patch (4.15 KB, patch)
2015-08-19 10:40 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff
Patch (4.19 KB, patch)
2015-08-19 11:00 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (600.68 KB, application/zip)
2015-08-19 11:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (598.59 KB, application/zip)
2015-08-19 12:51 PDT, Build Bot
no flags Details
Patch (5.06 KB, patch)
2015-08-19 15:31 PDT, Jinyoung Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jinyoung Hur 2015-08-13 23:20:57 PDT
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.
Comment 1 Jinyoung Hur 2015-08-13 23:34:43 PDT
Created attachment 258989 [details]
Patch
Comment 2 Alex Christensen 2015-08-17 10:00:16 PDT
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?
Comment 3 Said Abou-Hallawa 2015-08-17 10:01:26 PDT
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.
Comment 4 Jinyoung Hur 2015-08-17 16:57:48 PDT
(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.
Comment 5 Jinyoung Hur 2015-08-17 17:00:22 PDT
(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.
Comment 6 Jinyoung Hur 2015-08-19 10:40:36 PDT
Created attachment 259377 [details]
Patch
Comment 7 Jinyoung Hur 2015-08-19 11:00:16 PDT
Created attachment 259379 [details]
Patch
Comment 8 Build Bot 2015-08-19 11:53:56 PDT
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
Comment 9 Build Bot 2015-08-19 11:54:00 PDT
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
Comment 10 Build Bot 2015-08-19 12:51:08 PDT
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
Comment 11 Build Bot 2015-08-19 12:51:10 PDT
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
Comment 12 Jinyoung Hur 2015-08-19 15:31:47 PDT
Created attachment 259416 [details]
Patch
Comment 13 Alex Christensen 2015-08-19 17:05:15 PDT
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!
Comment 14 Jinyoung Hur 2015-08-19 17:08:03 PDT
(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!
Comment 15 Radar WebKit Bug Importer 2015-08-19 17:19:35 PDT
<rdar://problem/22353988>
Comment 16 WebKit Commit Bot 2015-08-19 17:27:09 PDT
Comment on attachment 259416 [details]
Patch

Clearing flags on attachment: 259416

Committed r188666: <http://trac.webkit.org/changeset/188666>
Comment 17 WebKit Commit Bot 2015-08-19 17:27:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Alex Christensen 2015-08-19 17:35:02 PDT
This fixed conformance/textures/texture-npot.html
Comment 19 Brent Fulgham 2015-08-19 18:03:15 PDT
(In reply to comment #18)
> This fixed conformance/textures/texture-npot.html

Hey! A PROGRESSION!

Nice work Jinyoung!
Comment 20 Jinyoung Hur 2015-08-19 18:04:35 PDT
(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!
Comment 21 Brent Fulgham 2016-03-21 17:11:43 PDT
*** Bug 100402 has been marked as a duplicate of this bug. ***