Differential D7132

Add OpenGL debug logging using QOpenGLDebugLogger
ClosedPublic
Actions

Authored by alvinhochun on Aug 4 2017, 3:05 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Commits
R37:c1403f01cb33: Change OpenGL setDefaultFormat check to safe assert
R37:1ffe3b59c29c: Add OpenGL debug logging using QOpenGLDebugLogger
R37:b58749ea7bdd: Change OpenGL setDefaultFormat check to safe assert
R37:f638b81f0365: Rename enableOpenGLDebugging config
R37:21127cec004d: Remove redundant useVerboseOpenGLDebugOutput config
R37:2aee263845e6: Add OpenGL debug logging using QOpenGLDebugLogger
R37:6b78f931b779: Remove redundant useVerboseOpenGLDebugOutput config
R37:dc6cd47193ee: Rename enableOpenGLDebugging config
Summary
Allow debug message logging from the OpenGL driver if it supports the
extension `GL_KHR_debug`.

This adds two options to the config file `kritadisplayrc`:

 - `EnableOpenGLDebug`: Set to `true` to enable logging. Default is
   `false`.
 - `OpenGLDebugSynchronous`: Set to `true` to log in synchronous mode.
   This is useful for setting a breakpoint at `openglOnMessageLogged`
   and checking the backtrace to see the cause of the message. Default
   is `false` for asynchronous mode.

Alternatively, the environment variable `KRITA_OPENGL_DEBUG` can also be
used to enable the logging:

 - (not set): Use the settings from the config file.
 - `sync`: Enable logging in synchronous mode.
 - (other values): Enable logging in asynchronous mode.

I think this might be useful for development. It might not be able to catch misbehaving drivers though.

I used this when checking incompatibility of existing OpenGL code with ANGLE.

(This patch applies against 'master')

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alvinhochun created this revision.Aug 4 2017, 3:05 PM
Restricted Application added a subscriber: woltherav. · View Herald TranscriptAug 4 2017, 3:05 PM
dkazakov requested changes to this revision.Aug 7 2017, 2:45 PM
dkazakov added a subscriber: dkazakov.

Hi, @alvinhochun!

The patch seem to work fine here. Here are my notes:

  1. Please convert an extra setDefaltFormat() call in KisOpenGL::initialize() into a safe assert. Otherwise it is not understandable, which way of openGL initialization is considered correct, and which not.
  2. [disputable] See an inline comment about sync option
  3. [real problem] After you patch we have three different options for openGL debugging:
    • KisConfig::enableOpenGLDebugging() shows performance metrics of the canvas and dumps glSync() ratio to stdout. Should probably renamed into something like "Debug openGL Performance". Mind you, it should also be renamed in the preferences dialog in the performace tab.
    • KisConfig::useVerboseOpenGLDebugOutput() enables a debugging message that is already enabled. Can be removed I guess.
    • You option.
libs/ui/opengl/kis_opengl.cpp
256

I have a feeling that the code would be a bit more readable if isSynchronous would have been passed to setDefaultFormat() alongside the debugging switch. Otherwise it is extremely difficult to find out that the two options are related and depend on each other. Though it is disputable of course.

This revision now requires changes to proceed.Aug 7 2017, 2:45 PM
alvinhochun added a comment.Aug 7 2017, 6:14 PM
In D7132#133362, @dkazakov wrote:

Hi, @alvinhochun!

The patch seem to work fine here. Here are my notes:

  1. Please convert an extra setDefaltFormat() call in KisOpenGL::initialize() into a safe assert. Otherwise it is not understandable, which way of openGL initialization is considered correct, and which not.

Okay, I'll change that in the updated diff but I'll make it an individual commit. Is it KIS_SAFE_ASSERT_RECOVER that I should use?

  1. [disputable] See an inline comment about sync option

Replied.

  1. [real problem] After you patch we have three different options for openGL debugging:
    • KisConfig::enableOpenGLDebugging() shows performance metrics of the canvas and dumps glSync() ratio to stdout. Should probably renamed into something like "Debug openGL Performance". Mind you, it should also be renamed in the preferences dialog in the performace tab.
    • KisConfig::useVerboseOpenGLDebugOutput() enables a debugging message that is already enabled. Can be removed I guess.
    • You option.

I'll remove KisConfig::useVerboseOpenGLDebugOutput() in another commit.

KisConfig::enableOpenGLDebugging() on the other hand seems to be something worth keeping and is for a completely different purpose than what this patch does. It's a good idea to rename it, but should the config entry key also be changed? Changing it will reset the option for those who happen to have it enabled. Also, this will be yet another commit.

libs/ui/opengl/kis_opengl.cpp
256

Make sense. Originally I was thinking this can be toggled at run-time, but it requires more code which I don't really plan to add. I will just move this into setDefaultFormat.

alvinhochun updated this revision to Diff 17886.Aug 8 2017, 3:37 PM
alvinhochun edited edge metadata.
alvinhochun marked 2 inline comments as done.

Made the changes, please comment.

dkazakov accepted this revision.Aug 9 2017, 8:16 AM

Hi, @alvinhochun!

The patch looks fine now. Please consider adding format initialization to the recovery section of the assert (see inline comment) and then push :)

libs/ui/opengl/kis_opengl.cpp
64

I think it is worth adding setDefaultFormat(); into the recovery section. Recovery section will be executed on the user's computer when the assetion is failed and the user will choose to ignore it, or when the safe asserts are just disabled: then the recovery branch will be taken automatically on failure.

This revision is now accepted and ready to land.Aug 9 2017, 8:16 AM
Closed by commit R37:2aee263845e6: Add OpenGL debug logging using QOpenGLDebugLogger (authored by alvinhochun). · Explain WhyAug 9 2017, 8:30 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mkrita/main.cc (12 lines)
Mlibs/ui/opengl/kis_opengl.h (2 lines)
Mlibs/ui/opengl/kis_opengl.cpp (30 lines)

Diff 17918

View Options

krita/main.cc

Loading...
View Options

libs/ui/opengl/kis_opengl.h

Loading...
View Options

libs/ui/opengl/kis_opengl.cpp

Loading...