Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272823004:
Add a base mipmap level walkaround for Intel MacOSX (Closed)
DescriptionAdd a base mipmap level walkaround for Intel MacOSX
glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not 0.
This CL simply sets it to 0 beforehand and restores it afterward.
BUG=640506
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/e58b47ecc4ef49e2b43526131cdd35ac00055c18
Cr-Commit-Position: refs/heads/master@{#414719}
Patch Set 1 #Patch Set 2 : Fix the unintialized variable #Patch Set 3 : Whitelist the expectation for all macosx platforms #Patch Set 4 : test other GPUs without the walkaround #
Total comments: 1
Patch Set 5 : use texture->base_level() instead #Patch Set 6 : rebase #Patch Set 7 : re-rebase #
Total comments: 1
Messages
Total messages: 64 (49 generated)
Description was changed from ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not zero. This forcedly sets it 0 beforehand and restores it afterward. BUG= ========== to ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not zero. This forcedly sets it 0 beforehand and restores it afterward. BUG= 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 ==========
Description was changed from ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not zero. This forcedly sets it 0 beforehand and restores it afterward. BUG= 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 ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not zero. This forcedly sets it 0 beforehand and restores it afterward. BUG=640506 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 ==========
The CQ bit was checked by jie.a.chen@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: 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 jie.a.chen@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: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
The CQ bit was checked by jie.a.chen@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 checked by jie.a.chen@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jie.a.chen@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.
The CQ bit was checked by jie.a.chen@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jie.a.chen@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.
Description was changed from ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not zero. This forcedly sets it 0 beforehand and restores it afterward. BUG=640506 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 ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not 0. This CL simply sets it to 0 beforehand and restores it afterward. BUG=640506 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 ==========
jie.a.chen@intel.com changed reviewers: + kbr@chromium.org, qiankun.miao@intel.com, yunchao.he@intel.com, zmo@chromium.org
Please have a look! BTW, the test has passed on other MacOSX platforms already, so I incidentally whitelist them as well.
Great finding! A minor optimization, otherwise looks good. Can you add a simple conformance test to demonstrate this bug so we can show Apple? (although it's caught in texturesize.html, but it's hard to tell) https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:15701: GLint base_level = 0; You should just use texture->base_level(), and no need to query the driver later.
The CQ bit was checked by jie.a.chen@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...
On 2016/08/25 18:06:55, Zhenyao Mo wrote: > Great finding! > > A minor optimization, otherwise looks good. > > Can you add a simple conformance test to demonstrate this bug so we can show > Apple? (although it's caught in texturesize.html, but it's hard to tell) > > https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder.cc:15701: GLint base_level = 0; > You should just use texture->base_level(), and no need to query the driver > later. Okay! I will add it soon, Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by jie.a.chen@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jie.a.chen@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jie.a.chen@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.
On 2016/08/26 00:42:51, jchen10 wrote: > On 2016/08/25 18:06:55, Zhenyao Mo wrote: > > Great finding! > > > > A minor optimization, otherwise looks good. > > > > Can you add a simple conformance test to demonstrate this bug so we can show > > Apple? (although it's caught in texturesize.html, but it's hard to tell) > > > > > https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:15701: GLint base_level = 0; > > You should just use texture->base_level(), and no need to query the driver > > later. > > Okay! I will add it soon, Thanks! Please have a look at the test: https://github.com/KhronosGroup/WebGL/pull/1990! Thanks!
On 2016/08/26 13:13:11, jchen10 wrote: > On 2016/08/26 00:42:51, jchen10 wrote: > > On 2016/08/25 18:06:55, Zhenyao Mo wrote: > > > Great finding! > > > > > > A minor optimization, otherwise looks good. > > > > > > Can you add a simple conformance test to demonstrate this bug so we can show > > > Apple? (although it's caught in texturesize.html, but it's hard to tell) > > > > > > > > > https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > > > > > https://codereview.chromium.org/2272823004/diff/60001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:15701: GLint base_level = 0; > > > You should just use texture->base_level(), and no need to query the driver > > > later. > > > > Okay! I will add it soon, Thanks! > > Please have a look at the test: https://github.com/KhronosGroup/WebGL/pull/1990! > Thanks! LGTM
The CQ bit was checked by zmo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not 0. This CL simply sets it to 0 beforehand and restores it afterward. BUG=640506 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 ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not 0. This CL simply sets it to 0 beforehand and restores it afterward. BUG=640506 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 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not 0. This CL simply sets it to 0 beforehand and restores it afterward. BUG=640506 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 ========== Add a base mipmap level walkaround for Intel MacOSX glTexStorage* work incorrectly if the GL_TEXTURE_BASE_LEVEL is not 0. This CL simply sets it to 0 beforehand and restores it afterward. BUG=640506 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/e58b47ecc4ef49e2b43526131cdd35ac00055c18 Cr-Commit-Position: refs/heads/master@{#414719} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e58b47ecc4ef49e2b43526131cdd35ac00055c18 Cr-Commit-Position: refs/heads/master@{#414719}
Message was sent while issue was closed.
https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", Curious: the workaround is for Mac Intel only, but why the failing tests also begin to pass on Mac AMD and Mac NVidia? (See the removed entries)
Message was sent while issue was closed.
On 2016/08/26 17:38:58, Zhenyao Mo wrote: > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > File gpu/config/gpu_driver_bug_list_json.cc (right): > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", > Curious: the workaround is for Mac Intel only, but why the failing tests also > begin to pass on Mac AMD and Mac NVidia? (See the removed entries) Fails on https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%...
Message was sent while issue was closed.
On 2016/08/26 21:08:38, ynovikov wrote: > On 2016/08/26 17:38:58, Zhenyao Mo wrote: > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", > > Curious: the workaround is for Mac Intel only, but why the failing tests also > > begin to pass on Mac AMD and Mac NVidia? (See the removed entries) > > Fails on > https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%... We will need to add back the entry
Message was sent while issue was closed.
On 2016/08/26 17:38:58, Zhenyao Mo wrote: > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > File gpu/config/gpu_driver_bug_list_json.cc (right): > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", > Curious: the workaround is for Mac Intel only, but why the failing tests also > begin to pass on Mac AMD and Mac NVidia? (See the removed entries) The walkaround itself is irrelevant to AMD and NV. I occasionally found trybots had been able to pass on AMD and NV as well, so I removed entries to keep the expectation current.
Message was sent while issue was closed.
On 2016/08/26 21:17:22, Zhenyao Mo wrote: > On 2016/08/26 21:08:38, ynovikov wrote: > > On 2016/08/26 17:38:58, Zhenyao Mo wrote: > > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > > gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", > > > Curious: the workaround is for Mac Intel only, but why the failing tests > also > > > begin to pass on Mac AMD and Mac NVidia? (See the removed entries) > > > > Fails on > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%... > > We will need to add back the entry My MacbookPro has dual GPUs, Intel and AMD. The test fails if I switches to AMD GPU. And the walkaround can also make the test pass for AMD. But according to trybots, AMD and NV could pass without it. So I am not sure whether to apply the workaround to AMD as well, or simply add the entry back to the expectation.
Message was sent while issue was closed.
On 2016/08/26 23:05:00, jchen10 wrote: > On 2016/08/26 21:17:22, Zhenyao Mo wrote: > > On 2016/08/26 21:08:38, ynovikov wrote: > > > On 2016/08/26 17:38:58, Zhenyao Mo wrote: > > > > > > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > > > gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", > > > > Curious: the workaround is for Mac Intel only, but why the failing tests > > also > > > > begin to pass on Mac AMD and Mac NVidia? (See the removed entries) > > > > > > Fails on > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%... > > > > We will need to add back the entry > > My MacbookPro has dual GPUs, Intel and AMD. The test fails if I switches to AMD > GPU. And the walkaround can also make the test pass for AMD. But according to > trybots, AMD and NV could pass without it. So I am not sure whether to apply the > workaround to AMD as well, or simply add the entry back to the expectation. That's a different Mac GPU from the ones on the try server.
Message was sent while issue was closed.
On 2016/08/26 23:41:57, Zhenyao Mo wrote: > On 2016/08/26 23:05:00, jchen10 wrote: > > On 2016/08/26 21:17:22, Zhenyao Mo wrote: > > > On 2016/08/26 21:08:38, ynovikov wrote: > > > > On 2016/08/26 17:38:58, Zhenyao Mo wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > > > > File gpu/config/gpu_driver_bug_list_json.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2272823004/diff/120001/gpu/config/gpu_driver_... > > > > > gpu/config/gpu_driver_bug_list_json.cc:1967: "vendor_id": "0x8086", > > > > > Curious: the workaround is for Mac Intel only, but why the failing tests > > > also > > > > > begin to pass on Mac AMD and Mac NVidia? (See the removed entries) > > > > > > > > Fails on > > > > > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%... > > > > > > We will need to add back the entry > > > > My MacbookPro has dual GPUs, Intel and AMD. The test fails if I switches to > AMD > > GPU. And the walkaround can also make the test pass for AMD. But according to > > trybots, AMD and NV could pass without it. So I am not sure whether to apply > the > > workaround to AMD as well, or simply add the entry back to the expectation. > > That's a different Mac GPU from the ones on the try server. Suppressing this failure just on this AMD GPU in https://codereview.chromium.org/2281073004/ . For future reference, you can find the PCI IDs of the GPUs in any of the Telemetry-based GPU tests. It's in the info log dumped near the start of the test. You can look at trace_test or some of the smaller tests in order to have to look through fewer logs. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
