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

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

Created:
2 years, 6 months ago by qiankun
Modified:
2 years, 6 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

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.
2 years, 6 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 ...
2 years, 6 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 ...
2 years, 6 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 ...
2 years, 6 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 ...
2 years, 6 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 ...
2 years, 6 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
2 years, 6 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)
2 years, 6 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
2 years, 6 months ago (2015-10-18 19:32:11 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
2 years, 6 months ago (2015-10-18 20:47:18 UTC) #15
commit-bot: I haz the power
2 years, 6 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}

Powered by Google App Engine
This is Rietveld 408576698