gpu: Use decoder state restoration for scoped_binders

gpu: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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/2762833003
Cr-Commit-Position: refs/heads/master@{#460746}
Committed: https://chromium.googlesource.com/chromium/src/+/afded6c6f693337fcf57c6de5fb20cdb6a5f8a82

[Chandan, 2017-03-21 06:32:13]

Description was changed from

==========
gpu: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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

to

==========
gpu: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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-21 06:35:57]

Description was changed from

==========
gpu: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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-21 06:35:58]

c.padhi@samsung.com changed reviewers:
+ a.suchit@chromium.org, dcastagna@chromium.org, uzair.jaleel@samsung.com

[Chandan, 2017-03-21 06:38:58]

On 2017/03/21 06:35:58, Chandan wrote:
> mailto:c.padhi@samsung.com changed reviewers:
> + mailto:a.suchit@chromium.org, mailto:dcastagna@chromium.org,
mailto:uzair.jaleel@samsung.com

PTAL.

[Chandan, 2017-03-22 07:25:15]

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

[Chandan, 2017-03-22 07:26:21]

On 2017/03/22 07:25:15, Chandan wrote:
> mailto:c.padhi@samsung.com changed reviewers:
> + mailto:jbauman@chromium.org, mailto:kbr@chromium.org,
mailto:piman@chromium.org

@reviewers, PTAL!

[piman, 2017-03-22 21:28:26]

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

https://codereview.chromium.org/2762833003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_state.cc:462:
RestoreVertexAttribArray(attrib_index);
This needs to pass in |attrib_manager|. RestoreVertexAttribArrays can be called
with either |vertex_attrib_manager| or |default_vertex_attrib_manager|, so we
can't assume it's the former.

https://codereview.chromium.org/2762833003/diff/1/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/2762833003/diff/1/ui/gl/scoped_binders.cc#new...
ui/gl/scoped_binders.cc:147: state_restorer_->RestoreVertexAttribArray(index_);
RestoreVertexAttribArray has a side effect of binding the buffer corresponding
to the vertex attrib array state, but will not restore the currently bound
buffer. So it's not equivalent to the !state_restorer_ case, and might cause
some surprise at the call site (ScopedVertexAttribArray implicitly going out of
scope causes the current buffer binding to change).

[Chandan, 2017-03-23 09:19:39]

Uploaded patchset 2. PTAL. Thank you.

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

https://codereview.chromium.org/2762833003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_state.cc:462:
RestoreVertexAttribArray(attrib_index);
Have removed these changes in context_state.cc in patchset 2.

https://codereview.chromium.org/2762833003/diff/1/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/2762833003/diff/1/ui/gl/scoped_binders.cc#new...
ui/gl/scoped_binders.cc:147: state_restorer_->RestoreVertexAttribArray(index_);
Thanks for pointing this out. Made necessary changes in patchset 2 where we use
GLES2DecoderImpl::RestoreStateForAttrib that restores currently bound buffer.

[piman, 2017-03-23 20:28:38]

lgtm

[Chandan, 2017-03-23 20:34:56]

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

[commit-bot: I haz the power, 2017-03-23 20:35:36]

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-03-23 21:29:41]

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

[commit-bot: I haz the power, 2017-03-23 21:29:43]

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_...)

[Ken Russell (switch to Gerrit), 2017-03-23 21:41:45]

On 2017/03/23 21:29:43, commit-bot: I haz the power wrote:
> 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_...)

Looks like a real failure caused by this patch.

[Chandan, 2017-03-24 09:18:56]

On 2017/03/23 21:41:45, Ken Russell wrote:
> On 2017/03/23 21:29:43, commit-bot: I haz the power wrote:
> > 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_...)
> 
> Looks like a real failure caused by this patch.

As per build logs, all the 4 tests are failing due to failure of
VTDecompressionSessionCreate(--) function with below error:
[43792:771:0323/134948.867568:30371248834014:WARNING:vt_video_decode_accelerator_mac.cc(169)]
Failed to create VTDecompressionSession: Error Domain=NSOSStatusErrorDomain
Code=-8973 "The operation couldn?t be completed. (OSStatus error -8973.)"
(codecOpenErr) (-8973)
This failure is only on MAC OS.

I do not have much idea on this. Also, I am wondering how this failure is
related to this CL.
If anybody can throw some light on this failure, please?

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

