gpu: Support other buffer targets for ScopedBufferBinder

gpu: Support other buffer targets for ScopedBufferBinder

As a result of [1], ScopedBufferBinder supports decoder state restoration only for GL_ARRAY_BUFFER. 
This CL now extends its support for other buffer targets which are supported at the context level.

[1] https://codereview.chromium.org/2762833003/

BUG=None
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/2788793003
Cr-Commit-Position: refs/heads/master@{#462376}
Committed: https://chromium.googlesource.com/chromium/src/+/b0a7f41f21e310fb708a01ea0cdf81816d4df386

[Chandan, 2017-03-31 08:57:34]

Description was changed from

==========
gpu: Remove BufferTargetIsSupported()

In case of ScopedBufferBinder, BufferTargetIsSupported() restricts
buffer target to GL_ARRAY_BUFFER, even though other targets are
supported at the context level. Hence, removing it.

This CL is a fixup for https://codereview.chromium.org/2762833003/.

BUG=None
==========

to

==========
gpu: Remove BufferTargetIsSupported()

In case of ScopedBufferBinder, BufferTargetIsSupported() restricts
buffer target to GL_ARRAY_BUFFER, even though other targets are
supported at the context level. Hence, removing it.

This CL is a fixup for https://codereview.chromium.org/2762833003/.

BUG=None
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
==========

[Chandan, 2017-03-31 08:58:44]

Description was changed from

==========
gpu: Remove BufferTargetIsSupported()

In case of ScopedBufferBinder, BufferTargetIsSupported() restricts
buffer target to GL_ARRAY_BUFFER, even though other targets are
supported at the context level. Hence, removing it.

This CL is a fixup for https://codereview.chromium.org/2762833003/.

BUG=None
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

==========
gpu: Remove BufferTargetIsSupported()

In case of ScopedBufferBinder, BufferTargetIsSupported() restricts
buffer target to GL_ARRAY_BUFFER, even though other targets are
supported at the context level. Hence, removing it.

This CL is a fixup for https://codereview.chromium.org/2762833003/.

BUG=None
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
==========

[Chandan, 2017-03-31 08:58:44]

c.padhi@samsung.com changed reviewers:
+ piman@chromium.org

[Chandan, 2017-03-31 09:08:17]

PTAL. Thank you.

[piman, 2017-04-04 00:27:25]

https://codereview.chromium.org/2788793003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/2788793003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_state.cc:309: void
ContextState::RestoreBufferBinding(unsigned int target) const {
If we are not going to implement the other targets, I would suggest renaming
this function to RestoreArrayBufferBinding, and removing the target parameter.

[Chandan, 2017-04-04 12:11:38]

Description was changed from

==========
gpu: Remove BufferTargetIsSupported()

In case of ScopedBufferBinder, BufferTargetIsSupported() restricts
buffer target to GL_ARRAY_BUFFER, even though other targets are
supported at the context level. Hence, removing it.

This CL is a fixup for https://codereview.chromium.org/2762833003/.

BUG=None
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

==========
gpu: Support other buffer targets for ScopedBufferBinder

As a result of [1], ScopedBufferBinder supports decoder state restoration only
for GL_ARRAY_BUFFER. This CL now extends its support for other buffer targets
which are supported at the context level.

[1] https://codereview.chromium.org/2762833003/

BUG=None
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
==========

[Chandan, 2017-04-04 12:11:57]

Description was changed from

==========
gpu: Support other buffer targets for ScopedBufferBinder

As a result of [1], ScopedBufferBinder supports decoder state restoration only
for GL_ARRAY_BUFFER. This CL now extends its support for other buffer targets
which are supported at the context level.

[1] https://codereview.chromium.org/2762833003/

BUG=None
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

==========
gpu: Support other buffer targets for ScopedBufferBinder

As a result of [1], ScopedBufferBinder supports decoder state restoration only
for GL_ARRAY_BUFFER. 
This CL now extends its support for other buffer targets which are supported at
the context level.

[1] https://codereview.chromium.org/2762833003/

BUG=None
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
==========

[Chandan, 2017-04-04 12:14:00]

Please review patch set 2. Thank you.

https://codereview.chromium.org/2788793003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/2788793003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_state.cc:309: void
ContextState::RestoreBufferBinding(unsigned int target) const {
On 2017/04/04 00:27:25, piman wrote:
> If we are not going to implement the other targets, I would suggest renaming
> this function to RestoreArrayBufferBinding, and removing the target parameter.

I did not add other targets in BufferTargetIsSupported() to comply with
!state_restorer_ case in ScopedBufferBinder where only one target
GL_ARRAY_BUFFER is supported. 
We can definitely implement the other targets. I have uploaded patchset 2 with
support for other targets.

[piman, 2017-04-04 17:48:43]

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5605:
glPixelStorei(GL_PACK_ROW_LENGTH,
We should only do these if we're on an ES3 context. Maybe you can use
state_.UpdatePackParameters() and state_.UpdateUnpackParameters().

[Chandan, 2017-04-04 18:18:46]

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5605:
glPixelStorei(GL_PACK_ROW_LENGTH,
On 2017/04/04 17:48:43, piman wrote:
> We should only do these if we're on an ES3 context. Maybe you can use
> state_.UpdatePackParameters() and state_.UpdateUnpackParameters().

Thanks. As I see, these two methods are private. 
Will make these public?

[piman, 2017-04-04 19:59:15]

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5605:
glPixelStorei(GL_PACK_ROW_LENGTH,
On 2017/04/04 18:18:45, Chandan wrote:
> On 2017/04/04 17:48:43, piman wrote:
> > We should only do these if we're on an ES3 context. Maybe you can use
> > state_.UpdatePackParameters() and state_.UpdateUnpackParameters().
> 
> Thanks. As I see, these two methods are private. 
> Will make these public?

Alternatively, move this code to ContextState.

[Chandan, 2017-04-05 11:23:52]

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5605:
glPixelStorei(GL_PACK_ROW_LENGTH,
On 2017/04/04 19:59:14, piman wrote:
> On 2017/04/04 18:18:45, Chandan wrote:
> > On 2017/04/04 17:48:43, piman wrote:
> > > We should only do these if we're on an ES3 context. Maybe you can use
> > > state_.UpdatePackParameters() and state_.UpdateUnpackParameters().
> > 
> > Thanks. As I see, these two methods are private. 
> > Will make these public?
> 
> Alternatively, move this code to ContextState.

The idea was to reuse BufferManager::GetBufferInfoForTarget instead of writing
similar code again to get bound buffer. However, it is not accessible from
ContextState. Therefore, I consciously did not move this code to ContextState to
avoid code duplication.
Another alternative I see is to pass BufferManager pointer to ContextState and
then access it there to get the buffer.
Making Update{Pack,UnPack}Parameters() public looks cleaner. WDYT?
I have uploaded patchset 2 which makes these 2 methods public in ContextState.
PTAL.

[Chandan, 2017-04-05 11:24:15]

On 2017/04/05 11:23:52, Chandan wrote:
>
https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2788793003/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:5605:
> glPixelStorei(GL_PACK_ROW_LENGTH,
> On 2017/04/04 19:59:14, piman wrote:
> > On 2017/04/04 18:18:45, Chandan wrote:
> > > On 2017/04/04 17:48:43, piman wrote:
> > > > We should only do these if we're on an ES3 context. Maybe you can use
> > > > state_.UpdatePackParameters() and state_.UpdateUnpackParameters().
> > > 
> > > Thanks. As I see, these two methods are private. 
> > > Will make these public?
> > 
> > Alternatively, move this code to ContextState.
> 
> The idea was to reuse BufferManager::GetBufferInfoForTarget instead of writing
> similar code again to get bound buffer. However, it is not accessible from
> ContextState. Therefore, I consciously did not move this code to ContextState
to
> avoid code duplication.
> Another alternative I see is to pass BufferManager pointer to ContextState and
> then access it there to get the buffer.
> Making Update{Pack,UnPack}Parameters() public looks cleaner. WDYT?
> I have uploaded patchset 2 which makes these 2 methods public in ContextState.
> PTAL.

Correction patchset 3.

[piman, 2017-04-05 20:54:22]

lgtm

[Chandan, 2017-04-06 02:02:58]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-04-06 02:03:26]

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-06 02:07:53]

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

[commit-bot: I haz the power, 2017-04-06 02:07:54]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Chandan, 2017-04-06 05:41:45]

c.padhi@samsung.com changed reviewers:
+ a.suchit@chromium.org

[Chandan, 2017-04-06 06:21:42]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-04-06 06:28:56]

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-06 06:56:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491459702721740,
"parent_rev": "940c2909d9cb0cc3ddcd9cfa2a279f12e96bad45", "commit_rev":
"b0a7f41f21e310fb708a01ea0cdf81816d4df386"}

[commit-bot: I haz the power, 2017-04-06 06:57:38]

Description was changed from

==========
gpu: Support other buffer targets for ScopedBufferBinder

As a result of [1], ScopedBufferBinder supports decoder state restoration only
for GL_ARRAY_BUFFER. 
This CL now extends its support for other buffer targets which are supported at
the context level.

[1] https://codereview.chromium.org/2762833003/

BUG=None
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

==========
gpu: Support other buffer targets for ScopedBufferBinder

As a result of [1], ScopedBufferBinder supports decoder state restoration only
for GL_ARRAY_BUFFER. 
This CL now extends its support for other buffer targets which are supported at
the context level.

[1] https://codereview.chromium.org/2762833003/

BUG=None
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/2788793003
Cr-Commit-Position: refs/heads/master@{#462376}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0a7f41f21e310fb708a01ea0cdf...
==========

[commit-bot: I haz the power, 2017-04-06 06:57:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b0a7f41f21e310fb708a01ea0cdf...