Move WebGLDrawBuffers::satisfiesWebGLRequirements checks into the command buffer service. |
||||
Issue description
When a WebGL application attempts to enable the draw buffers extension, the WebGLDrawBuffers::supported function is called which in turn calls context->extensionsUtil()->supportsExtension("GL_EXT_draw_buffers")
This function returns true if the extension is requestable (not enabled yet) or already enabled.
WebGLDrawBuffers::supported proceeds to call WebGLDrawBuffers::satisfiesWebGLRequirements which does a bunch of tests for extra webgl requirements including GL calls using EXT_draw_buffers enums.
This is a problem if the extension is requestable but not enabled yet, GL errors will be generated when attempting to do the extra checks.
The extra check code should be moved into the command buffer service. It can be done in the FeatureInfo class and the extension would only be marked as requestable if it satisfies the WebGL requirements.
Kai, interested in fixing this? It will be very useful for both command buffers.
,
Feb 16 2017
I think this is just an oversight. WebGLDrawBuffers::WebGLDrawBuffers calls context->extensionsUtil()->ensureExtensionEnabled("GL_EXT_draw_buffers"). The extension's supposed to be enabled already before calling WebGLDrawBuffers::supported. We should move things around to ensure that GL_EXT_draw_buffers has been enabled before calling satisfiesWebGLRequirements.
,
Feb 16 2017
WebGLDrawBuffers::supported is a static function, I believe it's called before the WebGLDrawBuffers object is constructed.
,
Feb 16 2017
The issue we saw is that satisfiesWebGLRequirements is using enums from other extensions - not just EXT_draw_buffers:
bool supportsDepth =
(extensionsUtil->supportsExtension("GL_CHROMIUM_depth_texture") ||
extensionsUtil->supportsExtension("GL_OES_depth_texture") ||
extensionsUtil->supportsExtension("GL_ARB_depth_texture"));
bool supportsDepthStencil =
(extensionsUtil->supportsExtension("GL_EXT_packed_depth_stencil") ||
extensionsUtil->supportsExtension("GL_OES_packed_depth_stencil"));
without enabling them.
,
Feb 16 2017
Geoff: what I meant is that if we moved the call to:
context->extensionsUtil()->ensureExtensionEnabled("GL_EXT_draw_buffers")
out of the WebGLDrawBuffers constructor and into the static "supported" function, that would fix that particular ordering problem.
Kai: thanks for that info. We should fix that. I think that some of this code predates the Blink fork. Of those extensions, the only ones that should be advertised to the command buffer client are GL_CHROMIUM_depth_texture and GL_OES_packed_depth_stencil (per the AddExtensionString calls in src/gpu/command_buffer/service/feature_info.cc).
,
Feb 16 2017
Ken, if we call ensureExtensionEnabled(), the extension will be enabled in command buffer. Then we will have to add back validations to the blink side.
,
Feb 17 2017
Understood, but I'd hoped that having this extension enabled in the command buffer wouldn't have a large number of side-effects. I'd forgotten that it unfortunately does: it affects the behavior of the shader translator, among other things.
,
Feb 22 2017
Snagging this.
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b9d2510e14348d81ca532a5abe7b5cfc7d011d5 commit 3b9d2510e14348d81ca532a5abe7b5cfc7d011d5 Author: geofflang <geofflang@chromium.org> Date: Mon Feb 27 17:44:24 2017 Move the WebGL DrawBuffers support check to the command buffer service. BUG= 693233 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2711783003 Cr-Commit-Position: refs/heads/master@{#453247} [modify] https://crrev.com/3b9d2510e14348d81ca532a5abe7b5cfc7d011d5/gpu/command_buffer/service/feature_info.cc [modify] https://crrev.com/3b9d2510e14348d81ca532a5abe7b5cfc7d011d5/third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.cpp [modify] https://crrev.com/3b9d2510e14348d81ca532a5abe7b5cfc7d011d5/third_party/WebKit/Source/modules/webgl/WebGLDrawBuffers.h
,
Mar 14 2017
I think this is done, Geoff please reopen if I'm wrong.
,
Jun 20 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by kainino@chromium.org
, Feb 16 2017