From fbe4bca6fb3b1e75682c3c450cfad2dbaeb28c34 Mon Sep 17 00:00:00 2001 From: baldurk Date: Wed, 28 Aug 2019 12:38:28 +0100 Subject: [PATCH] Add illegal workaround for broken Android skia library * Skia had a bug (which has been fixed) where it would incorrectly check for glGetError() to see if glProgramBinary() had succeeded. This is completely wrong and it should be checking the link status. When RenderDoc silently dropped the function call to leave the link status as invalid, Skia deleted the program and then tried to use it anyway leading to incorrect rendering. * Potential other fixes in order of preference: - Spec-compliant: Return a random program binary format each time the program runs. This should prevent a correctly-written program from re-using cached binaries, but of course Skia ignores the binary format and uploads it anyway. We'd hit the same broken error check and we're back to square one. - Spec-compliant: Write a program binary format of our own that embeds all the shader source and replays it again. Would be valid and work, but is very complex. - Non-spec-compliant: Fake the error code that Skia is looking for. In a program which is written correctly we then poison glGetError() and cause unpredictable and possibly serious errors elsewhere. * Instead we just return 0 for GL_NUM_PROGRAM_BINARY_FORMATS on Android, and Skia turns off its caching entirely. This is not spec-compliant either since the spec requires at least one format in the list that is returned by glGetProgramBinary, but if the Android OS is going to break the spec at us then we'll break it right back. * Bottom-line: Android is an absolutely horrible operating system that is broken at every turn and no-one should be forced to deal with it. --- renderdoc/driver/gl/wrappers/gl_get_funcs.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/renderdoc/driver/gl/wrappers/gl_get_funcs.cpp b/renderdoc/driver/gl/wrappers/gl_get_funcs.cpp index 39ecb3f9a..624b321af 100644 --- a/renderdoc/driver/gl/wrappers/gl_get_funcs.cpp +++ b/renderdoc/driver/gl/wrappers/gl_get_funcs.cpp @@ -218,6 +218,28 @@ void WrappedOpenGL::glGetIntegerv(GLenum pname, GLint *params) *params = (GLint)GetCtxData().glExts.size(); return; } +#if ENABLED(RDOC_ANDROID) + else if(pname == eGL_NUM_PROGRAM_BINARY_FORMATS) + { + // This is not spec-compliant. The spec is written in a convoluted way but has a self-consistent + // loop requiring this to be non-zero: + // + // - The program binary format list must include the binaryFormat returned by glGetProgramBinary + // - glGetProgramBinary is always valid to call on a linked program. + // - The original extension says that calling glGetProgramBinary is illegal when + // GL_NUM_PROGRAM_BINARY_FORMATS is 0, which would potentially allow a weird but valid reading + // where either the list could be empty because "the binaryFormat returned by + // glGetProgramBinary" does not exist because there's no valid way to call the function. + // + // The short answer is that doing this is invalid and so we only do it on Android where the + // OPERATING SYSTEM ships a buggy library where this is the only feasible workaround, short of + // implementing our own program binary format that contains the original source and + // reconstructing shaders out of it. + if(params) + *params = 0; + return; + } +#endif else if(pname == eGL_DEBUG_TOOL_PURPOSE_EXT) { if(params)