From 6c34fdefd7d44ddc505ea3a79a08845a5b8d3d47 Mon Sep 17 00:00:00 2001
From: Jeff Young <jeff@rokeby.ie>
Date: Fri, 12 Oct 2018 23:42:44 +0100
Subject: [PATCH] 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 | 2 +
common/gal/opengl/opengl_gal.cpp | 101 +++++++++++------------
common/gal/opengl/utils.cpp | 39 ++++++++-
common/kiway.cpp | 12 +--
include/gal/graphics_abstraction_layer.h | 31 ++++++-
include/gal/opengl/opengl_gal.h | 5 ++
utils/kicad-ogltest/kicad-ogltest.cpp | 2 +
7 files changed, 127 insertions(+), 65 deletions(-)
diff --git a/common/draw_panel_gal.cpp b/common/draw_panel_gal.cpp
index 941effbc30..a5b6f3bad2 100644
--- a/common/draw_panel_gal.cpp
+++ b/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() );
diff --git a/common/gal/opengl/opengl_gal.cpp b/common/gal/opengl/opengl_gal.cpp
index 83c0ac820e..f0ed31b2c0 100644
--- a/common/gal/opengl/opengl_gal.cpp
+++ b/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();
- mainBuffer = compositor->CreateBuffer();
- overlayBuffer = compositor->CreateBuffer();
- }
- catch( std::runtime_error& )
- {
- GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
- throw; // DRAW_PANEL_GAL will handle it
- }
+ // Prepare rendering target buffers
+ compositor->Initialize();
+ mainBuffer = compositor->CreateBuffer();
+ overlayBuffer = compositor->CreateBuffer();
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)
- if( !GLEW_VERSION_2_1 )
- throw std::runtime_error( "OpenGL 2.1 or higher is required!" );
+ // Check the OpenGL version (minimum 2.1 is required)
+ if( !GLEW_VERSION_2_1 )
+ 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 )
- enableGlDebug( true );
+ if( GLEW_ARB_debug_output )
+ enableGlDebug( true );
#endif
#endif
- // Framebuffers have to be supported
- if( !GLEW_EXT_framebuffer_object )
- throw std::runtime_error( "Framebuffer objects are not supported!" );
+ // Framebuffers have to be supported
+ if( !GLEW_EXT_framebuffer_object )
+ throw std::runtime_error( "Framebuffer objects are not supported!" );
- // Vertex buffer has to be supported
- if( !GLEW_ARB_vertex_buffer_object )
- throw std::runtime_error( "Vertex buffer objects are not supported!" );
+ // Vertex buffer has to be supported
+ if( !GLEW_ARB_vertex_buffer_object )
+ throw std::runtime_error( "Vertex buffer objects are not supported!" );
- // Prepare shaders
- if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_VERTEX, BUILTIN_SHADERS::kicad_vertex_shader ) )
- throw std::runtime_error( "Cannot compile vertex shader!" );
+ // Prepare shaders
+ if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_VERTEX, BUILTIN_SHADERS::kicad_vertex_shader ) )
+ throw std::runtime_error( "Cannot compile vertex shader!" );
- if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_FRAGMENT, BUILTIN_SHADERS::kicad_fragment_shader ) )
- throw std::runtime_error( "Cannot compile fragment shader!" );
+ if( !shader->IsLinked() && !shader->LoadShaderFromStrings( SHADER_TYPE_FRAGMENT, BUILTIN_SHADERS::kicad_fragment_shader ) )
+ throw std::runtime_error( "Cannot compile fragment shader!" );
- if( !shader->IsLinked() && !shader->Link() )
- throw std::runtime_error( "Cannot link the shaders!" );
+ if( !shader->IsLinked() && !shader->Link() )
+ throw std::runtime_error( "Cannot link the shaders!" );
- // Check if video card supports textures big enough to fit the font atlas
- int maxTextureSize;
- glGetIntegerv( GL_MAX_TEXTURE_SIZE, &maxTextureSize );
+ // Check if video card supports textures big enough to fit the font atlas
+ int maxTextureSize;
+ glGetIntegerv( GL_MAX_TEXTURE_SIZE, &maxTextureSize );
- 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& )
+ if( maxTextureSize < (int) font_image.width || maxTextureSize < (int)font_image.height )
{
- GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
- throw;
+ // TODO implement software texture scaling
+ // 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;
}
diff --git a/common/gal/opengl/utils.cpp b/common/gal/opengl/utils.cpp
index 64faeec788..5b06ed82dc 100644
--- a/common/gal/opengl/utils.cpp
+++ b/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:
diff --git a/common/kiway.cpp b/common/kiway.cpp
index 39932339b0..08fe54b351 100644
--- a/common/kiway.cpp
+++ b/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() )
- frame->CommonSettingsChanged();
- }
-
- for( unsigned i=0; i < KIWAY_PLAYER_COUNT; ++i )
- {
- KIWAY_PLAYER* frame = GetPlayerFrame( ( FRAME_T )i );
-
- if( frame && !frame->IsActive() )
+ if( frame )
frame->CommonSettingsChanged();
}
}
diff --git a/include/gal/graphics_abstraction_layer.h b/include/gal/graphics_abstraction_layer.h
index bdffc3d2e6..a22e308e4b 100644
--- a/include/gal/graphics_abstraction_layer.h
+++ b/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_ */
diff --git a/include/gal/opengl/opengl_gal.h b/include/gal/opengl/opengl_gal.h
index 72a82d48d3..a87973cfc8 100644
--- a/include/gal/opengl/opengl_gal.h
+++ b/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;
diff --git a/utils/kicad-ogltest/kicad-ogltest.cpp b/utils/kicad-ogltest/kicad-ogltest.cpp
index 64bdf1d982..1b4ea4d8d6 100644
--- a/utils/kicad-ogltest/kicad-ogltest.cpp
+++ b/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();