From 514eac93e62d836963be5264a37fb62e9d2632fe Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 13 Mar 2018 20:38:09 +0000 Subject: [PATCH] [4.0] Ensured ImageView requests inside ResourceReady signal handler are queued. Deferring ImageView load requests until after ResourceReady signal handler. has completed ensures that attempting to re-load failed images doesn't fail to send a second ResourceReady callback. An application can still cause an infinite loop if it doesn't have a max retry count when attempting to re-load failed images inside the signal handler. This is considered to be an application bug, not a DALi bug. ( Control::ResourceReady signal is not a one-shot signal). Change-Id: I2c505623ce5e02d3ae67e6e06fd80d5108dc8ade Signed-off-by: David Steele --- .../toolkit-event-thread-callback.cpp | 6 +- .../src/dali-toolkit/utc-Dali-ImageView.cpp | 54 +++++++++ .../internal/visuals/texture-manager-impl.cpp | 122 +++++++++++++-------- .../internal/visuals/texture-manager-impl.h | 44 +++++++- 4 files changed, 178 insertions(+), 48 deletions(-) diff --git a/automated-tests/src/dali-toolkit/dali-toolkit-test-utils/toolkit-event-thread-callback.cpp b/automated-tests/src/dali-toolkit/dali-toolkit-test-utils/toolkit-event-thread-callback.cpp index 5b8a324..02f3d8f 100644 --- a/automated-tests/src/dali-toolkit/dali-toolkit-test-utils/toolkit-event-thread-callback.cpp +++ b/automated-tests/src/dali-toolkit/dali-toolkit-test-utils/toolkit-event-thread-callback.cpp @@ -77,7 +77,7 @@ bool EventThreadCallback::WaitingForTrigger() struct timespec now; clock_gettime( CLOCK_REALTIME, &now ); if( now.tv_nsec < 999900000 ) // 999, 900, 000 - now.tv_nsec += 100000; + now.tv_nsec += 1000; else { now.tv_sec += 1; @@ -124,6 +124,10 @@ bool WaitForEventThreadTrigger( int triggerCount, int timeoutInSeconds ) Dali::CallbackBase::Execute( *callback ); triggerCount--; } + if( triggerCount <= 0 ) + { + break; + } } } clock_gettime( CLOCK_REALTIME, &now ); diff --git a/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp b/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp index c9cbdc5..25a397c 100644 --- a/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp +++ b/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp @@ -1619,3 +1619,57 @@ int UtcDaliImageViewResourceReadySignalWithReusedImage02(void) END_TEST; } + + +static int gFailCounter = 0; +const int MAX_RETRIES(3); + +void ReloadImage(ImageView imageView) +{ + Property::Map imageImmediateLoadingMap; + imageImmediateLoadingMap[ ImageVisual::Property::URL ] = "Non-existant-image.jpg"; + imageImmediateLoadingMap[ ImageVisual::Property::LOAD_POLICY ] = ImageVisual::LoadPolicy::IMMEDIATE; + + tet_infoline("Immediate load an image"); + imageView.SetProperty( ImageView::Property::IMAGE, imageImmediateLoadingMap ); +} + +void ResourceFailedReload( Control control ) +{ + gFailCounter++; + if( gFailCounter < MAX_RETRIES ) + { + ReloadImage(ImageView::DownCast(control)); + } +} + +int UtcDaliImageViewReloadFailedOnResourceReadySignal(void) +{ + tet_infoline("Test Setting Image that was already loaded by another ImageView and still getting ResourceReadySignal when staged."); + + ToolkitTestApplication application; + + gFailCounter = 0; + + ImageView imageView = ImageView::New(); + imageView.ResourceReadySignal().Connect( &ResourceFailedReload ); + DALI_TEST_EQUALS( gFailCounter, 0, TEST_LOCATION ); + ReloadImage(imageView); + + // loading started, this waits for the loader thread to complete + DALI_TEST_EQUALS( Test::WaitForEventThreadTrigger( 1 ), true, TEST_LOCATION ); + application.SendNotification(); + + DALI_TEST_EQUALS( gFailCounter, 1, TEST_LOCATION ); + + DALI_TEST_EQUALS( Test::WaitForEventThreadTrigger( 1 ), true, TEST_LOCATION ); + application.SendNotification(); + + DALI_TEST_EQUALS( gFailCounter, 2, TEST_LOCATION ); + + DALI_TEST_EQUALS( Test::WaitForEventThreadTrigger( 1 ), true, TEST_LOCATION ); + application.SendNotification(); + DALI_TEST_EQUALS( gFailCounter, 3, TEST_LOCATION ); + + END_TEST; +} diff --git a/dali-toolkit/internal/visuals/texture-manager-impl.cpp b/dali-toolkit/internal/visuals/texture-manager-impl.cpp index 7f62d22..4de3fb5 100755 --- a/dali-toolkit/internal/visuals/texture-manager-impl.cpp +++ b/dali-toolkit/internal/visuals/texture-manager-impl.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017 Samsung Electronics Co., Ltd. + * Copyright (c) 2018 Samsung Electronics Co., Ltd. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -116,7 +116,10 @@ TextureManager::MaskingData::MaskingData() TextureManager::TextureManager() : mAsyncLocalLoaders( GetNumberOfLocalLoaderThreads(), [&]() { return AsyncLoadingHelper(*this); } ), mAsyncRemoteLoaders( GetNumberOfRemoteLoaderThreads(), [&]() { return AsyncLoadingHelper(*this); } ), - mCurrentTextureId( 0 ) + mExternalTextures(), + mLoadQueue(), + mCurrentTextureId( 0 ), + mQueueLoadFlag(false) { } @@ -374,8 +377,7 @@ TextureManager::TextureId TextureManager::RequestLoadInternal( case TextureManager::LOAD_FAILED: // Failed notifies observer which then stops observing. case TextureManager::NOT_STARTED: { - LoadTexture( textureInfo ); - ObserveTexture( textureInfo, observer ); + LoadOrQueueTexture( textureInfo, observer ); // If called inside NotifyObservers, queues until afterwards break; } case TextureManager::LOADING: @@ -573,26 +575,67 @@ TextureSet TextureManager::RemoveExternalTexture( const std::string& url ) return TextureSet(); } -bool TextureManager::LoadTexture( TextureInfo& textureInfo ) +void TextureManager::LoadOrQueueTexture( TextureInfo& textureInfo, TextureUploadObserver* observer ) +{ + switch( textureInfo.loadState ) + { + case NOT_STARTED: + case LOAD_FAILED: + { + if( mQueueLoadFlag ) + { + QueueLoadTexture( textureInfo, observer ); + } + else + { + LoadTexture( textureInfo, observer ); + } + break; + } + case LOADING: + case UPLOADED: + case CANCELLED: + case LOAD_FINISHED: + case WAITING_FOR_MASK: + { + break; + } + } +} + +void TextureManager::QueueLoadTexture( TextureInfo& textureInfo, TextureUploadObserver* observer ) { - bool success = true; + auto textureId = textureInfo.textureId; + mLoadQueue.PushBack( LoadQueueElement( textureId, observer) ); +} - if( textureInfo.loadState == NOT_STARTED ) +void TextureManager::LoadTexture( TextureInfo& textureInfo, TextureUploadObserver* observer ) +{ + textureInfo.loadState = LOADING; + if( !textureInfo.loadSynchronously ) { - textureInfo.loadState = LOADING; + auto& loadersContainer = textureInfo.url.IsLocalResource() ? mAsyncLocalLoaders : mAsyncRemoteLoaders; + auto loadingHelperIt = loadersContainer.GetNext(); + DALI_ASSERT_ALWAYS(loadingHelperIt != loadersContainer.End()); + loadingHelperIt->Load(textureInfo.textureId, textureInfo.url, + textureInfo.desiredSize, textureInfo.fittingMode, + textureInfo.samplingMode, textureInfo.orientationCorrection ); + } + ObserveTexture( textureInfo, observer ); +} - if( !textureInfo.loadSynchronously ) +void TextureManager::ProcessQueuedTextures() +{ + for( auto&& element : mLoadQueue ) + { + int cacheIndex = GetCacheIndexFromId( element.mTextureId ); + if( cacheIndex != INVALID_CACHE_INDEX ) { - auto& loadersContainer = textureInfo.url.IsLocalResource() ? mAsyncLocalLoaders : mAsyncRemoteLoaders; - auto loadingHelperIt = loadersContainer.GetNext(); - DALI_ASSERT_ALWAYS(loadingHelperIt != loadersContainer.End()); - loadingHelperIt->Load(textureInfo.textureId, textureInfo.url, - textureInfo.desiredSize, textureInfo.fittingMode, - textureInfo.samplingMode, textureInfo.orientationCorrection ); + TextureInfo& textureInfo( mTextureInfoContainer[cacheIndex] ); + LoadTexture( textureInfo, element.mObserver ); } } - - return success; + mLoadQueue.Clear(); } void TextureManager::ObserveTexture( TextureInfo& textureInfo, @@ -733,7 +776,6 @@ void TextureManager::ApplyMask( pixelBuffer.ApplyMask( maskPixelBuffer, contentScale, cropToMask ); } - void TextureManager::UploadTexture( Devel::PixelBuffer& pixelBuffer, TextureInfo& textureInfo ) { if( textureInfo.useAtlas != USE_ATLAS ) @@ -773,53 +815,47 @@ void TextureManager::NotifyObservers( TextureInfo& textureInfo, bool success ) // If there is an observer: Notify the load is complete, whether successful or not, // and erase it from the list - unsigned int observerCount = textureInfo.observerList.Count(); TextureInfo* info = &textureInfo; - while( observerCount ) + mQueueLoadFlag = true; + + while( info->observerList.Count() ) { TextureUploadObserver* observer = info->observerList[0]; // During UploadComplete() a Control ResourceReady() signal is emitted. // During that signal the app may add remove /add Textures (e.g. via - // ImageViews). At this point no more observers can be added to the - // observerList, because textureInfo.loadState = UPLOADED. However it is - // possible for observers to be removed, hence we check the observer list - // count every iteration. - - // The reference to the textureInfo struct can also become invalidated, - // because new load requests can modify the mTextureInfoContainer list - // (e.g. if more requests are pushed back it can cause the list to be - // resized invalidating the reference to the TextureInfo ). + // ImageViews). + // It is possible for observers to be removed from the observer list, + // and it is also possible for the mTextureInfoContainer to be modified, + // invalidating the reference to the textureInfo struct. + // Texture load requests for the same URL are deferred until the end of this + // method. observer->UploadComplete( success, info->textureId, info->textureSet, info->useAtlas, info->atlasRect, info->preMultiplied ); observer->DestructionSignal().Disconnect( this, &TextureManager::ObserverDestroyed ); - // Get the textureInfo from the container again as it may have been - // invalidated, - + // Get the textureInfo from the container again as it may have been invalidated. int textureInfoIndex = GetCacheIndexFromId( textureId ); if( textureInfoIndex == INVALID_CACHE_INDEX) { return; // texture has been removed - can stop. } - info = &mTextureInfoContainer[ textureInfoIndex ]; - observerCount = info->observerList.Count(); - if ( observerCount > 0 ) + + // remove the observer that was just triggered if it's still in the list + for( TextureInfo::ObserverListType::Iterator j = info->observerList.Begin(); j != info->observerList.End(); ++j ) { - // remove the observer that was just triggered if it's still in the list - for( TextureInfo::ObserverListType::Iterator j = info->observerList.Begin(); j != info->observerList.End(); ++j ) + if( *j == observer ) { - if( *j == observer ) - { - info->observerList.Erase( j ); - observerCount--; - break; - } + info->observerList.Erase( j ); + break; } } } + + mQueueLoadFlag = false; + ProcessQueuedTextures(); } TextureManager::TextureId TextureManager::GenerateUniqueTextureId() diff --git a/dali-toolkit/internal/visuals/texture-manager-impl.h b/dali-toolkit/internal/visuals/texture-manager-impl.h index 5b6a98e..b0e2606 100755 --- a/dali-toolkit/internal/visuals/texture-manager-impl.h +++ b/dali-toolkit/internal/visuals/texture-manager-impl.h @@ -388,6 +388,8 @@ private: typedef size_t TextureHash; ///< The type used to store the hash used for Texture caching. + // Structs: + /** * @brief This struct is used to manage the life-cycle of Texture loading and caching. */ @@ -459,7 +461,20 @@ private: bool preMultiplied:1; ///< true if the image's color was multiplied by it's alpha }; - // Structs: + /** + * Structure to hold info about a texture load queued during NotifyObservers + */ + struct LoadQueueElement + { + LoadQueueElement( TextureId textureId, TextureUploadObserver* observer ) + : mTextureId( textureId ), + mObserver( observer ) + { + } + + TextureId mTextureId; ///< The texture id of the requested load. + TextureUploadObserver* mObserver; ///< Observer of texture load. + }; /** * Struct to hold information about a requested Async load. @@ -483,16 +498,35 @@ private: typedef std::vector TextureInfoContainerType; ///< The container type used to manage the life-cycle and caching of Textures /** + * @brief Initiate a load or queue load if NotifyObservers is invoking callbacks + * @param[in] textureInfo The TextureInfo struct associated with the Texture + * @param[in] observer The observer wishing to observe the texture upload + */ + void LoadOrQueueTexture( TextureInfo& textureInfo, TextureUploadObserver* observer ); + + /** + * @brief Queue a texture load to be subsequently handled by ProcessQueuedTextures. + * @param[in] textureInfo The TextureInfo struct associated with the Texture + * @param[in] observer The observer wishing to observe the texture upload + */ + void QueueLoadTexture( TextureInfo& textureInfo, TextureUploadObserver* observer ); + + /** * @brief Used internally to initiate a load. * @param[in] textureInfo The TextureInfo struct associated with the Texture - * @return True if the load was initiated + * @param[in] observer The observer wishing to observe the texture upload + */ + void LoadTexture( TextureInfo& textureInfo, TextureUploadObserver* observer ); + + /** + * @brief Initiate load of textures queued whilst NotifyObservers invoking callbacks. */ - bool LoadTexture( TextureInfo& textureInfo ); + void ProcessQueuedTextures(); /** * Add the observer to the observer list * @param[in] textureInfo The TextureInfo struct associated with the texture - * observer The observer wishing to observe the texture upload + * @param[in] observer The observer wishing to observe the texture upload */ void ObserveTexture( TextureInfo & textureInfo, TextureUploadObserver* observer ); @@ -710,7 +744,9 @@ private: // Member Variables: RoundRobinContainerView< AsyncLoadingHelper > mAsyncLocalLoaders; ///< The Asynchronous image loaders used to provide all local async loads RoundRobinContainerView< AsyncLoadingHelper > mAsyncRemoteLoaders; ///< The Asynchronous image loaders used to provide all remote async loads std::vector< ExternalTextureInfo > mExternalTextures; ///< Externally provided textures + Dali::Vector mLoadQueue; ///< Queue of textures to load after NotifyObservers TextureId mCurrentTextureId; ///< The current value used for the unique Texture Id generation + bool mQueueLoadFlag; ///< Flag that causes Load Textures to be queued. }; -- 2.7.4