On 2017/03/24 09:18:56, Chandan wrote:
> On 2017/03/23 21:41:45, Ken Russell wrote:
> > On 2017/03/23 21:29:43, commit-bot: I haz the power wrote:
> > > 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_...)
> > 
> > Looks like a real failure caused by this patch.
> 
> As per build logs, all the 4 tests are failing due to failure of
> VTDecompressionSessionCreate(--) function with below error:
>
[43792:771:0323/134948.867568:30371248834014:WARNING:vt_video_decode_accelerator_mac.cc(169)]
> Failed to create VTDecompressionSession: Error Domain=NSOSStatusErrorDomain
> Code=-8973 "The operation couldn?t be completed. (OSStatus error -8973.)"
> (codecOpenErr) (-8973)
> This failure is only on MAC OS.
> 
> I do not have much idea on this. Also, I am wondering how this failure is
> related to this CL.
> If anybody can throw some light on this failure, please?

Please try building on macOS and running with and without your patch. Debugging
it in that manner will probably shed some light. Thanks.

Note that your patch failed multiple try jobs on mac_chromium_rel_ng in the same
way and was the only patch failing in that manner, so I think it's unlikely that
your patch hit a bug that was on top-of-tree at the time.

[Chandan, 2017-03-24 19:47:12]

On 2017/03/24 18:23:51, Ken Russell wrote:
> On 2017/03/24 09:18:56, Chandan wrote:
> > On 2017/03/23 21:41:45, Ken Russell wrote:
> > > On 2017/03/23 21:29:43, commit-bot: I haz the power wrote:
> > > > 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_...)
> > > 
> > > Looks like a real failure caused by this patch.
> > 
> > As per build logs, all the 4 tests are failing due to failure of
> > VTDecompressionSessionCreate(--) function with below error:
> >
>
[43792:771:0323/134948.867568:30371248834014:WARNING:vt_video_decode_accelerator_mac.cc(169)]
> > Failed to create VTDecompressionSession: Error Domain=NSOSStatusErrorDomain
> > Code=-8973 "The operation couldn?t be completed. (OSStatus error -8973.)"
> > (codecOpenErr) (-8973)
> > This failure is only on MAC OS.
> > 
> > I do not have much idea on this. Also, I am wondering how this failure is
> > related to this CL.
> > If anybody can throw some light on this failure, please?
> 
> Please try building on macOS and running with and without your patch.
Debugging
> it in that manner will probably shed some light. Thanks.
> 
> Note that your patch failed multiple try jobs on mac_chromium_rel_ng in the
same
> way and was the only patch failing in that manner, so I think it's unlikely
that
> your patch hit a bug that was on top-of-tree at the time.
I would have definitely done that, but unfortunately I do not have a macOS setup
with me. :( We generally work on a Linux machine. Thanks.

[Chandan, 2017-03-24 19:51:05]

On 2017/03/24 18:23:51, Ken Russell wrote:
> On 2017/03/24 09:18:56, Chandan wrote:
> > On 2017/03/23 21:41:45, Ken Russell wrote:
> > > On 2017/03/23 21:29:43, commit-bot: I haz the power wrote:
> > > > 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_...)
> > > 
> > > Looks like a real failure caused by this patch.
> > 
> > As per build logs, all the 4 tests are failing due to failure of
> > VTDecompressionSessionCreate(--) function with below error:
> >
>
[43792:771:0323/134948.867568:30371248834014:WARNING:vt_video_decode_accelerator_mac.cc(169)]
> > Failed to create VTDecompressionSession: Error Domain=NSOSStatusErrorDomain
> > Code=-8973 "The operation couldn?t be completed. (OSStatus error -8973.)"
> > (codecOpenErr) (-8973)
> > This failure is only on MAC OS.
> > 
> > I do not have much idea on this. Also, I am wondering how this failure is
> > related to this CL.
> > If anybody can throw some light on this failure, please?
> 
> Please try building on macOS and running with and without your patch.
Debugging
> it in that manner will probably shed some light. Thanks.
> 
> Note that your patch failed multiple try jobs on mac_chromium_rel_ng in the
same
> way and was the only patch failing in that manner, so I think it's unlikely
that
> your patch hit a bug that was on top-of-tree at the time.
I would have definitely done that, but unfortunately I do not have a macOS setup
with me. :( We generally work on a Linux machine. Thanks.

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

On 2017/03/24 19:51:05, Chandan wrote:
> On 2017/03/24 18:23:51, Ken Russell wrote:
> > On 2017/03/24 09:18:56, Chandan wrote:
> > > On 2017/03/23 21:41:45, Ken Russell wrote:
> > > > On 2017/03/23 21:29:43, commit-bot: I haz the power wrote:
> > > > > 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_...)
> > > > 
> > > > Looks like a real failure caused by this patch.
> > > 
> > > As per build logs, all the 4 tests are failing due to failure of
> > > VTDecompressionSessionCreate(--) function with below error:
> > >
> >
>
[43792:771:0323/134948.867568:30371248834014:WARNING:vt_video_decode_accelerator_mac.cc(169)]
> > > Failed to create VTDecompressionSession: Error
Domain=NSOSStatusErrorDomain
> > > Code=-8973 "The operation couldn?t be completed. (OSStatus error -8973.)"
> > > (codecOpenErr) (-8973)
> > > This failure is only on MAC OS.
> > > 
> > > I do not have much idea on this. Also, I am wondering how this failure is
> > > related to this CL.
> > > If anybody can throw some light on this failure, please?
> > 
> > Please try building on macOS and running with and without your patch.
> Debugging
> > it in that manner will probably shed some light. Thanks.
> > 
> > Note that your patch failed multiple try jobs on mac_chromium_rel_ng in the
> same
> > way and was the only patch failing in that manner, so I think it's unlikely
> that
> > your patch hit a bug that was on top-of-tree at the time.
> I would have definitely done that, but unfortunately I do not have a macOS
setup
> with me. :( We generally work on a Linux machine. Thanks.

In that case, you can send tryjobs to mac_chromium_rel_ng containing more
logging to figure out what's going wrong, but until you debug the test failure
this CL won't be able to land.

[Chandan, 2017-03-30 12:48:26]

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

[Chandan, 2017-03-30 12:48:26]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2762833003/#ps160001
(title: "Fixed MediaColorTest failures on macOS")

[commit-bot: I haz the power, 2017-03-30 12:48:58]

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-03-30 13:54:47]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490878106502160,
"parent_rev": "29783ad5a5cbc0569d4a5578c82feca23223a621", "commit_rev":
"3ba509a88e31b9127720da700c13539a33ec7ef3"}

