Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2443553002:
Create correct GLES3 context for GLES3 unittest (Closed)
DescriptionCreate correct GLES3 context for GLES3 unittest
Also, we should enable --enable-unsafe-es3-apis for gles3 tests.
BUG=429053
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
Patch Set 1 #
Total comments: 12
Patch Set 2 : remove context_type since it is equivalent to enable_es3 #
Total comments: 3
Messages
Total messages: 27 (5 generated)
Description was changed from ========== Create correct GLES3 context for GLES3 unittest Also, we should enable --enable-unsafe-es3-apis for gles3 tests. BUG=429053 ========== to ========== Create correct GLES3 context for GLES3 unittest Also, we should enable --enable-unsafe-es3-apis for gles3 tests. BUG=429053 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 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
qiankun.miao@intel.com changed reviewers: + kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
PTAL. https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/test_helper.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/test_helper.cc:578: GLenum status_rgba = GL_FRAMEBUFFER_COMPLETE; See the actual executed code here: https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info....
Thanks for cleaning this up. I do have some extra suggestions. https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/test_helper.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/test_helper.cc:593: if (status_rgba == GL_FRAMEBUFFER_COMPLETE) { I don't think I understand the motivation of this change. We don't have code path to modify |status_rgba| so this is always true. Then why do we need this? https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), This is really messy. I think the right fix is to change FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, we always set feature_info_->context_type_ to ES3, so here we don't even need the enable_es3 arg.
https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/test_helper.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/test_helper.cc:593: if (status_rgba == GL_FRAMEBUFFER_COMPLETE) { On 2016/10/21 17:51:20, Zhenyao Mo wrote: > I don't think I understand the motivation of this change. We don't have code > path to modify |status_rgba| so this is always true. Then why do we need this? Yes, this always is true. I used it to align with the actual executed code, here https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info..... But previous code is wrong here because the following code is only executed when gl_info.is_es3 == false (it's in the else branch of if statement at line 583). https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/21 17:51:20, Zhenyao Mo wrote: > This is really messy. I think the right fix is to change > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, > we always set feature_info_->context_type_ to ES3, so here we don't even need > the enable_es3 arg. We may want to create ES2 context even the ES3 apis are enabled. So, can we set default context type as ES3?
On 2016/10/22 11:47:13, qiankun wrote: > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > File gpu/command_buffer/service/test_helper.cc (right): > > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > gpu/command_buffer/service/test_helper.cc:593: if (status_rgba == > GL_FRAMEBUFFER_COMPLETE) { > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > I don't think I understand the motivation of this change. We don't have code > > path to modify |status_rgba| so this is always true. Then why do we need this? > > Yes, this always is true. I used it to align with the actual executed code, here > https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info..... > > But previous code is wrong here because the following code is only executed when > gl_info.is_es3 == false (it's in the else branch of if statement at line 583). > > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > File gpu/command_buffer/service/texture_manager_unittest.cc (right): > > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? > CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > This is really messy. I think the right fix is to change > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, > > we always set feature_info_->context_type_ to ES3, so here we don't even need > > the enable_es3 arg. > > We may want to create ES2 context even the ES3 apis are enabled. So, can we set > default context type as ES3? zmo@, please take a look at my last comments, thanks.
https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/test_helper.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/test_helper.cc:593: if (status_rgba == GL_FRAMEBUFFER_COMPLETE) { On 2016/10/22 11:47:13, qiankun wrote: > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > I don't think I understand the motivation of this change. We don't have code > > path to modify |status_rgba| so this is always true. Then why do we need this? > > Yes, this always is true. I used it to align with the actual executed code, here > https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info..... > > But previous code is wrong here because the following code is only executed when > gl_info.is_es3 == false (it's in the else branch of if statement at line 583). I see. It's fine then. https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/22 11:47:13, qiankun wrote: > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > This is really messy. I think the right fix is to change > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, > > we always set feature_info_->context_type_ to ES3, so here we don't even need > > the enable_es3 arg. > > We may want to create ES2 context even the ES3 apis are enabled. So, can we set > default context type as ES3? I don't think we want to change test behaviors. We might dramatically change the test coverage and ES2 is still critical because except WebGL2, everything else (compositing, rasterization, etc) all runs on ES3.
https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/21 17:51:20, Zhenyao Mo wrote: > This is really messy. I think the right fix is to change > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, > we always set feature_info_->context_type_ to ES3, so here we don't even need > the enable_es3 arg. So, according your last comments we cannot do this change. Keep setting context type in test when needed.
On 2016/10/25 23:08:13, qiankun wrote: > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > File gpu/command_buffer/service/texture_manager_unittest.cc (right): > > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? > CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > This is really messy. I think the right fix is to change > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, > > we always set feature_info_->context_type_ to ES3, so here we don't even need > > the enable_es3 arg. > > So, according your last comments we cannot do this change. Keep setting context > type in test when needed. That's what I meant. Let's do minimal necessary changes at this point. We are stablizing things down.
On 2016/10/25 23:44:18, Zhenyao Mo wrote: > On 2016/10/25 23:08:13, qiankun wrote: > > > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/texture_manager_unittest.cc (right): > > > > > https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... > > gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? > > CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), > > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > > This is really messy. I think the right fix is to change > > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed > in, > > > we always set feature_info_->context_type_ to ES3, so here we don't even > need > > > the enable_es3 arg. > > > > So, according your last comments we cannot do this change. Keep setting > context > > type in test when needed. > > That's what I meant. Let's do minimal necessary changes at this point. We are > stablizing things down. I followed this rule in this CL. Just modify context type to ES3 only if the test does ES3 testing.
https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/25 23:08:13, qiankun wrote: > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > This is really messy. I think the right fix is to change > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed in, > > we always set feature_info_->context_type_ to ES3, so here we don't even need > > the enable_es3 arg. > > So, according your last comments we cannot do this change. Keep setting context > type in test when needed. The inconsistency between FeatureInfo::context_type() and enable_es3 is really nasty and confusing. Totally not your fault, but my concern is this change makes it even worse. That's why I suggest if we wanna clean this, we clean it up from the root. Adding more hacks will create a debt to pay in the near future.
https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/26 00:41:06, Zhenyao Mo wrote: > On 2016/10/25 23:08:13, qiankun wrote: > > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > > This is really messy. I think the right fix is to change > > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed > in, > > > we always set feature_info_->context_type_ to ES3, so here we don't even > need > > > the enable_es3 arg. > > > > So, according your last comments we cannot do this change. Keep setting > context > > type in test when needed. > > The inconsistency between FeatureInfo::context_type() and enable_es3 is really > nasty and confusing. Totally not your fault, but my concern is this change makes > it even worse. That's why I suggest if we wanna clean this, we clean it up from > the root. Adding more hacks will create a debt to pay in the near future. Featureinfo has a default context_type which is set at construction time. SetupFeatureInfo() is used to make FeatureInfo::context_type() be consistent with which context we want to use for current test. We always have to set which context_type (es2 or es3) will be used at the beginning of each testing. I think it's clear here. Maybe I should change "enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type()" to "enable_es3 ? CONTEXT_TYPE_OPENGLES3 : CONTEXT_TYPE_OPENGLES2". The similar logic is still required after remove enable-unsafe-es3-apis flag, see yunchao's CL: https://codereview.chromium.org/2444813002.
The fix is not good, see my comments inline. Thanks a lot! https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/28 14:31:21, qiankun wrote: > On 2016/10/26 00:41:06, Zhenyao Mo wrote: > > On 2016/10/25 23:08:13, qiankun wrote: > > > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > > > This is really messy. I think the right fix is to change > > > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is passed > > in, > > > > we always set feature_info_->context_type_ to ES3, so here we don't even > > need > > > > the enable_es3 arg. > > > > > > So, according your last comments we cannot do this change. Keep setting > > context > > > type in test when needed. > > > > The inconsistency between FeatureInfo::context_type() and enable_es3 is really > > nasty and confusing. Totally not your fault, but my concern is this change > makes > > it even worse. That's why I suggest if we wanna clean this, we clean it up > from > > the root. Adding more hacks will create a debt to pay in the near future. > > Featureinfo has a default context_type which is set at construction time. > SetupFeatureInfo() is used to make FeatureInfo::context_type() be consistent > with which context we want to use for current test. We always have to set which > context_type (es2 or es3) will be used at the beginning of each testing. I think > it's clear here. Maybe I should change "enable_es3 ? CONTEXT_TYPE_OPENGLES3 : > feature_info_->context_type()" to "enable_es3 ? CONTEXT_TYPE_OPENGLES3 : > CONTEXT_TYPE_OPENGLES2". The similar logic is still required after remove > enable-unsafe-es3-apis flag, see yunchao's CL: > https://codereview.chromium.org/2444813002. The original code is not correct, but the real fix depends on 'unsafe' mode removal. And the same issue occur in other tests too, not only this one. As I have said in crbug/654787, when you run gpu_unittest with --enable-unsafe-es3-apis, you will found many failures. All of these failures are caused by the same reason. As zhenyao said, this fix is not good. Because we only need one parameter, either the 'context_type' or the 'enable_es3'. But again, it will work only if 'unsafe' mode is removed. I have a local fix for all of these failures, it works if https://codereview.chromium.org/2444813002 or a similar patch is landed (after 'unsafe' mode is removed).
Thank you all for your review. If the code doesn't look good to you, I can try to land only other part of this CL which fixes bug in TestHelper::SetupFeatureInfoInitExpectationsWithGLVersion. This is needed to fix conformance2/textures/misc/tex-image-with-bad-args.html. My point is for ES3 testing, we should enable enable-unsafe-es3-apis and use ES3 context type for current code base. But, maybe we can remove unsafe mode in a short time. So it's not a big problem:) https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/texture_manager_unittest.cc (right): https://codereview.chromium.org/2443553002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/texture_manager_unittest.cc:113: enable_es3 ? CONTEXT_TYPE_OPENGLES3 : feature_info_->context_type(), On 2016/10/31 15:18:05, yunchao wrote: > On 2016/10/28 14:31:21, qiankun wrote: > > On 2016/10/26 00:41:06, Zhenyao Mo wrote: > > > On 2016/10/25 23:08:13, qiankun wrote: > > > > On 2016/10/21 17:51:20, Zhenyao Mo wrote: > > > > > This is really messy. I think the right fix is to change > > > > > FeatureInfo::InitializeBasicState(), and if kEnableUnsafeES3APIs is > passed > > > in, > > > > > we always set feature_info_->context_type_ to ES3, so here we don't even > > > need > > > > > the enable_es3 arg. > > > > > > > > So, according your last comments we cannot do this change. Keep setting > > > context > > > > type in test when needed. > > > > > > The inconsistency between FeatureInfo::context_type() and enable_es3 is > really > > > nasty and confusing. Totally not your fault, but my concern is this change > > makes > > > it even worse. That's why I suggest if we wanna clean this, we clean it up > > from > > > the root. Adding more hacks will create a debt to pay in the near future. > > > > Featureinfo has a default context_type which is set at construction time. > > SetupFeatureInfo() is used to make FeatureInfo::context_type() be consistent > > with which context we want to use for current test. We always have to set > which > > context_type (es2 or es3) will be used at the beginning of each testing. I > think > > it's clear here. Maybe I should change "enable_es3 ? CONTEXT_TYPE_OPENGLES3 : > > feature_info_->context_type()" to "enable_es3 ? CONTEXT_TYPE_OPENGLES3 : > > CONTEXT_TYPE_OPENGLES2". The similar logic is still required after remove > > enable-unsafe-es3-apis flag, see yunchao's CL: > > https://codereview.chromium.org/2444813002. > > The original code is not correct, but the real fix depends on 'unsafe' mode > removal. And the same issue occur in other tests too, not only this one. > As I have said in crbug/654787, when you run gpu_unittest with > --enable-unsafe-es3-apis, you will found many failures. All of these failures > are caused by the same reason. > > As zhenyao said, this fix is not good. Because we only need one parameter, > either the 'context_type' or the 'enable_es3'. But again, it will work only if > 'unsafe' mode is removed. > > I have a local fix for all of these failures, it works if > https://codereview.chromium.org/2444813002 or a similar patch is landed (after > 'unsafe' mode is removed). In this CL, I fixed some ES3 testing for texture manager which blocks fix of conformance2/textures/misc/tex-image-with-bad-args.html. For current command buffer code base, I didn't find a good solution and don't intend to fix all failures related to enable-unsafe-es3-apis removal here. And even after removing enable-unsafe-es3-apis, I see you still need to initialize feature_info according context type. I think it's easy to apply the same fix here as other places when removing unsafe mode.
https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/test_helper.h (left): https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/test_helper.h:119: ContextType context_type, context_type was removed from argument list. This should be clean now. Zhenyao&Yunchao, would you mind to take a look at the new CL? Thanks!
zmo@chromium.org changed reviewers: + kainino@chromium.org
https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:4082: command_line.AppendSwitch(switches::kEnableUnsafeES3APIs); Qiankun: sorry about this, but we are in the process of removing this switch. So this CL is definitely in the opposite direction.
https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:4082: command_line.AppendSwitch(switches::kEnableUnsafeES3APIs); On 2016/11/01 22:28:45, Zhenyao Mo wrote: > Qiankun: sorry about this, but we are in the process of removing this switch. > So this CL is definitely in the opposite direction. So, we are not able to add new ES3 tests and fix bugs like conformance2/textures/misc/tex-image-with-bad-args.html before unsafe mode removal work is done. The initial motivation of this CL was to remove blocking issue to fix above CTS test failure. We just need to remove this command line for unsafe mode removal.
On 2016/11/02 01:43:14, qiankun wrote: > https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): > > https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:4082: > command_line.AppendSwitch(switches::kEnableUnsafeES3APIs); > On 2016/11/01 22:28:45, Zhenyao Mo wrote: > > Qiankun: sorry about this, but we are in the process of removing this switch. > > So this CL is definitely in the opposite direction. > > So, we are not able to add new ES3 tests and fix bugs like > conformance2/textures/misc/tex-image-with-bad-args.html before unsafe mode > removal work is done. The initial motivation of this CL was to remove blocking > issue to fix above CTS test failure. > > We just need to remove this command line for unsafe mode removal. Qiankun, sorry if I'm blocking you. I hope to get my CL done tomorrow if possible. Until then, you may go ahead with the tests using the kEnableUnsafeES3APIs switch - I can remove that when I rebase. However, your change to SetupFeatureInfoInitExpectationsWithGLVersion will conflict with mine - sorry! - We want to remove the enable_es3 flag instead of the context_type. Is it possible for you to move forward without making that change?
On 2016/11/02 03:00:25, Kai Ninomiya wrote: > On 2016/11/02 01:43:14, qiankun wrote: > > > https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc > (right): > > > > > https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:4082: > > command_line.AppendSwitch(switches::kEnableUnsafeES3APIs); > > On 2016/11/01 22:28:45, Zhenyao Mo wrote: > > > Qiankun: sorry about this, but we are in the process of removing this > switch. > > > So this CL is definitely in the opposite direction. > > > > So, we are not able to add new ES3 tests and fix bugs like > > conformance2/textures/misc/tex-image-with-bad-args.html before unsafe mode > > removal work is done. The initial motivation of this CL was to remove blocking > > issue to fix above CTS test failure. > > > > We just need to remove this command line for unsafe mode removal. > > Qiankun, sorry if I'm blocking you. I hope to get my CL done tomorrow if > possible. > > Until then, you may go ahead with the tests using the kEnableUnsafeES3APIs > switch - I can remove that when I rebase. However, your change to > SetupFeatureInfoInitExpectationsWithGLVersion will conflict with mine - sorry! - > We want to remove the enable_es3 flag instead of the context_type. Is it > possible for you to move forward without making that change? Glad to know your CL will be ready soon! Do you intend to also remove enable_es3 from SetupFeatureInfo(const char* gl_extensions, const char* gl_version, bool enable_es3)? From my side, both context_type and enable_es3 are OK to pass to SetupFeatureInfoInitExpectationsWithGLVersion.
Yes, I'm trying to remove it there too. On Tue, Nov 1, 2016, 8:15 PM <qiankun.miao@intel.com> wrote: On 2016/11/02 03:00:25, Kai Ninomiya wrote: > On 2016/11/02 01:43:14, qiankun wrote: > > > https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc > (right): > > > > > https://codereview.chromium.org/2443553002/diff/60001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:4082: > > command_line.AppendSwitch(switches::kEnableUnsafeES3APIs); > > On 2016/11/01 22:28:45, Zhenyao Mo wrote: > > > Qiankun: sorry about this, but we are in the process of removing this > switch. > > > So this CL is definitely in the opposite direction. > > > > So, we are not able to add new ES3 tests and fix bugs like > > conformance2/textures/misc/tex-image-with-bad-args.html before unsafe mode > > removal work is done. The initial motivation of this CL was to remove blocking > > issue to fix above CTS test failure. > > > > We just need to remove this command line for unsafe mode removal. > > Qiankun, sorry if I'm blocking you. I hope to get my CL done tomorrow if > possible. > > Until then, you may go ahead with the tests using the kEnableUnsafeES3APIs > switch - I can remove that when I rebase. However, your change to > SetupFeatureInfoInitExpectationsWithGLVersion will conflict with mine - sorry! - > We want to remove the enable_es3 flag instead of the context_type. Is it > possible for you to move forward without making that change? Glad to know your CL will be ready soon! Do you intend to also remove enable_es3 from SetupFeatureInfo(const char* gl_extensions, const char* gl_version, bool enable_es3)? From my side, both context_type and enable_es3 are OK to pass to SetupFeatureInfoInitExpectationsWithGLVersion. https://codereview.chromium.org/2443553002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> On 2016/11/02 03:15:26, qiankun wrote: > > Glad to know your CL will be ready soon! > > Do you intend to also remove enable_es3 from SetupFeatureInfo(const char* > > gl_extensions, const char* gl_version, bool enable_es3)? > > From my side, both context_type and enable_es3 are OK to pass to > > SetupFeatureInfoInitExpectationsWithGLVersion. > > Yes, I'm trying to remove it there too. I decided I will leave enable_es3 for SharedTextureTest::SetupFeatureInfo in my current CL. I may switch it to a ContextType afterward in a new CL.
On 2016/11/02 20:35:15, Kai Ninomiya wrote: > > On 2016/11/02 03:15:26, qiankun wrote: > > > Glad to know your CL will be ready soon! > > > Do you intend to also remove enable_es3 from SetupFeatureInfo(const char* > > > gl_extensions, const char* gl_version, bool enable_es3)? > > > From my side, both context_type and enable_es3 are OK to pass to > > > SetupFeatureInfoInitExpectationsWithGLVersion. > > > > Yes, I'm trying to remove it there too. > > I decided I will leave enable_es3 for SharedTextureTest::SetupFeatureInfo in my > current CL. I may switch it to a ContextType afterward in a new CL. The first change has landed: https://codereview.chromium.org/2471853002/ And second one is almost there: https://codereview.chromium.org/2471913003/
On 2016/11/03 02:05:09, Kai Ninomiya wrote: > On 2016/11/02 20:35:15, Kai Ninomiya wrote: > > > On 2016/11/02 03:15:26, qiankun wrote: > > > > Glad to know your CL will be ready soon! > > > > Do you intend to also remove enable_es3 from SetupFeatureInfo(const char* > > > > gl_extensions, const char* gl_version, bool enable_es3)? > > > > From my side, both context_type and enable_es3 are OK to pass to > > > > SetupFeatureInfoInitExpectationsWithGLVersion. > > > > > > Yes, I'm trying to remove it there too. > > > > I decided I will leave enable_es3 for SharedTextureTest::SetupFeatureInfo in > my > > current CL. I may switch it to a ContextType afterward in a new CL. > > The first change has landed: > > https://codereview.chromium.org/2471853002/ > > And second one is almost there: > > https://codereview.chromium.org/2471913003/ Most of this CL was superseded by the above CLs, so it should probably be closed. But aside from the NULL/nullptr change, there is one change that's different from what I did: if (status_rgba == GL_FRAMEBUFFER_COMPLETE) { I skipped that check entirely. If this matches the implementation code better, as qiankun@ suggested, it might be good to add. zmo@, what do you think? I would also add this assertion to make the intention clearer: ASSERT_TRUE(status_rgba == GL_FRAMEBUFFER_COMPLETE); if (status_rgba == GL_FRAMEBUFFER_COMPLETE) {
On 2016/11/05 19:48:10, Kai Ninomiya wrote: > On 2016/11/03 02:05:09, Kai Ninomiya wrote: > > On 2016/11/02 20:35:15, Kai Ninomiya wrote: > > > > On 2016/11/02 03:15:26, qiankun wrote: > > > > > Glad to know your CL will be ready soon! > > > > > Do you intend to also remove enable_es3 from SetupFeatureInfo(const > char* > > > > > gl_extensions, const char* gl_version, bool enable_es3)? > > > > > From my side, both context_type and enable_es3 are OK to pass to > > > > > SetupFeatureInfoInitExpectationsWithGLVersion. > > > > > > > > Yes, I'm trying to remove it there too. > > > > > > I decided I will leave enable_es3 for SharedTextureTest::SetupFeatureInfo in > > my > > > current CL. I may switch it to a ContextType afterward in a new CL. > > > > The first change has landed: > > > > https://codereview.chromium.org/2471853002/ > > > > And second one is almost there: > > > > https://codereview.chromium.org/2471913003/ > > Most of this CL was superseded by the above CLs, so it should probably be > closed. But aside from the NULL/nullptr change, there is one change that's > different from what I did: > if (status_rgba == GL_FRAMEBUFFER_COMPLETE) { > I skipped that check entirely. > > If this matches the implementation code better, as qiankun@ suggested, it might > be good to add. zmo@, what do you think? > > I would also add this assertion to make the intention clearer: > ASSERT_TRUE(status_rgba == GL_FRAMEBUFFER_COMPLETE); > if (status_rgba == GL_FRAMEBUFFER_COMPLETE) { Kai, thanks for your work. This CL can be closed now. I landed the issue you mentioned in https://codereview.chromium.org/2474593002/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
