Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693653002:
cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. (Closed)
|
Created:
3 years, 10 months ago by dshwang Modified:
3 years, 9 months ago Reviewers:
inactive_dshwang_plz_cc_intel, Zhenyao Mo, piman, Ken Russell (switch to Gerrit), Michael Achenbach CC:
chromium-reviews, piman+watch_chromium.org, Henrik Grunell, Michael Achenbach Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncmaa, gpu: use R8 image texture when GL_NV_image_formats is available.
Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.1.
https://bugs.freedesktop.org/show_bug.cgi?id=98480
So CMAA can use R8 texture, instead of RGBA8.
In addition, CMAA implementation checks R8 support tests, and it slows down GPU
initialization. Remove all unnecessary tests and just check certain extensions.
BUG=535198, 642290
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
Review-Url: https://codereview.chromium.org/2693653002
Cr-Commit-Position: refs/heads/master@{#455170}
Committed: https://chromium.googlesource.com/chromium/src/+/8d2609f6aa6f0a9f8ae238b8af06770337d726eb
Patch Set 1 #Patch Set 2 : fix wrong format of comment #
Total comments: 10
Patch Set 3 : check "GL_ARB_texture_storage" #Patch Set 4 : RestoreActiveTexture after initialization #
Total comments: 1
Messages
Total messages: 43 (27 generated)
Description was changed from ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.x. So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 ========== to ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.x. So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 dongseong.hwang@chromium.org to run a CQ dry run
dongseong.hwang@intel.com changed reviewers: + zmo@chromium.org
zmo@, could you review?
Description was changed from ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.x. So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.x. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 ==========
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 dongseong.hwang@chromium.org 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 ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.x. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.1. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dongseong.hwang@intel.com changed reviewers: + piman@chromium.org
Zhenyao, do you have a chance to review it?
dongseong.hwang@intel.com changed reviewers: - piman@chromium.org, zmo@chromium.org
dongseong.hwang@intel.com changed reviewers: + piman@chromium.org, zmo@chromium.org
On 2017/02/21 21:50:23, dshwang wrote: > Zhenyao, do you have a chance to review it? Hi Zhenyao, could you review?
dongseong.hwang@intel.com changed reviewers: + kbr@chromium.org
Ken, could you review this change? It makes webgl initialzation faster on chromebook.
On 2017/03/01 21:54:05, dshwang wrote: > Ken, could you review this change? It makes webgl initialzation faster on > chromebook. Sorry for the delay - I was overwhelmed the past couple days. Will get back to you today.
Sorry about the delay. A couple questions, not exactly a result or your CL, but I want to double check anyway. https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:70: DCHECK(decoder->GetGLContext()->HasExtension( Is it validated somewhere before this that it's either ES 3.1 or above, or it's GL with this extension? https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:80: glBindTexture(GL_TEXTURE_2D, rgba8ui_texture); A question: although not new to this CL, but here we changed texture/fbo bindings. Do we need to restore them? https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:81: glTexStorage2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8UI, 4, 4); tex storage isn't part of core 4.1 or lower. I think it's much safer to call TexImage2D. Again, not new to this CL, but since you are touching this code and I am reading it... (or if we made sure only on 4.2 or above, or check tex_storage extension, then we are also good) https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:578: const char header_gl30[] = shouldn't this name be gl130 instead?
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Thank you for reviewing! https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:70: DCHECK(decoder->GetGLContext()->HasExtension( On 2017/03/01 23:52:33, Zhenyao Mo wrote: > Is it validated somewhere before this that it's either ES 3.1 or above, or it's > GL with this extension? yes, here https://cs.chromium.org/chromium/src/gpu/command_buffer/service/feature_info.... https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:80: glBindTexture(GL_TEXTURE_2D, rgba8ui_texture); On 2017/03/01 23:52:33, Zhenyao Mo wrote: > A question: although not new to this CL, but here we changed texture/fbo > bindings. Do we need to restore them? below code anyway bind texture/fbo, so not necessary. In addition, as far as I know, glDeleteFramebuffersEXT and glDeleteTextures unbind by itself. https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:81: glTexStorage2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8UI, 4, 4); On 2017/03/01 23:52:33, Zhenyao Mo wrote: > tex storage isn't part of core 4.1 or lower. I think it's much safer to call > TexImage2D. Again, not new to this CL, but since you are touching this code and > I am reading it... (or if we made sure only on 4.2 or above, or check > tex_storage extension, then we are also good) Good point! The implementation uses glTexStorage2DEXT() repeatedly. I missed "GL_ARB_texture_storage" check when landing the initial commit, because this extensions is widely supported. New patch set has this check in feature_info.cc https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:578: const char header_gl30[] = On 2017/03/01 23:52:33, Zhenyao Mo wrote: > shouldn't this name be gl130 instead? Done.
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.
https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:80: glBindTexture(GL_TEXTURE_2D, rgba8ui_texture); On 2017/03/02 20:27:03, dshwang wrote: > On 2017/03/01 23:52:33, Zhenyao Mo wrote: > > A question: although not new to this CL, but here we changed texture/fbo > > bindings. Do we need to restore them? > > below code anyway bind texture/fbo, so not necessary. In addition, as far as I > know, glDeleteFramebuffersEXT and glDeleteTextures unbind by itself. What I mean is the bindings before entering this code is lost. Later bind/delete doesn't change this fact. If we don't recover them, we will leak state changes into other code.
https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2693653002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:80: glBindTexture(GL_TEXTURE_2D, rgba8ui_texture); On 2017/03/06 03:36:54, Zhenyao Mo wrote: > On 2017/03/02 20:27:03, dshwang wrote: > > On 2017/03/01 23:52:33, Zhenyao Mo wrote: > > > A question: although not new to this CL, but here we changed texture/fbo > > > bindings. Do we need to restore them? > > > > below code anyway bind texture/fbo, so not necessary. In addition, as far as I > > know, glDeleteFramebuffersEXT and glDeleteTextures unbind by itself. > > What I mean is the bindings before entering this code is lost. Later bind/delete > doesn't change this fact. If we don't recover them, we will leak state changes > into other code. Before leaving CMAA code, CMAA always restore texture unit binding. https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_app...
Description was changed from ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.1. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.1. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 dongseong.hwang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2693653002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2693653002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:109: decoder->RestoreFramebufferBindings(); > What I mean is the bindings before entering this code is lost. Later bind/delete > doesn't change this fact. If we don't recover them, we will leak state changes > into other code. You are right! It's bug. I fixed it in new patch set.
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_...)
On 2017/03/07 03:13:31, dshwang wrote: > https://codereview.chromium.org/2693653002/diff/60001/gpu/command_buffer/serv... > File > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc > (right): > > https://codereview.chromium.org/2693653002/diff/60001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:109: > decoder->RestoreFramebufferBindings(); > > What I mean is the bindings before entering this code is lost. Later > bind/delete > > doesn't change this fact. If we don't recover them, we will leak state > changes > > into other code. > > You are right! It's bug. I fixed it in new patch set. lgtm
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488909266348790,
"parent_rev": "241d9acf14c1a9f4629525df07b003071033167c", "commit_rev":
"8d2609f6aa6f0a9f8ae238b8af06770337d726eb"}
Message was sent while issue was closed.
Description was changed from ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.1. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 ========== cmaa, gpu: use R8 image texture when GL_NV_image_formats is available. Recently Mesa supports GL_NV_image_formats extension for r8 image texture for ES 3.1. https://bugs.freedesktop.org/show_bug.cgi?id=98480 So CMAA can use R8 texture, instead of RGBA8. In addition, CMAA implementation checks R8 support tests, and it slows down GPU initialization. Remove all unnecessary tests and just check certain extensions. BUG=535198, 642290 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 Review-Url: https://codereview.chromium.org/2693653002 Cr-Commit-Position: refs/heads/master@{#455170} Committed: https://chromium.googlesource.com/chromium/src/+/8d2609f6aa6f0a9f8ae238b8af06... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8d2609f6aa6f0a9f8ae238b8af06...
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
Possibly this breaks: https://build.chromium.org/p/chromium.gpu/builders/Linux%20Debug%20%28NVIDIA%... Error from the output: [4533:4533:0307/120003.054303:1731388651:ERROR:gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc(647)] ApplyFramebufferAttachmentCMAAINTEL: shader compilation failed: GL_FRAGMENT_SHADER shader compilation failed: 0(1) : error C0000: syntax error, unexpected $undefined at token "<undefined>" 0(1) : error C0155: integer constant overflow 0(4) : warning C7022: unrecognized profile specifier "highp" 0(4) : warning C7022: unrecognized profile specifier "precision" ...
Message was sent while issue was closed.
On 2017/03/09 09:34:04, Michael Achenbach wrote: > Possibly this breaks: > https://build.chromium.org/p/chromium.gpu/builders/Linux%20Debug%20%28NVIDIA%... > > Error from the output: > [4533:4533:0307/120003.054303:1731388651:ERROR:gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc(647)] > ApplyFramebufferAttachmentCMAAINTEL: shader compilation failed: > GL_FRAGMENT_SHADER shader compilation failed: 0(1) : error C0000: syntax error, > unexpected $undefined at token "<undefined>" > 0(1) : error C0155: integer constant overflow > 0(4) : warning C7022: unrecognized profile specifier "highp" > 0(4) : warning C7022: unrecognized profile specifier "precision" > ... Yes, looks like this is the culprit. Let's revert.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2738213003/ by zmo@chromium.org. The reason for reverting is: This breaks: https://build.chromium.org/p/chromium.gpu/builders/Linux%20Debug%20%28NVIDIA%... Error from the output: [4533:4533:0307/120003.054303:1731388651:ERROR:gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc(647)] ApplyFramebufferAttachmentCMAAINTEL: shader compilation failed: GL_FRAGMENT_SHADER shader compilation failed: 0(1) : error C0000: syntax error, unexpected $undefined at token "<undefined>" 0(1) : error C0155: integer constant overflow 0(4) : warning C7022: unrecognized profile specifier "highp" 0(4) : warning C7022: unrecognized profile specifier "precision". |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
