Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827573007:
Reset TexImage2D base level to workaround Intel mac driver bug (Closed)
|
Created:
3 years, 8 months ago by yizhou.jiang Modified:
3 years, 6 months ago CC:
chromium-reviews, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset TexImage2D base level to workaround Intel mac driver bug.
If texture's base level is not 0, update texture, level 0
will result in wrong pixel of base level image. To workaround
the bug, we could reset texture base level to 0 before
texImage2D and recover base level later.
BUG=705865
TEST=conformance2/textures/misc/tex-base-level-bug.html
TEST=conformance2/textures/misc/tex-mipmap-levels.html
TEST=Service/GLES3DecoderManualInitTest.SetTextureBaseLevelBeforeTexImage2D*
TEST=TextureManagerTest.SetTextureBaseLevelBeforeTexImage2D
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
Patch Set 1 : Workaround texImage2D bug on macos. #Patch Set 2 : Workaround texImage2D bug on macos. #Patch Set 3 : Reset texture base level to workaround bug of texImage2D on macos. #
Total comments: 9
Patch Set 4 : revise driver bug list #Patch Set 5 : Refactor #
Total comments: 8
Patch Set 6 : Add gpu unittests #
Total comments: 5
Messages
Total messages: 81 (65 generated)
Description was changed from ========== Workaround texImage2D bug on macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel in reading base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= ========== to ========== Workaround texImage2D bug on macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel in reading base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= 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 ==========
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
The CQ bit was unchecked by yizhou.jiang@intel.com
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Workaround texImage2D bug on macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel in reading base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= 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 ========== Workaround texImage2D bug on macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel in reading base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= 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 ==========
yizhou.jiang@intel.com changed reviewers: + qiankun.miao@intel.com
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Workaround texImage2D bug on macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel in reading base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= 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 ========== Reset texImage2D base level in gles2_cmd_decoder.cc to workaround driver bug of macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= 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 ==========
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reset texImage2D base level in gles2_cmd_decoder.cc to workaround driver bug of macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG= 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 ========== Reset texImage2D base level in gles2_cmd_decoder.cc to workaround driver bug of macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yizhou.jiang@intel.com changed reviewers: + yunchao.he@intel.com
On 2017/04/25 02:00:13, yizhou.jiang wrote: > mailto:yizhou.jiang@intel.com changed reviewers: > + mailto:yunchao.he@intel.com yunchao and qiankun, the patch reset texImage2D base level to workaround mac driver bug. Cts test is under review(https://github.com/KhronosGroup/WebGL/pull/2378). PTAL.
In description, "Reset texImage2D base level in gles2_cmd_decoder.cc to workaround driver bug of macos." isn't needed. Add tile of "Reset texImage2D base level to workaround driver bug" to description. https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:326: // texImage2D Merge line 325 and 326. https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:2609: if (texture_ref_ && Isn't texture_ref_ always non-null? You can just pass in "GLint base_level" and "bool reset_base_level" (a flag indicating if do the resetting or not). https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:3264: if (texture_ref && texture_ref should be always non-null here. https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_list.json (right): https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_list.json:2381: "cr_bugs": [705865], Restrict vendor here. We only saw the bug on Intel Mac. You can check if it exists for AMD Mac. https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_list.json:2383: "type": "macosx" add MacOS version restriction.
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yunchao and qiankun, please take another look. https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:326: // texImage2D On 2017/04/26 04:16:20, qiankun wrote: > Merge line 325 and 326. Done. https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:2609: if (texture_ref_ && On 2017/04/26 04:16:19, qiankun wrote: > Isn't texture_ref_ always non-null? You can just pass in "GLint base_level" and > "bool reset_base_level" (a flag indicating if do the resetting or not). Done. https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:3264: if (texture_ref && On 2017/04/26 04:16:20, qiankun wrote: > texture_ref should be always non-null here. Done. https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_list.json (right): https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_list.json:2381: "cr_bugs": [705865], On 2017/04/26 04:16:20, qiankun wrote: > Restrict vendor here. We only saw the bug on Intel Mac. You can check if it > exists for AMD Mac. Currently add Intel vendor ID and OS version as we don't have env for AMD or Nvidia on macos over 10.12.4.
Description was changed from ========== Reset texImage2D base level in gles2_cmd_decoder.cc to workaround driver bug of macos. If texture's base level is not 0, update texture level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 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 ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 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 ==========
Patchset #5 (id:200001) has been deleted
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:220001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yizhou.jiang@intel.com changed reviewers: + kbr@chromium.org, zmo@chromium.org
Description was changed from ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 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 ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 TEST=conformance2/textures/misc/tex-base-level-bug.html 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 ==========
On 2017/04/27 04:23:22, yizhou.jiang wrote: > yunchao and qiankun, please take another look. > > https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:326: // texImage2D > On 2017/04/26 04:16:20, qiankun wrote: > > Merge line 325 and 326. > > Done. > > https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:2609: if (texture_ref_ && > On 2017/04/26 04:16:19, qiankun wrote: > > Isn't texture_ref_ always non-null? You can just pass in "GLint base_level" > and > > "bool reset_base_level" (a flag indicating if do the resetting or not). > > Done. > > https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... > File gpu/command_buffer/service/texture_manager.cc (right): > > https://codereview.chromium.org/2827573007/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/texture_manager.cc:3264: if (texture_ref && > On 2017/04/26 04:16:20, qiankun wrote: > > texture_ref should be always non-null here. > > Done. > > https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... > File gpu/config/gpu_driver_bug_list.json (right): > > https://codereview.chromium.org/2827573007/diff/120001/gpu/config/gpu_driver_... > gpu/config/gpu_driver_bug_list.json:2381: "cr_bugs": [705865], > On 2017/04/26 04:16:20, qiankun wrote: > > Restrict vendor here. We only saw the bug on Intel Mac. You can check if it > > exists for AMD Mac. > > Currently add Intel vendor ID and OS version as we don't have env for AMD or > Nvidia on macos over 10.12.4. @zmo @kbr The patch fix Intel mac driver bug. Please take a look. Thanks.
Description was changed from ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 TEST=conformance2/textures/misc/tex-base-level-bug.html 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 ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 TEST=conformance2/textures/misc/tex-base-level-bug.html TEST=conformance2/textures/misc/tex-mipmap-levels.html 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 ==========
kbr@chromium.org changed reviewers: + jbauman@chromium.org
Please add some unit tests for this, for example to gles2_cmd_decoder_unittest_textures.cc and texture_manager_unittest.cc . Create a FeatureInfo with this workaround enabled, and ensure that the expected calls are made. A few other concerns and questions. CC'ing jbauman as zmo's on vacation. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:325: // Reset a texture's base level and restore it after calling texImage2D Please revise: "Reset the base level of the texture currently bound to the GL_TEXTURE_2D target and restore it after calling texImage2D." But also see the comment below about the texture target. If this is generalized, then revise this comment to match. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:329: bool reset_base_level) Please change the constructor to take in the target as an argument and DCHECK(target == GL_TEXTURE_2D) inside this. Otherwise it will be too easy for this class to be misused and do the wrong thing. Or: if this workaround may be needed for other texture targets like GL_TEXTURE_RECTANGLE_ARB too, just have this class apply the workaround to the target. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:6319: glTexImage2D(target, 0, internal_format, 1, 1, 0, format, type, nullptr); Can you think of any way to add a TexImage2DHelper to the command buffer, put the use of the ScopedTextureBaseLevelResetter inside it, and change the glTexImage2D calls here and throughout to call it? The fact that glTexImage2D calls need to be paired with ScopedTextureBaseLevelResetter is error-prone and will lead to bugs in the future as other people edit this file and add more calls to glTexImage2D. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:3265: glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, 0); Does this have the correct effect in all cases? The glTexImage2D call below refers to args.target, not GL_TEXTURE_2D. Is this necessary even when args.target != GL_TEXTURE_2D?
yizhou.jiang@intel.com changed reviewers: + yang.gu@intel.com
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
The CQ bit was checked by yizhou.jiang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@Ken @Zhenyao, Sorry to reply you after so long. I was on vacation last month. I have added two gpu unit tests and updated the code. Please take a look when you have time. Thanks! https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:325: // Reset a texture's base level and restore it after calling texImage2D On 2017/04/28 19:04:25, Ken Russell wrote: > Please revise: "Reset the base level of the texture currently bound to the > GL_TEXTURE_2D target and restore it after calling texImage2D." > > But also see the comment below about the texture target. If this is generalized, > then revise this comment to match. Done. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:329: bool reset_base_level) On 2017/04/28 19:04:25, Ken Russell wrote: > Please change the constructor to take in the target as an argument and > DCHECK(target == GL_TEXTURE_2D) inside this. Otherwise it will be too easy for > this class to be misused and do the wrong thing. > > Or: if this workaround may be needed for other texture targets like > GL_TEXTURE_RECTANGLE_ARB too, just have this class apply the workaround to the > target. Done. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:6319: glTexImage2D(target, 0, internal_format, 1, 1, 0, format, type, nullptr); On 2017/04/28 19:04:25, Ken Russell wrote: > Can you think of any way to add a TexImage2DHelper to the command buffer, put > the use of the ScopedTextureBaseLevelResetter inside it, and change the > glTexImage2D calls here and throughout to call it? > > The fact that glTexImage2D calls need to be paired with > ScopedTextureBaseLevelResetter is error-prone and will lead to bugs in the > future as other people edit this file and add more calls to glTexImage2D. Done. https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2827573007/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:3265: glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, 0); On 2017/04/28 19:04:25, Ken Russell wrote: > Does this have the correct effect in all cases? The glTexImage2D call below > refers to args.target, not GL_TEXTURE_2D. Is this necessary even when > args.target != GL_TEXTURE_2D? No, I have only investigated GL_TEXTURE_2D, I will add check if args.target is GL_TEXTURE_2D. https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.h:1138: I move DoTexImage to public in order to test it in Texture_manager_unittests.cc.
Description was changed from ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 TEST=conformance2/textures/misc/tex-base-level-bug.html TEST=conformance2/textures/misc/tex-mipmap-levels.html 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 ========== Reset TexImage2D base level to workaround Intel mac driver bug. If texture's base level is not 0, update texture, level 0 will result in wrong pixel of base level image. To workaround the bug, we could reset texture base level to 0 before texImage2D and recover base level later. BUG=705865 TEST=conformance2/textures/misc/tex-base-level-bug.html TEST=conformance2/textures/misc/tex-mipmap-levels.html TEST=Service/GLES3DecoderManualInitTest.SetTextureBaseLevelBeforeTexImage2D* TEST=TextureManagerTest.SetTextureBaseLevelBeforeTexImage2D 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 ==========
This is really not a good solution. I did a grep, in gpu/command_buffer/service, there are these files that contain glTexImage2D calls feature_info.cc gles2_cmd_copy_tex_image.cc gles2_cmd_copy_texture_chromium.cc gles2_cmd_decoder.cc gles2_cmd_srgb_converter.cc texture_manager.cc Your CL only covers some of these. Even if you cover them all, there is no guarantee that future CLs that call into glTexImage2D will remember to go through this workaround. I'd rather not introduce this messy workaround all over the places that are destined to fail. Instead, maybe you can look into wiring up this workaround into ui/gl/gl_gl_api_implementation.cc I haven't really studied so I can't say if it's feasible and how much wiring there needs to be. https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:331: explicit ScopedTextureBaseLevelResetter(GLenum target, nit: no need for explicit because you have three args not one. https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:338: if (reset_base_level_) { && base_level https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:344: if (reset_base_level_) { && base_level https://codereview.chromium.org/2827573007/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:699: friend class ScopedTextureBaseLevelResetter; Why this needs to be a friend?
At this point, I'd say let's get a workaround in ANGLE and forget about command buffer side.
On 2017/06/09 21:28:50, Zhenyao Mo wrote: > At this point, I'd say let's get a workaround in ANGLE and forget about command > buffer side. Ok. I will close this patch.
Message was sent while issue was closed.
On 2017/06/09 21:28:50, Zhenyao Mo wrote: > At this point, I'd say let's get a workaround in ANGLE and forget about command > buffer side. @Zhenyao, I tried to modify angle code in chromium on mac, then build with ninja -C out/Debug chrome, but the angle code isn't compiled. In addition, Chromium crashed with cmd srgs like --pass-through-cmd-decoder or --use-gl=angle. I guess angle api translation is not supported in chromium on mac. I have raised a question in the angle project forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE).
Message was sent while issue was closed.
On 2017/06/13 06:50:49, yizhou.jiang wrote: > On 2017/06/09 21:28:50, Zhenyao Mo wrote: > > At this point, I'd say let's get a workaround in ANGLE and forget about > command > > buffer side. > > @Zhenyao, I tried to modify angle code in chromium on mac, then build with ninja > -C out/Debug chrome, but the angle code isn't compiled. In addition, Chromium > crashed with cmd srgs like --pass-through-cmd-decoder or --use-gl=angle. I > guess angle api translation is not supported in chromium on mac. I have raised a > question in the angle project > forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE). I don't think they work on Mac yet. If you do it on Windows or probably Linux, then they should work.
Message was sent while issue was closed.
On 2017/06/13 17:27:32, Zhenyao Mo wrote: > On 2017/06/13 06:50:49, yizhou.jiang wrote: > > On 2017/06/09 21:28:50, Zhenyao Mo wrote: > > > At this point, I'd say let's get a workaround in ANGLE and forget about > > command > > > buffer side. > > > > @Zhenyao, I tried to modify angle code in chromium on mac, then build with > ninja > > -C out/Debug chrome, but the angle code isn't compiled. In addition, Chromium > > crashed with cmd srgs like --pass-through-cmd-decoder or --use-gl=angle. I > > guess angle api translation is not supported in chromium on mac. I have raised > a > > question in the angle project > > forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE). > > I don't think they work on Mac yet. If you do it on Windows or probably Linux, > then they should work. @Zhenyao, the bug is detected on Intel driver on Mac OS 10.12.4, we can't workaround it in angle now since angle isn't supported in Chromium on Mac yet. So could you file a radar to Apple, we can help to write a radar description for this issue.
Message was sent while issue was closed.
On 2017/06/14 02:51:26, yizhou.jiang wrote: > On 2017/06/13 17:27:32, Zhenyao Mo wrote: > > On 2017/06/13 06:50:49, yizhou.jiang wrote: > > > On 2017/06/09 21:28:50, Zhenyao Mo wrote: > > > > At this point, I'd say let's get a workaround in ANGLE and forget about > > > command > > > > buffer side. > > > > > > @Zhenyao, I tried to modify angle code in chromium on mac, then build with > > ninja > > > -C out/Debug chrome, but the angle code isn't compiled. In addition, > Chromium > > > crashed with cmd srgs like --pass-through-cmd-decoder or --use-gl=angle. I > > > guess angle api translation is not supported in chromium on mac. I have > raised > > a > > > question in the angle project > > > forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE). > > > > I don't think they work on Mac yet. If you do it on Windows or probably > Linux, > > then they should work. > > @Zhenyao, the bug is detected on Intel driver on Mac OS 10.12.4, we can't > workaround it in angle now since angle isn't supported in Chromium on Mac yet. > So could you file a radar to Apple, we can help to write a radar description for > this issue. Sure. We can file a radar. Please consider working it around in ui/gl/gl_gl_api_implementation.cc.
Message was sent while issue was closed.
On 2017/06/14 03:13:40, Zhenyao Mo wrote: > On 2017/06/14 02:51:26, yizhou.jiang wrote: > > On 2017/06/13 17:27:32, Zhenyao Mo wrote: > > > On 2017/06/13 06:50:49, yizhou.jiang wrote: > > > > On 2017/06/09 21:28:50, Zhenyao Mo wrote: > > > > > At this point, I'd say let's get a workaround in ANGLE and forget about > > > > command > > > > > buffer side. > > > > > > > > @Zhenyao, I tried to modify angle code in chromium on mac, then build with > > > ninja > > > > -C out/Debug chrome, but the angle code isn't compiled. In addition, > > Chromium > > > > crashed with cmd srgs like --pass-through-cmd-decoder or --use-gl=angle. > I > > > > guess angle api translation is not supported in chromium on mac. I have > > raised > > > a > > > > question in the angle project > > > > forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE). > > > > > > I don't think they work on Mac yet. If you do it on Windows or probably > > Linux, > > > then they should work. > > > > @Zhenyao, the bug is detected on Intel driver on Mac OS 10.12.4, we can't > > workaround it in angle now since angle isn't supported in Chromium on Mac yet. > > So could you file a radar to Apple, we can help to write a radar description > for > > this issue. > > Sure. We can file a radar. > > Please consider working it around in ui/gl/gl_gl_api_implementation.cc. Ok. I have written a radar description doc(https://docs.google.com/a/intel.com/document/d/1gaUHcTNcmuKCkegQrbNJTc9BZl_p2rhILbO9glee0lY/edit?usp=sharing), please help to file it.
Message was sent while issue was closed.
On 2017/06/14 08:57:25, yizhou.jiang wrote: > On 2017/06/14 03:13:40, Zhenyao Mo wrote: > > On 2017/06/14 02:51:26, yizhou.jiang wrote: > > > On 2017/06/13 17:27:32, Zhenyao Mo wrote: > > > > On 2017/06/13 06:50:49, yizhou.jiang wrote: > > > > > On 2017/06/09 21:28:50, Zhenyao Mo wrote: > > > > > > At this point, I'd say let's get a workaround in ANGLE and forget > about > > > > > command > > > > > > buffer side. > > > > > > > > > > @Zhenyao, I tried to modify angle code in chromium on mac, then build > with > > > > ninja > > > > > -C out/Debug chrome, but the angle code isn't compiled. In addition, > > > Chromium > > > > > crashed with cmd srgs like --pass-through-cmd-decoder or --use-gl=angle. > > > I > > > > > guess angle api translation is not supported in chromium on mac. I have > > > raised > > > > a > > > > > question in the angle project > > > > > forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE). > > > > > > > > I don't think they work on Mac yet. If you do it on Windows or probably > > > Linux, > > > > then they should work. > > > > > > @Zhenyao, the bug is detected on Intel driver on Mac OS 10.12.4, we can't > > > workaround it in angle now since angle isn't supported in Chromium on Mac > yet. > > > So could you file a radar to Apple, we can help to write a radar description > > for > > > this issue. > > > > Sure. We can file a radar. > > > > Please consider working it around in ui/gl/gl_gl_api_implementation.cc. > > Ok. I have written a radar description > doc(https://docs.google.com/a/intel.com/document/d/1gaUHcTNcmuKCkegQrbNJTc9BZl_p2rhILbO9glee0lY/edit?usp=sharing), > please help to file it. Thanks. Edited your description; please see the changes I made for future Radars. Filed the revised description as Radar 32803036.
Message was sent while issue was closed.
On 2017/06/15 23:21:09, Ken Russell wrote: > On 2017/06/14 08:57:25, yizhou.jiang wrote: > > On 2017/06/14 03:13:40, Zhenyao Mo wrote: > > > On 2017/06/14 02:51:26, yizhou.jiang wrote: > > > > On 2017/06/13 17:27:32, Zhenyao Mo wrote: > > > > > On 2017/06/13 06:50:49, yizhou.jiang wrote: > > > > > > On 2017/06/09 21:28:50, Zhenyao Mo wrote: > > > > > > > At this point, I'd say let's get a workaround in ANGLE and forget > > about > > > > > > command > > > > > > > buffer side. > > > > > > > > > > > > @Zhenyao, I tried to modify angle code in chromium on mac, then build > > with > > > > > ninja > > > > > > -C out/Debug chrome, but the angle code isn't compiled. In addition, > > > > Chromium > > > > > > crashed with cmd srgs like --pass-through-cmd-decoder or > --use-gl=angle. > > > > > I > > > > > > guess angle api translation is not supported in chromium on mac. I > have > > > > raised > > > > > a > > > > > > question in the angle project > > > > > > > forum(https://groups.google.com/forum/#!topic/angleproject/Tz34p471YpE). > > > > > > > > > > I don't think they work on Mac yet. If you do it on Windows or probably > > > > Linux, > > > > > then they should work. > > > > > > > > @Zhenyao, the bug is detected on Intel driver on Mac OS 10.12.4, we can't > > > > workaround it in angle now since angle isn't supported in Chromium on Mac > > yet. > > > > So could you file a radar to Apple, we can help to write a radar > description > > > for > > > > this issue. > > > > > > Sure. We can file a radar. > > > > > > Please consider working it around in ui/gl/gl_gl_api_implementation.cc. > > > > Ok. I have written a radar description > > > doc(https://docs.google.com/a/intel.com/document/d/1gaUHcTNcmuKCkegQrbNJTc9BZl_p2rhILbO9glee0lY/edit?usp=sharing), > > please help to file it. > > Thanks. Edited your description; please see the changes I made for future > Radars. > > Filed the revised description as Radar 32803036. Thanks for your help! |
