Simplified the glyph reference counting

This fixes a bug found when opening/closing the text-selection popup.
The reference counts are not incremented correctly e.g. when a word contains
the same letter twice. This results in the atlas becoming corrupted.

+ More Debug macros for glyph-atlas

Change-Id: Ib91681bc76e037ec4f6765bdc6ecbc9f592c20e3

diff --git a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.cpp b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.cpp
index 1053ea9..fe8da41 100644 (file)
--- a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.cpp
+++ b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.cpp
@@ -20,11 +20,17 @@
 // EXTERNAL INCLUDES
 #include <dali/public-api/actors/image-actor.h>
 #include <dali/public-api/common/stage.h>
+#include <dali/integration-api/debug.h>
 
 #define MAKE_SHADER(A)#A
 
 namespace
 {
+
+#if defined(DEBUG_ENABLED)
+  Debug::Filter* gLogFilter = Debug::Filter::New(Debug::Concise, true, "LOG_TEXT_RENDERING");
+#endif
+
 const char* VERTEX_SHADER = MAKE_SHADER(
 attribute mediump vec2    aPosition;
 attribute mediump vec2    aTexCoord;
@@ -77,7 +83,8 @@ void main()
   gl_FragColor = vec4(uColor.rgb, uColor.a*color.r);
 }
 );
-}
+
+} // unnamed namespace
 
 namespace Dali
 {
@@ -95,32 +102,12 @@ AtlasGlyphManager::AtlasGlyphManager()
   mShadowShader = Shader::New( VERTEX_SHADER_SHADOW, FRAGMENT_SHADER_SHADOW, Dali::Shader::HINT_MODIFIES_GEOMETRY );
 }
 
