Fix state management of transform feedback buffers.

Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

WebGLTransformFeedback was incorrectly implemented as WebGLSharedObject.
Fixed by changing to WebGLContextObject.

BUG=696345
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2773843002
Cr-Commit-Position: refs/heads/master@{#466766}
Committed: https://chromium.googlesource.com/chromium/src/+/772c234787651b285f4bee7d5a714fadcf7c0bd4

[Ken Russell (switch to Gerrit), 2017-03-23 22:08:04]

Description was changed from

==========
Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

BUG=696345
==========

to

==========
Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

BUG=696345
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2017-03-23 22:09:53]

kbr@chromium.org changed reviewers:
+ kainino@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2017-03-23 22:09:53]

zmo, kainino: please review. Thanks.

Tested with the new test in https://github.com/KhronosGroup/WebGL/pull/2345 .

[Kai Ninomiya, 2017-03-23 23:26:34]

LGTM, one nit

https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp (right):

https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp:49: default:
"default:" seems unclear to me. Why not do either 2 explicit cases, or "if
(m_type == TFTypeUser)"?

[Ken Russell (switch to Gerrit), 2017-03-23 23:31:32]

https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp (right):

https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp:49: default:
On 2017/03/23 23:26:34, Kai Ninomiya wrote:
> "default:" seems unclear to me. Why not do either 2 explicit cases, or "if
> (m_type == TFTypeUser)"?

Good point; this was sloppy on my part. I copied this from the VAO code. Done.

[Zhenyao Mo, 2017-03-23 23:54:19]

On 2017/03/23 23:31:32, Ken Russell wrote:
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp
(right):
> 
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp:49:
default:
> On 2017/03/23 23:26:34, Kai Ninomiya wrote:
> > "default:" seems unclear to me. Why not do either 2 explicit cases, or "if
> > (m_type == TFTypeUser)"?
> 
> Good point; this was sloppy on my part. I copied this from the VAO code. Done.

LGTM

[Ken Russell (switch to Gerrit), 2017-03-24 05:02:37]

On 2017/03/23 23:54:19, Zhenyao Mo wrote:
> On 2017/03/23 23:31:32, Ken Russell wrote:
> >
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp:49:
> default:
> > On 2017/03/23 23:26:34, Kai Ninomiya wrote:
> > > "default:" seems unclear to me. Why not do either 2 explicit cases, or "if
> > > (m_type == TFTypeUser)"?
> > 
> > Good point; this was sloppy on my part. I copied this from the VAO code.
Done.
> 
> LGTM

Thanks. Note: I'm holding off landing this while we resolve the issue in
https://github.com/KhronosGroup/WebGL/pull/2345 . The state management is
clearly broken, but the text spec about disallowing buffers from being bound
both to a transform feedback binding point and any other binding point is in
conflict with this, since it allows simultaneous binding to unbound transform
feedback objects as well as e.g. ARRAY_BUFFER.

[Ken Russell (switch to Gerrit), 2017-03-24 18:12:24]

On 2017/03/24 05:02:37, Ken Russell wrote:
> On 2017/03/23 23:54:19, Zhenyao Mo wrote:
> > On 2017/03/23 23:31:32, Ken Russell wrote:
> > >
> >
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> > > File third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> > > third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp:49:
> > default:
> > > On 2017/03/23 23:26:34, Kai Ninomiya wrote:
> > > > "default:" seems unclear to me. Why not do either 2 explicit cases, or
"if
> > > > (m_type == TFTypeUser)"?
> > > 
> > > Good point; this was sloppy on my part. I copied this from the VAO code.
> Done.
> > 
> > LGTM
> 
> Thanks. Note: I'm holding off landing this while we resolve the issue in
> https://github.com/KhronosGroup/WebGL/pull/2345 . The state management is
> clearly broken, but the text spec about disallowing buffers from being bound
> both to a transform feedback binding point and any other binding point is in
> conflict with this, since it allows simultaneous binding to unbound transform
> feedback objects as well as e.g. ARRAY_BUFFER.

Gregg subsequently filed https://github.com/KhronosGroup/WebGL/issues/2346 and
I'm going to work on resolving that. The fix for that will subsume this fix.

