Implement RAII locking for GAL updating.

Fixes a crash when typing fast in the place footprint filter box.

Also adds a bunch more checking to GAL locking, including making
sure the same person unlocks as locked, and preventing piece-meal
calls (the RAII objects must be used).

common/draw_panel_gal.cpp

@@ -160,8 +160,7 @@ void EDA_DRAW_PANEL_GAL::onPaint( wxPaintEvent& WXUNUSED( aEvent ) )
         GetParentEDAFrame()->SetScrollCenterPosition( wxPoint( center.x, center.y ) );
     }
 
-    // Drawing to a zero-width or zero-height GAL is fraught with peril.
+    if( !m_gal->IsVisible() )
-    if( GetClientRect().IsEmpty() )
         return;
 
     m_pendingRefresh = false;
@@ -182,9 +181,8 @@ void EDA_DRAW_PANEL_GAL::onPaint( wxPaintEvent& WXUNUSED( aEvent ) )
     {
         m_view->UpdateItems();
 
-        KIGFX::GAL_CONTEXT_LOCKER locker( m_gal );
+        KIGFX::GAL_DRAWING_CONTEXT ctx( m_gal );
 
-        m_gal->BeginDrawing();
         m_gal->SetClearColor( settings->GetBackgroundColor() );
         m_gal->SetGridColor( settings->GetGridColor() );
         m_gal->SetCursorColor( settings->GetCursorColor() );
@@ -211,7 +209,6 @@ void EDA_DRAW_PANEL_GAL::onPaint( wxPaintEvent& WXUNUSED( aEvent ) )
         }
 
         m_gal->DrawCursor( m_viewControls->GetCursorPosition() );
-        m_gal->EndDrawing();
     }
     catch( std::runtime_error& err )
     {

common/gal/cairo/cairo_gal.cpp

@@ -126,7 +126,7 @@ bool CAIRO_GAL::updatedGalDisplayOptions( const GAL_DISPLAY_OPTIONS& aOptions )
 }
 
 
-void CAIRO_GAL::BeginDrawing()
+void CAIRO_GAL::beginDrawing()
 {
     initSurface();
 
@@ -138,7 +138,7 @@ void CAIRO_GAL::BeginDrawing()
 }
 
 
