Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1323613005:
Better state tracking and validation for bindBufferBase and bindBufferRange (Closed)
DescriptionBetter state tracking and validation for bindBufferBase and bindBufferRange
BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201834
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed feedback #
Total comments: 3
Patch Set 3 : Addressed kbr@'s feedback #
Total comments: 2
Patch Set 4 : Fix logic errors pointed out by zmo@ #
Messages
Total messages: 18 (6 generated)
bajones@chromium.org changed reviewers: + kbr@chromium.org, zmo@chromium.org
This implements not only better validation for bindBufferRange and bindBufferBase, but also starts tracking indexed Transform Feedback and Uniform Buffer bind points so that they can be properly validated against and so that the associated getIndexedParameter call will work.
LGTM with nits fixed https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... Source/modules/webgl/WebGL2RenderingContextBase.cpp:87: GLint maxTransformFeedbackSeparateAttribs; initialize it 0 https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2086: { The comparison only makes sense if buffer != nullptr, so please add an ASSERT here. https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2100: { ASSERT(buffer) https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2121: { ASSERT(buffer) https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2160: if (isBufferBoundToTransformFeedback(buffer)) { you can combine this with the previous if clause with ||
On 2015/09/04 18:10:44, Zhenyao Mo wrote: > LGTM with nits fixed Thanks! Fixed everything, with one alteration: > https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:2160: if > (isBufferBoundToTransformFeedback(buffer)) { > you can combine this with the previous if clause with || I could, but looking at the code I realized it was doing unnecessary work. Restructured as: if (target == GL_TRANSFORM_FEEDBACK_BUFFER) { if (isBufferBoundToNonTransformFeedback(buffer)) { /* error */ } } else if (isBufferBoundToTransformFeedback(buffer)) { /* error */ }
On 2015/09/04 18:10:44, Zhenyao Mo wrote: > LGTM with nits fixed Thanks! Fixed everything, with one alteration: > https://codereview.chromium.org/1323613005/diff/1/Source/modules/webgl/WebGL2... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:2160: if > (isBufferBoundToTransformFeedback(buffer)) { > you can combine this with the previous if clause with || I could, but looking at the code I realized it was doing unnecessary work. Restructured as: if (target == GL_TRANSFORM_FEEDBACK_BUFFER) { if (isBufferBoundToNonTransformFeedback(buffer)) { /* error */ } } else if (isBufferBoundToTransformFeedback(buffer)) { /* error */ }
The CQ bit was checked by bajones@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/1323613005/#ps20001 (title: "Addressed feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323613005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323613005/20001
LGTM overall with a couple of concerns. https://codereview.chromium.org/1323613005/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1323613005/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2092: for (size_t i = 0; i < m_boundIndexedTransformFeedbackBuffers.size(); ++i) { Any idea how large this array is in practice? Concerned about the overhead that will be introduced for each binding operation. Have you run some WebGL samples that draw lots of geometry to make sure there's no visible performance regression introduced? (We should add more of these to the tough_webgl_cases benchmark.) https://codereview.chromium.org/1323613005/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2115: for (size_t i = 0; i < m_boundIndexedUniformBuffers.size(); ++i) { Same concern here. https://codereview.chromium.org/1323613005/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.h (right): https://codereview.chromium.org/1323613005/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.h:200: bool validateBufferTargetCompatiblity(const char*, GLenum, WebGLBuffer*); typo: Compatiblity -> Compatibility
The CQ bit was unchecked by bajones@chromium.org
On 2015/09/04 20:51:32, Ken Russell wrote: > Any idea how large this array is in practice? Just checkout on my Quadro 600: 4 Transform feeback buffers (that's not bad) and 84 Uniform Buffers (oof!) Those numbers are in line with publicly reported stats: http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_... http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_... To address that I implemented the same optimization we have for checking texture units: I track the highest actively bound Uniform Buffer index and only check up to there. This of course can be sub optimal if a developer decides they absolutely MUST use the 83rd index while leaving the rest empty, but in that case I say they're asking for problems anyway. Didn't bother doing the same for Transform feedback buffers because: 4. PTAL!
https://codereview.chromium.org/1323613005/diff/40001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1323613005/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2115: for (size_t i = 0; i < m_maxBoundUniformBufferIndex; ++i) { <= https://codereview.chromium.org/1323613005/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:2280: if (m_boundIndexedUniformBuffers[index].get()) index -> i
On 2015/09/04 22:24:08, Zhenyao Mo wrote: > https://codereview.chromium.org/1323613005/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:2115: for (size_t i = 0; i < > m_maxBoundUniformBufferIndex; ++i) { > <= Derp. :P > https://codereview.chromium.org/1323613005/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:2280: if > (m_boundIndexedUniformBuffers[index].get()) > index -> i Double Derp! :( Thanks for pointing those out! Fixed.
LGTM again.
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/1323613005/#ps60001 (title: "Fix logic errors pointed out by zmo@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323613005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323613005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201834 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
