Add a glBegin/End pair to make glBindFramebuffer work

Add a glBegin/End pair to make glBindFramebuffer work

It appears that this fixes the issue where rendered contents wouldn't
appear on the screen. I say "it appears" because this bug is very
difficult to repro, and so I can never say it's actually gone, just
that I wasn't able to make it happen anymore.

I can come up with a line of reasoning that the glBegin/End causes
the driver to go and validate its internal state. Perhaps sometimes the
dirty bit for the FBO is missed (and doing the glBegin/End immediately
after changing the FBO ensure it's picked up). Ultimately it is just a
guess.

This workaround was discovered by accident through the following
sequence of steps when debugging:
1. Draw a triangle to the CAOpenGLLayer after drawing the texture, to
   ensure that we are actually getting draw calls
   - This appeared and kept updating even when the texture stopped
     updating
2. Draw a triangle to the FBO's texture from the CAOpenGLLayer's
   context, to make sure changes to the texture would go through
   - This appeared and kept updating even when the textures topped
     updating (so it's probably a problem with the command buffer
     context).
3. Draw a triangle to the FBO's texture from the command buffer's
   context just before glSwapBuffers
   - This triangle never appeared, so I tried the next experiment.
4. Draw a triangle to the FBO's texture right after it is bound using
   glBindFramebufferEXT
   - Suddenly the bug went away (and I never saw the triangle, because
     it was drawn over).
5. Just do a glBegin/End with program 0, since we found this was enough to
   work around driver bugs in the past.
   - Still couldn't repro the bug with this.

BUG=435786
Committed: https://crrev.com/4ff12a73e3ec88084f1803f3fbbe998e079d5c8e
Cr-Commit-Position: refs/heads/master@{#308166}

[ccameron, 2014-12-11 03:51:27]

ccameron@chromium.org changed reviewers:
+ kbr@chromium.org

[ccameron, 2014-12-11 03:52:40]

I think we need to update the Unified OpenGL Debugging Methodology from
https://code.google.com/p/chromium/issues/detail?id=318877#c12

[Ken Russell (switch to Gerrit), 2014-12-11 07:06:37]

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4288: glEnd();
I'm surprised this compiles. This file is supposed to be making all of its calls
through the GL bindings in ui/gl/ and they only include the OpenGL ES entry
points, which don't include glBegin or glEnd. It's likely that some header is
pulling in either the system OpenGL headers or the Mesa headers. Could you
please rebuild this file via ninja with -v, get the compiler's command line for
it, and capture the preprocessor's output to figure out how this is happening?
It shouldn't be possible to call these functions from here.

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4291: glUseProgram(old_program);
Note: this isn't guaranteed to be able to fully restore the previous state. If
it happens the user recently unsuccessfully relinked the program that's current
(defined by glUseProgram), then it won't be valid to call glUseProgram again on
the program object. This will have user-visible side effects.

Is your intent to only do this workaround in the "compositor's context"? Is the
test of the bound FBO against GetBackbufferServiceId() sufficient to do that? A
way to further restrict this workaround would be to only run it if
(!force_webgl_glsl_validation_) -- but that wouldn't take into account contexts
for Pepper 3D.

[ccameron, 2014-12-11 09:57:31]

Good point -- I've moved the location of the workaround to avoid the ODR issues.

I beefed up the logic for avoiding any user-visible side-effects. I simply skip
the workaround if the program can't be re-bound (this has not affected the
effectiveness of the workaround). I only check for delete and link status -- not
sure if I need any more (validation seems relevant only for execution, and has
side-effects).

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4288: glEnd();
On 2014/12/11 07:06:37, Ken Russell wrote:
> I'm surprised this compiles. This file is supposed to be making all of its
calls
> through the GL bindings in ui/gl/ and they only include the OpenGL ES entry
> points, which don't include glBegin or glEnd. It's likely that some header is
> pulling in either the system OpenGL headers or the Mesa headers. Could you
> please rebuild this file via ninja with -v, get the compiler's command line
for
> it, and capture the preprocessor's output to figure out how this is happening?
> It shouldn't be possible to call these functions from here.

Interesting! The command line is at the bottom. I've worked around this by
moving the action to ImageTransportSurfaceFBO.

[1/109] clang++ -MMD -MF
obj/gpu/command_buffer/service/gpu.gles2_cmd_decoder.o.d
-DV8_DEPRECATION_WARNINGS -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0
-DGOOGLE_CHROME_BUILD -DCR_CLANG_REVISION=223109 -DENABLE_RLZ -DCOMPONENT_BUILD
-DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP
-DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DUSE_PROPRIETARY_CODECS
-DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS
-DENABLE_HIDPI=1 -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1
-DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DENABLE_WIFI_BOOTSTRAPPING=1 -DENABLE_LOAD_COMPLETION_HACKS=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DGPU_IMPLEMENTATION -DPROTOBUF_USE_DLLS
-DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DSKIA_DLL
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
'-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT -DSK_SUPPORT_LEGACY_TEXTRENDERMODE
-DSK_IGNORE_GPU_LAYER_HOISTING -DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DMESA_EGL_NO_X11_HEADERS -DUSE_LIBPCI=1 -DUSE_OPENSSL=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DOFFICIAL_BUILD
-DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../..
-I../../third_party/khronos -I../../gpu -I../../skia/config -Igen/protoc_out
-I../../third_party/protobuf -I../../third_party/protobuf/src
-I../../third_party/re2 -I../../third_party/skia/src/core
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext
-I../../third_party/skia/include/utils/mac -I../../third_party/icu/source/i18n
-I../../third_party/icu/source/common -Igen/ui/gl
-I../../third_party/mesa/src/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
-O2 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-mmacosx-version-min=10.6 -arch x86_64 -Wall -Wendif-labels -Wextra
-Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wheader-hygiene -Wno-char-subscripts
-Wno-unneeded-internal-declaration -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register
-Wno-inconsistent-missing-override -std=c++11 -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
/Users/ccameron/chrome-dev-official/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
-fno-strict-aliasing  -c ../../gpu/command_buffer/service/gles2_cmd_decoder.cc
-o obj/gpu/command_buffer/service/gpu.gles2_cmd_decoder.o

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4291: glUseProgram(old_program);
On 2014/12/11 07:06:37, Ken Russell wrote:
> Note: this isn't guaranteed to be able to fully restore the previous state. If
> it happens the user recently unsuccessfully relinked the program that's
current
> (defined by glUseProgram), then it won't be valid to call glUseProgram again
on
> the program object. This will have user-visible side effects.

Okay, I was curious about how this could fail. I've updated this to only call
glUseProgram(0) if the previously-bound program
- has a successful last-link status
- does not have a true deleted status
Empirically those were the only ways I was able to hit errors -- is this enough
to ensure no user-visible changes?

> Is your intent to only do this workaround in the "compositor's context"? Is
the
> test of the bound FBO against GetBackbufferServiceId() sufficient to do that?
A
> way to further restrict this workaround would be to only run it if
> (!force_webgl_glsl_validation_) -- but that wouldn't take into account
contexts
> for Pepper 3D.

By moving this to the ImageTransportSurfaceFBO, I've effectively limited its
scope to compositor contexts (AFAIK).

[ccameron, 2014-12-12 00:21:21]

Ping this again (I think this version is ready).

[Ken Russell (switch to Gerrit), 2014-12-12 02:30:45]

Thanks for reorganizing this. The new version LGTM.

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/797533002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4291: glUseProgram(old_program);
On 2014/12/11 09:57:31, ccameron1 wrote:
> On 2014/12/11 07:06:37, Ken Russell wrote:
> > Note: this isn't guaranteed to be able to fully restore the previous state.
If
> > it happens the user recently unsuccessfully relinked the program that's
> current
> > (defined by glUseProgram), then it won't be valid to call glUseProgram again
> on
> > the program object. This will have user-visible side effects.
> 
> Okay, I was curious about how this could fail. I've updated this to only call
> glUseProgram(0) if the previously-bound program
> - has a successful last-link status
> - does not have a true deleted status
> Empirically those were the only ways I was able to hit errors -- is this
enough
> to ensure no user-visible changes?

I think that should be sufficient.

> > Is your intent to only do this workaround in the "compositor's context"? Is
> the
> > test of the bound FBO against GetBackbufferServiceId() sufficient to do
that?
> A
> > way to further restrict this workaround would be to only run it if
> > (!force_webgl_glsl_validation_) -- but that wouldn't take into account
> contexts
> > for Pepper 3D.
> 
> By moving this to the ImageTransportSurfaceFBO, I've effectively limited its
> scope to compositor contexts (AFAIK). 

OK.

https://codereview.chromium.org/797533002/diff/20001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/797533002/diff/20001/ui/gl/gl_surface.h#newco...
ui/gl/gl_surface.h:93: virtual void WasBound();
Naming nit: consider NotifyWasBound or similar. WasBound sounds like a query.

[ccameron, 2014-12-12 16:17:36]

Thanks!

https://codereview.chromium.org/797533002/diff/20001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/797533002/diff/20001/ui/gl/gl_surface.h#newco...
ui/gl/gl_surface.h:93: virtual void WasBound();
On 2014/12/12 02:30:44, Ken Russell wrote:
> Naming nit: consider NotifyWasBound or similar. WasBound sounds like a query.

Done. Yeah, I wasn't happy with this name either.

[ccameron, 2014-12-12 16:17:42]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-12-12 16:19:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797533002/40001

[commit-bot: I haz the power, 2014-12-12 16:25:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 16:25:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2014-12-12 19:11:25]

Adding Victor to stamp command buffer changes

[bajones, 2014-12-12 21:30:07]

On 2014/12/12 at 19:11:25, ccameron wrote:
> Adding Victor to stamp command buffer changes

This is terrible, but... drivers. :(

LGTM gpu/command_buffer

[ccameron, 2014-12-12 21:32:26]

Thanks! (and sorry for showing you things that you now can't unsee).

[ccameron, 2014-12-12 21:32:39]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2014-12-12 21:32:39]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2014-12-12 21:32:40]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:33:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797533002/40001

[commit-bot: I haz the power, 2014-12-12 21:38:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-12 21:39:21]

Patchset 3 (id:??) landed as
https://crrev.com/4ff12a73e3ec88084f1803f3fbbe998e079d5c8e
Cr-Commit-Position: refs/heads/master@{#308166}