Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1449043005:
webgl: use immutable texture for the default FBO. (Closed)
|
Created:
5 years, 1 month ago by dshwang Modified:
4 years, 5 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, vmpstr+blinkwatch_chromium.org, dshwang, jbroman, Justin Novosad, danakj, blink-reviews-platform-graphics_chromium.org, Rik, f(malita), blink-reviews, piman+watch_chromium.org, Stephen Chennney, rwlbuis, Zhenyao Mo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwebgl: use immutable texture for the default FBO.
Use immutable texture for the default FBO like chromium compositor.
In theory, it speeds up drawing speed, because immutable texture skips texture
validation on drawing call.
In addition, immutable texture allows GL_INTEL_framebuffer_CMAA more optimization.
BUG=557848
TEST=
WebglConformance.conformance_renderbuffers_framebuffer_state_restoration
WebglConformance.conformance_textures_misc_texture_size_cube_maps
WebglConformance.conformance_textures_misc_texture_sub_image_cube_maps
WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgb_rgb_unsigned_byte
WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgb_rgb_unsigned_short_5_6_5
WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_byte
WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_short_4_4_4_4
WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_short_5_5_5_1
Committed: https://crrev.com/fb9d8e245ac84c6c0806e86e7884b4775a84b4b5
Cr-Commit-Position: refs/heads/master@{#361335}
Patch Set 1 #Patch Set 2 : fix layout tests failure #
Total comments: 7
Messages
Total messages: 17 (3 generated)
dongseong.hwang@intel.com changed reviewers: + adrian.belgun@intel.com, kbr@chromium.org
kbr@, could you review? Adrian said in offline that CMAA requires immutable texture to avoid additional copy. Currently, he is implementing it in Mesa.
This seems fine in principle but breaks a bunch of pixel and layout tests on all of the platforms. Please investigate and send for re-review once they're fixed.
On 2015/11/19 02:49:18, Ken Russell wrote: > This seems fine in principle but breaks a bunch of pixel and layout tests on all > of the platforms. Please investigate and send for re-review once they're fixed. I missed one line. I'm fixed. Could you review again? https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:309: m_context->copySubTextureCHROMIUM(GL_TEXTURE_2D, m_colorBuffer.textureId, frontColorBufferMailbox->textureInfo.textureId, copyTextureCHROMIUM must not used for immutable texture as target, because it can redefine the texture.
https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:309: m_context->copySubTextureCHROMIUM(GL_TEXTURE_2D, m_colorBuffer.textureId, frontColorBufferMailbox->textureInfo.textureId, On 2015/11/19 12:33:00, dshwang wrote: > copyTextureCHROMIUM must not used for immutable texture as target, because it > can redefine the texture. If this code was relying on the copyTextureCHROMIUM call to resize the texture in the mailbox, then what's doing the same when the canvas is resized now? It looks to me like this introduces a bug when preserveDrawingBuffer=true and the canvas is resized, since nobody clears out the recycled mailboxes. I think you can catch the bug by drawing to a 128x128 canvas, resizing it larger (to 256x256), re-rendering so that a different color is drawn for fragments with x or y > 128, and reading back the lower-right corner to verify the canvas was resized correctly. Can you please write such a test and verify that it passes? Especially if it doesn't, could you submit a pull request to https://github.com/KhronosGroup/WebGL under either conformance/context or conformance/canvas adding one?
Description was changed from ========== webgl: use immutable texture for the default FBO. Use immutable texture for the default FBO like chromium compositor. In theory, it speeds up drawing speed, because immutable texture skips texture validation on drawing call. In addition, immutable texture allows GL_INTEL_framebuffer_CMAA more optimization. BUG=557848 TEST= WebglConformance.conformance_textures_misc_texture_size_cube_maps WebglConformance.conformance_textures_misc_texture_sub_image_cube_maps WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgb_rgb_unsigned_byte WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgb_rgb_unsigned_short_5_6_5 WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_byte WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_short_4_4_4_4 WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_short_5_5_5_1 ========== to ========== webgl: use immutable texture for the default FBO. Use immutable texture for the default FBO like chromium compositor. In theory, it speeds up drawing speed, because immutable texture skips texture validation on drawing call. In addition, immutable texture allows GL_INTEL_framebuffer_CMAA more optimization. BUG=557848 TEST= WebglConformance.conformance_renderbuffers_framebuffer_state_restoration WebglConformance.conformance_textures_misc_texture_size_cube_maps WebglConformance.conformance_textures_misc_texture_sub_image_cube_maps WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgb_rgb_unsigned_byte WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgb_rgb_unsigned_short_5_6_5 WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_byte WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_short_4_4_4_4 WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_rgba_rgba_unsigned_short_5_5_5_1 ==========
copyTextureCHROMIUM call doesn't resize the texture. see below comments. https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:277: RefPtr<MailboxInfo> frontColorBufferMailbox = recycledMailbox(); resizing code is here. it calls allocateTextureMemory() eventually and resizes mailbox texture. https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:309: m_context->copySubTextureCHROMIUM(GL_TEXTURE_2D, m_colorBuffer.textureId, frontColorBufferMailbox->textureInfo.textureId, On 2015/11/19 21:47:41, Ken Russell wrote: > On 2015/11/19 12:33:00, dshwang wrote: > > copyTextureCHROMIUM must not used for immutable texture as target, because it > > can redefine the texture. > > If this code was relying on the copyTextureCHROMIUM call to resize the texture > in the mailbox, then what's doing the same when the canvas is resized now? It > looks to me like this introduces a bug when preserveDrawingBuffer=true and the > canvas is resized, since nobody clears out the recycled mailboxes. I think you > can catch the bug by drawing to a 128x128 canvas, resizing it larger (to > 256x256), re-rendering so that a different color is drawn for fragments with x > or y > 128, and reading back the lower-right corner to verify the canvas was > resized correctly. Can you please write such a test and verify that it passes? > Especially if it doesn't, could you submit a pull request to > https://github.com/KhronosGroup/WebGL under either conformance/context or > conformance/canvas adding one? This code doesn't rely on copyTextureCHROMIUM call to resize the texture. copyTextureCHROMIUM emits GL_INVALID_OPERATION if the target texture is immutable. https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... As this CL changes target texture to immutable, we can not use copyTextureCHROMIUM. the source and target texture always existsm so we can use copySubTextureCHROMIUM. On the other hands, the test idea is good. WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_* covers what you explain in discard case. If you still think the test is needed, let me extend WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_* or WebglConformance.conformance_textures_misc_texture_size.
LGTM with a couple of comments. CC'ing bajones and zmo. https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:277: RefPtr<MailboxInfo> frontColorBufferMailbox = recycledMailbox(); On 2015/11/20 09:31:32, dshwang wrote: > resizing code is here. it calls allocateTextureMemory() eventually and resizes > mailbox texture. OK, thanks, I had missed the fact that recycledMailbox() checked the size of the texture associated with the recycled mailbox. https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:309: m_context->copySubTextureCHROMIUM(GL_TEXTURE_2D, m_colorBuffer.textureId, frontColorBufferMailbox->textureInfo.textureId, On 2015/11/20 09:31:32, dshwang wrote: > On 2015/11/19 21:47:41, Ken Russell wrote: > > On 2015/11/19 12:33:00, dshwang wrote: > > > copyTextureCHROMIUM must not used for immutable texture as target, because > it > > > can redefine the texture. > > > > If this code was relying on the copyTextureCHROMIUM call to resize the texture > > in the mailbox, then what's doing the same when the canvas is resized now? It > > looks to me like this introduces a bug when preserveDrawingBuffer=true and the > > canvas is resized, since nobody clears out the recycled mailboxes. I think you > > can catch the bug by drawing to a 128x128 canvas, resizing it larger (to > > 256x256), re-rendering so that a different color is drawn for fragments with x > > or y > 128, and reading back the lower-right corner to verify the canvas was > > resized correctly. Can you please write such a test and verify that it passes? > > Especially if it doesn't, could you submit a pull request to > > https://github.com/KhronosGroup/WebGL under either conformance/context or > > conformance/canvas adding one? > > This code doesn't rely on copyTextureCHROMIUM call to resize the texture. > copyTextureCHROMIUM emits GL_INVALID_OPERATION if the target texture is > immutable. > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > As this CL changes target texture to immutable, we can not use > copyTextureCHROMIUM. the source and target texture always existsm so we can use > copySubTextureCHROMIUM. > > On the other hands, the test idea is good. > WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_* > covers what you explain in discard case. > If you still think the test is needed, let me extend > WebglConformance.conformance_textures_webgl_canvas_tex_image_and_sub_image_2d_with_webgl_canvas_* > or WebglConformance.conformance_textures_misc_texture_size. I went ahead and wrote the test I had in mind in https://github.com/KhronosGroup/WebGL/pull/1325 . Please verify that this test continues to pass with your CL. We should roll forward the WebGL conformance tests to pick this up.
LGTM as well.
Thank you for reviewing! https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1449043005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:309: m_context->copySubTextureCHROMIUM(GL_TEXTURE_2D, m_colorBuffer.textureId, frontColorBufferMailbox->textureInfo.textureId, On 2015/11/23 22:26:31, Ken Russell wrote: > I went ahead and wrote the test I had in mind in > https://github.com/KhronosGroup/WebGL/pull/1325 . Please verify that this test > continues to pass with your CL. We should roll forward the WebGL conformance > tests to pick this up. Thank you for adding test! The test works well with this CL. In addition, I checked the test cover this code path.
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1449043005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1449043005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fb9d8e245ac84c6c0806e86e7884b4775a84b4b5 Cr-Commit-Position: refs/heads/master@{#361335}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1474763002/ by dongseong.hwang@intel.com. The reason for reverting is: following test is broken. WebglConformance.deqp_functional_gles3_readpixel.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2135753003/ by cwallez@chromium.org. The reason for reverting is: https://bugs.chromium.org/p/chromium/issues/detail?id=626742.
Message was sent while issue was closed.
On 2016/07/08 21:23:38, Corentin Wallez wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2135753003/ by mailto:cwallez@chromium.org. > > The reason for reverting is: > https://bugs.chromium.org/p/chromium/issues/detail?id=626742. It turned out the revert patch didn't apply correctly. Instead, proceeding with https://codereview.chromium.org/2131863003/ . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