-AtlasGlyphManager::~AtlasGlyphManager()
-{
-  // Clear up any remaining references
-  for ( std::vector< FontGlyphRecord >::iterator fontGlyphRecordIt = mFontGlyphRecords.begin();
-        fontGlyphRecordIt != mFontGlyphRecords.end();
-        ++fontGlyphRecordIt )
-  {
-    for ( Vector< GlyphRecordEntry >::Iterator glyphRecordEntryIt = fontGlyphRecordIt->mGlyphRecords.Begin();
-          glyphRecordEntryIt != fontGlyphRecordIt->mGlyphRecords.End();
-          ++glyphRecordEntryIt )
-    {
-      mAtlasManager.Remove( glyphRecordEntryIt->mImageId );
-    }
-  }
-}
-
-AtlasGlyphManagerPtr AtlasGlyphManager::New()
-{
-  AtlasGlyphManagerPtr internal = new AtlasGlyphManager();
-  return internal;
-}
-
 void AtlasGlyphManager::Add( const Text::GlyphInfo& glyph,
                              const BufferImage& bitmap,
                              Dali::Toolkit::AtlasManager::AtlasSlot& slot )
 {
+  DALI_LOG_INFO( gLogFilter, Debug::General, "Added glyph, font: %d index: %d\n", glyph.fontId, glyph.index );
+
   mAtlasManager.Add( bitmap, slot );
 
   GlyphRecordEntry record;
@@ -166,7 +153,7 @@ void AtlasGlyphManager::StitchMesh( Toolkit::AtlasManager::Mesh2D& first,
 }
 
 bool AtlasGlyphManager::Cached( Text::FontId fontId,
-                                uint32_t index,
+                                Text::GlyphIndex index,
                                 Dali::Toolkit::AtlasManager::AtlasSlot& slot )
 {
   for ( std::vector< FontGlyphRecord >::iterator fontGlyphRecordIt = mFontGlyphRecords.begin();
@@ -215,41 +202,65 @@ Pixel::Format AtlasGlyphManager::GetPixelFormat( uint32_t atlasId )
 
 const Toolkit::AtlasGlyphManager::Metrics& AtlasGlyphManager::GetMetrics()
 {
+  std::ostringstream verboseMetrics;
+
   mMetrics.mGlyphCount = 0u;
   for ( std::vector< FontGlyphRecord >::iterator fontGlyphRecordIt = mFontGlyphRecords.begin();
         fontGlyphRecordIt != mFontGlyphRecords.end();
         ++fontGlyphRecordIt )
   {
     mMetrics.mGlyphCount += fontGlyphRecordIt->mGlyphRecords.Size();
+
+    verboseMetrics << "[FontId " << fontGlyphRecordIt->mFontId << " Glyph ";
+    for ( Vector< GlyphRecordEntry >::Iterator glyphRecordEntryIt = fontGlyphRecordIt->mGlyphRecords.Begin();
+          glyphRecordEntryIt != fontGlyphRecordIt->mGlyphRecords.End();
+          ++glyphRecordEntryIt )
+    {
+      verboseMetrics << glyphRecordEntryIt->mIndex << "(" << glyphRecordEntryIt->mCount << ") ";
+    }
+    verboseMetrics << "] ";
   }
+  mMetrics.mVerboseGlyphCounts = verboseMetrics.str();
+
   mAtlasManager.GetMetrics( mMetrics.mAtlasMetrics );
+
   return mMetrics;
 }
 
-void AtlasGlyphManager::AdjustReferenceCount( Text::FontId fontId, uint32_t imageId, int32_t delta )
+void AtlasGlyphManager::AdjustReferenceCount( Text::FontId fontId, Text::GlyphIndex index, int32_t delta )
 {
-  for ( std::vector< FontGlyphRecord >::iterator fontGlyphRecordIt = mFontGlyphRecords.begin();
-        fontGlyphRecordIt != mFontGlyphRecords.end();
-        ++fontGlyphRecordIt )
+  if( 0 != delta )
   {
-    if ( fontGlyphRecordIt->mFontId == fontId )
+    DALI_LOG_INFO( gLogFilter, Debug::General, "AdjustReferenceCount %d, font: %d index: %d\n", delta, fontId, index );
+
+    for ( std::vector< FontGlyphRecord >::iterator fontGlyphRecordIt = mFontGlyphRecords.begin();
+          fontGlyphRecordIt != mFontGlyphRecords.end();
+          ++fontGlyphRecordIt )
     {
-      for ( Vector< GlyphRecordEntry >::Iterator glyphRecordIt = fontGlyphRecordIt->mGlyphRecords.Begin();
-            glyphRecordIt != fontGlyphRecordIt->mGlyphRecords.End();
-            ++glyphRecordIt )
+      if ( fontGlyphRecordIt->mFontId == fontId )
       {
-        if ( glyphRecordIt->mImageId == imageId )
+        for ( Vector< GlyphRecordEntry >::Iterator glyphRecordIt = fontGlyphRecordIt->mGlyphRecords.Begin();
+              glyphRecordIt != fontGlyphRecordIt->mGlyphRecords.End();
+              ++glyphRecordIt )
         {
-          glyphRecordIt->mCount += delta;
-          if ( !glyphRecordIt->mCount )
+          if ( glyphRecordIt->mIndex == index )
           {
-            mAtlasManager.Remove( glyphRecordIt->mImageId );
-            fontGlyphRecordIt->mGlyphRecords.Remove( glyphRecordIt );
+            glyphRecordIt->mCount += delta;
+            DALI_ASSERT_DEBUG( glyphRecordIt->mCount >= 0 && "Glyph ref-count should not be negative" );
+
+            if ( !glyphRecordIt->mCount )
+            {
+              mAtlasManager.Remove( glyphRecordIt->mImageId );
+              fontGlyphRecordIt->mGlyphRecords.Remove( glyphRecordIt );
+            }
+            return;
           }
-          return;
         }
       }
     }
+
+    // Should not arrive here
+    DALI_ASSERT_DEBUG( false && "Failed to adjust ref-count" );
   }
 }
 
@@ -263,6 +274,11 @@ Sampler AtlasGlyphManager::GetSampler( uint32_t atlasId ) const
   return mAtlasManager.GetSampler( atlasId );
 }
 
+AtlasGlyphManager::~AtlasGlyphManager()
+{
+  // mAtlasManager handle is automatically released here
+}
+
 } // namespace Internal
 
 } // namespace Toolkit

diff --git a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.h b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.h
index 5a90d17..ce937a8 100644 (file)
--- a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.h
+++ b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager-impl.h
@@ -62,14 +62,10 @@ public:
     Vector< GlyphRecordEntry > mGlyphRecords;
   };
 
-  AtlasGlyphManager();
-
-  virtual ~AtlasGlyphManager();
-
-/**
-   * Create a new AtlasGlyphManager
+  /**
+   * @brief Constructor
    */
-  static AtlasGlyphManagerPtr New();
+  AtlasGlyphManager();
 
   /**
    * @copydoc Toolkit::AtlasGlyphManager::Add
@@ -116,7 +112,7 @@ public:
   /**
    * @copydoc toolkit::AtlasGlyphManager::AdjustReferenceCount
    */
-  void AdjustReferenceCount( Text::FontId fontId, uint32_t imageId, int32_t delta );
+  void AdjustReferenceCount( Text::FontId fontId, Text::GlyphIndex index, int32_t delta );
 
   /**
    * @copydoc Toolkit::AtlasGlyphManager::GetMaterial
@@ -149,6 +145,13 @@ public:
     return mShadowShader;
   }
 
+protected:
+
+  /**
+   * A reference counted object may only be deleted by calling Unreference()
+   */
+  virtual ~AtlasGlyphManager();
+
 private:
 
   Dali::Toolkit::AtlasManager mAtlasManager;          ///> Atlas Manager created by GlyphManager

