OpenGL(ES) debugging improvements #14772

lrusak · 2018-11-01T21:57:39Z

This change allows us to actually use the VerifyGLState() calls that are littered throughout our render system.

Using these changes we can get valuable information about debugging our OpenGL(ES) systems.

This also add a new OpenGL(ES) logging component to enable this logging.

So now you will see errors like so

14:34:49.921 T:139935484173888   ERROR: In file: /home/lukas/Documents/git/xbmc/xbmc/guilib/TextureGL.cpp function: LoadToGPU line: 202
14:34:49.921 T:139935484173888   DEBUG: Scissor test enabled: True
14:34:49.921 T:139935484173888   DEBUG: Scissor box: 0, 0, 1920, 1080
14:34:49.921 T:139935484173888   DEBUG: Viewport: 0, 0, 1920, 1080
14:34:49.921 T:139935484173888   DEBUG: Projection Matrix:
                                             1.125      0.000      0.000      0.000
                                             0.000      2.000      0.000      0.000
                                             0.000      0.000     -1.020     -1.000
                                             0.000      0.000     -1090.909   0.000
14:34:49.921 T:139935484173888   DEBUG: Modelview Matrix:
                                             1.000      0.000      0.000      0.000
                                             0.000     -1.000      0.000      0.000
                                             0.000      0.000     -1.000      0.000
                                            -960.000    540.000   -1080.000   1.000

More and/or different GL debugging can also be added later.

The only thing I'm not sure about here is if we should hard fail like I have it set after 5 OpenGL(ES) errors. Otherwise the errors will run up the log (if you have OpenGL(ES) component logging enabled) and kodi will continue to function.

The last commit adds the shader source to log if it failed to compile. This has been an issue for a while because we manipulate the shader source by inserting and appending to it. This creates an issue where the error line thrown by the glsl compilation doesn't match what is in the source file. With this we can clearly see the line that is provided by the error.

This now shows up like so:

14:34:49.507 T:139935484173888   ERROR: 0:32(7): error: syntax error, unexpected NEW_IDENTIFIER, expecting ',' or ';'
14:34:49.507 T:139935484173888   ERROR: GL: Error compiling fragment shader: gles_shader_fonts.frag gles_tonemap.frag
14:34:49.507 T:139935484173888   DEBUG: GL: fragment shader source:
                                              1: /*
                                              2:  *      Copyright (C) 2010-2013 Team XBMC
                                              3:  *      http://xbmc.org
                                              4:  *
                                              5:  *  This Program is free software; you can redistribute it and/or modify
                                              6:  *  it under the terms of the GNU General Public License as published by
                                              7:  *  the Free Software Foundation; either version 2, or (at your option)
                                              8:  *  any later version.
                                              9:  *
                                             10:  *  This Program is distributed in the hope that it will be useful,
                                             11:  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
                                             12:  *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
                                             13:  *  GNU General Public License for more details.
                                             14:  *
                                             15:  *  You should have received a copy of the GNU General Public License
                                             16:  *  along with XBMC; see the file COPYING.  If not, see
                                             17:  *  <http://www.gnu.org/licenses/>.
                                             18:  *
                                             19:  */
                                             20:
                                             21: #version 100
                                             22:
                                             23: precision mediump float;
                                             24: uniform sampler2D m_samp0;
                                             25: varying vec4 m_cord0;
                                             26: varying lowp vec4 m_colour;
                                             27:
                                             28: void main ()
                                             29: {
                                             30:   vec4 rgb;
                                             31:
                                             32:   yolo swag
                                             33:
                                             34:   rgb.rgb = m_colour.rgb;
                                             35:   rgb.a = m_colour.a * texture2D(m_samp0, m_cord0.xy).a;
                                             36:
                                             37: #if defined(KODI_LIMITED_RANGE)
                                             38:   rgb.rgb *= (235.0 - 16.0) / 255.0;
                                             39:   rgb.rgb += 16.0 / 255.0;
                                             40: #endif
                                             41:
                                             42:   gl_FragColor = rgb;
                                             43: }
                                             44: float tonemap(float val)
                                             45: {
                                             46:   return val * (1.0 + val / (m_toneP1 * m_toneP1)) / (1.0 + val);
                                             47: }
                                             48:

