Better exception handling and context locking for GAL.

This prevents deadlocks when exceptions are thrown and the context
ends up not getting unlocked.

It also removes an earlier hack to try and minimize this which
didn't work anyway.

common/draw_panel_gal.cpp

@@ -182,6 +182,8 @@ void EDA_DRAW_PANEL_GAL::onPaint( wxPaintEvent& WXUNUSED( aEvent ) )
     {
         m_view->UpdateItems();
 
+        KIGFX::GAL_CONTEXT_LOCKER locker( m_gal );
+
         m_gal->BeginDrawing();
         m_gal->SetClearColor( settings->GetBackgroundColor() );
         m_gal->SetGridColor( settings->GetGridColor() );

common/gal/opengl/opengl_gal.cpp

@@ -337,7 +337,8 @@ void OPENGL_GAL::BeginDrawing()
     if( !isInitialized )
         init();
 
-    GL_CONTEXT_MANAGER::Get().LockCtx( glPrivContext, this );
+    wxASSERT_MSG( isContextLocked, "You must create a local GAL_CONTEXT_LOCKER instance "
+                                   "before calling GAL::BeginDrawing()." );
 
     // Set up the view port
     glMatrixMode( GL_PROJECTION );
@@ -348,18 +349,10 @@ void OPENGL_GAL::BeginDrawing()
 
     if( !isFramebufferInitialized )
     {
-        try
+        // Prepare rendering target buffers
-        {
+        compositor->Initialize();
-            // Prepare rendering target buffers
+        mainBuffer = compositor->CreateBuffer();
-            compositor->Initialize();
+        overlayBuffer = compositor->CreateBuffer();
-            mainBuffer = compositor->CreateBuffer();
-            overlayBuffer = compositor->CreateBuffer();
-        }
-        catch( std::runtime_error& )
-        {
-            GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
-            throw;      // DRAW_PANEL_GAL will handle it
-        }
 
         isFramebufferInitialized = true;
     }
@@ -470,6 +463,8 @@ void OPENGL_GAL::BeginDrawing()
 
 void OPENGL_GAL::EndDrawing()
 {
+    wxASSERT_MSG( isContextLocked, "What happened to the context lock?" );
+
 #ifdef __WXDEBUG__
     PROF_COUNTER totalRealTime( "OPENGL_GAL::EndDrawing()", true );
 #endif /* __WXDEBUG__ */
@@ -493,7 +488,6 @@ void OPENGL_GAL::EndDrawing()
     blitCursor();
 
     SwapBuffers();
-    GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
 
 #ifdef __WXDEBUG__
     totalRealTime.Stop();
@@ -502,6 +496,20 @@ void OPENGL_GAL::EndDrawing()
 }
 
 
+void OPENGL_GAL::lockContext()
+{
+    GL_CONTEXT_MANAGER::Get().LockCtx( glPrivContext, this );
+    isContextLocked = true;
+}
+
+
+void OPENGL_GAL::unlockContext()
+{
+    isContextLocked = false;
+    GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
+}
+
+
 void OPENGL_GAL::BeginUpdate()
 {
     if( !IsShownOnScreen() || GetClientRect().IsEmpty() )
@@ -1953,59 +1961,51 @@ void OPENGL_GAL::init()
 {
     wxASSERT( IsShownOnScreen() );
 
-    GL_CONTEXT_MANAGER::Get().LockCtx( glPrivContext, this );
+    GAL_CONTEXT_LOCKER locker( this );
 
     GLenum err = glewInit();
 
-    try
+    if( GLEW_OK != err )
-    {
+        throw std::runtime_error( (const char*) glewGetErrorString( err ) );
-        if( GLEW_OK != err )
-            throw std::runtime_error( (const char*) glewGetErrorString( err ) );
 
-        // Check the OpenGL version (minimum 2.1 is required)
+    // Check the OpenGL version (minimum 2.1 is required)
-        if( !GLEW_VERSION_2_1 )
+    if( !GLEW_VERSION_2_1 )
-            throw std::runtime_error( "OpenGL 2.1 or higher is required!" );
+        throw std::runtime_error( "OpenGL 2.1 or higher is required!" );
 
 #if defined (__LINUX__)      // calling enableGlDebug crashes opengl on some OS (OSX and some Windows)
 #ifdef DEBUG
-        if( GLEW_ARB_debug_output )
+    if( GLEW_ARB_debug_output )
-            enableGlDebug( true );
+        enableGlDebug( true );
 #endif
 #endif
 
-        // Framebuffers have to be supported
+    // Framebuffers have to be supported
-        if( !GLEW_EXT_framebuffer_object )
+    if( !GLEW_EXT_framebuffer_object )
-            throw std::runtime_error( "Framebuffer objects are not supported!" );
+        throw std::runtime_error( "Framebuffer objects are not supported!" );
 
-        // Vertex buffer has to be supported
+    // Vertex buffer has to be supported
-        if( !GLEW_ARB_vertex_buffer_object )
+    if( !GLEW_ARB_vertex_buffer_object )
-            throw std::runtime_error( "Vertex buffer objects are not supported!" );
+        throw std::runtime_error( "Vertex buffer objects are not supported!" );
 
-        // Prepare shaders
+    // Prepare shaders
-        if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_VERTEX, BUILTIN_SHADERS::kicad_vertex_shader ) )
+    if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_VERTEX, BUILTIN_SHADERS::kicad_vertex_shader ) )
-            throw std::runtime_error( "Cannot compile vertex shader!" );
+        throw std::runtime_error( "Cannot compile vertex shader!" );
 
