Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2298613010:
gpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore(). (Closed)
Descriptiongpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore().
CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we
expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill
random color on the FBO in the case. It causes ugly artifact in the later CMAA
phase.
This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and
glDrawArrays. It fixes the bug as well as speeds up copy.
BUG=642290
TEST=chromeos camera app on Samus
https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhifgngh
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/b2d7e66345c253f353d1d3c555964f6e95e26b9c
Cr-Commit-Position: refs/heads/master@{#416538}
Patch Set 1 #Patch Set 2 : remove imageStore() path #
Total comments: 2
Patch Set 3 : fix unnecessary bluring #Patch Set 4 : use glCopyTexSubImage2D() to copy texture #
Total comments: 1
Patch Set 5 : fix small errata #
Messages
Total messages: 37 (29 generated)
Description was changed from ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL explicitly fill vec4(0.) on the FBO. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... ========== to ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL explicitly fill vec4(0.) on the FBO. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dongseong.hwang@intel.com changed reviewers: + piman@chromium.org, robert.bradford@intel.com
piman@: Please review changes. It fixs the release blocker issue.
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 ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL explicitly fill vec4(0.) on the FBO. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copy RGBA8 texture on RGBA8 FBO to avoid above case. In addition, it makes the code simpler. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patchset #2 (id:20001) has been deleted
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...
dongseong.hwang@intel.com changed reviewers: + adrian.belgun@intel.com, jon.kennedy@intel.com
I remove whole imageStore() copy path, because it's not necessary in my opinion. https://codereview.chromium.org/2298613010/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (left): https://codereview.chromium.org/2298613010/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:1905: imageStore(outTexture, screenPosI, pixel); simple fix is adding following line here. outColor = vec4(0.); However, this solution make this code and later CMAA phase entangled. vec4(0.) hack requires the knowledge of implementation detail of next phase. So I remove whole imageStore() path. adrian@, jon@, do you have specific reason to prefer imageStore() to FBO write. In my opinion, it's not necessary. RGBA8 to RGBA8 copy works well with FBO write. In addition, ApplyCMAAEffectTexture() already writes something on RGBA8 and R8 FBO. https://codereview.chromium.org/2298613010/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2298613010/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:223: glDisable(GL_BLEND); These was in CopyTexture(), but ApplyCMAAEffectTexture() also needs them. so move it here. It is not related to this CL anyway.
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...
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...
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...
use glCopyTexSubImage2D() to copy texture, instead of custom shader and glDrawArrays. It fixes the bug as well as speed up copy. https://codereview.chromium.org/2298613010/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2298613010/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:558: glCopyTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 0, 0, width_, height_); Use glCopyTexSubImage2D, because we know the source and dest texture is always RGBA8 or RGBA or RGB, and target is always GL_TEXTURE_2D. In addition, it speeds up. It's always fater then glDrawArrays in all platforms. Linux (Intel IvyBridge i7-3520M): 16 us vs 6 us -> 260% faster Android (Nexus 5): 331.8 us vs 252.7 us -> 31% faster See https://src.chromium.org/viewvc/chrome?revision=289160&view=revision
Description was changed from ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copy RGBA8 texture on RGBA8 FBO to avoid above case. In addition, it makes the code simpler. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copy RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copy RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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
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.
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...
The CQ bit was unchecked by dongseong.hwang@intel.com
Description was changed from ========== gpu, cmaa: explicit clear the framebuffer on imageStore texture copy. CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore(). CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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@intel.com
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 ========== gpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore(). CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore(). CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== gpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore(). CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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 ========== gpu, cmaa: copy RGBA8 texture via glCopyTexSubImage2D() instead of imageStore(). CMAA uses imageStore() to copy RGBA8 to RGBA8 texture. At the moment, we expect the FBO is not updated. However, some drivers (e.g. Intel Mesa) fill random color on the FBO in the case. It causes ugly artifact in the later CMAA phase. This CL copies RGBA8 texture via glCopyTexSubImage2D(), instead of custom shader and glDrawArrays. It fixes the bug as well as speeds up copy. BUG=642290 TEST=chromeos camera app on Samus https://chrome.google.com/webstore/detail/camera/hfhhnacclhffhdffklopdkcgdhif... 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/b2d7e66345c253f353d1d3c555964f6e95e26b9c Cr-Commit-Position: refs/heads/master@{#416538} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b2d7e66345c253f353d1d3c555964f6e95e26b9c Cr-Commit-Position: refs/heads/master@{#416538} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
