Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2080943002:
gpu: Use a VAO as required by the core profile in ClearFramebuffer (Closed)
|
Created:
4 years, 6 months ago by Corentin Wallez Modified:
4 years, 5 months ago CC:
chromium-reviews, piman+watch_chromium.org, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: Use a VAO as required by the core profile in ClearFramebuffer
glVertexAttribPointer requires that a VAO is bound when using the OpenGL
core profile.
BUG=598902
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/61e2da5b097776feecc81890cc68bc3f57172a08
Cr-Commit-Position: refs/heads/master@{#401097}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #
Messages
Total messages: 26 (9 generated)
Description was changed from ========== gpu: Use a VAO as required by the core profile in ClearFramebuffer glVertexAttribPointer requires that a VAO is bound when using the OpenGL core profile. BUG=598902 ========== to ========== gpu: Use a VAO as required by the core profile in ClearFramebuffer glVertexAttribPointer requires that a VAO is bound when using the OpenGL core profile. BUG=598902 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
cwallez@chromium.org changed reviewers: + geofflang@chromium.org, zmo@chromium.org
PTAL, this is a core profile fix needed to enable WebGL2 on Intel on Linux.
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2080943002/1
On 2016/06/20 20:54:45, Corentin Wallez wrote: > PTAL, this is a core profile fix needed to enable WebGL2 on Intel on Linux. Help me understand: why this isn't an issue on Mac or Linux with NVidia (with --use-gl=angle) but only on Linux with Intel?
It seems more work is needed. AT this point we will need to support both path, with and without native vao support, assuming we don't have the support everywhere. https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc (right): https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:76: glGenVertexArraysOES(1, &vao_); initialize vao_ to 0 in constructor. and dcheck it's 0 above like buffer_id_. https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:95: glDeleteVertexArraysOES(1, &vao_); Also set vao_ to 0 like buffer_id_ https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:147: glEnableVertexAttribArray(kVertexPositionAttrib); Shouldn't you also set all the vao states up in Initialize, like we did with buffer_id_, so you don't have to do it in every clear? Simply binding to vao_ will do? https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:185: decoder->RestoreAllAttributes(); This seems like an over kill. You just need to restore the vao bindings here, nothing else.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/20 at 21:16:21, zmo wrote: > It seems more work is needed. AT this point we will need to support both path, with and without native vao support, assuming we don't have the support everywhere. > On 2016/06/20 at 21:07:58, zmo wrote: > On 2016/06/20 20:54:45, Corentin Wallez wrote: > > PTAL, this is a core profile fix needed to enable WebGL2 on Intel on Linux. > > Help me understand: why this isn't an issue on Mac or Linux with NVidia (with --use-gl=angle) but only on Linux with Intel? https://www.opengl.org/registry/doc/glspec33.core.20100311.pdf page 344 (section E.2): Calling VertexAttribPointer when no buffer object or no vertex array object is bound will generate an INVALID_OPERATION error, as will calling any array drawing command when no vertex array object is bound. I'm not sure why this is working on Mac, ANGLE always uses VAOs and has a "real" VAO used as a backing object for the default OpenGL ES VAO. > https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc (right): > > https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:76: glGenVertexArraysOES(1, &vao_); > initialize vao_ to 0 in constructor. and dcheck it's 0 above like buffer_id_. > Done > https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:95: glDeleteVertexArraysOES(1, &vao_); > Also set vao_ to 0 like buffer_id_ > Done > https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:147: glEnableVertexAttribArray(kVertexPositionAttrib); > Shouldn't you also set all the vao states up in Initialize, like we did with buffer_id_, so you don't have to do it in every clear? Simply binding to vao_ will do? > Done > https://codereview.chromium.org/2080943002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_clear_framebuffer.cc:185: decoder->RestoreAllAttributes(); > This seems like an over kill. You just need to restore the vao bindings here, nothing else. RestoreAllAttributes only restores the vao binding and the default vertex attribs so it seems pretty fast. Do you still want to add a RestoreVertexArrayBinding that doesn't reset the default vertex attribs?
lgtm
The CQ bit was checked by cwallez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2080943002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_optional_gpu_tests_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
I can't make sense of the Windows AMD failure. Trying again.
The CQ bit was checked by cwallez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2080943002/20001
The mac AMD failures on same tests with an exception from GL driver. It might be an AMD driver issue. Do you have access to a Mac AMD to reproduce?
On 2016/06/21 at 19:31:58, zmo wrote: > The mac AMD failures on same tests with an exception from GL driver. > > It might be an AMD driver issue. Do you have access to a Mac AMD to reproduce? Yes, might be a good time to dust it off. I also have a Linux AMD machine.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== gpu: Use a VAO as required by the core profile in ClearFramebuffer glVertexAttribPointer requires that a VAO is bound when using the OpenGL core profile. BUG=598902 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Use a VAO as required by the core profile in ClearFramebuffer glVertexAttribPointer requires that a VAO is bound when using the OpenGL core profile. BUG=598902 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/61e2da5b097776feecc81890cc68bc3f57172a08 Cr-Commit-Position: refs/heads/master@{#401097} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/61e2da5b097776feecc81890cc68bc3f57172a08 Cr-Commit-Position: refs/heads/master@{#401097}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2125293002/ by cwallez@chromium.org. The reason for reverting is: Causes an assert in the IMG GPU driver in Android 4.4.
Message was sent while issue was closed.
Why does Linux care about the 'gl_clear_broken' workaround? This codepath should only be taken on Android+IMG (which is also what didn't like this patch).
Message was sent while issue was closed.
Description was changed from ========== gpu: Use a VAO as required by the core profile in ClearFramebuffer glVertexAttribPointer requires that a VAO is bound when using the OpenGL core profile. BUG=598902 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/61e2da5b097776feecc81890cc68bc3f57172a08 Cr-Commit-Position: refs/heads/master@{#401097} ========== to ========== gpu: Use a VAO as required by the core profile in ClearFramebuffer glVertexAttribPointer requires that a VAO is bound when using the OpenGL core profile. BUG=598902 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/61e2da5b097776feecc81890cc68bc3f57172a08 Cr-Commit-Position: refs/heads/master@{#401097} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
