From: Paul Wisbey Date: Tue, 7 Jul 2015 14:03:07 +0000 (+0100) Subject: Simplified the glyph reference counting X-Git-Tag: dali_1.0.49~9^2 X-Git-Url: http://review.tizen.org/git/?p=platform%2Fcore%2Fuifw%2Fdali-toolkit.git;a=commitdiff_plain;h=0edfab6278cc238252d0ef899c0198df4ff953dc 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 --- 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 #include +#include #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 --- 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 --- 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 --- 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 --- 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& 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& glyphs ) { MaxBlockSize maxBlockSize; @@ -779,7 +745,6 @@ AtlasRenderer::AtlasRenderer() AtlasRenderer::~AtlasRenderer() { - Vector< GlyphInfo > emptyGlyphs; - mImpl->RemoveText( emptyGlyphs ); + mImpl->RemoveText(); delete mImpl; }