From ac3ed022835c00313bd0ed044268fe728a1d50d4 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Mon, 2 Jan 2023 22:14:22 -0500 Subject: [PATCH] Rework bitmap cache and enable it Fixes https://gitlab.com/kicad/code/kicad/-/issues/12405 --- common/bitmap_base.cpp | 46 +++++++++-- common/dialogs/panel_image_editor.cpp | 4 +- common/gal/opengl/opengl_gal.cpp | 81 +++++++++++++------ .../sch_plugins/kicad/sch_sexpr_parser.cpp | 1 - .../sch_plugins/legacy/sch_legacy_plugin.cpp | 1 - include/bitmap_base.h | 30 +++---- pcbnew/plugins/kicad/pcb_parser.cpp | 1 - 7 files changed, 107 insertions(+), 57 deletions(-) diff --git a/common/bitmap_base.cpp b/common/bitmap_base.cpp index 8731c34cd5..6eb5c5cf6e 100644 --- a/common/bitmap_base.cpp +++ b/common/bitmap_base.cpp @@ -54,16 +54,36 @@ BITMAP_BASE::BITMAP_BASE( const BITMAP_BASE& aSchBitmap ) if( aSchBitmap.m_image ) { - m_image = new wxImage( *aSchBitmap.m_image ); - m_bitmap = new wxBitmap( *m_image ); + m_image = new wxImage( *aSchBitmap.m_image ); + m_bitmap = new wxBitmap( *m_image ); + m_imageId = aSchBitmap.m_imageId; } } +void BITMAP_BASE::SetImage( wxImage* aImage ) +{ + delete m_image; + m_image = aImage; + rebuildBitmap(); +} + + +void BITMAP_BASE::rebuildBitmap() +{ + if( m_bitmap ) + delete m_bitmap; + + m_bitmap = new wxBitmap( *m_image ); + m_imageId = KIID(); +} + + void BITMAP_BASE::ImportData( BITMAP_BASE* aItem ) { *m_image = *aItem->m_image; *m_bitmap = *aItem->m_bitmap; + m_imageId = aItem->m_imageId; m_scale = aItem->m_scale; m_ppi = aItem->m_ppi; m_pixelSizeIu = aItem->m_pixelSizeIu; @@ -79,7 +99,7 @@ bool BITMAP_BASE::ReadImageFile( wxInputStream& aInStream ) delete m_image; m_image = new_image.release(); - m_bitmap = new wxBitmap( *m_image ); + rebuildBitmap(); return true; } @@ -97,7 +117,7 @@ bool BITMAP_BASE::ReadImageFile( const wxString& aFullFilename ) delete m_image; m_image = new_image; - m_bitmap = new wxBitmap( *m_image ); + rebuildBitmap(); return true; } @@ -331,8 +351,8 @@ void BITMAP_BASE::Mirror( bool aVertically ) { if( m_image ) { - *m_image = m_image->Mirror( not aVertically ); - RebuildBitmap(); + *m_image = m_image->Mirror( not aVertically ); + rebuildBitmap(); } } @@ -341,8 +361,18 @@ void BITMAP_BASE::Rotate( bool aRotateCCW ) { if( m_image ) { - *m_image = m_image->Rotate90( aRotateCCW ); - RebuildBitmap(); + *m_image = m_image->Rotate90( aRotateCCW ); + rebuildBitmap(); + } +} + + +void BITMAP_BASE::ConvertToGreyscale() +{ + if( m_image ) + { + *m_image = m_image->ConvertToGreyscale(); + rebuildBitmap(); } } diff --git a/common/dialogs/panel_image_editor.cpp b/common/dialogs/panel_image_editor.cpp index 15f90d25b7..472dc58600 100644 --- a/common/dialogs/panel_image_editor.cpp +++ b/common/dialogs/panel_image_editor.cpp @@ -47,9 +47,7 @@ PANEL_IMAGE_EDITOR::PANEL_IMAGE_EDITOR( wxWindow* aParent, BITMAP_BASE* aItem ) void PANEL_IMAGE_EDITOR::OnGreyScaleConvert( wxCommandEvent& event ) { - wxImage& image = *m_workingImage->GetImageData(); - image = image.ConvertToGreyscale(); - m_workingImage->RebuildBitmap(); + m_workingImage->ConvertToGreyscale(); m_panelDraw->Refresh(); } diff --git a/common/gal/opengl/opengl_gal.cpp b/common/gal/opengl/opengl_gal.cpp index 3ef8ea4aec..4d1e38030f 100644 --- a/common/gal/opengl/opengl_gal.cpp +++ b/common/gal/opengl/opengl_gal.cpp @@ -56,22 +56,11 @@ #include #include #include +#include using namespace std::placeholders; using namespace KIGFX; -// Currently the bitmap cache is disabled because the code has serious issues and -// work fine does not work fine: -// created GL textures are never deleted when a bitmap is deleted -// the key to retrieve a CACHED_BITMAP is the BITMAP_BASE items* pointer. -// Because in code many BITMAP_BASE items are temporary cloned for drawing purposes, -// it creates a lot of CACHED_BITMAP never deleted thus created memory leak -// So to reenable the bitmaps cache, serious changes in code are needed: -// At least: -// - use a key that works on cloned BITMAP_BASE items -// - handle rotated bitmaps without create a new cached item -// - add a "garbage collector" to delete not existing BITMAP_BASE items -// After tests, caching bitmaps do not speedup significantly drawings. -#define DISABLE_BITMAP_CACHE +//#define DISABLE_BITMAP_CACHE // The current font is "Ubuntu Mono" available under Ubuntu Font Licence 1.0 // (see ubuntu-font-licence-1.0.txt for details) @@ -93,7 +82,9 @@ namespace KIGFX class GL_BITMAP_CACHE { public: - GL_BITMAP_CACHE() {} + GL_BITMAP_CACHE() : + m_cacheSize( 0 ) + {} ~GL_BITMAP_CACHE(); @@ -104,11 +95,18 @@ private: { GLuint id; int w, h; + size_t size; }; GLuint cacheBitmap( const BITMAP_BASE* aBitmap ); - std::map< const BITMAP_BASE*, CACHED_BITMAP> m_bitmaps; + const size_t m_cacheMaxElements = 50; + const size_t m_cacheMaxSize = 256 * 1024 * 1024; + + std::map m_bitmaps; + std::list m_cacheLru; + size_t m_cacheSize; + std::list m_freedTextureIds; }; }; // namespace KIGFX @@ -124,17 +122,12 @@ GL_BITMAP_CACHE::~GL_BITMAP_CACHE() GLuint GL_BITMAP_CACHE::RequestBitmap( const BITMAP_BASE* aBitmap ) { #ifndef DISABLE_BITMAP_CACHE - auto it = m_bitmaps.find( aBitmap ); + auto it = m_bitmaps.find( aBitmap->GetImageID() ); if( it != m_bitmaps.end() ) { - // A bitmap is found in cache bitmap. Ensure the associated texture - // is still valid. - // It can be destroyed somewhere or the corresponding bitmap can be - // modifed (rotated) - if( ( it->second.w == aBitmap->GetSizePixels().x ) && - ( it->second.h == aBitmap->GetSizePixels().y ) && - glIsTexture( it->second.id ) ) + // A bitmap is found in cache bitmap. Ensure the associated texture is still valid. + if( glIsTexture( it->second.id ) ) { return it->second.id; } @@ -142,6 +135,15 @@ GLuint GL_BITMAP_CACHE::RequestBitmap( const BITMAP_BASE* aBitmap ) { // Delete the invalid bitmap cache and its data glDeleteTextures( 1, &it->second.id ); + m_freedTextureIds.emplace_back( it->second.id ); + + auto listIt = std::find( m_cacheLru.begin(), m_cacheLru.end(), it->first ); + + if( listIt != m_cacheLru.end() ) + m_cacheLru.erase( listIt ); + + m_cacheSize -= it->second.size; + m_bitmaps.erase( it ); } @@ -169,10 +171,21 @@ GLuint GL_BITMAP_CACHE::cacheBitmap( const BITMAP_BASE* aBitmap ) extra_w = 4 - extra_w; GLuint textureID; - glGenTextures( 1, &textureID ); + + if( m_freedTextureIds.empty() ) + { + glGenTextures( 1, &textureID ); + } + else + { + textureID = m_freedTextureIds.front(); + m_freedTextureIds.pop_front(); + } // make_unique initializes this to 0, so extra pixels are transparent - auto buf = std::make_unique( ( bmp.w + extra_w ) * bmp.h * 4 ); + bmp.size = ( bmp.w + extra_w ) * bmp.h * 4; + auto buf = std::make_unique( bmp.size ); + const wxImage& imgData = *aBitmap->GetImageData(); for( int y = 0; y < bmp.h; y++ ) @@ -205,7 +218,23 @@ GLuint GL_BITMAP_CACHE::cacheBitmap( const BITMAP_BASE* aBitmap ) bmp.id = textureID; #ifndef DISABLE_BITMAP_CACHE - m_bitmaps[aBitmap] = bmp; + m_cacheLru.emplace_back( aBitmap->GetImageID() ); + m_cacheSize += bmp.size; + m_bitmaps.emplace( aBitmap->GetImageID(), std::move( bmp ) ); + + if( m_cacheLru.size() > m_cacheMaxElements + || m_cacheSize > m_cacheMaxSize ) + { + KIID last = m_cacheLru.front(); + CACHED_BITMAP& cachedBitmap = m_bitmaps[last]; + + m_cacheSize -= cachedBitmap.size; + glDeleteTextures( 1, &cachedBitmap.id ); + m_freedTextureIds.emplace_back( cachedBitmap.id ); + + m_bitmaps.erase( last ); + m_cacheLru.pop_front(); + } #endif return textureID; diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp index 49dc7c44b3..24f2053806 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp @@ -2968,7 +2968,6 @@ SCH_BITMAP* SCH_SEXPR_PARSER::parseImage() wxMemoryInputStream istream( stream ); image->LoadFile( istream, wxBITMAP_TYPE_PNG ); bitmap->GetImage()->SetImage( image ); - bitmap->GetImage()->SetBitmap( new wxBitmap( *image ) ); break; } diff --git a/eeschema/sch_plugins/legacy/sch_legacy_plugin.cpp b/eeschema/sch_plugins/legacy/sch_legacy_plugin.cpp index b1b12345a0..fcd97ef670 100644 --- a/eeschema/sch_plugins/legacy/sch_legacy_plugin.cpp +++ b/eeschema/sch_plugins/legacy/sch_legacy_plugin.cpp @@ -692,7 +692,6 @@ SCH_BITMAP* SCH_LEGACY_PLUGIN::loadBitmap( LINE_READER& aReader ) wxMemoryInputStream istream( stream ); image->LoadFile( istream, wxBITMAP_TYPE_PNG ); bitmap->GetImage()->SetImage( image ); - bitmap->GetImage()->SetBitmap( new wxBitmap( *image ) ); break; } diff --git a/include/bitmap_base.h b/include/bitmap_base.h index 16a0d480a0..fc68898c28 100644 --- a/include/bitmap_base.h +++ b/include/bitmap_base.h @@ -27,6 +27,7 @@ #include #include +#include #include namespace KIGFX @@ -69,27 +70,12 @@ public: wxImage* GetImageData() { return m_image; } const wxImage* GetImageData() const { return m_image; } - void SetImage( wxImage* aImage ) - { - delete m_image; - m_image = aImage; - } + void SetImage( wxImage* aImage ); double GetScale() const { return m_scale; } void SetScale( double aScale ) { m_scale = aScale; } - /* - * Rebuild the internal bitmap used to draw/plot image. - * - * This must be called after a #m_image change. - */ - void RebuildBitmap() { *m_bitmap = wxBitmap( *m_image ); } - - void SetBitmap( wxBitmap* aBitMap ) - { - delete m_bitmap; - m_bitmap = aBitMap; - } + KIID GetImageID() const { return m_imageId; } /** * Copy aItem image to this object and update #m_bitmap. @@ -219,6 +205,8 @@ public: */ void Rotate( bool aRotateCCW ); + void ConvertToGreyscale(); + /** * Plot bitmap on plotter. * @@ -233,6 +221,13 @@ public: const KIGFX::COLOR4D& aDefaultColor, int aDefaultPensize ) const; private: + /* + * Rebuild the internal bitmap used to draw/plot image. + * + * This must be called after a #m_image change. + */ + void rebuildBitmap(); + double m_scale; // The scaling factor of the bitmap // With m_pixelSizeIu, controls the actual draw size wxImage* m_image; // the raw image data (png format) @@ -242,6 +237,7 @@ private: // to internal KiCad units // Usually does not change int m_ppi; // the bitmap definition. the default is 300PPI + KIID m_imageId; }; diff --git a/pcbnew/plugins/kicad/pcb_parser.cpp b/pcbnew/plugins/kicad/pcb_parser.cpp index fb162bf1ba..13826cb722 100644 --- a/pcbnew/plugins/kicad/pcb_parser.cpp +++ b/pcbnew/plugins/kicad/pcb_parser.cpp @@ -2903,7 +2903,6 @@ PCB_BITMAP* PCB_PARSER::parsePCB_BITMAP( BOARD_ITEM* aParent ) wxMemoryInputStream istream( stream ); image->LoadFile( istream, wxBITMAP_TYPE_PNG ); bitmap->GetImage()->SetImage( image ); - bitmap->GetImage()->SetBitmap( new wxBitmap( *image ) ); break; }