Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2329523002:
Save/restore PBO bindings in DrawingBuffer (Closed)
|
Created:
4 years, 3 months ago by Kai Ninomiya Modified:
4 years, 3 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Justin Novosad, haraken, Rik, f(malita), blink-reviews, piman+watch_chromium.org, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis, qiankun Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSave/restore PBO bindings in DrawingBuffer
DrawingBuffer::prepareTextureMailboxInternal
This prevents TexImage2D from being called internally with a
leaked pixel unpack buffer object bound.
BUG=644572
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/2c847da844d0f3f802069754b840bc36c3366c87
Cr-Commit-Position: refs/heads/master@{#417815}
Patch Set 1 #
Total comments: 5
Patch Set 2 : do this conditionally on WebGL2 #Patch Set 3 : fix PBO deletion #Patch Set 4 : oops #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 ========== to ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
kainino@chromium.org changed reviewers: + kbr@chromium.org, zmo@chromium.org
On 2016/09/09 17:56:32, Kai Ninomiya wrote: > mailto:kainino@chromium.org changed reviewers: > + mailto:kbr@chromium.org, mailto:zmo@chromium.org (PTAL.)
https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1391: contextGL()->BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); You can't do this in WebGL1 because if it's implemented on top of ES2, then PIXEL_UNPACK_BUFFER is invalid enum Here and all other places.
I'm mainly just pointing out what zmo@ already pointed out. https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:327: m_gl->BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); Here also, this needs to only be executed if the context is WebGL 2.0 / ES 3.0. https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:962: m_gl->BindBuffer(GL_PIXEL_UNPACK_BUFFER, m_pixelUnpackBufferBinding); This must only be done for WebGL 2.0 / ES 3.0 contexts.
https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1391: contextGL()->BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); On 2016/09/09 18:58:04, Zhenyao Mo wrote: > You can't do this in WebGL1 because if it's implemented on top of ES2, then > PIXEL_UNPACK_BUFFER is invalid enum > > Here and all other places. Done. https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:327: m_gl->BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); On 2016/09/09 20:10:33, Ken Russell wrote: > Here also, this needs to only be executed if the context is WebGL 2.0 / ES 3.0. Done.
On 2016/09/09 20:30:45, Kai Ninomiya wrote: > https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1391: > contextGL()->BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); > On 2016/09/09 18:58:04, Zhenyao Mo wrote: > > You can't do this in WebGL1 because if it's implemented on top of ES2, then > > PIXEL_UNPACK_BUFFER is invalid enum > > > > Here and all other places. > > Done. > > https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/2329523002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:327: > m_gl->BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); > On 2016/09/09 20:10:33, Ken Russell wrote: > > Here also, this needs to only be executed if the context is WebGL 2.0 / ES > 3.0. > > Done. lgtm
Very nice. LGTM too.
The CQ bit was checked by kainino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kainino@chromium.org
The CQ bit was checked by kainino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kainino@chromium.org
Updated to fix test failure
Great. Still LGTM.
The CQ bit was checked by kainino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2329523002/#ps40001 (title: "fix PBO deletion")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kainino@chromium.org
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/2329523002/#ps60001 (title: "oops")
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 ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Save/restore PBO bindings in DrawingBuffer DrawingBuffer::prepareTextureMailboxInternal This prevents TexImage2D from being called internally with a leaked pixel unpack buffer object bound. BUG=644572 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/2c847da844d0f3f802069754b840bc36c3366c87 Cr-Commit-Position: refs/heads/master@{#417815} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2c847da844d0f3f802069754b840bc36c3366c87 Cr-Commit-Position: refs/heads/master@{#417815} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
