Bug 40175 – Bring framebuffer functions to GLES2 conformance

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

1) target has to be FRAMEBUFFER
2) attachment has to be COLOR0, DEPTH, STENCIL, or DEPTH_STENCIL
3) level in framebufferTexImage2D has to be 0
4) has to check whether a user-created framebuffer is bound (otherwise the internal framebuffer will be wrongly used).
   4.1) For checkFramebufferStatus(), return FRAMEBUFFER_COMPLETE if no user-created framebuffer is bound.
   4.2) For getFramebufferAttachmentParameter, return default values if no user-created framebuffer is bound.

[Zhenyao Mo]

A mistake. For 4.2, should generate INVALID_OPERATION and return 0.

[Zhenyao Mo]

Created attachment 57926 [details]
patch

[Kenneth Russell]

Comment on attachment 57926 [details]
patch

Looks good to me. Upon first glance it looked like some of the checks might be redundant, but it turns out that OpenGL 3.3 accepts more enums for calls like glCheckFramebufferStatus than OpenGL ES 2.0 does, so all of the checks you've added are needed. The test case looks good.

[Dimitri Glazkov (Google)]

Comment on attachment 57926 [details]
patch

rs=me.

[WebKit Commit Bot]

Comment on attachment 57926 [details]
patch

Rejecting patch 57926 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
ing Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19071 test cases.
fast/dom/Window/window-property-descriptors.html -> failed

Exiting early after 1 failures. 6789 tests run.
130.08s total testing time

6788 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3175098

[WebKit Commit Bot]

Comment on attachment 57926 [details]
patch

Rejecting patch 57926 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 57926 [details]
patch

Rejecting patch 57926 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
ing Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19072 test cases.
fast/dom/Window/window-property-descriptors.html -> failed

Exiting early after 1 failures. 6790 tests run.
129.73s total testing time

6789 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3195102

[WebKit Commit Bot]

Comment on attachment 57926 [details]
patch

Rejecting patch 57926 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
ing Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19079 test cases.
fast/dom/Window/window-property-descriptors.html -> failed

Exiting early after 1 failures. 6791 tests run.
127.44s total testing time

6790 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3226234

[Zhenyao Mo]

One more try failed.  I looked at the patch again.  The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js.

Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try.

The only problem I see is that we have a few other WebGL conformance tests that we might want to add to LayoutTests, and they also use assertMsg, so they also need to be rewritten.

[Kenneth Russell]

(In reply to comment #9)
> One more try failed.  I looked at the patch again.  The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js.
> 
> Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try.

Did you run this test on your machine? If it is a real failure then it will reproduce there.

run-webkit-tests [--debug] LayoutTests/fast/dom/Window/window-property-descriptors.html

[Kenneth Russell]

(In reply to comment #10)
> (In reply to comment #9)
> > One more try failed.  I looked at the patch again.  The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js.
> > 
> > Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try.
> 
> Did you run this test on your machine? If it is a real failure then it will reproduce there.
> 
> run-webkit-tests [--debug] LayoutTests/fast/dom/Window/window-property-descriptors.html

I just applied your patch and found that this test does in fact fail with your patch applied. Run it on your own machine to see the error reported. You should consider doing full LayoutTest runs ("run-webkit-tests [--debug]") for changes that touch non-WebGL code.

[Zhenyao Mo]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > One more try failed.  I looked at the patch again.  The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js.
> > > 
> > > Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try.
> > 
> > Did you run this test on your machine? If it is a real failure then it will reproduce there.
> > 
> > run-webkit-tests [--debug] LayoutTests/fast/dom/Window/window-property-descriptors.html
> 
> I just applied your patch and found that this test does in fact fail with your patch applied. Run it on your own machine to see the error reported. You should consider doing full LayoutTest runs ("run-webkit-tests [--debug]") for changes that touch non-WebGL code.

OK, I could reproduce it by run-webkit-tests.  Originally I tested with run-safari and open the failed test and saw no FAIL, so I thought I could not reproduce the error.  Actually because I added a new function in js-test-pre.js, the expected txt will have one extra PASS line, that's why it failed.

[Zhenyao Mo]

Created attachment 58497 [details]
revised patch

Moved assertMsg() to webgl-test.js since it's only used by WebGL.  The rest is the same.

[Kenneth Russell]

Looks fine.

[Dimitri Glazkov (Google)]

Comment on attachment 58497 [details]
revised patch

let's try again.

[WebKit Commit Bot]

Comment on attachment 58497 [details]
revised patch

Clearing flags on attachment: 58497

Committed r61185: <http://trac.webkit.org/changeset/61185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61185 might have broken SnowLeopard Intel Release (Tests)

[Csaba Osztrogonác]

(In reply to comment #18)
> http://trac.webkit.org/changeset/61185 might have broken SnowLeopard Intel Release (Tests)

pretty diff here: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r61186%20%2811683%29/fast/canvas/webgl/texture-npot-pretty-diff.html