-        if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_FRAGMENT, BUILTIN_SHADERS::kicad_fragment_shader ) )
+    if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_FRAGMENT, BUILTIN_SHADERS::kicad_fragment_shader ) )
-            throw std::runtime_error( "Cannot compile fragment shader!" );
+        throw std::runtime_error( "Cannot compile fragment shader!" );
 
-        if( !shader->IsLinked() && !shader->Link() )
+    if( !shader->IsLinked() && !shader->Link() )
-            throw std::runtime_error( "Cannot link the shaders!" );
+        throw std::runtime_error( "Cannot link the shaders!" );
 
-        // Check if video card supports textures big enough to fit the font atlas
+    // Check if video card supports textures big enough to fit the font atlas
-        int maxTextureSize;
+    int maxTextureSize;
-        glGetIntegerv( GL_MAX_TEXTURE_SIZE, &maxTextureSize );
+    glGetIntegerv( GL_MAX_TEXTURE_SIZE, &maxTextureSize );
 
-        if( maxTextureSize < (int) font_image.width || maxTextureSize < (int)font_image.height )
+    if( maxTextureSize < (int) font_image.width || maxTextureSize < (int)font_image.height )
-        {
-            // TODO implement software texture scaling
-            // for bitmap fonts and use a higher resolution texture?
-            throw std::runtime_error( "Requested texture size is not supported" );
-        }
-    }
-    catch( std::runtime_error& )
     {
-        GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
+        // TODO implement software texture scaling
-        throw;
+        // for bitmap fonts and use a higher resolution texture?
+        throw std::runtime_error( "Requested texture size is not supported" );
     }
 
     cachedManager = new VERTEX_MANAGER( true );
@@ -2017,7 +2017,6 @@ void OPENGL_GAL::init()
     nonCachedManager->SetShader( *shader );
     overlayManager->SetShader( *shader );
 
-    GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
     isInitialized = true;
 }
 

common/gal/opengl/utils.cpp

@@ -51,7 +51,44 @@ int checkGlError( const std::string& aInfo, bool aThrow )
             break;
 
         case GL_INVALID_FRAMEBUFFER_OPERATION:
-            errorMsg = wxString::Format( "Error: %s: invalid framebuffer operation", aInfo );
+        {
+            GLenum status = glCheckFramebufferStatusEXT( GL_FRAMEBUFFER_EXT );
+
+            if( status != GL_FRAMEBUFFER_COMPLETE_EXT )
+            {
+                switch( status )
+                {
+                case GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT_EXT:
+                    errorMsg = "The framebuffer attachment points are incomplete.";
+                    break;
+                case GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT_EXT:
+                    errorMsg = "No images attached to the framebuffer.";
+                    break;
+                case GL_FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER_EXT:
+                    errorMsg = "The framebuffer does not have at least one image attached to it.";
+                    break;
+                case GL_FRAMEBUFFER_INCOMPLETE_READ_BUFFER_EXT:
+                    errorMsg = "The framebuffer read buffer is incomplete.";
+                    break;
+                case GL_FRAMEBUFFER_UNSUPPORTED_EXT:
+                    errorMsg = "The combination of internal formats of the attached images violates an implementation-dependent set of restrictions.";
+                    break;
+                case GL_FRAMEBUFFER_INCOMPLETE_MULTISAMPLE_EXT:
+                    errorMsg = "GL_RENDERBUFFER_SAMPLES is not the same for all attached renderbuffers.";
+                    break;
+                case GL_FRAMEBUFFER_INCOMPLETE_LAYER_TARGETS_EXT:
+                    errorMsg = "Framebuffer incomplete layer targets errors.";
+                    break;
+                case GL_FRAMEBUFFER_INCOMPLETE_DIMENSIONS_EXT:
+                    errorMsg = "Framebuffer attachments have different dimensions";
+                    break;
+                default:
+                    errorMsg = "Unknown incomplete framebufer error";
+                }
+            }
+            else
+                errorMsg = wxString::Format( "Error: %s: invalid framebuffer operation", aInfo );
+        }
             break;
 
         case GL_OUT_OF_MEMORY:

common/kiway.cpp

@@ -443,21 +443,11 @@ void KIWAY::CommonSettingsChanged()
     }
 #endif
 
-    // OK, this is a hack, but it keeps OpenGL from puking when updating
-    // something like grid settings when several OpenGL canvasses are open.
     for( unsigned i=0;  i < KIWAY_PLAYER_COUNT;  ++i )
     {
         KIWAY_PLAYER* frame = GetPlayerFrame( ( FRAME_T )i );
 
-        if( frame && frame->IsActive() )
+        if( frame )
-            frame->CommonSettingsChanged();
-    }
-
-    for( unsigned i=0;  i < KIWAY_PLAYER_COUNT;  ++i )
-    {
-        KIWAY_PLAYER* frame = GetPlayerFrame( ( FRAME_T )i );
-
-        if( frame && !frame->IsActive() )
             frame->CommonSettingsChanged();
     }
 }

include/gal/graphics_abstraction_layer.h

@@ -56,8 +56,10 @@ namespace KIGFX
  * for drawing purposes these are transformed to screen units with this layer. So zooming is handled here as well.
  *
  */
-class GAL: GAL_DISPLAY_OPTIONS_OBSERVER
+class GAL : GAL_DISPLAY_OPTIONS_OBSERVER
 {
+    friend class GAL_CONTEXT_LOCKER;
+
 public:
     // Constructor / Destructor
     GAL( GAL_DISPLAY_OPTIONS& aOptions );
@@ -1058,6 +1060,10 @@ protected:
     /// Instance of object that stores information about how to draw texts
     STROKE_FONT        strokeFont;
 
+    virtual void lockContext() {}
+
+    virtual void unlockContext() {}
+
     /// Compute the scaling factor for the world->screen matrix
     inline void computeWorldScale()
     {
@@ -1121,6 +1127,27 @@ private:
         bool                m_mirrored;
     } textProperties;
 };
-}    // namespace KIGFX
+
+
+class GAL_CONTEXT_LOCKER
+{
+public:
+    GAL_CONTEXT_LOCKER( GAL* aGal ) :
+        m_gal( aGal )
+    {
+        m_gal->lockContext();
+    }
+
+    ~GAL_CONTEXT_LOCKER()
+    {
+        m_gal->unlockContext();
+    }
+
+private:
+    GAL* m_gal;
+};
+
+
+};    // namespace KIGFX
 
 #endif /* GRAPHICSABSTRACTIONLAYER_H_ */

include/gal/opengl/opengl_gal.h

@@ -328,10 +328,15 @@ private:
     bool                    isInitialized;              ///< Basic initialization flag, has to be done
                                                         ///< when the window is visible
     bool                    isGrouping;                 ///< Was a group started?
+    bool                    isContextLocked;            ///< Used for assertion checking
     GLint                   ufm_worldPixelSize;
 
     std::unique_ptr<GL_BITMAP_CACHE>         bitmapCache;
 
+    void lockContext();
+
+    void unlockContext();
+
     ///< Update handler for OpenGL settings
     bool updatedGalDisplayOptions( const GAL_DISPLAY_OPTIONS& aOptions ) override;
 

utils/kicad-ogltest/kicad-ogltest.cpp

@@ -132,6 +132,8 @@ bool OGLTEST_APP::OnInit()
         printf( "INFO: Found OpenGL version %ld.%ld\n", major, minor );
     }
 
+    KIGFX::GAL_CONTEXT_LOCKER locker( canvas );
+
     canvas->BeginDrawing();
     printf( "INFO: Successfully called OPENGL_GAL::BeginDrawing\n" );
     canvas->EndDrawing();