[commit-bot: I haz the power, 2017-03-30 14:00:24]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490878106502160,
"parent_rev": "d9a86ecf88f14c23e6526824822a230b23dafb1c", "commit_rev":
"afded6c6f693337fcf57c6de5fb20cdb6a5f8a82"}

[commit-bot: I haz the power, 2017-03-30 14:01:12]

Description was changed from

==========
gpu: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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: Use decoder state restoration for scoped_binders

This commit adds decoder state restoration for Scoped
{ActiveTexture,UseProgram,VertexAttribArray,BufferBinder}.

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/2762833003
Cr-Commit-Position: refs/heads/master@{#460746}
Committed:
https://chromium.googlesource.com/chromium/src/+/afded6c6f693337fcf57c6de5fb2...
==========

[commit-bot: I haz the power, 2017-03-30 14:01:13]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/afded6c6f693337fcf57c6de5fb2...

[piman, 2017-03-30 18:31:43]

It's generally recommended to get a new review if you change the patch in
non-trivial ways after the LGTM. I'm not sure I agree with that new logic

https://codereview.chromium.org/2762833003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/2762833003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_state.cc:320: if
(BufferTargetIsSupported(target))
Why this logic? Other targets *are* supported at the context level. It would be
extremely surprising for users to use a ScopedBufferBinder to not work based on
the target

[Chandan, 2017-03-30 19:21:06]

On 2017/03/30 18:31:43, piman wrote:
> It's generally recommended to get a new review if you change the patch in
> non-trivial ways after the LGTM. I'm not sure I agree with that new logic
> 
Sure. I wasn't aware of this. I will take care of it the next time.

[Chandan, 2017-03-30 19:22:39]

https://codereview.chromium.org/2762833003/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/2762833003/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_state.cc:320: if
(BufferTargetIsSupported(target))
On 2017/03/30 18:31:43, piman wrote:
> Why this logic? Other targets *are* supported at the context level. It would
be
> extremely surprising for users to use a ScopedBufferBinder to not work based
on
> the target

I added this logic to keep it consistent with !state_restorer_ case in
ScopedBufferBinder ctor.
So, target will be passed on to glBindBuffer without any check?