Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1405013002:
Update texImage2DCanvasByGPU path in texImage2D/texSubImage2D for WebGL 2.0 (Closed)
DescriptionUpdate 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 #Messages
Total messages: 16 (5 generated)
qiankun.miao@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL. With this CL, all conformance2/textures/canvas/*, conformance2/textures/image/*, conformance2/textures/svg_image/*, conformance2/textures/video/*, conformance2/textures/webgl_canvas/* should pass.
Looks like a good change, but I have a request for readability. https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4360: if (!canvas->renderingContext() || !canvas->renderingContext()->isAccelerated() || isFloatType || isIntegerFormat || isSRGBFormat) { The newly added format checks make this difficult to read. This would probably be cleaner as a switch statement. https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4620: if (!canvas->renderingContext() || !canvas->renderingContext()->isAccelerated() || isFloatType || isIntegerFormat || isSRGBFormat) { Same here.
Thanks for review. I updated the CL. PTAL. https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4360: if (!canvas->renderingContext() || !canvas->renderingContext()->isAccelerated() || isFloatType || isIntegerFormat || isSRGBFormat) { On 2015/10/15 17:17:58, bajones wrote: > The newly added format checks make this difficult to read. This would probably > be cleaner as a switch statement. Done. https://codereview.chromium.org/1405013002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4620: if (!canvas->renderingContext() || !canvas->renderingContext()->isAccelerated() || isFloatType || isIntegerFormat || isSRGBFormat) { On 2015/10/15 17:17:58, bajones wrote: > Same here. Done.
LGTM https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4430: // texImage2DCanvasByGPU relies on copyTextureCHROMIUM which doesn't support float/integer/sRGB internal format. I am actually working on upgrading copyTextureCHROMIUM's shader path to handle for formats. However, landing this first seems like a good and correct idea. We can revisit once my work lands. Could you add a "FIXME: relax the constrains if copyTextureCHROMIUM is upgraded to handle more formats." here?
lgtm overall https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4430: // texImage2DCanvasByGPU relies on copyTextureCHROMIUM which doesn't support float/integer/sRGB internal format. On 2015/10/16 18:52:16, Zhenyao Mo wrote: > I am actually working on upgrading copyTextureCHROMIUM's shader path to handle > for formats. However, landing this first seems like a good and correct idea. > We can revisit once my work lands. > > Could you add a "FIXME: relax the constrains if copyTextureCHROMIUM is upgraded > to handle more formats." here? Could you also add the same FIXME below? Also, it would be great if you could create one function that would take in the type and internal format, do these checks, and use that function from here and texSubImage2D, below.
Thank you all for review. I fixed your comments in the new CL. https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4430: // texImage2DCanvasByGPU relies on copyTextureCHROMIUM which doesn't support float/integer/sRGB internal format. On 2015/10/16 18:52:16, Zhenyao Mo wrote: > I am actually working on upgrading copyTextureCHROMIUM's shader path to handle > for formats. However, landing this first seems like a good and correct idea. > We can revisit once my work lands. > > Could you add a "FIXME: relax the constrains if copyTextureCHROMIUM is upgraded > to handle more formats." here? Done. https://codereview.chromium.org/1405013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4430: // texImage2DCanvasByGPU relies on copyTextureCHROMIUM which doesn't support float/integer/sRGB internal format. On 2015/10/16 22:09:44, Ken Russell wrote: > On 2015/10/16 18:52:16, Zhenyao Mo wrote: > > I am actually working on upgrading copyTextureCHROMIUM's shader path to handle > > for formats. However, landing this first seems like a good and correct idea. > > We can revisit once my work lands. > > > > Could you add a "FIXME: relax the constrains if copyTextureCHROMIUM is > upgraded > > to handle more formats." here? > > Could you also add the same FIXME below? Also, it would be great if you could > create one function that would take in the type and internal format, do these > checks, and use that function from here and texSubImage2D, below. Done.
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1405013002/#ps40001 (title: "Add FIXME and wrap the checks in one function")
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by kbr@chromium.org
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
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bafb8676c0d635818999a355508d357665438b1a Cr-Commit-Position: refs/heads/master@{#354708} |