So you can clearly see the error is at line 32 (among other problems)

xbmc/utils/GLUtils.h


 #include "system_gl.h"

 void _VerifyGLState(const char* szfile, const char* szfunction, int lineno);
-#if defined(GL_DEBUGGING) && (defined(HAS_GL) || defined(HAS_GLES))


xbmc/guilib/Shader.cpp

@@ -102,9 +109,27 @@ bool CShader::InsertSource(const std::string& filename, const std::string& loc)

  m_source.insert(locPos, temp);

+  m_filename.append(" " + filename);


yol · 2018-11-02T12:35:06Z

Have you looked into the GL(ES) debugging extensions? Or is that not helpful for your use case?

lrusak · 2018-11-02T16:22:29Z

Have you looked into the GL(ES) debugging extensions? Or is that not helpful for your use case?

@pkerling I'm not sure what you are referring to?

I have just pushed a fixup that checks if there is a gl error before checking the settings level. This makes it so the settings check isn't getting hammered.

yol · 2018-11-02T19:16:41Z

set EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR and use https://www.khronos.org/registry/OpenGL/extensions/KHR/KHR_debug.txt

Instead of having to call glGetError all the time (which is discouraged in release builds), GL will just tell you when something goes wrong.

lrusak · 2018-11-03T21:48:08Z

@FernetMenta @pkerling I've updated this to use the opengl callbacks

I left in the updated code for VerifyGLState but changed it back to needing GL_DEBUGGING to be defined. If this code isn't wanted I can just drop it but I think it improves the output and code quality quite a bit.

I've only added the callback for OpenGLES as there are many defines that are different between OpenGL and OpenGLES.

We'll also see how well the other OpenGLES platforms conform to the spec after jenkins is done building.

lrusak · 2018-11-04T00:24:25Z

pushed a commit to add egl debug callback also

17:22:14.738 T:140267524795968   DEBUG: EGL Debugging:
                                            Error: EGL_BAD_SURFACE
                                            Command: eglSwapBuffers
                                            Type: EGL_DEBUG_MSG_ERROR_KHR
                                            Thread Label: 0x0
                                            Object Label: 0x0
                                            Message: dri2_swap_buffers

xbmc/utils/EGLUtils.cpp

@@ -71,7 +71,68 @@ std::array<std::pair<EGLint, const char*>, 32> eglAttributes =
  X(EGL_TRANSPARENT_GREEN_VALUE),
  X(EGL_TRANSPARENT_BLUE_VALUE)
 };
+
+std::array<std::pair<EGLenum, const char*>, 15> eglErrors =


xbmc/utils/EGLUtils.cpp

