From 16925cc74efb2ef6c82cb6c5e9faebea1522dc58 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Fri, 26 Oct 2018 22:25:09 +0100 Subject: [PATCH] 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 | 7 +- common/gal/cairo/cairo_gal.cpp | 4 +- common/gal/opengl/opengl_gal.cpp | 70 +++++++++------ common/view/view.cpp | 103 ++++++++++++----------- include/gal/cairo/cairo_gal.h | 12 +-- include/gal/graphics_abstraction_layer.h | 76 +++++++++++++---- include/gal/opengl/opengl_gal.h | 29 ++++--- utils/kicad-ogltest/kicad-ogltest.cpp | 9 +- 8 files changed, 189 insertions(+), 121 deletions(-) diff --git a/common/draw_panel_gal.cpp b/common/draw_panel_gal.cpp index 4295ce4bc4..4a95a46bfb 100644 --- a/common/draw_panel_gal.cpp +++ b/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( GetClientRect().IsEmpty() ) + if( !m_gal->IsVisible() ) 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 ) { diff --git a/common/gal/cairo/cairo_gal.cpp b/common/gal/cairo/cairo_gal.cpp index 06b0acbd71..e929ed3957 100644 --- a/common/gal/cairo/cairo_gal.cpp +++ b/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(); diff --git a/common/gal/opengl/opengl_gal.cpp b/common/gal/opengl/opengl_gal.cpp index 8a93a6e6f7..d5059e7699 100644 --- a/common/gal/opengl/opengl_gal.cpp +++ b/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 ), - cachedManager( nullptr ), nonCachedManager( nullptr ), overlayManager( nullptr ), mainBuffer( 0 ), overlayBuffer( 0 ) + mouseListener( aMouseListener ), + 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", - wxT( "OPENGL_GAL::BeginDrawing(): %.1f ms" ), totalRealTime.msecs() ); + wxLogTrace( "GAL_PROFILE", 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() ) - return; + wxASSERT_MSG( isContextLocked, "GAL_UPDATE_CONTEXT RAII object should have locked context. " + "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(); diff --git a/common/view/view.cpp b/common/view/view.cpp index 364da756a6..fdad8435ff 100644 --- a/common/view/view.cpp +++ b/common/view/view.cpp @@ -771,39 +771,44 @@ void VIEW::UpdateLayerColor( int aLayer ) r.SetMaximum(); - m_gal->BeginUpdate(); - updateItemsColor visitor( aLayer, m_painter, m_gal ); - m_layers[aLayer].items->Query( r, visitor ); - MarkTargetDirty( m_layers[aLayer].target ); - m_gal->EndUpdate(); + if( m_gal->IsVisible() ) + { + GAL_UPDATE_CONTEXT ctx( m_gal ); + + 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(); - - for( VIEW_ITEM* item : m_allItems ) + if( m_gal->IsVisible() ) { - auto viewData = item->viewPrivData(); + GAL_UPDATE_CONTEXT ctx( m_gal ); - if( !viewData ) - continue; - - int layers[VIEW::VIEW_MAX_LAYERS], layers_count; - viewData->getLayers( layers, layers_count ); - - for( int i = 0; i < layers_count; ++i ) + for( VIEW_ITEM* item : m_allItems ) { - const COLOR4D color = m_painter->GetSettings()->GetColor( item, layers[i] ); - int group = viewData->getGroup( layers[i] ); + auto viewData = item->viewPrivData(); - if( group >= 0 ) - m_gal->ChangeGroupColor( group, color ); + if( !viewData ) + 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 ) - continue; - - int layers[VIEW::VIEW_MAX_LAYERS], layers_count; - viewData->getLayers( layers, layers_count ); - - for( int i = 0; i < layers_count; ++i ) + for( VIEW_ITEM* item : m_allItems ) { - int group = viewData->getGroup( layers[i] ); + auto viewData = item->viewPrivData(); - if( group >= 0 ) - m_gal->ChangeGroupDepth( group, m_layers[layers[i]].renderingOrder ); + if( !viewData ) + 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(); - - for( VIEW_ITEM* item : m_allItems ) + if( m_gal->IsVisible() ) { - auto viewData = item->viewPrivData(); + GAL_UPDATE_CONTEXT ctx( m_gal ); - if( !viewData ) - continue; - - if( viewData->m_requiredUpdate != NONE ) + for( VIEW_ITEM* item : m_allItems ) { - invalidateItem( item, viewData->m_requiredUpdate ); - viewData->m_requiredUpdate = NONE; + auto viewData = item->viewPrivData(); + + if( !viewData ) + continue; + + if( viewData->m_requiredUpdate != NONE ) + { + invalidateItem( item, viewData->m_requiredUpdate ); + viewData->m_requiredUpdate = NONE; + } } } - - m_gal->EndUpdate(); } diff --git a/include/gal/cairo/cairo_gal.h b/include/gal/cairo/cairo_gal.h index 7d7cd43878..5e74865298 100644 --- a/include/gal/cairo/cairo_gal.h +++ b/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; diff --git a/include/gal/graphics_abstraction_layer.h b/include/gal/graphics_abstraction_layer.h index a22e308e4b..ddecf6b470 100644 --- a/include/gal/graphics_abstraction_layer.h +++ b/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(); + } }; diff --git a/include/gal/opengl/opengl_gal.h b/include/gal/opengl/opengl_gal.h index e6cb3ca2f1..1aaa0bf6f9 100644 --- a/include/gal/opengl/opengl_gal.h +++ b/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 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; diff --git a/utils/kicad-ogltest/kicad-ogltest.cpp b/utils/kicad-ogltest/kicad-ogltest.cpp index 1b4ea4d8d6..31157b900b 100644 --- a/utils/kicad-ogltest/kicad-ogltest.cpp +++ b/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 ); - - canvas->BeginDrawing(); - printf( "INFO: Successfully called OPENGL_GAL::BeginDrawing\n" ); - canvas->EndDrawing(); + { + KIGFX::GAL_DRAWING_CONTEXT ctx( canvas ); + printf( "INFO: Successfully called OPENGL_GAL::beginDrawing\n" ); + } bool supported = wxGLCanvas::IsDisplaySupported( &glAttributes[0] );