Implement TransformFeedbackManager in GPU command buffer.

Implement TransformFeedbackManager in GPU command buffer.

A few design points:
1) create a "default" transform feedback for each context (instead of using
   the default 0). This will make context switching easier.
2) Emulate BindBufferRange behaviors on GL 4.1 or lower by tracking indexed
   buffer bindings.

WHat's left to be done:
1) Intercept GetIntegeri_v and GetIntegeri64_v on GL 4.1 or lower (due to the
   emulation)
2) Hook up indexed buffer bindings to Program (for uniform buffer bindings)

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6c468baf6600fe4846e73203ce2e83398ee73813
Cr-Commit-Position: refs/heads/master@{#391605}

[Zhenyao Mo, 2016-04-25 18:10:35]

Description was changed from

==========
Implement TransformFeedbackManager in GPU command buffer.

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
==========

to

==========
Implement TransformFeedbackManager in GPU command buffer.

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-04-25 20:13:57]

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

[commit-bot: I haz the power, 2016-04-25 20:14:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/20001

[commit-bot: I haz the power, 2016-04-25 20:35:06]

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

[commit-bot: I haz the power, 2016-04-25 20:35:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhenyao Mo, 2016-04-25 21:06:24]

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

[commit-bot: I haz the power, 2016-04-25 21:06:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/10017
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/10017

[Zhenyao Mo, 2016-04-25 22:43:33]

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

[commit-bot: I haz the power, 2016-04-25 22:43:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/50001

[Zhenyao Mo, 2016-04-26 00:25:31]

Patchset #3 (id:10017) has been deleted

[Zhenyao Mo, 2016-04-26 00:25:42]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2016-04-26 00:25:54]

Patchset #1 (id:20001) has been deleted

[Zhenyao Mo, 2016-04-26 23:36:12]

Description was changed from

==========
Implement TransformFeedbackManager in GPU command buffer.

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Implement TransformFeedbackManager in GPU command buffer.

A few design points:
1) create a "default" transform feedback for each context (instead of using
   the default 0). This will make context switching easier.
2) Emulate BindBufferRange behaviors on GL 4.1 or lower by tracking indexed
   buffer bindings.

WHat's left to be done:
1) Intercept GetIntegeri_v and GetIntegeri64_v on GL 4.1 or lower (due to the
   emulation)
2) Hook up indexed buffer bindings to Program (for uniform buffer bindings)

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-04-26 23:41:39]

piman: PTAL (sorry about the size of this CL, but it's hard to break down
further)

Others: FYI (feel free to review)
qiankun: specific apologies. you asked a while back for implementing the
transformfeedback manager in command buffer and I told you not to because we are
migrating to MANGLE. I was really hoping we can get by through just passing down
the call to the driver. Unfortunately, there is a driver behavior difference in
BindBufferRange between Desktop GL 4.1 and 4.2, and in order to emulate the ES3
behavior, we will have to implement TransformFeedbackManager and indexed buffer
binding tracking.

[Zhenyao Mo, 2016-04-26 23:41:46]

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

[commit-bot: I haz the power, 2016-04-26 23:42:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/90001

[Zhenyao Mo, 2016-04-26 23:43:51]

In theory this CL should also remove the flakiness of
deqp/functional/gles3/negativeshaderapi.html on Linux, which is caused by
virtual gl contexts implementation calls glUseProgram while a transform feedback
is active, found by @cwallez.

[commit-bot: I haz the power, 2016-04-26 23:52:52]

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

[commit-bot: I haz the power, 2016-04-26 23:52:53]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Zhenyao Mo, 2016-04-26 23:58:16]

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