diff --git a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.cpp b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.cpp
index 7e1facd..e71ad89 100644 (file)
--- a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.cpp
+++ b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.cpp
@@ -128,9 +128,9 @@ const Toolkit::AtlasGlyphManager::Metrics& AtlasGlyphManager::GetMetrics()
   return GetImplementation(*this).GetMetrics();
 }
 
-void AtlasGlyphManager::AdjustReferenceCount( Text::FontId fontId, uint32_t imageId, int32_t delta )
+void AtlasGlyphManager::AdjustReferenceCount( Text::FontId fontId, Text::GlyphIndex index, int32_t delta )
 {
-  GetImplementation(*this).AdjustReferenceCount( fontId, imageId, delta );
+  GetImplementation(*this).AdjustReferenceCount( fontId, index, delta );
 }
 
 Shader AtlasGlyphManager::GetEffectBufferShader() const

diff --git a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.h b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.h
index 6096612..26dea0b 100644 (file)
--- a/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.h
+++ b/dali-toolkit/internal/text/rendering/atlas/atlas-glyph-manager.h
@@ -51,6 +51,7 @@ public:
     {}
 
     uint32_t mGlyphCount;                   // number of glyphs being managed
+    std::string mVerboseGlyphCounts;         // a verbose list of the glyphs + ref counts
     AtlasManager::Metrics mAtlasMetrics;    // metrics from the Atlas Manager
   };
 
@@ -173,13 +174,13 @@ public:
   const Metrics& GetMetrics();
 
   /**
-   * @brief Adjust the reference count for an imageId and remove cache entry if it becomes free
+   * @brief Adjust the reference count for glyph
    *
-   * @param[in] fontId the font this image came from
-   * @param[in] imageId The imageId
-   * @param[in] delta adjustment to make to reference count
+   * @param[in] fontId The font this image came from
+   * @param[in] index The index of the glyph
+   * @param[in] delta The adjustment to make to the reference count
    */
-  void AdjustReferenceCount( Text::FontId fontId, uint32_t imageId, int32_t delta );
+  void AdjustReferenceCount( Text::FontId fontId, Text::GlyphIndex index, int32_t delta );
 
   /**
    * @brief Get Shader used for rendering glyph effect buffers

diff --git a/dali-toolkit/internal/text/rendering/atlas/text-atlas-renderer.cpp b/dali-toolkit/internal/text/rendering/atlas/text-atlas-renderer.cpp
index a9810c9..d0cedf4 100644 (file)
--- a/dali-toolkit/internal/text/rendering/atlas/text-atlas-renderer.cpp
+++ b/dali-toolkit/internal/text/rendering/atlas/text-atlas-renderer.cpp
@@ -134,14 +134,11 @@ struct AtlasRenderer::Impl : public ConnectionTracker
       style = STYLE_DROP_SHADOW;
     }
 
-    if ( mTextCache.Size() )
-    {
-      // Update the glyph cache with any changes to current text
-      RemoveText( glyphs );
-    }
-
     CalculateBlocksSize( glyphs );
 
+    // Avoid emptying mTextCache (& removing references) until after incremented references for the new text
+    Vector< TextCacheEntry > newTextCache;
+
     for( uint32_t i = 0, glyphSize = glyphs.Size(); i < glyphSize; ++i )
     {
       const GlyphInfo& glyph = glyphs[ i ];
@@ -235,13 +232,18 @@ struct AtlasRenderer::Impl : public ConnectionTracker
             mGlyphManager.Add( glyph, bitmap, slot );
           }
         }
+        else
+        {
+          // We have 2+ copies of the same glyph
+          mGlyphManager.AdjustReferenceCount( glyph.fontId, glyph.index, 1/*increment*/ );
+        }
 
         // Generate mesh data for this quad, plugging in our supplied position
         mGlyphManager.GenerateMeshData( slot.mImageId, position, newMesh );
         textCacheEntry.mFontId = glyph.fontId;
         textCacheEntry.mImageId = slot.mImageId;
         textCacheEntry.mIndex = glyph.index;
-        mTextCache.PushBack( textCacheEntry );
+        newTextCache.PushBack( textCacheEntry );
 
         // Find an existing mesh data object to attach to ( or create a new one, if we can't find one using the same atlas)
         StitchTextMesh( meshContainer,
@@ -256,6 +258,10 @@ struct AtlasRenderer::Impl : public ConnectionTracker
       }
     }
 
