Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(57)

Issue 1405013002: Update texImage2DCanvasByGPU path in texImage2D/texSubImage2D for WebGL 2.0 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 9 months ago by qiankun
Modified:
1 year, 9 months ago
CC:
chromium-reviews, blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Update texImage2DCanvasByGPU path in texImage2D/texSubImage2D for WebGL 2.0 texImage2DCanvasByGPU path doesn't support some new internalformats/formats/types of WebGL 2.0. Because texImage2DCanvasByGPU depends on copyTextureCHROMIUM and copyTexImage2D, which have some constraints on these new formats/types, e.g. we cannot use copyTexImage2D to copy from a source texture with GL_RGBA internalformat to a target texture with integer internalformat. BUG=532708 TEST=conformance2/texture/canvas/* Committed: https://crrev.com/bafb8676c0d635818999a355508d357665438b1a Cr-Commit-Position: refs/heads/master@{#354708}

Patch Set 1 #

Total comments: 4

Patch Set 2 : use switch #

Total comments: 4

Patch Set 3 : Add FIXME and wrap the checks in one function #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -5 lines) Patch
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp View 1 2 3 chunks +88 lines, -5 lines 0 comments Download
Trybot results:  mac_chromium_rel_ng   win8_chromium_ng   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_gn_rel   ios_rel_device_ninja   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios_dbg_simulator_ninja   linux_chromium_chromeos_compile_dbg_ng   android_chromium_gn_compile_rel   linux_android_rel_ng   linux_chromium_asan_rel_ng   android_chromium_gn_compile_dbg   chromium_presubmit   chromeos_x86-generic_chromium_compile_only_ng   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   android_clang_dbg_recipe   chromeos_daisy_chromium_compile_only_ng   android_arm64_dbg_recipe   linux_chromium_chromeos_ozone_rel_ng   android_compile_dbg   linux_chromium_chromeos_rel_ng   linux_chromium_compile_dbg_32_ng   linux_chromium_gn_chromeos_rel   cast_shell_android   win_chromium_rel_ng   win8_chromium_ng   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_gn_rel   mac_chromium_compile_dbg_ng   ios_rel_device_ninja   ios_dbg_simulator_ninja   linux_chromium_chromeos_compile_dbg_ng   mac_chromium_rel_ng   android_chromium_gn_compile_rel   linux_android_rel_ng   linux_chromium_asan_rel_ng   android_chromium_gn_compile_dbg   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   cast_shell_linux   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   android_clang_dbg_recipe   android_arm64_dbg_recipe   android_compile_dbg   linux_chromium_compile_dbg_32_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_gn_chromeos_rel   cast_shell_android 
Commit queue not available (can’t edit this change).

Messages

Total messages: 16 (5 generated)
qiankun
PTAL. With this CL, all conformance2/textures/canvas/*, conformance2/textures/image/*, conformance2/textures/svg_image/*, conformance2/textures/video/*, conformance2/textures/webgl_canvas/* should pass.
1 year, 9 months ago (2015-10-15 16:59:40 UTC) #2
bajones
Looks like a good change, but I have a request for readability. https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp ...
1 year, 9 months ago (2015-10-15 17:17:58 UTC) #3
qiankun
Thanks for review. I updated the CL. PTAL. https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode4360 third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4360: if ...
1 year, 9 months ago (2015-10-16 17:19:35 UTC) #4
Zhenyao Mo
LGTM https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode4430 third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4430: // texImage2DCanvasByGPU relies on copyTextureCHROMIUM which doesn't support ...
1 year, 9 months ago (2015-10-16 18:52:16 UTC) #5
Ken Russell (switch to Gerrit)
lgtm overall https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode4430 third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4430: // texImage2DCanvasByGPU relies on copyTextureCHROMIUM which doesn't ...
1 year, 9 months ago (2015-10-16 22:09:44 UTC) #6
qiankun
Thank you all for review. I fixed your comments in the new CL. https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp File ...
1 year, 9 months ago (2015-10-17 08:10:40 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405013002/40001
1 year, 9 months ago (2015-10-17 08:12:48 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/122089)
1 year, 9 months ago (2015-10-17 11:23:50 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405013002/40001
1 year, 9 months ago (2015-10-18 19:32:11 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
1 year, 9 months ago (2015-10-18 20:47:18 UTC) #15
commit-bot: I haz the power
1 year, 9 months ago (2015-10-18 20:48:08 UTC) #16
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/bafb8676c0d635818999a355508d357665438b1a
Cr-Commit-Position: refs/heads/master@{#354708}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973