[commit-bot: I haz the power, 2016-04-26 23:58:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/110001

[commit-bot: I haz the power, 2016-04-27 01:02:32]

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

[commit-bot: I haz the power, 2016-04-27 01:02:33]

Dry run: This issue passed the CQ dry run.

[piman, 2016-04-28 01:38:48]

The spec calls out a few other things:
- DrawArrays/DrawArraysInstanced should raise GL_INVALID_OPERATION while
feedback is active and either it would overflow the buffers or the feedback mode
doesn't match the primitive mode
- DrawElements/DrawElementsInstanced/DrawRangeElements should raise
GL_INVALID_OPERATION is feedback is active (regardless of mode)
- BeginTransformFeedback should raise GL_INVALID_OPERATION if the necessary
buffers aren't bound
- GL_INVALID_OPERATION is raised if the bound program changes (e.g. glUseProgram
or link) while feedback is active, including on Resume if this happened while
the feedback was swapped out into an inactive one.

I think at least some of these would not be generated by the desktop GL driver -
in case we wanted to rely on that - so I think it needs to be done explicitly.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:282:
std::unique_ptr<TransformFeedbackManager> transform_feedback_manager_;
So, actually, transform feedback objects are not shared across contexts, like
query objects, vertex array objects, and... framebuffer objects (oops - this was
left undefined in GLES2, but precised in GLES3). This is clarified in the
Appendix D.

So, the transform_feedback_manager_ should be in the GLES2DecoderImpl.
(separately from this patch, we should investigate moving framebuffer_manager_
to the decoder too, but that's trickier).

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3584: if
(transform_feedback_manager()) {
nit: transform_feedback_manager() is always non-null, isn't it?

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3717: if (transform_feedback &&
!transform_feedback->deleted()) {
nit: because the transform feedback objects are not shared, I don't think the
deleted() case exists any more (the RemoveTransformFeedback would remove the
last reference).

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5132: }
If the currently bound transform feedback is active and not paused, we need to
fail with GL_INVALID_OPERATION

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8905: return transform_feedback
&& !transform_feedback->deleted();
nit: same as above, I don't think deleted() is possible any more.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8905: return transform_feedback
&& !transform_feedback->deleted();
Also, technically, IsTransformFeedback should only return GL_TRUE if the object
was bound at least once (and not deleted). The code as it is would create it in
GenTransformFeedbacks, so this would return true before the first bind.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
File gpu/command_buffer/service/indexed_buffer_binding_host.cc (right):

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:88: if (buffer &&
gl_version_info.IsLowerThanGL(4, 2)) {
nit: could we check the gl version in the constructor, and keep a flag
"needs_emulation_" or something?

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:107: // written to the
buffer.
Note, this will cause a GL_INVALID_OPERATION when glBeginTransformFeedback is
called, if the required buffers are not bound.

GLES3 would not cause a GL_INVALID_OPERATION at that point, but instead would
cause it at DrawArrays time if it would cause any primitive to be written to the
buffer.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:114: adjusted_size =
(adjusted_size >> 2) << 2;
nit: you can use adjusted_size & ~3;

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:154: // changes?
I don't think so. Though TBH the spec is extremely unclear about what happens
when BufferData is called - especially while the transform feedback is active.
Doubly so if this is done from another context.

[Zhenyao Mo, 2016-04-28 20:27:12]

Patchset #3 (id:90001) has been deleted

[Zhenyao Mo, 2016-04-28 20:27:25]

Patchset #2 (id:70001) has been deleted

[Zhenyao Mo, 2016-04-28 20:27:54]

Patchset #1 (id:50001) has been deleted

[Zhenyao Mo, 2016-04-29 21:01:56]

Patchset #2 (id:130001) has been deleted

[Zhenyao Mo, 2016-04-29 21:07:54]

piman: PTAL

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:282:
std::unique_ptr<TransformFeedbackManager> transform_feedback_manager_;
On 2016/04/28 01:38:47, piman wrote:
> So, actually, transform feedback objects are not shared across contexts, like
> query objects, vertex array objects, and... framebuffer objects (oops - this
was
> left undefined in GLES2, but precised in GLES3). This is clarified in the
> Appendix D.
> 
> So, the transform_feedback_manager_ should be in the GLES2DecoderImpl.
> (separately from this patch, we should investigate moving framebuffer_manager_
> to the decoder too, but that's trickier).

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3584: if
(transform_feedback_manager()) {
On 2016/04/28 01:38:47, piman wrote:
> nit: transform_feedback_manager() is always non-null, isn't it?

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3717: if (transform_feedback &&
!transform_feedback->deleted()) {
On 2016/04/28 01:38:47, piman wrote:
> nit: because the transform feedback objects are not shared, I don't think the
> deleted() case exists any more (the RemoveTransformFeedback would remove the
> last reference).

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5132: }
On 2016/04/28 01:38:47, piman wrote:
> If the currently bound transform feedback is active and not paused, we need to
> fail with GL_INVALID_OPERATION

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8905: return transform_feedback
&& !transform_feedback->deleted();
On 2016/04/28 01:38:47, piman wrote:
> Also, technically, IsTransformFeedback should only return GL_TRUE if the
object
> was bound at least once (and not deleted). The code as it is would create it
in
> GenTransformFeedbacks, so this would return true before the first bind.

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8905: return transform_feedback
&& !transform_feedback->deleted();
On 2016/04/28 01:38:47, piman wrote:
> nit: same as above, I don't think deleted() is possible any more.

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
File gpu/command_buffer/service/indexed_buffer_binding_host.cc (right):

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:88: if (buffer &&
gl_version_info.IsLowerThanGL(4, 2)) {
On 2016/04/28 01:38:47, piman wrote:
> nit: could we check the gl version in the constructor, and keep a flag
> "needs_emulation_" or something?

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:107: // written to the
buffer.
On 2016/04/28 01:38:47, piman OOO back 2016-5-2 wrote:
> Note, this will cause a GL_INVALID_OPERATION when glBeginTransformFeedback is
> called, if the required buffers are not bound.
> 
> GLES3 would not cause a GL_INVALID_OPERATION at that point, but instead would
> cause it at DrawArrays time if it would cause any primitive to be written to
the
> buffer.

It turns out glBindBufferBase(target, index, service_id) here works on MacOSX.
The GL 4.1 spec is ambiguous in this area, but let's go down this easy route
unless there is a driver fails here.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:114: adjusted_size =
(adjusted_size >> 2) << 2;
On 2016/04/28 01:38:47, piman wrote:
> nit: you can use adjusted_size & ~3;

Done.

https://codereview.chromium.org/1922633002/diff/110001/gpu/command_buffer/ser...
gpu/command_buffer/service/indexed_buffer_binding_host.cc:154: // changes?
On 2016/04/28 01:38:47, piman wrote:
> I don't think so. Though TBH the spec is extremely unclear about what happens
> when BufferData is called - especially while the transform feedback is active.
> Doubly so if this is done from another context.

Acknowledged.

[Zhenyao Mo, 2016-04-29 21:08:02]

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

[commit-bot: I haz the power, 2016-04-29 21:08:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/150001

[Zhenyao Mo, 2016-04-29 21:09:04]

Note that the latest CL still doesn't do the draw call transformfeedback
validation.  This CL is big and complicated enough.  Will do that in a followup
CL.

[commit-bot: I haz the power, 2016-04-29 22:16:49]

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

[commit-bot: I haz the power, 2016-04-29 22:16:50]

Dry run: This issue passed the CQ dry run.

[piman, 2016-05-02 22:28:14]

lgtm

[Zhenyao Mo, 2016-05-04 17:27:03]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2016-05-04 17:27:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922633002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922633002/150001

[commit-bot: I haz the power, 2016-05-04 20:01:01]

Committed patchset #2 (id:150001)

[commit-bot: I haz the power, 2016-05-04 20:03:10]

Description was changed from

==========
Implement TransformFeedbackManager in GPU command buffer.

A few design points:
1) create a "default" transform feedback for each context (instead of using
   the default 0). This will make context switching easier.
2) Emulate BindBufferRange behaviors on GL 4.1 or lower by tracking indexed
   buffer bindings.

WHat's left to be done:
1) Intercept GetIntegeri_v and GetIntegeri64_v on GL 4.1 or lower (due to the
   emulation)
2) Hook up indexed buffer bindings to Program (for uniform buffer bindings)

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Implement TransformFeedbackManager in GPU command buffer.

A few design points:
1) create a "default" transform feedback for each context (instead of using
   the default 0). This will make context switching easier.
2) Emulate BindBufferRange behaviors on GL 4.1 or lower by tracking indexed
   buffer bindings.

WHat's left to be done:
1) Intercept GetIntegeri_v and GetIntegeri64_v on GL 4.1 or lower (due to the
   emulation)
2) Hook up indexed buffer bindings to Program (for uniform buffer bindings)

BUG=604436
TEST=gpu_unittests,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6c468baf6600fe4846e73203ce2e83398ee73813
Cr-Commit-Position: refs/heads/master@{#391605}
==========

[commit-bot: I haz the power, 2016-05-04 20:03:11]

Patchset 2 (id:??) landed as
https://crrev.com/6c468baf6600fe4846e73203ce2e83398ee73813
Cr-Commit-Position: refs/heads/master@{#391605}