+    // Now remove references for the old text
+    RemoveText();
+    mTextCache.Swap( newTextCache );
+
     if ( underlineEnabled )
     {
       // Check to see if any of the text needs an underline
@@ -292,9 +298,12 @@ struct AtlasRenderer::Impl : public ConnectionTracker
                                                 metrics.mGlyphCount,
                                                 metrics.mAtlasMetrics.mAtlasCount,
                                                 metrics.mAtlasMetrics.mTextureMemoryUsed / 1024 );
+
+    DALI_LOG_INFO( gLogFilter, Debug::Verbose, "%s\n", metrics.mVerboseGlyphCounts.c_str() );
+
     for ( uint32_t i = 0; i < metrics.mAtlasMetrics.mAtlasCount; ++i )
     {
-      DALI_LOG_INFO( gLogFilter, Debug::Verbose, "Atlas [%i] %sPixels: %s Size: %ix%i, BlockSize: %ix%i, BlocksUsed: %i/%i\n",
+      DALI_LOG_INFO( gLogFilter, Debug::Verbose, "   Atlas [%i] %sPixels: %s Size: %ix%i, BlockSize: %ix%i, BlocksUsed: %i/%i\n",
                                                  i + 1, i > 8 ? "" : " ",
                                                  metrics.mAtlasMetrics.mAtlasMetrics[ i ].mPixelFormat == Pixel::L8 ? "L8  " : "BGRA",
                                                  metrics.mAtlasMetrics.mAtlasMetrics[ i ].mSize.mWidth,
@@ -307,6 +316,15 @@ struct AtlasRenderer::Impl : public ConnectionTracker
 #endif
   }
 
+  void RemoveText()
+  {
+    for ( Vector< TextCacheEntry >::Iterator oldTextIter = mTextCache.Begin(); oldTextIter != mTextCache.End(); ++oldTextIter )
+    {
+      mGlyphManager.AdjustReferenceCount( oldTextIter->mFontId, oldTextIter->mIndex, -1/*decrement*/ );
+    }
+    mTextCache.Resize( 0 );
+  }
+
   Actor CreateMeshActor( const MeshRecord& meshRecord )
   {
     PropertyBuffer quadVertices = PropertyBuffer::New( mQuadVertexFormat, meshRecord.mMesh.mVertices.Size() );
@@ -442,58 +460,6 @@ struct AtlasRenderer::Impl : public ConnectionTracker
     }
   }
 
-  void RemoveText( const Vector<GlyphInfo>& glyphs )
-  {
-    Vector< CheckEntry > checked;
-    CheckEntry checkEntry;
-
-    for ( Vector< TextCacheEntry >::Iterator tCit = mTextCache.Begin(); tCit != mTextCache.End(); ++tCit )
-    {
-      uint32_t index = tCit->mIndex;
-      uint32_t fontId = tCit->mFontId;
-
-      // Check that this character has not already been checked...
-      bool wasChecked = false;
-      for ( Vector< CheckEntry >::Iterator cEit = checked.Begin(); cEit != checked.End(); ++cEit )
-      {
-        if ( fontId == cEit->mFontId && index == cEit->mIndex )
-        {
-          wasChecked = true;
-        }
-      }
-
-      if ( !wasChecked )
-      {
-
-        int32_t newCount = 0;
-        int32_t oldCount = 0;
-
-        // How many times does this character occur in the old text ?
-        for ( Vector< TextCacheEntry >::Iterator oTcit = mTextCache.Begin(); oTcit != mTextCache.End(); ++oTcit )
-        {
-          if ( fontId == oTcit->mFontId && index == oTcit->mIndex )
-          {
-            oldCount++;
-          }
-        }
-
-        // And how many times in the new ?
-        for ( Vector< GlyphInfo >::Iterator cGit = glyphs.Begin(); cGit != glyphs.End(); ++cGit )
-        {
-          if ( fontId == cGit->fontId && index == cGit->index )
-          {
-            newCount++;
-          }
-        }
-        mGlyphManager.AdjustReferenceCount( fontId, tCit->mImageId, newCount - oldCount );
-        checkEntry.mIndex = index;
-        checkEntry.mFontId = fontId;
-        checked.PushBack( checkEntry );
-      }
-    }
-    mTextCache.Resize( 0 );
-  }
-
   void CalculateBlocksSize( const Vector<GlyphInfo>& glyphs )
   {
     MaxBlockSize maxBlockSize;
@@ -779,7 +745,6 @@ AtlasRenderer::AtlasRenderer()
 
 AtlasRenderer::~AtlasRenderer()
 {
-  Vector< GlyphInfo > emptyGlyphs;
-  mImpl->RemoveText( emptyGlyphs );
+  mImpl->RemoveText();
   delete mImpl;
 }