Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Otherwise, there will be some weird GL errors on some GL drivers.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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

Committed: https://crrev.com/17ad0f1ca0367fbb6d28cd9ed3d4d60a44e0e655
Cr-Commit-Position: refs/heads/master@{#417685}

[qiankun, 2016-09-02 13:34:10]

Description was changed from

==========
Set PIXEL_UNPACK_BUFFER as 0 at gl decoder initialization time

Otherwise, context creation will fail due to gl error.

BUG=641643
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
==========

to

==========
Set PIXEL_UNPACK_BUFFER as 0 at gl decoder initialization time

Otherwise, context creation will fail due to gl error.

BUG=641643
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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
==========

[qiankun, 2016-09-02 15:35:32]

qiankun.miao@intel.com changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[qiankun, 2016-09-02 15:35:32]

PTAL.

[Zhenyao Mo, 2016-09-02 17:10:23]

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

https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3011:
glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
Thanks for tracking down the problem, but this is not the right fix.  The
problem is in virtual context initialization.  We need to fix it at the root
cause instead.

[qiankun, 2016-09-02 23:12:02]

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

https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3011:
glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
On 2016/09/02 17:10:23, Zhenyao Mo wrote:
> Thanks for tracking down the problem, but this is not the right fix.  The
> problem is in virtual context initialization.  We need to fix it at the root
> cause instead.

Do you mean moving the code to virtual context initialization? If not, I will
dig it further next week.

[Zhenyao Mo, 2016-09-02 23:17:42]

On 2016/09/02 23:12:02, qiankun wrote:
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:3011:
> glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
> On 2016/09/02 17:10:23, Zhenyao Mo wrote:
> > Thanks for tracking down the problem, but this is not the right fix.  The
> > problem is in virtual context initialization.  We need to fix it at the root
> > cause instead.
> 
> Do you mean moving the code to virtual context initialization? If not, I will
> dig it further next week.

There is a bad bug in virtual context initialization, which caused this
flakiness and probably other flakiness on Linux.

Kai should be able to explain better than me.

[Zhenyao Mo, 2016-09-02 23:18:05]

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

[Zhenyao Mo, 2016-09-02 23:18:06]

+kainino

[Kai Ninomiya, 2016-09-03 01:30:46]

In investigating this crash, Ken and I found that there was an apparent
misordering of some initialization in the gpu command buffer. IIRC, the problem
is that GLContextVirtual is initialized [1] before GLES2Decoder is initialized
[2], but GLContextVirtual needs it to be initialized [3] [4].

However, I do not yet have a deep understanding of the problem, so this may be
imprecise.

Ken and I plan to revisit this on Tuesday after the holiday weekend.

-Kai

[1]
https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_command_buffer_stub....
[2]
https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_command_buffer_stub....
[3]
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gl_context_vi...
[4]
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?rcl=1472843917&l=221

[Ken Russell (switch to Gerrit), 2016-09-07 03:37:00]

kbr@chromium.org changed reviewers:
+ sunnyps@chromium.org, vmiura@chromium.org

[Ken Russell (switch to Gerrit), 2016-09-07 03:37:01]

After a lot of investigation this might in fact be the correct and simplest way
to go. I would like to delegate this review to one of the other CC'd folks
however.

[Zhenyao Mo, 2016-09-07 03:55:16]

On 2016/09/07 03:37:01, Ken Russell wrote:
> After a lot of investigation this might in fact be the correct and simplest
way
> to go. I would like to delegate this review to one of the other CC'd folks
> however.

kbr, I'll ask you about the investigation tomorrow.

[vmiura, 2016-09-07 05:11:01]

On 2016/09/07 03:55:16, Zhenyao Mo wrote:
> On 2016/09/07 03:37:01, Ken Russell wrote:
> > After a lot of investigation this might in fact be the correct and simplest
> way
> > to go. I would like to delegate this review to one of the other CC'd folks
> > however.
> 
> kbr, I'll ask you about the investigation tomorrow.

I think I agree wit kbr@.  See
https://bugs.chromium.org/p/chromium/issues/detail?id=644572#c3.

[Zhenyao Mo, 2016-09-08 18:19:23]

qiankun, per discussion, let's land this after you address the comments.

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

https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
(version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
This should be feature_info_->IsES3Capable()

[qiankun, 2016-09-09 02:06:44]

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

https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
(version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
On 2016/09/08 18:19:23, Zhenyao Mo wrote:
> This should be feature_info_->IsES3Capable()

IsES3Capable() cannot be used until group_->Initialize() (in line 3013) is done,
because gl_version_info_ is initialized in FeatureInfo::InitializeFeatures().

see below callstack:
#2 0x561847d6e382 gpu::gles2::FeatureInfo::InitializeFeatures()
#3 0x561847d6c1c8 gpu::gles2::FeatureInfo::Initialize()
#4 0x561848dd4e8a gpu::gles2::ContextGroup::Initialize()
#5 0x561848e13dbf gpu::gles2::GLES2DecoderImpl::Initialize()

[Zhenyao Mo, 2016-09-09 02:32:50]

On 2016/09/09 02:06:44, qiankun wrote:
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
> (version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
> On 2016/09/08 18:19:23, Zhenyao Mo wrote:
> > This should be feature_info_->IsES3Capable()
> 
> IsES3Capable() cannot be used until group_->Initialize() (in line 3013) is
done,
> because gl_version_info_ is initialized in FeatureInfo::InitializeFeatures().
> 
> see below callstack:
> #2 0x561847d6e382 gpu::gles2::FeatureInfo::InitializeFeatures()
> #3 0x561847d6c1c8 gpu::gles2::FeatureInfo::Initialize()
> #4 0x561848dd4e8a gpu::gles2::ContextGroup::Initialize()
> #5 0x561848e13dbf gpu::gles2::GLES2DecoderImpl::Initialize()

Then is it possible to move clearing PIXEL_UNPACK_BUFFER into TextureManager? We
only care about it because the black textures.

[qiankun, 2016-09-09 02:32:57]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 02:33:19]

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

[Kai Ninomiya, 2016-09-09 02:44:27]

On 2016/09/09 02:32:50, Zhenyao Mo wrote:
> On 2016/09/09 02:06:44, qiankun wrote:
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
> > (version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
> > On 2016/09/08 18:19:23, Zhenyao Mo wrote:
> > > This should be feature_info_->IsES3Capable()
> > 
> > IsES3Capable() cannot be used until group_->Initialize() (in line 3013) is
> done,
> > because gl_version_info_ is initialized in
FeatureInfo::InitializeFeatures().
> > 
> > see below callstack:
> > #2 0x561847d6e382 gpu::gles2::FeatureInfo::InitializeFeatures()
> > #3 0x561847d6c1c8 gpu::gles2::FeatureInfo::Initialize()
> > #4 0x561848dd4e8a gpu::gles2::ContextGroup::Initialize()
> > #5 0x561848e13dbf gpu::gles2::GLES2DecoderImpl::Initialize()
> 
> Then is it possible to move clearing PIXEL_UNPACK_BUFFER into TextureManager?
We
> only care about it because the black textures.

On my workstation I'm running into TexImage2D errors in places outside of
TextureManager. See the stack traces here:
https://bugs.chromium.org/p/chromium/issues/detail?id=644572

(Also, should we change the BUG= on this CL to point to that bug?)

[qiankun, 2016-09-09 02:50:32]

Description was changed from

==========
Set PIXEL_UNPACK_BUFFER as 0 at gl decoder initialization time

Otherwise, context creation will fail due to gl error.

BUG=641643
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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

==========
Set PIXEL_UNPACK_BUFFER as 0 at gl decoder initialization time

Otherwise, context creation will fail due to gl error.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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
==========

[qiankun, 2016-09-09 03:08:09]

On 2016/09/09 02:32:50, Zhenyao Mo wrote:
> On 2016/09/09 02:06:44, qiankun wrote:
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
> > (version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
> > On 2016/09/08 18:19:23, Zhenyao Mo wrote:
> > > This should be feature_info_->IsES3Capable()
> > 
> > IsES3Capable() cannot be used until group_->Initialize() (in line 3013) is
> done,
> > because gl_version_info_ is initialized in
FeatureInfo::InitializeFeatures().
> > 
> > see below callstack:
> > #2 0x561847d6e382 gpu::gles2::FeatureInfo::InitializeFeatures()
> > #3 0x561847d6c1c8 gpu::gles2::FeatureInfo::Initialize()
> > #4 0x561848dd4e8a gpu::gles2::ContextGroup::Initialize()
> > #5 0x561848e13dbf gpu::gles2::GLES2DecoderImpl::Initialize()
> 
> Then is it possible to move clearing PIXEL_UNPACK_BUFFER into TextureManager?
We
> only care about it because the black textures.

I saw gl error from gpu::gles2::GLES2DecoderImpl::Initialize():
[30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(2349)]
[GroupMarkerNotSet(crbug.com/242999)!:D055F1168E090000]GL ERROR
:GL_INVALID_OPERATION : BackFramebuff
revious GL command
[30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(4847)]
GLES2DecoderImpl::ResizeOffscreenFramebuffer failed to allocate storage for
offscreen target color te
[30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(3270)] Could not allocate
offscreen buffer storage.

Kai, I updated bug id in commit description.

[commit-bot: I haz the power, 2016-09-09 03:28:43]

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

[commit-bot: I haz the power, 2016-09-09 03:28:44]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-09-09 03:31:15]

On 2016/09/09 03:08:09, qiankun wrote:
> On 2016/09/09 02:32:50, Zhenyao Mo wrote:
> > On 2016/09/09 02:06:44, qiankun wrote:
> > >
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
> > > (version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
> > > On 2016/09/08 18:19:23, Zhenyao Mo wrote:
> > > > This should be feature_info_->IsES3Capable()
> > > 
> > > IsES3Capable() cannot be used until group_->Initialize() (in line 3013) is
> > done,
> > > because gl_version_info_ is initialized in
> FeatureInfo::InitializeFeatures().
> > > 
> > > see below callstack:
> > > #2 0x561847d6e382 gpu::gles2::FeatureInfo::InitializeFeatures()
> > > #3 0x561847d6c1c8 gpu::gles2::FeatureInfo::Initialize()
> > > #4 0x561848dd4e8a gpu::gles2::ContextGroup::Initialize()
> > > #5 0x561848e13dbf gpu::gles2::GLES2DecoderImpl::Initialize()
> > 
> > Then is it possible to move clearing PIXEL_UNPACK_BUFFER into
TextureManager?
> We
> > only care about it because the black textures.
> 
> I saw gl error from gpu::gles2::GLES2DecoderImpl::Initialize():
> [30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(2349)]
> [GroupMarkerNotSet(crbug.com/242999)!:D055F1168E090000]GL ERROR
> :GL_INVALID_OPERATION : BackFramebuff
> revious GL command
> [30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(4847)]
> GLES2DecoderImpl::ResizeOffscreenFramebuffer failed to allocate storage for
> offscreen target color te
> [30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(3270)] Could not allocate
> offscreen buffer storage.
> 
> Kai, I updated bug id in commit description.

If not texture_manager, maybe somewhere in ContextGroup::Initialize() after
FeatureInfo::Initialize()?

[Zhenyao Mo, 2016-09-09 03:34:03]

On 2016/09/09 03:31:15, Zhenyao Mo wrote:
> On 2016/09/09 03:08:09, qiankun wrote:
> > On 2016/09/09 02:32:50, Zhenyao Mo wrote:
> > > On 2016/09/09 02:06:44, qiankun wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2302323002/diff/40001/gpu/command_buffer/serv...
> > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:3010: if
> > > > (version_info->IsAtLeastGL(3, 2) || version_info->IsAtLeastGLES(3, 0))
> > > > On 2016/09/08 18:19:23, Zhenyao Mo wrote:
> > > > > This should be feature_info_->IsES3Capable()
> > > > 
> > > > IsES3Capable() cannot be used until group_->Initialize() (in line 3013)
is
> > > done,
> > > > because gl_version_info_ is initialized in
> > FeatureInfo::InitializeFeatures().
> > > > 
> > > > see below callstack:
> > > > #2 0x561847d6e382 gpu::gles2::FeatureInfo::InitializeFeatures()
> > > > #3 0x561847d6c1c8 gpu::gles2::FeatureInfo::Initialize()
> > > > #4 0x561848dd4e8a gpu::gles2::ContextGroup::Initialize()
> > > > #5 0x561848e13dbf gpu::gles2::GLES2DecoderImpl::Initialize()
> > > 
> > > Then is it possible to move clearing PIXEL_UNPACK_BUFFER into
> TextureManager?
> > We
> > > only care about it because the black textures.
> > 
> > I saw gl error from gpu::gles2::GLES2DecoderImpl::Initialize():
> > [30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(2349)]
> > [GroupMarkerNotSet(crbug.com/242999)!:D055F1168E090000]GL ERROR
> > :GL_INVALID_OPERATION : BackFramebuff
> > revious GL command
> > [30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(4847)]
> > GLES2DecoderImpl::ResizeOffscreenFramebuffer failed to allocate storage for
> > offscreen target color te
> > [30134:30134:0909/110120:ERROR:gles2_cmd_decoder.cc(3270)] Could not
allocate
> > offscreen buffer storage.
> > 
> > Kai, I updated bug id in commit description.
> 
> If not texture_manager, maybe somewhere in ContextGroup::Initialize() after
> FeatureInfo::Initialize()?

To clarify: I want to know where the error comes from.  If it comes inside
ContextGroup::Initialize(), then we can definitely move this code into there. 
If not, and it's from somewhere in later decoder's Initialize(), then maybe we
can do this after FeatureInfo is initialized?

[qiankun, 2016-09-09 09:47:07]

Description was changed from

==========
Set PIXEL_UNPACK_BUFFER as 0 at gl decoder initialization time

Otherwise, context creation will fail due to gl error.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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

==========
Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Otherwise, there will be some weird GL errors on some GL drivers.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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
==========

[qiankun, 2016-09-09 09:52:48]

Updated the code. Please review again.

https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1785: if
(feature_info_->gl_version_info().is_es3_capable) {
Using this other than feature_info_->IsES3Capable() here because it's difficult
to check feature_info_->IsES3Capable() in gpu_unittests when implementing
SetupTextureManagerInitExpectations in test_helper.cc. I took whole day but
didn't find a good solution. If you have better method, please tell me.  Thanks.

[Zhenyao Mo, 2016-09-09 15:17:08]

https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1785: if
(feature_info_->gl_version_info().is_es3_capable) {
On 2016/09/09 09:52:48, qiankun wrote:
> Using this other than feature_info_->IsES3Capable() here because it's
difficult
> to check feature_info_->IsES3Capable() in gpu_unittests when implementing
> SetupTextureManagerInitExpectations in test_helper.cc. I took whole day but
> didn't find a good solution. If you have better method, please tell me. 
Thanks.

You can use feature_info_->IsES3Capable() here and use
gl_version_info.is_es3_capable in the test.  There are some slight difference
between the two, but as far as it doesn't affect the tests, I feel it's no big
deal.

Or is this difference causing tests to fail?

[qiankun, 2016-09-09 18:16:26]

https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1785: if
(feature_info_->gl_version_info().is_es3_capable) {
On 2016/09/09 15:17:08, Zhenyao Mo wrote:
> On 2016/09/09 09:52:48, qiankun wrote:
> > Using this other than feature_info_->IsES3Capable() here because it's
> difficult
> > to check feature_info_->IsES3Capable() in gpu_unittests when implementing
> > SetupTextureManagerInitExpectations in test_helper.cc. I took whole day but
> > didn't find a good solution. If you have better method, please tell me. 
> Thanks.
> 
> You can use feature_info_->IsES3Capable() here and use
> gl_version_info.is_es3_capable in the test.  There are some slight difference
> between the two, but as far as it doesn't affect the tests, I feel it's no big
> deal.
> 
> Or is this difference causing tests to fail?

For some test cases, if --enable-unsafe-es3-apis is set,
gl_version_info.is_es3_capable and feature_info_->IsES3Capable() are different.
This makes EXPECT_CALL check fail.

[Kai Ninomiya, 2016-09-09 18:40:33]

On 2016/09/09 18:16:26, qiankun wrote:
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/texture_manager.cc (right):
> 
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> gpu/command_buffer/service/texture_manager.cc:1785: if
> (feature_info_->gl_version_info().is_es3_capable) {
> On 2016/09/09 15:17:08, Zhenyao Mo wrote:
> > On 2016/09/09 09:52:48, qiankun wrote:
> > > Using this other than feature_info_->IsES3Capable() here because it's
> > difficult
> > > to check feature_info_->IsES3Capable() in gpu_unittests when implementing
> > > SetupTextureManagerInitExpectations in test_helper.cc. I took whole day
but
> > > didn't find a good solution. If you have better method, please tell me. 
> > Thanks.
> > 
> > You can use feature_info_->IsES3Capable() here and use
> > gl_version_info.is_es3_capable in the test.  There are some slight
difference
> > between the two, but as far as it doesn't affect the tests, I feel it's no
big
> > deal.
> > 
> > Or is this difference causing tests to fail?
> 
> For some test cases, if --enable-unsafe-es3-apis is set,
> gl_version_info.is_es3_capable and feature_info_->IsES3Capable() are
different.
> This makes EXPECT_CALL check fail.

I ran into a similar problem. If I remember correctly, IsES3Capable checks the
flag while the tests cannot do that. I'm not sure if it will help, but looking
at this CL may help find the solution:
https://codereview.chromium.org/2291753002

[Zhenyao Mo, 2016-09-09 18:48:02]

On 2016/09/09 18:40:33, Kai Ninomiya wrote:
> On 2016/09/09 18:16:26, qiankun wrote:
> >
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/texture_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/texture_manager.cc:1785: if
> > (feature_info_->gl_version_info().is_es3_capable) {
> > On 2016/09/09 15:17:08, Zhenyao Mo wrote:
> > > On 2016/09/09 09:52:48, qiankun wrote:
> > > > Using this other than feature_info_->IsES3Capable() here because it's
> > > difficult
> > > > to check feature_info_->IsES3Capable() in gpu_unittests when
implementing
> > > > SetupTextureManagerInitExpectations in test_helper.cc. I took whole day
> but
> > > > didn't find a good solution. If you have better method, please tell me. 
> > > Thanks.
> > > 
> > > You can use feature_info_->IsES3Capable() here and use
> > > gl_version_info.is_es3_capable in the test.  There are some slight
> difference
> > > between the two, but as far as it doesn't affect the tests, I feel it's no
> big
> > > deal.
> > > 
> > > Or is this difference causing tests to fail?
> > 
> > For some test cases, if --enable-unsafe-es3-apis is set,
> > gl_version_info.is_es3_capable and feature_info_->IsES3Capable() are
> different.
> > This makes EXPECT_CALL check fail.
> 
> I ran into a similar problem. If I remember correctly, IsES3Capable checks the
> flag while the tests cannot do that. I'm not sure if it will help, but looking
> at this CL may help find the solution:
> https://codereview.chromium.org/2291753002

Let's land this as is then.  The flag will be take out soon and we can
consolidate the two calls to be the same.

[Zhenyao Mo, 2016-09-09 18:48:37]

On 2016/09/09 18:48:02, Zhenyao Mo wrote:
> On 2016/09/09 18:40:33, Kai Ninomiya wrote:
> > On 2016/09/09 18:16:26, qiankun wrote:
> > >
> >
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> > > File gpu/command_buffer/service/texture_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> > > gpu/command_buffer/service/texture_manager.cc:1785: if
> > > (feature_info_->gl_version_info().is_es3_capable) {
> > > On 2016/09/09 15:17:08, Zhenyao Mo wrote:
> > > > On 2016/09/09 09:52:48, qiankun wrote:
> > > > > Using this other than feature_info_->IsES3Capable() here because it's
> > > > difficult
> > > > > to check feature_info_->IsES3Capable() in gpu_unittests when
> implementing
> > > > > SetupTextureManagerInitExpectations in test_helper.cc. I took whole
day
> > but
> > > > > didn't find a good solution. If you have better method, please tell
me. 
> > > > Thanks.
> > > > 
> > > > You can use feature_info_->IsES3Capable() here and use
> > > > gl_version_info.is_es3_capable in the test.  There are some slight
> > difference
> > > > between the two, but as far as it doesn't affect the tests, I feel it's
no
> > big
> > > > deal.
> > > > 
> > > > Or is this difference causing tests to fail?
> > > 
> > > For some test cases, if --enable-unsafe-es3-apis is set,
> > > gl_version_info.is_es3_capable and feature_info_->IsES3Capable() are
> > different.
> > > This makes EXPECT_CALL check fail.
> > 
> > I ran into a similar problem. If I remember correctly, IsES3Capable checks
the
> > flag while the tests cannot do that. I'm not sure if it will help, but
looking
> > at this CL may help find the solution:
> > https://codereview.chromium.org/2291753002
> 
> Let's land this as is then.  The flag will be take out soon and we can
> consolidate the two calls to be the same.

lgtm

[qiankun, 2016-09-09 18:49:47]

On 2016/09/09 18:40:33, Kai Ninomiya wrote:
> On 2016/09/09 18:16:26, qiankun wrote:
> >
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/texture_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2302323002/diff/80001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/texture_manager.cc:1785: if
> > (feature_info_->gl_version_info().is_es3_capable) {
> > On 2016/09/09 15:17:08, Zhenyao Mo wrote:
> > > On 2016/09/09 09:52:48, qiankun wrote:
> > > > Using this other than feature_info_->IsES3Capable() here because it's
> > > difficult
> > > > to check feature_info_->IsES3Capable() in gpu_unittests when
implementing
> > > > SetupTextureManagerInitExpectations in test_helper.cc. I took whole day
> but
> > > > didn't find a good solution. If you have better method, please tell me. 
> > > Thanks.
> > > 
> > > You can use feature_info_->IsES3Capable() here and use
> > > gl_version_info.is_es3_capable in the test.  There are some slight
> difference
> > > between the two, but as far as it doesn't affect the tests, I feel it's no
> big
> > > deal.
> > > 
> > > Or is this difference causing tests to fail?
> > 
> > For some test cases, if --enable-unsafe-es3-apis is set,
> > gl_version_info.is_es3_capable and feature_info_->IsES3Capable() are
> different.
> > This makes EXPECT_CALL check fail.
> 
> I ran into a similar problem. If I remember correctly, IsES3Capable checks the
> flag while the tests cannot do that. I'm not sure if it will help, but looking
> at this CL may help find the solution:
> https://codereview.chromium.org/2291753002

I saw "gl_info.is_es3_capable && enable_es3" is used in your CL. I tried this
method, but it only works for part of the tests.

[qiankun, 2016-09-09 18:55:11]

Thank you all for review and suggestions. Landing now.

[qiankun, 2016-09-09 18:55:35]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2016-09-09 18:55:55]

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

[Ken Russell (switch to Gerrit), 2016-09-09 19:00:57]

On 2016/09/09 18:55:55, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Note: the failure
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg...
is http://crbug.com/633067 .

[vmiura, 2016-09-09 19:26:11]

Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
TextureManager::Initialize()?

If so, that will only be done on the 1st context in a ContextGroup, and doesn't
seem sufficient.  We should reset this state on all new contexts.

[Zhenyao Mo, 2016-09-09 20:05:23]

On 2016/09/09 19:26:11, vmiura wrote:
> Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> TextureManager::Initialize()?
> 
> If so, that will only be done on the 1st context in a ContextGroup, and
doesn't
> seem sufficient.  We should reset this state on all new contexts.
If I understand this correctly, we do reset it for new contexts at ContextState
level, but that's not early enough.  We only need to do this here (duplicated
step) for creating black fallback textures.

[commit-bot: I haz the power, 2016-09-09 20:14:22]

Description was changed from

==========
Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Otherwise, there will be some weird GL errors on some GL drivers.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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

==========
Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Otherwise, there will be some weird GL errors on some GL drivers.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-09 20:16:15]

Description was changed from

==========
Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Otherwise, there will be some weird GL errors on some GL drivers.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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

==========
Reset PIXEL_UNPACK_BUFFER at texture manager initialization time

Otherwise, there will be some weird GL errors on some GL drivers.

BUG=641643, 644572
TEST=conformance2/misc/getextension-while-pbo-bound-stability.html
CQ_INCLUDE_TRYBOTS=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

Committed: https://crrev.com/17ad0f1ca0367fbb6d28cd9ed3d4d60a44e0e655
Cr-Commit-Position: refs/heads/master@{#417685}
==========

[commit-bot: I haz the power, 2016-09-09 20:16:17]

Patchset 5 (id:??) landed as
https://crrev.com/17ad0f1ca0367fbb6d28cd9ed3d4d60a44e0e655
Cr-Commit-Position: refs/heads/master@{#417685}

[qiankun, 2016-09-10 01:16:59]

On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> On 2016/09/09 19:26:11, vmiura wrote:
> > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > TextureManager::Initialize()?
> > 
> > If so, that will only be done on the 1st context in a ContextGroup, and
> doesn't
> > seem sufficient.  We should reset this state on all new contexts.
> If I understand this correctly, we do reset it for new contexts at
ContextState
> level, but that's not early enough.  We only need to do this here (duplicated
> step) for creating black fallback textures.

Per my investigation, it's what zmo said it only cleared GL_PIXEL_UNPACK_BUFFER
when creating black fallback textures.

[vmiura, 2016-09-10 04:50:17]

On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> On 2016/09/09 19:26:11, vmiura wrote:
> > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > TextureManager::Initialize()?
> > 
> > If so, that will only be done on the 1st context in a ContextGroup, and
> doesn't
> > seem sufficient.  We should reset this state on all new contexts.
> If I understand this correctly, we do reset it for new contexts at
ContextState
> level, but that's not early enough.  We only need to do this here (duplicated
> step) for creating black fallback textures.

I might be misunderstanding something that is initializing it, so please bear
with me.

We reset the state via:

  GLContext::MakeVirtuallyCurrent()
   -> GLES2DecoderImpl::RestoreState()
     -> ContextState::RestoreState()
       -> ContextState::RestoreBufferBindings()

However, when a context is first created and made current, the Decoder is not
initialized and the call to ContextState::RestoreState() is skipped.

 
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...

This means the context may be in a bad state, until a future call to
MakeVirtuallyCurrent.

[Zhenyao Mo, 2016-09-10 15:46:59]

On 2016/09/10 04:50:17, vmiura wrote:
> On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> > On 2016/09/09 19:26:11, vmiura wrote:
> > > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > > TextureManager::Initialize()?
> > > 
> > > If so, that will only be done on the 1st context in a ContextGroup, and
> > doesn't
> > > seem sufficient.  We should reset this state on all new contexts.
> > If I understand this correctly, we do reset it for new contexts at
> ContextState
> > level, but that's not early enough.  We only need to do this here
(duplicated
> > step) for creating black fallback textures.
> 
> I might be misunderstanding something that is initializing it, so please bear
> with me.
> 
> We reset the state via:
> 
>   GLContext::MakeVirtuallyCurrent()
>    -> GLES2DecoderImpl::RestoreState()
>      -> ContextState::RestoreState()
>        -> ContextState::RestoreBufferBindings()
> 
> However, when a context is first created and made current, the Decoder is not
> initialized and the call to ContextState::RestoreState() is skipped.
> 
>  
>
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...
> 
> This means the context may be in a bad state, until a future call to
> MakeVirtuallyCurrent.

You are absolutely right.  It's not just the PIXEL_UNPACK_BUFFER, all other
states could be incorrect and could make the black textures setup go wrong, for
example, the UNPACK_* parameters.

So the best solution is to find a way to call ContextState::RestoreState().  If
that turns out to be too challenging, the next viable solution might be delay
the black textures setup and do it on demand later.

[vmiura, 2016-09-12 04:18:52]

message: On 2016/09/10 04:50:17, vmiura wrote:
> On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> > On 2016/09/09 19:26:11, vmiura wrote:
> > > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > > TextureManager::Initialize()?
> > > 
> > > If so, that will only be done on the 1st context in a ContextGroup, and
> > doesn't
> > > seem sufficient.  We should reset this state on all new contexts.
> > If I understand this correctly, we do reset it for new contexts at
> ContextState
> > level, but that's not early enough.  We only need to do this here
(duplicated
> > step) for creating black fallback textures.
> 
> I might be misunderstanding something that is initializing it, so please bear
> with me.
> 
> We reset the state via:
> 
>   GLContext::MakeVirtuallyCurrent()
>    -> GLES2DecoderImpl::RestoreState()
>      -> ContextState::RestoreState()
>        -> ContextState::RestoreBufferBindings()
> 
> However, when a context is first created and made current, the Decoder is not
> initialized and the call to ContextState::RestoreState() is skipped.
> 
>  
>
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...
> 
> This means the context may be in a bad state, until a future call to
> MakeVirtuallyCurrent.

Hmm, it looks like we do end up calling ContextState::RestoreBufferBindings()
from GLES2DecoderImpl::Initialize(), but not in a deliberate way.  Constructing
ClearFramebufferResourceManager calls GLES2DecoderImpl::GLES2DecoderImpl() to
restore the state it damages.

 
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_cle...

It seems correct to initialize this where we initialize other state here:

 
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...

We're initializing only:

  state_.InitCapabilities(NULL);
  state_.InitState(NULL);

Should we run a full ::RestoreState() here instead to ensure all virtualized
state is clean?

void ContextState::RestoreGlobalState(const ContextState* prev_state) const {
  InitCapabilities(prev_state);
  InitState(prev_state);
}

void ContextState::RestoreState(const ContextState* prev_state) {
  RestoreAllTextureUnitBindings(prev_state);
  RestoreVertexAttribs();
  RestoreBufferBindings();
  RestoreRenderbufferBindings();
  RestoreProgramSettings(prev_state, true);
  RestoreIndexedUniformBufferBindings(prev_state);
  RestoreGlobalState(prev_state);
  ...

[vmiura, 2016-09-12 04:23:01]

On 2016/09/10 15:46:59, Zhenyao Mo wrote:
> On 2016/09/10 04:50:17, vmiura wrote:
> > On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> > > On 2016/09/09 19:26:11, vmiura wrote:
> > > > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > > > TextureManager::Initialize()?
> > > > 
> > > > If so, that will only be done on the 1st context in a ContextGroup, and
> > > doesn't
> > > > seem sufficient.  We should reset this state on all new contexts.
> > > If I understand this correctly, we do reset it for new contexts at
> > ContextState
> > > level, but that's not early enough.  We only need to do this here
> (duplicated
> > > step) for creating black fallback textures.
> > 
> > I might be misunderstanding something that is initializing it, so please
bear
> > with me.
> > 
> > We reset the state via:
> > 
> >   GLContext::MakeVirtuallyCurrent()
> >    -> GLES2DecoderImpl::RestoreState()
> >      -> ContextState::RestoreState()
> >        -> ContextState::RestoreBufferBindings()
> > 
> > However, when a context is first created and made current, the Decoder is
not
> > initialized and the call to ContextState::RestoreState() is skipped.
> > 
> >  
> >
>
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...
> > 
> > This means the context may be in a bad state, until a future call to
> > MakeVirtuallyCurrent.
> 
> You are absolutely right.  It's not just the PIXEL_UNPACK_BUFFER, all other
> states could be incorrect and could make the black textures setup go wrong,
for
> example, the UNPACK_* parameters.
> 
> So the best solution is to find a way to call ContextState::RestoreState(). 
If
> that turns out to be too challenging, the next viable solution might be delay
> the black textures setup and do it on demand later.

Perhaps rather than move things up into ContextGroup::Initialize, we should
defer ContextGroup black textures initialization until the end of
GLES2DecoderImpl::Initialize() for the 1st context in the ContextGroup.

[vmiura, 2016-09-12 04:27:12]

On 2016/09/12 04:23:01, vmiura wrote:
> On 2016/09/10 15:46:59, Zhenyao Mo wrote:
> > On 2016/09/10 04:50:17, vmiura wrote:
> > > On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> > > > On 2016/09/09 19:26:11, vmiura wrote:
> > > > > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > > > > TextureManager::Initialize()?
> > > > > 
> > > > > If so, that will only be done on the 1st context in a ContextGroup,
and
> > > > doesn't
> > > > > seem sufficient.  We should reset this state on all new contexts.
> > > > If I understand this correctly, we do reset it for new contexts at
> > > ContextState
> > > > level, but that's not early enough.  We only need to do this here
> > (duplicated
> > > > step) for creating black fallback textures.
> > > 
> > > I might be misunderstanding something that is initializing it, so please
> bear
> > > with me.
> > > 
> > > We reset the state via:
> > > 
> > >   GLContext::MakeVirtuallyCurrent()
> > >    -> GLES2DecoderImpl::RestoreState()
> > >      -> ContextState::RestoreState()
> > >        -> ContextState::RestoreBufferBindings()
> > > 
> > > However, when a context is first created and made current, the Decoder is
> not
> > > initialized and the call to ContextState::RestoreState() is skipped.
> > > 
> > >  
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...
> > > 
> > > This means the context may be in a bad state, until a future call to
> > > MakeVirtuallyCurrent.
> > 
> > You are absolutely right.  It's not just the PIXEL_UNPACK_BUFFER, all other
> > states could be incorrect and could make the black textures setup go wrong,
> for
> > example, the UNPACK_* parameters.
> > 
> > So the best solution is to find a way to call ContextState::RestoreState(). 
> If
> > that turns out to be too challenging, the next viable solution might be
delay
> > the black textures setup and do it on demand later.
> 
> Perhaps rather than move things up into ContextGroup::Initialize, we should
> defer ContextGroup black textures initialization until the end of
> GLES2DecoderImpl::Initialize() for the 1st context in the ContextGroup.

lol.. sorry that's what you said at the end XP.  I think just deferring the
setup sounds like a better solution to me.  At the time ContextGroup is
initialized right now, we don't have a ContextState to restore.

[Zhenyao Mo, 2016-09-12 21:39:42]

On 2016/09/12 04:27:12, vmiura wrote:
> On 2016/09/12 04:23:01, vmiura wrote:
> > On 2016/09/10 15:46:59, Zhenyao Mo wrote:
> > > On 2016/09/10 04:50:17, vmiura wrote:
> > > > On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> > > > > On 2016/09/09 19:26:11, vmiura wrote:
> > > > > > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > > > > > TextureManager::Initialize()?
> > > > > > 
> > > > > > If so, that will only be done on the 1st context in a ContextGroup,
> and
> > > > > doesn't
> > > > > > seem sufficient.  We should reset this state on all new contexts.
> > > > > If I understand this correctly, we do reset it for new contexts at
> > > > ContextState
> > > > > level, but that's not early enough.  We only need to do this here
> > > (duplicated
> > > > > step) for creating black fallback textures.
> > > > 
> > > > I might be misunderstanding something that is initializing it, so please
> > bear
> > > > with me.
> > > > 
> > > > We reset the state via:
> > > > 
> > > >   GLContext::MakeVirtuallyCurrent()
> > > >    -> GLES2DecoderImpl::RestoreState()
> > > >      -> ContextState::RestoreState()
> > > >        -> ContextState::RestoreBufferBindings()
> > > > 
> > > > However, when a context is first created and made current, the Decoder
is
> > not
> > > > initialized and the call to ContextState::RestoreState() is skipped.
> > > > 
> > > >  
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...
> > > > 
> > > > This means the context may be in a bad state, until a future call to
> > > > MakeVirtuallyCurrent.
> > > 
> > > You are absolutely right.  It's not just the PIXEL_UNPACK_BUFFER, all
other
> > > states could be incorrect and could make the black textures setup go
wrong,
> > for
> > > example, the UNPACK_* parameters.
> > > 
> > > So the best solution is to find a way to call
ContextState::RestoreState(). 
> > If
> > > that turns out to be too challenging, the next viable solution might be
> delay
> > > the black textures setup and do it on demand later.
> > 
> > Perhaps rather than move things up into ContextGroup::Initialize, we should
> > defer ContextGroup black textures initialization until the end of
> > GLES2DecoderImpl::Initialize() for the 1st context in the ContextGroup.
> 
> lol.. sorry that's what you said at the end XP.  I think just deferring the
> setup sounds like a better solution to me.  At the time ContextGroup is
> initialized right now, we don't have a ContextState to restore.

It seems we agree on the same path forward.

Qiankun, are you willing to take a shot at this? I will be out of office in the
following a few days, but Victor can answer questions if you have any.

[qiankun, 2016-09-13 00:25:25]

On 2016/09/12 21:39:42, Zhenyao Mo wrote:
> On 2016/09/12 04:27:12, vmiura wrote:
> > On 2016/09/12 04:23:01, vmiura wrote:
> > > On 2016/09/10 15:46:59, Zhenyao Mo wrote:
> > > > On 2016/09/10 04:50:17, vmiura wrote:
> > > > > On 2016/09/09 20:05:23, Zhenyao Mo wrote:
> > > > > > On 2016/09/09 19:26:11, vmiura wrote:
> > > > > > > Can I confirm, are we clearing GL_PIXEL_UNPACK_BUFFER only in
> > > > > > > TextureManager::Initialize()?
> > > > > > > 
> > > > > > > If so, that will only be done on the 1st context in a
ContextGroup,
> > and
> > > > > > doesn't
> > > > > > > seem sufficient.  We should reset this state on all new contexts.
> > > > > > If I understand this correctly, we do reset it for new contexts at
> > > > > ContextState
> > > > > > level, but that's not early enough.  We only need to do this here
> > > > (duplicated
> > > > > > step) for creating black fallback textures.
> > > > > 
> > > > > I might be misunderstanding something that is initializing it, so
please
> > > bear
> > > > > with me.
> > > > > 
> > > > > We reset the state via:
> > > > > 
> > > > >   GLContext::MakeVirtuallyCurrent()
> > > > >    -> GLES2DecoderImpl::RestoreState()
> > > > >      -> ContextState::RestoreState()
> > > > >        -> ContextState::RestoreBufferBindings()
> > > > > 
> > > > > However, when a context is first created and made current, the Decoder
> is
> > > not
> > > > > initialized and the call to ContextState::RestoreState() is skipped.
> > > > > 
> > > > >  
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gl/gl_context.cc?sq=package:chromium&...
> > > > > 
> > > > > This means the context may be in a bad state, until a future call to
> > > > > MakeVirtuallyCurrent.
> > > > 
> > > > You are absolutely right.  It's not just the PIXEL_UNPACK_BUFFER, all
> other
> > > > states could be incorrect and could make the black textures setup go
> wrong,
> > > for
> > > > example, the UNPACK_* parameters.
> > > > 
> > > > So the best solution is to find a way to call
> ContextState::RestoreState(). 
> > > If
> > > > that turns out to be too challenging, the next viable solution might be
> > delay
> > > > the black textures setup and do it on demand later.
> > > 
> > > Perhaps rather than move things up into ContextGroup::Initialize, we
should
> > > defer ContextGroup black textures initialization until the end of
> > > GLES2DecoderImpl::Initialize() for the 1st context in the ContextGroup.
> > 
> > lol.. sorry that's what you said at the end XP.  I think just deferring the
> > setup sounds like a better solution to me.  At the time ContextGroup is
> > initialized right now, we don't have a ContextState to restore.
> 
> It seems we agree on the same path forward.
> 
> Qiankun, are you willing to take a shot at this? I will be out of office in
the
> following a few days, but Victor can answer questions if you have any.

No problem. I can take a look at this.