Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2131863003:
WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. (Closed)
DescriptionWebGL: use storage texture for DrawingBuffer only if CMAA is enabled.
It's because Linux ATI bot fails
WebglConformance.conformance_textures_misc_tex_image_webgl
When finding the solution for ATI driver, rollback this patch.
BUG=557848
TEST=WebglConformance.conformance_textures_misc_tex_image_webgl
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/65f8f6c737278a80acee2bcd407d1fdc15426fa8
Cr-Commit-Position: refs/heads/master@{#404514}
Patch Set 1 #Patch Set 2 : WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. #Patch Set 3 : WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. #Messages
Total messages: 29 (7 generated)
Description was changed from ========== gpu: disable immutable texture for linux ATI drivers. It's because linux ATI drivers have the issue to bind an immutable texture to FBO. It's why Linux release ATI bot is broken. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl ========== to ========== gpu: disable immutable texture for linux ATI drivers. It's because linux ATI drivers have the issue to bind an immutable texture to FBO. It's why Linux release ATI bot is broken. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
dongseong.hwang@intel.com changed reviewers: + kbr@chromium.org, piman@chromium.org, zmo@chromium.org
piman@, could you review? After https://codereview.chromium.org/2125023002/ was landed, Linux ATI bot is broken. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... Only linux ATI bot is broken, because of WebglConformance.conformance_textures_misc_tex_image_webgl failure. The CL mades WebGL default FBO use immutable texture. conformance_textures_misc_tex_image_webgl checks texImage2D(another WebGL context). So ATI driver must have issue in CopyTextureCHROMIUM path. It must be the issue related to immutable texture and FBO. So this CL disables immutable texture on linux ATI.
On 2016/07/08 11:26:48, dshwang wrote: > piman@, could you review? > > After https://codereview.chromium.org/2125023002/ was landed, Linux ATI bot is > broken. > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > Only linux ATI bot is broken, because of > WebglConformance.conformance_textures_misc_tex_image_webgl failure. > > The CL mades WebGL default FBO use immutable texture. > conformance_textures_misc_tex_image_webgl checks texImage2D(another WebGL > context). So ATI driver must have issue in CopyTextureCHROMIUM path. > It must be the issue related to immutable texture and FBO. So this CL disables > immutable texture on linux ATI. Sorry but I disagree with this. We have TexStorage tests and they are passing fine on Linux AMD. TexStorage is a core functionality for ES3. I would not disable it for this test failure. You should dig a bit further to understand the true nature of this failure.
On 2016/07/08 13:37:35, Zhenyao Mo wrote: > Sorry but I disagree with this. We have TexStorage tests and they are passing > fine on Linux AMD. TexStorage is a core functionality for ES3. I would not > disable it for this test failure. You should dig a bit further to understand > the true nature of this failure. Thank you for feedback. I'm digging, but my all digging is speculative because I don't have ATI gpu. I want to upload new CL and make Linux Release (ATI) bot to test my hypothesis. Can I kick Linux Release (ATI) bot for a certain CL?
BTW, Linux Release (ATI) bot doesn't show browser LOG(ERROR). It said "No log
file" :(
Could someone fix it? If so, I can read LOG(ERROR) in the next run of the bot.
[ RUN ] WebglConformance.conformance_textures_misc_tex_image_webgl
(INFO) 2016-07-08 04:52:31,285 cache_temperature.EnsurePageCacheTemperature:55
PageCacheTemperature: any
[10798:10798:0708/045231:INFO:CONSOLE(11)] "at (0, 0) expected: 0,255,0,255 was
255,0,0,255", source: (11)
[10798:10798:0708/045231:INFO:CONSOLE(11)] "FAIL at (0, 0) expected: 0,255,0,255
was 255,0,0,255", source: (11)
(WARNING) 2016-07-08 04:52:31,528
desktop_browser_backend._GetMostRecentCrashpadMinidump:345 No path to
crashpad_database_util found
(INFO) 2016-07-08 04:52:31,528
desktop_browser_backend._GetMostRecentMinidump:406 No minidump found via
crashpad_database_util
(INFO) 2016-07-08 04:52:31,529 browser.DumpStateUponFailure:326 ***************
BROWSER STANDARD OUTPUT ***************
Can't get standard output with --show-stdout
(INFO) 2016-07-08 04:52:31,529 browser.DumpStateUponFailure:328
(INFO) 2016-07-08 04:52:31,529 browser.DumpStateUponFailure:331 *********** END
OF BROWSER STANDARD OUTPUT ************
(INFO) 2016-07-08 04:52:31,529 browser.DumpStateUponFailure:333
********************* BROWSER LOG *********************
(INFO) 2016-07-08 04:52:31,529 browser.DumpStateUponFailure:335 No log file
(INFO) 2016-07-08 04:52:31,529 browser.DumpStateUponFailure:338
***************** END OF BROWSER LOG ******************
(WARNING) 2016-07-08 04:52:31,529 shared_page_state.DumpStateUponFailure:144
Taking screenshots upon failures disabled.
Traceback (most recent call last):
File
"/tmp/isolated_run4X_bVC/third_party/catapult/telemetry/telemetry/internal/story_runner.py",
line 85, in _RunStoryAndProcessErrorIfNeeded
state.RunStory(results)
File "/tmp/isolated_run4X_bVC/content/test/gpu/gpu_tests/gpu_test_base.py",
line 122, in RunStory
RunStoryWithRetries(DesktopGpuSharedPageState, self, results)
File "/tmp/isolated_run4X_bVC/content/test/gpu/gpu_tests/gpu_test_base.py",
line 72, in RunStoryWithRetries
super(cls, shared_page_state).RunStory(results)
File
"/tmp/isolated_run4X_bVC/third_party/catapult/telemetry/telemetry/page/shared_page_state.py",
line 321, in RunStory
self._current_page, self._current_tab, results)
File
"/tmp/isolated_run4X_bVC/content/test/gpu/gpu_tests/webgl_conformance.py", line
96, in ValidateAndMeasurePage
raise page_test.Failure(messages)
Failure: at (0, 0) expected: 0,255,0,255 was 255,0,0,255
FAIL at (0, 0) expected: 0,255,0,255 was 255,0,0,255
Description was changed from ========== gpu: disable immutable texture for linux ATI drivers. It's because linux ATI drivers have the issue to bind an immutable texture to FBO. It's why Linux release ATI bot is broken. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. It's because Linux ATI bot fails WebglConformance.conformance_textures_misc_tex_image_webgl When finding the solution for ATI driver, rollback this patch. BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Zhenyao, I tried to resolve ATI root issue, but I could not. All code is sane, and I have no idea. So change my mind. Use storage texture only if CMAA is used instead of MSAA. When we find the solution for ATI driver, this CL will be reverted. Currently, only Intel Chromebook uses CMAA. What do you think?
Patchset #2 (id:20001) has been deleted
On 2016/07/08 15:15:33, dshwang wrote: > Zhenyao, I tried to resolve ATI root issue, but I could not. All code is sane, > and I have no idea. > > So change my mind. Use storage texture only if CMAA is used instead of MSAA. > When we find the solution for ATI driver, this CL will be reverted. > > Currently, only Intel Chromebook uses CMAA. > > What do you think? Sounds good to me as a temporary solution.
On 2016/07/08 15:45:12, Zhenyao Mo wrote: > On 2016/07/08 15:15:33, dshwang wrote: > > Zhenyao, I tried to resolve ATI root issue, but I could not. All code is sane, > > and I have no idea. > > > > So change my mind. Use storage texture only if CMAA is used instead of MSAA. > > When we find the solution for ATI driver, this CL will be reverted. > > > > Currently, only Intel Chromebook uses CMAA. > > > > What do you think? > > Sounds good to me as a temporary solution. It's failing most of the bots though.
On 2016/07/08 15:45:55, Zhenyao Mo wrote: > > Sounds good to me as a temporary solution. > > It's failing most of the bots though. oops, I use = instead of ==. sorry. new patch must be fine. could you review again?
lgtm, but I am not owner. kbr should review.
On 2016/07/08 16:02:12, Zhenyao Mo wrote: > lgtm, but I am not owner. > > kbr should review. Thank you for reviewing. kbr, could you review?
On 2016/07/08 at 16:02:58, dongseong.hwang wrote: > On 2016/07/08 16:02:12, Zhenyao Mo wrote: > > lgtm, but I am not owner. > > > > kbr should review. > > Thank you for reviewing. > > kbr, could you review? lgtm but neither am I an owner.
Dongseong, I am concerned that this code path -- allocating the DrawingBuffer's storage as an immutable texture -- is not going to be tested on Chromium's commit queue, or on any of the GPU configurations on even the chromium.gpu.fyi waterfall. That having been said, since Intel already has effectively assumed responsibility for the CMAA code path in Chromium, this lgtm. Talking with zmo@ offline, it would be better in the long run if it were possible to turn on the TexStorage2D code path for DrawingBuffer separately, so it could be tested on other platforms. It would be ideal if you could help debug the failure on this configuration, too.
The CQ bit was checked by kbr@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 ========== WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. It's because Linux ATI bot fails WebglConformance.conformance_textures_misc_tex_image_webgl When finding the solution for ATI driver, rollback this patch. BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. It's because Linux ATI bot fails WebglConformance.conformance_textures_misc_tex_image_webgl When finding the solution for ATI driver, rollback this patch. BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. It's because Linux ATI bot fails WebglConformance.conformance_textures_misc_tex_image_webgl When finding the solution for ATI driver, rollback this patch. BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== WebGL: use storage texture for DrawingBuffer only if CMAA is enabled. It's because Linux ATI bot fails WebglConformance.conformance_textures_misc_tex_image_webgl When finding the solution for ATI driver, rollback this patch. BUG=557848 TEST=WebglConformance.conformance_textures_misc_tex_image_webgl CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/65f8f6c737278a80acee2bcd407d1fdc15426fa8 Cr-Commit-Position: refs/heads/master@{#404514} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/65f8f6c737278a80acee2bcd407d1fdc15426fa8 Cr-Commit-Position: refs/heads/master@{#404514}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2133253003/ by dongseong.hwang@intel.com. The reason for reverting is: ATI GPU bots became green, no matter this CL. ATI GPU bot might be in bad shape for some reasons. So revert this redundant speculative fix..
Message was sent while issue was closed.
On 2016/07/08 21:36:40, Ken Russell wrote: > Dongseong, I am concerned that this code path -- allocating the DrawingBuffer's > storage as an immutable texture -- is not going to be tested on Chromium's > commit queue, or on any of the GPU configurations on even the chromium.gpu.fyi > waterfall. > > That having been said, since Intel already has effectively assumed > responsibility for the CMAA code path in Chromium, this lgtm. Talking with zmo@ > offline, it would be better in the long run if it were possible to turn on the > TexStorage2D code path for DrawingBuffer separately, so it could be tested on > other platforms. It would be ideal if you could help debug the failure on this > configuration, too. I agree on your concern. Interestingly, ATI GPU bot became green right before this CL was landed. so I reverted this CL. Let me keep eyes on ATI bot for a while. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A...
Message was sent while issue was closed.
On 2016/07/09 07:43:00, dshwang wrote: > On 2016/07/08 21:36:40, Ken Russell wrote: > > Dongseong, I am concerned that this code path -- allocating the > DrawingBuffer's > > storage as an immutable texture -- is not going to be tested on Chromium's > > commit queue, or on any of the GPU configurations on even the chromium.gpu.fyi > > waterfall. > > > > That having been said, since Intel already has effectively assumed > > responsibility for the CMAA code path in Chromium, this lgtm. Talking with > zmo@ > > offline, it would be better in the long run if it were possible to turn on the > > TexStorage2D code path for DrawingBuffer separately, so it could be tested on > > other platforms. It would be ideal if you could help debug the failure on this > > configuration, too. > > I agree on your concern. > > Interestingly, ATI GPU bot became green right before this CL was landed. > so I reverted this CL. Let me keep eyes on ATI bot for a while. > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... Fortunately, ATI GPU bot is still green. DrawingBuffer uses immutable texture on all the platform, which support immutable texture. https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A...
Message was sent while issue was closed.
On 2016/07/09 12:37:59, dshwang wrote: > On 2016/07/09 07:43:00, dshwang wrote: > > On 2016/07/08 21:36:40, Ken Russell wrote: > > > Dongseong, I am concerned that this code path -- allocating the > > DrawingBuffer's > > > storage as an immutable texture -- is not going to be tested on Chromium's > > > commit queue, or on any of the GPU configurations on even the > chromium.gpu.fyi > > > waterfall. > > > > > > That having been said, since Intel already has effectively assumed > > > responsibility for the CMAA code path in Chromium, this lgtm. Talking with > > zmo@ > > > offline, it would be better in the long run if it were possible to turn on > the > > > TexStorage2D code path for DrawingBuffer separately, so it could be tested > on > > > other platforms. It would be ideal if you could help debug the failure on > this > > > configuration, too. > > > > I agree on your concern. > > > > Interestingly, ATI GPU bot became green right before this CL was landed. > > so I reverted this CL. Let me keep eyes on ATI bot for a while. > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > Fortunately, ATI GPU bot is still green. DrawingBuffer uses immutable texture on > all the platform, which support immutable texture. > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... ouch, I missed the test is disabled. https://codereview.chromium.org/2134643004 Sorry for all the mess. Which is better solution? 1. current ToT (i.e. immutable texture with disabled test in ATI bot) 2. revert 2 CLs (i.e. immutable texture only if CMAA, and enable the test)
Message was sent while issue was closed.
On 2016/07/09 12:47:01, dshwang wrote: > On 2016/07/09 12:37:59, dshwang wrote: > > On 2016/07/09 07:43:00, dshwang wrote: > > > On 2016/07/08 21:36:40, Ken Russell wrote: > > > > Dongseong, I am concerned that this code path -- allocating the > > > DrawingBuffer's > > > > storage as an immutable texture -- is not going to be tested on Chromium's > > > > commit queue, or on any of the GPU configurations on even the > > chromium.gpu.fyi > > > > waterfall. > > > > > > > > That having been said, since Intel already has effectively assumed > > > > responsibility for the CMAA code path in Chromium, this lgtm. Talking with > > > zmo@ > > > > offline, it would be better in the long run if it were possible to turn on > > the > > > > TexStorage2D code path for DrawingBuffer separately, so it could be tested > > on > > > > other platforms. It would be ideal if you could help debug the failure on > > this > > > > configuration, too. > > > > > > I agree on your concern. > > > > > > Interestingly, ATI GPU bot became green right before this CL was landed. > > > so I reverted this CL. Let me keep eyes on ATI bot for a while. > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > > > Fortunately, ATI GPU bot is still green. DrawingBuffer uses immutable texture > on > > all the platform, which support immutable texture. > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > ouch, I missed the test is disabled. https://codereview.chromium.org/2134643004 > Sorry for all the mess. > > Which is better solution? > 1. current ToT (i.e. immutable texture with disabled test in ATI bot) > 2. revert 2 CLs (i.e. immutable texture only if CMAA, and enable the test) (2) is better. There are clearly problems with this code path and we don't want to leave it on for all users in all configurations.
Message was sent while issue was closed.
On 2016/07/11 19:57:17, Ken Russell wrote: > On 2016/07/09 12:47:01, dshwang wrote: > > On 2016/07/09 12:37:59, dshwang wrote: > > > On 2016/07/09 07:43:00, dshwang wrote: > > > > On 2016/07/08 21:36:40, Ken Russell wrote: > > > > > Dongseong, I am concerned that this code path -- allocating the > > > > DrawingBuffer's > > > > > storage as an immutable texture -- is not going to be tested on > Chromium's > > > > > commit queue, or on any of the GPU configurations on even the > > > chromium.gpu.fyi > > > > > waterfall. > > > > > > > > > > That having been said, since Intel already has effectively assumed > > > > > responsibility for the CMAA code path in Chromium, this lgtm. Talking > with > > > > zmo@ > > > > > offline, it would be better in the long run if it were possible to turn > on > > > the > > > > > TexStorage2D code path for DrawingBuffer separately, so it could be > tested > > > on > > > > > other platforms. It would be ideal if you could help debug the failure > on > > > this > > > > > configuration, too. > > > > > > > > I agree on your concern. > > > > > > > > Interestingly, ATI GPU bot became green right before this CL was landed. > > > > so I reverted this CL. Let me keep eyes on ATI bot for a while. > > > > > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > > > > > Fortunately, ATI GPU bot is still green. DrawingBuffer uses immutable > texture > > on > > > all the platform, which support immutable texture. > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > > > ouch, I missed the test is disabled. > https://codereview.chromium.org/2134643004 > > Sorry for all the mess. > > > > Which is better solution? > > 1. current ToT (i.e. immutable texture with disabled test in ATI bot) > > 2. revert 2 CLs (i.e. immutable texture only if CMAA, and enable the test) > > (2) is better. There are clearly problems with this code path and we don't want > to leave it on for all users in all configurations. Ok, thank you for response. I'll enable the test first, and then if ATI bot is in red, I'll revert the reverted CL. |