[Ken Russell (switch to Gerrit), 2017-04-21 17:00:38]

On 2017/03/24 18:12:24, Ken Russell wrote:
> On 2017/03/24 05:02:37, Ken Russell wrote:
> > On 2017/03/23 23:54:19, Zhenyao Mo wrote:
> > > On 2017/03/23 23:31:32, Ken Russell wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> > > > File third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2773843002/diff/1/third_party/WebKit/Source/m...
> > > > third_party/WebKit/Source/modules/webgl/WebGLTransformFeedback.cpp:49:
> > > default:
> > > > On 2017/03/23 23:26:34, Kai Ninomiya wrote:
> > > > > "default:" seems unclear to me. Why not do either 2 explicit cases, or
> "if
> > > > > (m_type == TFTypeUser)"?
> > > > 
> > > > Good point; this was sloppy on my part. I copied this from the VAO code.
> > Done.
> > > 
> > > LGTM
> > 
> > Thanks. Note: I'm holding off landing this while we resolve the issue in
> > https://github.com/KhronosGroup/WebGL/pull/2345 . The state management is
> > clearly broken, but the text spec about disallowing buffers from being bound
> > both to a transform feedback binding point and any other binding point is in
> > conflict with this, since it allows simultaneous binding to unbound
transform
> > feedback objects as well as e.g. ARRAY_BUFFER.
> 
> Gregg subsequently filed https://github.com/KhronosGroup/WebGL/issues/2346 and
> I'm going to work on resolving that. The fix for that will subsume this fix.

Coming back to this after some other work. After more thought, it'll be too big
a change to both fix this bug in the state management, and also change the rules
about binding buffers to transform feedback and non-transform-feedback binding
points.

I checked Firefox's code and it has the same problem, where binding a transform
feedback object can cause the current rules in the spec to be violated. There
are no tests for this.

CQ'ing this now and going to build on top of it as soon as it lands.

[Ken Russell (switch to Gerrit), 2017-04-21 17:00:57]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2017-04-21 17:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
kainino@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2773843002/#ps40001
(title: "Rebased after Blink renaming.")

[commit-bot: I haz the power, 2017-04-21 17:01:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 17:35:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 17:35:08]

Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[Ken Russell (switch to Gerrit), 2017-04-23 00:13:14]

Description was changed from

==========
Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

BUG=696345
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

WebGLTransformFeedback was incorrectly implemented as WebGLSharedObject.
Fixed by changing to WebGLContextObject.

BUG=696345
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2017-04-23 00:13:29]

The CQ bit was checked by kbr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-23 00:13:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-23 01:46:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-23 01:46:30]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-04-24 19:34:31]

The current patch set implements a good set of improved behavior. CQ'ing it.

[Ken Russell (switch to Gerrit), 2017-04-24 19:34:36]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2017-04-24 19:34:36]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
kainino@chromium.org
Link to the patchset: https://codereview.chromium.org/2773843002/#ps60001
(title: "Implemented proper attach/detach/unbind for WebGLTransformFeedback.")

[commit-bot: I haz the power, 2017-04-24 19:35:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 21:13:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493062476542300,
"parent_rev": "d50542f37d5733bdba90ec0b137127ecf37c8116", "commit_rev":
"772c234787651b285f4bee7d5a714fadcf7c0bd4"}

[commit-bot: I haz the power, 2017-04-24 21:13:59]

Description was changed from

==========
Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

WebGLTransformFeedback was incorrectly implemented as WebGLSharedObject.
Fixed by changing to WebGLContextObject.

BUG=696345
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix state management of transform feedback buffers.

New test being added in https://github.com/KhronosGroup/WebGL/pull/2345 .

WebGLTransformFeedback was incorrectly implemented as WebGLSharedObject.
Fixed by changing to WebGLContextObject.

BUG=696345
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2773843002
Cr-Commit-Position: refs/heads/master@{#466766}
Committed:
https://chromium.googlesource.com/chromium/src/+/772c234787651b285f4bee7d5a71...
==========

[commit-bot: I haz the power, 2017-04-24 21:14:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/772c234787651b285f4bee7d5a71...