-void CAIRO_GAL::EndDrawing()
+void CAIRO_GAL::endDrawing()
 {
     // Force remaining objects to be drawn
     Flush();

common/gal/opengl/opengl_gal.cpp

@@ -168,8 +168,16 @@ OPENGL_GAL::OPENGL_GAL( GAL_DISPLAY_OPTIONS& aDisplayOptions, wxWindow* aParent,
     GAL( aDisplayOptions ),
     HIDPI_GL_CANVAS( aParent, wxID_ANY, (int*) glAttributes, wxDefaultPosition, wxDefaultSize,
                 wxEXPAND, aName ),
-    mouseListener( aMouseListener ), paintListener( aPaintListener ), currentManager( nullptr ),
+    mouseListener( aMouseListener ),
-    cachedManager( nullptr ), nonCachedManager( nullptr ), overlayManager( nullptr ), mainBuffer( 0 ), overlayBuffer( 0 )
+    paintListener( aPaintListener ),
+    currentManager( nullptr ),
+    cachedManager( nullptr ),
+    nonCachedManager( nullptr ),
+    overlayManager( nullptr ),
+    mainBuffer( 0 ),
+    overlayBuffer( 0 ),
+    isContextLocked( false ),
+    lockClientCookie( 0 )
 {
 // IsDisplayAttr() handles WX_GL_{MAJOR,MINOR}_VERSION correctly only in 3.0.4
 // starting with 3.1.0 one should use wxGLContext::IsOk() (done by GL_CONTEXT_MANAGER)
@@ -325,21 +333,22 @@ double OPENGL_GAL::getWorldPixelSize() const
     return std::min( std::abs( matrix.GetScale().x ), std::abs( matrix.GetScale().y ) );
 }
 
-void OPENGL_GAL::BeginDrawing()
+void OPENGL_GAL::beginDrawing()
 {
-    if( !IsShownOnScreen() || GetClientRect().IsEmpty() )
-        return;
-
 #ifdef __WXDEBUG__
-    PROF_COUNTER totalRealTime( "OPENGL_GAL::BeginDrawing()", true );
+    PROF_COUNTER totalRealTime( "OPENGL_GAL::beginDrawing()", true );
 #endif /* __WXDEBUG__ */
 
+    wxASSERT_MSG( isContextLocked, "GAL_DRAWING_CONTEXT RAII object should have locked context. "
+                                   "Calling GAL::beginDrawing() directly is not allowed." );
+
+    wxASSERT_MSG( IsVisible(), "GAL::beginDrawing() must not be entered when GAL is not visible. "
+                               "Other drawing routines will expect everything to be initialized "
+                               "which will not be the case." );
+
     if( !isInitialized )
         init();
 
-    wxASSERT_MSG( isContextLocked, "You must create a local GAL_CONTEXT_LOCKER instance "
-                                   "before calling GAL::BeginDrawing()." );
-
     // Set up the view port
     glMatrixMode( GL_PROJECTION );
     glLoadIdentity();
@@ -455,18 +464,17 @@ void OPENGL_GAL::BeginDrawing()
 
 #ifdef __WXDEBUG__
     totalRealTime.Stop();
-    wxLogTrace( "GAL_PROFILE",
+    wxLogTrace( "GAL_PROFILE", wxT( "OPENGL_GAL::beginDrawing(): %.1f ms" ), totalRealTime.msecs() );
-                wxT( "OPENGL_GAL::BeginDrawing(): %.1f ms" ), totalRealTime.msecs() );
 #endif /* __WXDEBUG__ */
 }
 
 
-void OPENGL_GAL::EndDrawing()
+void OPENGL_GAL::endDrawing()
 {
     wxASSERT_MSG( isContextLocked, "What happened to the context lock?" );
 
 #ifdef __WXDEBUG__
-    PROF_COUNTER totalRealTime( "OPENGL_GAL::EndDrawing()", true );
+    PROF_COUNTER totalRealTime( "OPENGL_GAL::endDrawing()", true );
 #endif /* __WXDEBUG__ */
 
     // Cached & non-cached containers are rendered to the same buffer
@@ -491,45 +499,57 @@ void OPENGL_GAL::EndDrawing()
 
 #ifdef __WXDEBUG__
     totalRealTime.Stop();
-    wxLogTrace( "GAL_PROFILE", wxT( "OPENGL_GAL::EndDrawing(): %.1f ms" ), totalRealTime.msecs() );
+    wxLogTrace( "GAL_PROFILE", wxT( "OPENGL_GAL::endDrawing(): %.1f ms" ), totalRealTime.msecs() );
 #endif /* __WXDEBUG__ */
 }
 
 
-void OPENGL_GAL::lockContext()
+void OPENGL_GAL::lockContext( int aClientCookie )
 {
-    GL_CONTEXT_MANAGER::Get().LockCtx( glPrivContext, this );
+    wxASSERT_MSG( !isContextLocked, "Context already locked." );
     isContextLocked = true;
+    lockClientCookie = aClientCookie;
+
+    GL_CONTEXT_MANAGER::Get().LockCtx( glPrivContext, this );
 }
 
 
-void OPENGL_GAL::unlockContext()
+void OPENGL_GAL::unlockContext( int aClientCookie )
 {
+    wxASSERT_MSG( isContextLocked, "Context not locked.  A GAL_CONTEXT_LOCKER RAII object must "
+                                   "be stacked rather than making separate lock/unlock calls." );
+
+    wxASSERT_MSG( lockClientCookie == aClientCookie, "Context was locked by a different client. "
+                                                     "Should not be possible with RAII objects." );
+
     isContextLocked = false;
+
     GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
 }
 
 
-void OPENGL_GAL::BeginUpdate()
+void OPENGL_GAL::beginUpdate()
 {
-    if( !IsShownOnScreen() || GetClientRect().IsEmpty() )
+    wxASSERT_MSG( isContextLocked, "GAL_UPDATE_CONTEXT RAII object should have locked context. "
-        return;
+                                   "Calling this from anywhere else is not allowed." );
+
+    wxASSERT_MSG( IsVisible(), "GAL::beginUpdate() must not be entered when GAL is not visible. "
+                               "Other update routines will expect everything to be initialized "
+                               "which will not be the case." );
 
     if( !isInitialized )
         init();
 
-    GL_CONTEXT_MANAGER::Get().LockCtx( glPrivContext, this );
     cachedManager->Map();
 }
 
 
-void OPENGL_GAL::EndUpdate()
+void OPENGL_GAL::endUpdate()
 {
     if( !isInitialized )
         return;
 
     cachedManager->Unmap();
-    GL_CONTEXT_MANAGER::Get().UnlockCtx( glPrivContext );
 }
 
 
@@ -1969,7 +1989,7 @@ void OPENGL_GAL::init()
 {
     wxASSERT( IsShownOnScreen() );
 
-    GAL_CONTEXT_LOCKER locker( this );
+    wxASSERT_MSG( isContextLocked, "This should only be called from within a locked context." );
 
     GLenum err = glewInit();
 

common/view/view.cpp

@@ -771,39 +771,44 @@ void VIEW::UpdateLayerColor( int aLayer )
 
     r.SetMaximum();
 
-    m_gal->BeginUpdate();
+    if( m_gal->IsVisible() )
-    updateItemsColor visitor( aLayer, m_painter, m_gal );
+    {
-    m_layers[aLayer].items->Query( r, visitor );
+        GAL_UPDATE_CONTEXT ctx( m_gal );
-    MarkTargetDirty( m_layers[aLayer].target );
+
-    m_gal->EndUpdate();
+        updateItemsColor visitor( aLayer, m_painter, m_gal );
+        m_layers[aLayer].items->Query( r, visitor );
+        MarkTargetDirty( m_layers[aLayer].target );
+    }
 }
 
 
 void VIEW::UpdateAllLayersColor()
 {
-    m_gal->BeginUpdate();
+    if( m_gal->IsVisible() )
-
-    for( VIEW_ITEM* item : m_allItems )
     {
-        auto viewData = item->viewPrivData();
+        GAL_UPDATE_CONTEXT ctx( m_gal );
 
-        if( !viewData )
+        for( VIEW_ITEM* item : m_allItems )
-            continue;
-
-        int layers[VIEW::VIEW_MAX_LAYERS], layers_count;
-        viewData->getLayers( layers, layers_count );
-
-        for( int i = 0; i < layers_count; ++i )
         {
-            const COLOR4D color = m_painter->GetSettings()->GetColor( item, layers[i] );
+            auto viewData = item->viewPrivData();
-            int group = viewData->getGroup( layers[i] );
 
-            if( group >= 0 )
+            if( !viewData )
-                m_gal->ChangeGroupColor( group, color );
+                continue;
+
+            int layers[VIEW::VIEW_MAX_LAYERS], layers_count;
+            viewData->getLayers( layers, layers_count );
+
+            for( int i = 0; i < layers_count; ++i )
+            {
+                const COLOR4D color = m_painter->GetSettings()->GetColor( item, layers[i] );
+                int group = viewData->getGroup( layers[i] );
+
+                if( group >= 0 )
+                    m_gal->ChangeGroupColor( group, color );
+            }
         }
     }
 
-    m_gal->EndUpdate();
     MarkDirty();
 }
 
@@ -909,28 +914,31 @@ void VIEW::ClearTopLayers()
 void VIEW::UpdateAllLayersOrder()
 {
     sortLayers();
-    m_gal->BeginUpdate();
 
-    for( VIEW_ITEM* item : m_allItems )
+    if( m_gal->IsVisible() )
     {
-        auto viewData = item->viewPrivData();
+        GAL_UPDATE_CONTEXT ctx( m_gal );
 
-        if( !viewData )
+        for( VIEW_ITEM* item : m_allItems )
-            continue;
-
-        int layers[VIEW::VIEW_MAX_LAYERS], layers_count;
-        viewData->getLayers( layers, layers_count );
-
-        for( int i = 0; i < layers_count; ++i )
         {
-            int group = viewData->getGroup( layers[i] );
+            auto viewData = item->viewPrivData();
 
-            if( group >= 0 )
+            if( !viewData )
-                m_gal->ChangeGroupDepth( group, m_layers[layers[i]].renderingOrder );
+                continue;
+
+            int layers[VIEW::VIEW_MAX_LAYERS], layers_count;
+            viewData->getLayers( layers, layers_count );
+
+            for( int i = 0; i < layers_count; ++i )
+            {
+                int group = viewData->getGroup( layers[i] );
+
+                if( group >= 0 )
+                    m_gal->ChangeGroupDepth( group, m_layers[layers[i]].renderingOrder );
+            }
         }
     }
 
-    m_gal->EndUpdate();
     MarkDirty();
 }
 
@@ -1392,23 +1400,24 @@ void VIEW::RecacheAllItems()
 
 void VIEW::UpdateItems()
 {
-    m_gal->BeginUpdate();
+    if( m_gal->IsVisible() )
-
-    for( VIEW_ITEM* item : m_allItems )
     {
-        auto viewData = item->viewPrivData();
+        GAL_UPDATE_CONTEXT ctx( m_gal );
 
-        if( !viewData )
+        for( VIEW_ITEM* item : m_allItems )
-            continue;
-
-        if( viewData->m_requiredUpdate != NONE )
         {
-            invalidateItem( item, viewData->m_requiredUpdate );
+            auto viewData = item->viewPrivData();
-            viewData->m_requiredUpdate = NONE;
+
+            if( !viewData )
+                continue;
+
+            if( viewData->m_requiredUpdate != NONE )
+            {
+                invalidateItem( item, viewData->m_requiredUpdate );
+                viewData->m_requiredUpdate = NONE;
+            }
         }
     }
-
-    m_gal->EndUpdate();
 }
 
 

include/gal/cairo/cairo_gal.h

@@ -97,12 +97,6 @@ public:
     // Drawing methods
     // ---------------
 
-    /// @copydoc GAL::BeginDrawing()
-    virtual void BeginDrawing() override;
-
-    /// @copydoc GAL::EndDrawing()
-    virtual void EndDrawing() override;
-
     /// @copydoc GAL::DrawLine()
     virtual void DrawLine( const VECTOR2D& aStartPoint, const VECTOR2D& aEndPoint ) override;
 
@@ -356,6 +350,12 @@ private:
 
     int wxBufferWidth;
 
+    /// @copydoc GAL::BeginDrawing()
+    virtual void beginDrawing() override;
+
+    /// @copydoc GAL::EndDrawing()
+    virtual void endDrawing() override;
+
     ///> Cairo-specific update handlers
     bool updatedGalDisplayOptions( const GAL_DISPLAY_OPTIONS& aOptions ) override;
 

include/gal/graphics_abstraction_layer.h

@@ -58,7 +58,11 @@ namespace KIGFX
  */
 class GAL : GAL_DISPLAY_OPTIONS_OBSERVER
 {
+    // These friend declarations allow us to hide routines that should not be called.  The
+    // corresponding RAII objects must be used instead.
     friend class GAL_CONTEXT_LOCKER;
+    friend class GAL_UPDATE_CONTEXT;
+    friend class GAL_DRAWING_CONTEXT;
 
 public:
     // Constructor / Destructor
@@ -75,18 +79,6 @@ public:
     // Drawing methods
     // ---------------
 
-    /// @brief Begin the drawing, needs to be called for every new frame.
-    virtual void BeginDrawing() {};
-
-    /// @brief End the drawing, needs to be called for every new frame.
-    virtual void EndDrawing() {};
-
-    /// @brief Enables item update mode.
-    virtual void BeginUpdate() {}
-
-    /// @brief Disables item update mode.
-    virtual void EndUpdate() {}
-
     /**
      * @brief Draw a line.
      *
@@ -1060,9 +1052,25 @@ protected:
     /// Instance of object that stores information about how to draw texts
     STROKE_FONT        strokeFont;
 
-    virtual void lockContext() {}
+    /// Private: use GAL_CONTEXT_LOCKER RAII object
+    virtual void lockContext( int aClientCookie ) {}
 
-    virtual void unlockContext() {}
+    virtual void unlockContext( int aClientCookie ) {}
+
+    /// @brief Enables item update mode.
+    /// Private: use GAL_UPDATE_CONTEXT RAII object
+    virtual void beginUpdate() {}
+
+    /// @brief Disables item update mode.
+    virtual void endUpdate() {}
+
+    /// @brief Begin the drawing, needs to be called for every new frame.
+    /// Private: use GAL_DRAWING_CONTEXT RAII object
+    virtual void beginDrawing() {};
+
+    /// @brief End the drawing, needs to be called for every new frame.
+    /// Private: use GAL_DRAWING_CONTEXT RAII object
+    virtual void endDrawing() {};
 
     /// Compute the scaling factor for the world->screen matrix
     inline void computeWorldScale()
@@ -1135,16 +1143,50 @@ public:
     GAL_CONTEXT_LOCKER( GAL* aGal ) :
         m_gal( aGal )
     {
-        m_gal->lockContext();
+        m_cookie = rand();
+        m_gal->lockContext( m_cookie );
     }
 
     ~GAL_CONTEXT_LOCKER()
     {
-        m_gal->unlockContext();
+        m_gal->unlockContext( m_cookie );
     }
 
-private:
+protected:
     GAL* m_gal;
+    int  m_cookie;
+};
+
+
+class GAL_UPDATE_CONTEXT : public GAL_CONTEXT_LOCKER
+{
+public:
+    GAL_UPDATE_CONTEXT( GAL* aGal ) :
+            GAL_CONTEXT_LOCKER( aGal )
+    {
+        m_gal->beginUpdate();
+    }
+
+    ~GAL_UPDATE_CONTEXT()
+    {
+        m_gal->endUpdate();
+    }
+};
+
+
+class GAL_DRAWING_CONTEXT : public GAL_CONTEXT_LOCKER
+{
+public:
+    GAL_DRAWING_CONTEXT( GAL* aGal ) :
+            GAL_CONTEXT_LOCKER( aGal )
+    {
+        m_gal->beginDrawing();
+    }
+
+    ~GAL_DRAWING_CONTEXT()
+    {
+        m_gal->endDrawing();
+    }
 };
 
 

include/gal/opengl/opengl_gal.h

@@ -103,18 +103,6 @@ public:
     // Drawing methods
     // ---------------
 
-    /// @copydoc GAL::BeginDrawing()
-    virtual void BeginDrawing() override;
-
-    /// @copydoc GAL::EndDrawing()
-    virtual void EndDrawing() override;
-
-    /// @copydoc GAL::BeginUpdate()
-    virtual void BeginUpdate() override;
-
-    /// @copydoc GAL::EndUpdate()
-    virtual void EndUpdate() override;
-
     /// @copydoc GAL::DrawLine()
     virtual void DrawLine( const VECTOR2D& aStartPoint, const VECTOR2D& aEndPoint ) override;
 
@@ -329,13 +317,26 @@ private:
                                                         ///< when the window is visible
     bool                    isGrouping;                 ///< Was a group started?
     bool                    isContextLocked;            ///< Used for assertion checking
+    int                     lockClientCookie;
     GLint                   ufm_worldPixelSize;
 
     std::unique_ptr<GL_BITMAP_CACHE>         bitmapCache;
 
-    void lockContext() override;
+    void lockContext( int aClientCookie ) override;
 
-    void unlockContext() override;
+    void unlockContext( int aClientCookie ) override;
+
+    /// @copydoc GAL::BeginUpdate()
+    virtual void beginUpdate() override;
+
+    /// @copydoc GAL::EndUpdate()
+    virtual void endUpdate() override;
+
+    /// @copydoc GAL::BeginDrawing()
+    virtual void beginDrawing() override;
+
+    /// @copydoc GAL::EndDrawing()
+    virtual void endDrawing() override;
 
     ///< Update handler for OpenGL settings
     bool updatedGalDisplayOptions( const GAL_DISPLAY_OPTIONS& aOptions ) override;

utils/kicad-ogltest/kicad-ogltest.cpp

@@ -132,11 +132,10 @@ bool OGLTEST_APP::OnInit()
         printf( "INFO: Found OpenGL version %ld.%ld\n", major, minor );
     }
 
-    KIGFX::GAL_CONTEXT_LOCKER locker( canvas );
+    {
-
+        KIGFX::GAL_DRAWING_CONTEXT ctx( canvas );
-    canvas->BeginDrawing();
+        printf( "INFO: Successfully called OPENGL_GAL::beginDrawing\n" );
-    printf( "INFO: Successfully called OPENGL_GAL::BeginDrawing\n" );
+    }
-    canvas->EndDrawing();
 
     bool supported = wxGLCanvas::IsDisplaySupported( &glAttributes[0] );