+  {
+    if (eglError.first == error)
+    {
+      CLog::Log(LOGERROR, "{} ({})", what.c_str(), eglError.second);


xbmc/utils/EGLUtils.cpp

xbmc/utils/GLUtils.cpp

xbmc/utils/GLUtils.h

xbmc/utils/GLUtils.cpp

yol

More comments, sorry for not putting the first ones into a review comment

xbmc/utils/EGLUtils.cpp

xbmc/rendering/gles/RenderSystemGLES.cpp

xbmc/settings/AdvancedSettings.h

@@ -380,6 +380,8 @@ class CAdvancedSettings : public ISettingCallback, public ISettingsHandler
    False to show at the bottom of video (default) */
    bool m_videoAssFixedWorks;

+    bool m_openGlDebugging;


yol

Looking good now - I think also doing it for GL would be great!

xbmc/rendering/gles/RenderSystemGLES.h

xbmc/utils/EGLUtils.cpp

xbmc/guilib/Shader.h

yol

Still found some nits to pick, but good to merge for me

xbmc/guilib/Shader.h

xbmc/rendering/gles/RenderSystemGLES.cpp

xbmc/utils/EGLUtils.cpp

xbmc/guilib/Shader.h

yol · 2018-11-08T09:14:09Z

Also do we want to print a warning if opengl debugging is enabled but the required exts are not supported?

lrusak · 2018-11-08T17:53:40Z

@pkerling updated

@popcornmix as mentioned yesterday I was able to build kodi with these updated EGL headers patch for the raspberry pi firmware repo -> http://termbin.com/a23tf

The headers cannot be the stock khronos headers as broadcom seems to have extra includes for the eglext_bcrm.h file. However as @pkerling said the headers are backwards compatible. The only other issue being that you guys seem to define #define EGL_EGLEXT_PROTOTYPES by default which shouldn't be the case. That should be up to the application to decide and even then you should just query the eglprocaddress for most functions if they are needed.

xbmc/guilib/Shader.cpp

  return true;
 }

+const std::string CShader::GetSourceWithLineNumbers()


xbmc/guilib/Shader.h

@@ -32,11 +32,17 @@ namespace Shaders {
    virtual bool InsertSource(const std::string& filename, const std::string& loc);
    bool OK() const { return m_compiled; }

+    const std::string GetName() { return m_filenames; }


xbmc/guilib/Shader.h

@@ -32,11 +32,17 @@ namespace Shaders {
    virtual bool InsertSource(const std::string& filename, const std::string& loc);
    bool OK() const { return m_compiled; }

+    const std::string GetName() { return m_filenames; }
+    const std::string GetSourceWithLineNumbers();


lrusak · 2018-11-08T18:51:49Z

@Memphiz can you look to see what is happening with iOS? I don't have access to OSX to look into it.

seems that there is no definition for some values GL_DEBUG_SOURCE_API_KHR does apple define GL_KHR_debug in GLES2/glext.h ??

xbmc/rendering/gles/RenderSystemGLES.cpp

xbmc/utils/GLUtils.cpp

-                  matrix[ixx*4], matrix[ixx*4+1], matrix[ixx*4+2],      \
-                  matrix[ixx*4+3]);                                     \
-      }                                                                 \
+void KODI::UTILS::GL::GlErrorCallback(GLenum source, GLenum type, GLuint id, GLenum severity, GLsizei length, const GLchar* message, const void *userParam)


yol · 2018-11-09T17:29:48Z

#ifdefs keep getting worse and worse :-(

lrusak · 2018-11-09T17:35:07Z

@pkerling yes sir. It won't be a problem when RPi specifics get removed 😜

This has been an issue for a while because we manipulate the shader source by inserting and appending to it. This creates an issue where the error line thrown by the glsl compilation doesn't match what is in the source file. With this we can clearly see the line that is provided by the error.

lrusak · 2018-11-13T17:06:02Z

Can I get a sign off from the relevant people? Thanks! 🚀

Memphiz · 2018-11-14T06:05:02Z

Can‘t see any review button (anymore?) - as iOS is not involved anymore I am fine with whatever came out of this.

yol · 2018-11-14T07:54:28Z

If you intend to remove the #ifdefs at some point, add a Doxy \todo

lrusak · 2018-11-19T17:03:38Z

@FernetMenta are you ok with this?

FernetMenta · 2018-11-19T19:38:54Z

+1

lrusak · 2018-11-20T17:26:11Z

@MartijnKaijser ok to merge?

OpenGL(ES) debugging improvements

lrusak added Type: Feature non-breaking change which adds functionality Component: GUI rendering Platform: Android Platform: Linux Platform: macOS Platform: RPi Platform: iOS v18 Leia Platform: AMLogic labels Nov 1, 2018

lrusak added this to the Leia 18.0-beta5 milestone Nov 1, 2018

lrusak requested a review from FernetMenta November 1, 2018 21:57

FernetMenta reviewed Nov 2, 2018

View reviewed changes

MartijnKaijser removed this from the Leia 18.0-beta5 milestone Nov 3, 2018

lrusak force-pushed the opengl-debugging branch from 1516472 to d2e95ca Compare November 3, 2018 21:45