Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145093003:
Discard correct drawing buffer (Closed)
|
Created:
4 years, 5 months ago by qiankun Modified:
4 years, 5 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, piman+watch_chromium.org, Stephen Chennney, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis, Yang Gu, xinghua.cao Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDiscard correct drawing buffer
This partly revert https://codereview.chromium.org/1950053003. On
platform supported GL_EXT_discard_framebuffer extension, it causes a
WebGL conformance test regression:
conformance/state/state-uneffected-after-compositing.html
The root cause is current bound fbo will be discarded other than the
default fbo. We should discard default fbo correctly.
BUG=628472
Committed: https://crrev.com/fd383c4428d3a4e1041425214f001661dca8d173
Cr-Commit-Position: refs/heads/master@{#405689}
Patch Set 1 #Patch Set 2 : Discard correct drawing buffer #
Total comments: 3
Patch Set 3 : remove comments #Messages
Total messages: 25 (12 generated)
Description was changed from ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. If the default fbo is discarded, WebGLRenderingContext::clearIfComposited() call can early return. BUG=295792 ========== to ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=295792 ==========
qiankun.miao@intel.com changed reviewers: + erikchen@chromium.org, kbr@chromium.org, yunchao.he@intel.com
PTAL.
https://codereview.chromium.org/2145093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2145093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:296: m_gl->DiscardFramebufferEXT(GL_FRAMEBUFFER, 3, attachments); This only makes framebuffer undefined to save some wasted efforts to load/store logical buffer (color, depth, stencil) from/to fast memory. See https://www.khronos.org/registry/gles/extensions/EXT/EXT_discard_framebuffer.txt. WebGLRenderingContext::clearIfComposited() is still required to do real clearance.
LGTM. We have discussed this bug in detail offline today. PTAL.
yunchao.he@intel.com changed reviewers: + zmo@chromium.org
Add zmo@ too.
lgtm
lgtm https://codereview.chromium.org/2145093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2145093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:291: // It appears safe to overwrite the context's framebuffer binding in the Discard case since there will always be a This comment is unnecessary because there is a restoreFramebufferBindings() call right below.
https://codereview.chromium.org/2145093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2145093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:291: // It appears safe to overwrite the context's framebuffer binding in the Discard case since there will always be a On 2016/07/14 17:27:03, Zhenyao Mo wrote: > This comment is unnecessary because there is a restoreFramebufferBindings() call > right below. Thanks for reminding. The comments was added previously when restoreFramebufferBindings() was not always called.
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for fixing this. LGTM. In general it would be better to file a new bug, block it on the one that introduced the bug (http://crbug.com/607130 in this case), and reference that one, rather than a meta-bug like Issue 295792.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=295792 ========== to ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=628472 ==========
On 2016/07/15 00:27:48, Ken Russell wrote: > Thanks for fixing this. LGTM. > > In general it would be better to file a new bug, block it on the one that > introduced the bug (http://crbug.com/607130 in this case), and reference that > one, rather than a meta-bug like Issue 295792. Thanks for your good suggestion. I filed a bug and updated the description.
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, zmo@chromium.org, yunchao.he@intel.com Link to the patchset: https://codereview.chromium.org/2145093003/#ps40001 (title: "remove comments")
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 ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=628472 ========== to ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=628472 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=628472 ========== to ========== Discard correct drawing buffer This partly revert https://codereview.chromium.org/1950053003. On platform supported GL_EXT_discard_framebuffer extension, it causes a WebGL conformance test regression: conformance/state/state-uneffected-after-compositing.html The root cause is current bound fbo will be discarded other than the default fbo. We should discard default fbo correctly. BUG=628472 Committed: https://crrev.com/fd383c4428d3a4e1041425214f001661dca8d173 Cr-Commit-Position: refs/heads/master@{#405689} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fd383c4428d3a4e1041425214f001661dca8d173 Cr-Commit-Position: refs/heads/master@{#405689} |
