|
|
Descriptiongpu, cmaa: Don't blend isolated L shape because it's not a closed geometry.
Isolated L shape means neighbor pixels doesn't have any edges continuing
current two edge, as follows;
_ _
X| X| |X |X
¯¯ ¯¯
We don't want to blur an open geometry.
This CL fixes 2 WebGL conformance tests
https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-size-change.html
https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/copy-tex-image-and-sub-image-2d.html
Yang Gu checked all WebGL conformance tests are passed with CMAA.
BUG=535198
TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders"
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/9df37c48526a8f6b350687e34e75b2eca1961736
Cr-Commit-Position: refs/heads/master@{#403821}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 22 (11 generated)
Description was changed from ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" ========== to ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ==========
Description was changed from ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ========== gpu, cmaa: don't blend isolated L shape because it's not a closed geometry. Isolated L shape means the neighbor pixels doesn't have any edges continuing the current two edges, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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: + adrian.belgun@intel.com, jon.kennedy@intel.com, piman@chromium.org, zmo@chromium.org
jon, adrian, could you review? zmo@, could you review as owner? It's one of two patches to fix the two webgl tests. https://bugs.chromium.org/p/chromium/issues/detail?id=535198#c30
lgtm
https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:1576: bool((packedEdgesL & 0x02u) == 0x00u) && I didn't review the original CLs, but chrome doesn't allow c style type cast. You will need to do static_cast here (or is it really necessary? Also, indent is off.
lgtm https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:1576: bool((packedEdgesL & 0x02u) == 0x00u) && On 2016/07/05 01:47:27, Zhenyao Mo wrote: > I didn't review the original CLs, but chrome doesn't allow c style type cast. > You will need to do static_cast here (or is it really necessary? > > Also, indent is off. Never mind. Just realized it's shader source, so it doesn't matter. But could you still fix indent for readability purpose?
On 2016/07/05 01:50:39, Zhenyao Mo wrote: > lgtm > > https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... > File > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc > (right): > > https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:1576: > bool((packedEdgesL & 0x02u) == 0x00u) && > On 2016/07/05 01:47:27, Zhenyao Mo wrote: > > I didn't review the original CLs, but chrome doesn't allow c style type cast. > > You will need to do static_cast here (or is it really necessary? > > > > Also, indent is off. > > Never mind. Just realized it's shader source, so it doesn't matter. But could > you still fix indent for readability purpose? lgtm
After rethinking, I want to change the logic. The shader handles 2 edge case like gpu driver blacklist. The shader skips parallel case, large L case, and isolated L case, and then blend anything else. However, I think what CMAA really want is to blend only starecase case. So I want to change code to blend only starecase case and skips anything less. This change will make code more readible and maintainable. Guys, what do you think? I'll submit the proposal code soon. Previous description =========================================================================== gpu, cmaa: don't blend isolated L shape because it's not a closed geometry. Isolated L shape means the neighbor pixels doesn't have any edges continuing the current two edges, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2120293002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:1576: bool((packedEdgesL & 0x02u) == 0x00u) && On 2016/07/05 01:50:39, Zhenyao Mo wrote: > On 2016/07/05 01:47:27, Zhenyao Mo wrote: > > I didn't review the original CLs, but chrome doesn't allow c style type cast. > > You will need to do static_cast here (or is it really necessary? > > > > Also, indent is off. > > Never mind. Just realized it's shader source, so it doesn't matter. But could > you still fix indent for readability purpose? Yes, I will apply clang-format.
Description was changed from ========== gpu, cmaa: don't blend isolated L shape because it's not a closed geometry. Isolated L shape means the neighbor pixels doesn't have any edges continuing the current two edges, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ========== gpu, cmaa: handle only staircase case for 2 edge case Two edge case is a bit complicated. We want to handle only staircase case. The staircase case consists of small staircase and Z shape. Define "small staircase" as at least 2 texels form Z shape. Standard form Variants __ |__ __ __ |__ X| X| | X | X| ¯¯ ¯¯ ¯¯ ¯¯| ¯¯| Define "Z shape" as at least 4 texels form Z shape. __ __ X| ¯¯ ¯¯ Anything else will be skipped. e.g. Parallel |X| Isolated L shape _ _ X| X| |X |X ¯¯ ¯¯ Large L shape _ _ | | _ _ X| X| |X |X | ¯¯¯¯ ¯¯¯¯ | etc.. This CL fixes 2 WebGL conformance tests, which fail due to isolated L shape. https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ==========
On 2016/07/05 11:41:26, dshwang wrote: > After rethinking, I want to change the logic. > > The shader handles 2 edge case like gpu driver blacklist. > The shader skips parallel case, large L case, and isolated L case, and then > blend anything else. > > However, I think what CMAA really want is to blend only starecase case. > So I want to change code to blend only starecase case and skips anything less. > This change will make code more readible and maintainable. I submit the new patch and change the description. Jon, Adrian, Zhenyao, could you review again?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== gpu, cmaa: handle only staircase case for 2 edge case Two edge case is a bit complicated. We want to handle only staircase case. The staircase case consists of small staircase and Z shape. Define "small staircase" as at least 2 texels form Z shape. Standard form Variants __ |__ __ __ |__ X| X| | X | X| ¯¯ ¯¯ ¯¯ ¯¯| ¯¯| Define "Z shape" as at least 4 texels form Z shape. __ __ X| ¯¯ ¯¯ Anything else will be skipped. e.g. Parallel |X| Isolated L shape _ _ X| X| |X |X ¯¯ ¯¯ Large L shape _ _ | | _ _ X| X| |X |X | ¯¯¯¯ ¯¯¯¯ | etc.. This CL fixes 2 WebGL conformance tests, which fail due to isolated L shape. https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ==========
Patchset #2 (id:60001) has been deleted
On 2016/07/05 12:44:48, dshwang wrote: > On 2016/07/05 11:41:26, dshwang wrote: > > After rethinking, I want to change the logic. > > > > The shader handles 2 edge case like gpu driver blacklist. > > The shader skips parallel case, large L case, and isolated L case, and then > > blend anything else. > > > > However, I think what CMAA really want is to blend only starecase case. > > So I want to change code to blend only starecase case and skips anything less. > > This change will make code more readible and maintainable. > > I submit the new patch and change the description. > > Jon, Adrian, Zhenyao, could you review again? I found the sencond patch affects AA quality badly. My idea might be bad, or need to be improved. I file another CL and I'm working in progress https://codereview.chromium.org/2128453002/ Let me land original CL.
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: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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 ========== gpu, cmaa: Don't blend isolated L shape because it's not a closed geometry. Isolated L shape means neighbor pixels doesn't have any edges continuing current two edge, as follows; _ _ X| X| |X |X ¯¯ ¯¯ We don't want to blur an open geometry. This CL fixes 2 WebGL conformance tests https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-... https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/misc/co... Yang Gu checked all WebGL conformance tests are passed with CMAA. BUG=535198 TEST=Run a WebGL app with Chrome started with "--enable-cmaa-shaders" 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/9df37c48526a8f6b350687e34e75b2eca1961736 Cr-Commit-Position: refs/heads/master@{#403821} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/9df37c48526a8f6b350687e34e75b2eca1961736 Cr-Commit-Position: refs/heads/master@{#403821} |