Minor refactor about load/remove queue

1. Fix crash issue when we try to load mask image during ResourceReady
2. Remove useless duplicated codes for mRemoveQueue.
3. Make some API works even observer is nullptr (for mask case)
4. Notify observers timing changed when mask image postload

Change-Id: I73f4816c628d3808d7355e17d358226af2985db8
Signed-off-by: Eunki, Hong <eunkiki.hong@samsung.com>

diff --git a/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp b/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp
index 1559ae7..34aa8a3 100644 (file)
--- a/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp
+++ b/automated-tests/src/dali-toolkit/utc-Dali-ImageView.cpp
@@ -3474,7 +3474,7 @@ void OnResourceReadySignal06(Control control)
     // We hope this request result is return later than gImageView2.
 
     Property::Map map1;
-    map1[Toolkit::ImageVisual::Property::URL] = TEST_IMAGE_1;
+    map1[Toolkit::ImageVisual::Property::URL]            = TEST_IMAGE_1;
     map1[Toolkit::ImageVisual::Property::ALPHA_MASK_URL] = TEST_BROKEN_IMAGE_DEFAULT;
 
     gImageView3 = ImageView::New();
@@ -3482,9 +3482,9 @@ void OnResourceReadySignal06(Control control)
     gImageView3.ResourceReadySignal().Connect(&OnResourceReadySignal06);
 
     Property::Map map2;
-    map2[Toolkit::ImageVisual::Property::URL] = TEST_IMAGE_2;
+    map2[Toolkit::ImageVisual::Property::URL]            = TEST_IMAGE_2;
     map2[Toolkit::ImageVisual::Property::ALPHA_MASK_URL] = TEST_BROKEN_IMAGE_S;
-    gImageView4 = ImageView::New();
+    gImageView4                                          = ImageView::New();
     gImageView4.SetProperty(Toolkit::ImageView::Property::IMAGE, map2);
     gImageView4.ResourceReadySignal().Connect(&OnResourceReadySignal06);
 
@@ -3529,6 +3529,28 @@ void OnResourceReadySignal06(Control control)
   }
 }
 
+void OnResourceReadySignal07(Control control)
+{
+  gResourceReadySignalCounter++;
+  // Load masked image
+  tet_printf("rc %d %d\n", gResourceReadySignalCounter, static_cast<bool>(gImageView2));
+
+  if(!gImageView2)
+  {
+    auto scene = gImageView1.GetParent();
+
+    Property::Map map1;
+    map1[Toolkit::ImageVisual::Property::URL]            = TEST_IMAGE_1;
+    map1[Toolkit::ImageVisual::Property::ALPHA_MASK_URL] = TEST_BROKEN_IMAGE_DEFAULT;
+
+    gImageView2 = ImageView::New();
+    gImageView2.SetProperty(Toolkit::ImageView::Property::IMAGE, map1);
+    gImageView2.ResourceReadySignal().Connect(&OnResourceReadySignal07);
+
+    scene.Add(gImageView2);
+  }
+}
+
 } // namespace
 
 int UtcDaliImageViewSetImageOnResourceReadySignal01(void)
@@ -3966,7 +3988,7 @@ int UtcDaliImageViewSetImageOnResourceReadySignal06(void)
   gResourceReadySignal06ComesOrder = 0;
 
   Property::Map map;
-  map[Toolkit::ImageVisual::Property::URL] = "invalid.jpg";
+  map[Toolkit::ImageVisual::Property::URL]            = "invalid.jpg";
   map[Toolkit::ImageVisual::Property::ALPHA_MASK_URL] = "invalid.png";
 
   gImageView1 = ImageView::New(); // request invalid image, to make loading failed fast.
@@ -4015,6 +4037,57 @@ int UtcDaliImageViewSetImageOnResourceReadySignal06(void)
   END_TEST;
 }
 
+int UtcDaliImageViewSetImageOnResourceReadySignal07(void)
+{
+  tet_infoline("Test texturemanager's remove image & mask queue works well within signal handler 02.");
+
+  ToolkitTestApplication application;
+
+  gResourceReadySignalCounter = 0;
+
+  Property::Map map;
+  map[Toolkit::ImageVisual::Property::URL] = TEST_IMAGE_1;
+
+  // Clear image view for clear test
+
+  if(gImageView1)
+  {
+    gImageView1.Reset();
+  }
+  if(gImageView2)
+  {
+    gImageView2.Reset();
+  }
+
+  gImageView1 = ImageView::New();
+  gImageView1.SetProperty(Toolkit::ImageView::Property::IMAGE, map);
+  gImageView1.ResourceReadySignal().Connect(&OnResourceReadySignal07);
+  application.GetScene().Add(gImageView1);
+
+  application.SendNotification();
+  application.Render();
+
+  // Load gImageView1
+
+  DALI_TEST_EQUALS(Test::WaitForEventThreadTrigger(1), true, TEST_LOCATION);
+  DALI_TEST_EQUALS(gResourceReadySignalCounter, 1, TEST_LOCATION);
+
+  tet_infoline("load image1 done");
+
+  // Load gImageView2 and mask
+
+  DALI_TEST_EQUALS(Test::WaitForEventThreadTrigger(2), true, TEST_LOCATION);
+  DALI_TEST_EQUALS(gResourceReadySignalCounter, 1, TEST_LOCATION);
+
+  // gImageView2 mask apply done
+
+  DALI_TEST_EQUALS(Test::WaitForEventThreadTrigger(1), true, TEST_LOCATION);
+  DALI_TEST_EQUALS(gResourceReadySignalCounter, 2, TEST_LOCATION);
+
+  tet_infoline("load image2 done");
+  END_TEST;
+}
+
 int UtcDaliImageViewUseSameUrlWithAnimatedImageVisual(void)
 {
   tet_infoline("Test multiple views with same image in animated image visual");

diff --git a/dali-toolkit/internal/texture-manager/texture-manager-impl.cpp b/dali-toolkit/internal/texture-manager/texture-manager-impl.cpp
index f761781..ab237a3 100644 (file)
--- a/dali-toolkit/internal/texture-manager/texture-manager-impl.cpp
+++ b/dali-toolkit/internal/texture-manager/texture-manager-impl.cpp
@@ -711,29 +711,39 @@ void TextureManager::Remove(const TextureManager::TextureId& textureId, TextureU
     {
       TextureManager::TextureId maskTextureId = INVALID_TEXTURE_ID;
       TextureInfo&              textureInfo(mTextureCacheManager[textureCacheIndex]);
-      if(textureInfo.maskTextureId != INVALID_TEXTURE_ID)
+      // We only need to consider maskTextureId when texture's loadState is not CANCELLED. Because it is already deleted.
+      if(textureInfo.loadState != LoadState::CANCELLED)
       {
-        maskTextureId = textureInfo.maskTextureId;
+        if(textureInfo.maskTextureId != INVALID_TEXTURE_ID)
+        {
+          maskTextureId = textureInfo.maskTextureId;
+        }
       }
 
       // the case that LoadingQueue is working.
       if(mLoadingQueueTextureId != INVALID_TEXTURE_ID)
       {
         // If textureId is not same, this observer need to delete when ProcessRemoveQueue() is called.
-        TextureUploadObserver* queueObserver = nullptr;
-        if(mLoadingQueueTextureId != textureId)
+        // If textureId is same, we should not call RemoveTextureObserver.
+        // Because ObserverDestroyed signal already disconnected in NotifyObservers
+        TextureUploadObserver* queueObserver = observer;
+        if(mLoadingQueueTextureId == textureId)
         {
-          queueObserver = observer;
+          queueObserver = nullptr;
         }
 
-        // Remove textureId after NotifyObserver finished
-        if(maskTextureId != INVALID_TEXTURE_ID)
+        // Remove element from the mLoadQueue
+        for(auto&& element : mLoadQueue)
         {
-          if(textureInfo.loadState != LoadState::CANCELLED)
+          if(element.mTextureId == textureId && element.mObserver == observer)
           {
-            mRemoveQueue.PushBack(QueueElement(maskTextureId, nullptr));
+            // Do not erase the item. We will clear it later in ProcessLoadQueue().
+            element.mTextureId = INVALID_TEXTURE_ID;
+            element.mObserver  = nullptr;
+            break;
           }
         }
+
         mRemoveQueue.PushBack(QueueElement(textureId, queueObserver));
       }
       else
@@ -741,10 +751,7 @@ void TextureManager::Remove(const TextureManager::TextureId& textureId, TextureU
         // Remove its observer
         RemoveTextureObserver(textureInfo, observer);
 
-        // Keep loadState due to the textureInfo validate problem.
-        auto textureLoadState = textureInfo.loadState;
-
-        // Remove textureId in CacheManager
+        // Remove textureId in CacheManager. Now, textureInfo is invalidate.
         mTextureCacheManager.RemoveCache(textureInfo);
 
         // Remove maskTextureId in CacheManager
@@ -754,30 +761,11 @@ void TextureManager::Remove(const TextureManager::TextureId& textureId, TextureU
           if(maskCacheIndex != INVALID_CACHE_INDEX)
           {
             TextureInfo& maskTextureInfo(mTextureCacheManager[maskCacheIndex]);
-
-            // Only Remove maskTexture when texture's loadState is not CANCELLED. because it is already deleted.
-            if(textureLoadState != LoadState::CANCELLED)
-            {
-              mTextureCacheManager.RemoveCache(maskTextureInfo);
-            }
+            mTextureCacheManager.RemoveCache(maskTextureInfo);
           }
         }
       }
     }
-
-    if(observer)
-    {
-      // Remove element from the LoadQueue
-      for(auto&& element : mLoadQueue)
-      {
-        if(element.mObserver == observer)
-        {
-          // Do not erase the item. We will clear it later in ProcessLoadQueue().
-          element.mObserver = nullptr;
-          break;
-        }
-      }
-    }
   }
 }
 
@@ -888,7 +876,10 @@ void TextureManager::QueueLoadTexture(const TextureManager::TextureInfo& texture
   const auto& textureId = textureInfo.textureId;
   mLoadQueue.PushBack(QueueElement(textureId, observer));
 
-  observer->DestructionSignal().Connect(this, &TextureManager::ObserverDestroyed);
+  if(observer)
+  {
+    observer->DestructionSignal().Connect(this, &TextureManager::ObserverDestroyed);
+  }
 }
 
 void TextureManager::LoadTexture(TextureManager::TextureInfo& textureInfo, TextureUploadObserver* observer)
@@ -918,7 +909,7 @@ void TextureManager::ProcessLoadQueue()
 {
   for(auto&& element : mLoadQueue)
   {
-    if(!element.mObserver)
+    if(element.mTextureId == INVALID_TEXTURE_ID)
     {
       continue;
     }
@@ -929,7 +920,10 @@ void TextureManager::ProcessLoadQueue()
       TextureInfo& textureInfo(mTextureCacheManager[cacheIndex]);
       if((textureInfo.loadState == LoadState::UPLOADED) || (textureInfo.loadState == LoadState::LOAD_FINISHED && textureInfo.storageType == StorageType::RETURN_PIXEL_BUFFER))
       {
-        EmitLoadComplete(element.mObserver, textureInfo, true);
+        if(element.mObserver)
+        {
+          EmitLoadComplete(element.mObserver, textureInfo, true);
+        }
       }
       else if(textureInfo.loadState == LoadState::LOADING)
       {
@@ -948,15 +942,11 @@ void TextureManager::ProcessLoadQueue()
 
 void TextureManager::ProcessRemoveQueue()
 {
-  TextureCacheIndex textureCacheIndex = INVALID_CACHE_INDEX;
   for(auto&& element : mRemoveQueue)
   {
-    textureCacheIndex = mTextureCacheManager.GetCacheIndexFromId(element.mTextureId);
-    if(textureCacheIndex != INVALID_CACHE_INDEX)
+    if(element.mTextureId != INVALID_TEXTURE_ID)
     {
-      TextureInfo& textureInfo(mTextureCacheManager[textureCacheIndex]);
-      RemoveTextureObserver(textureInfo, element.mObserver);
-      mTextureCacheManager.RemoveCache(textureInfo);
+      Remove(element.mTextureId, element.mObserver);
     }
   }
   mRemoveQueue.Clear();
@@ -1127,10 +1117,13 @@ void TextureManager::CheckForWaitingTexture(TextureManager::TextureInfo& maskTex
     UploadTextures(pixelBuffers, maskTextureInfo);
   }
 
-  // Search the cache, checking if any texture has this texture id as a
-  // maskTextureId:
+  // Search the cache, checking if any texture has this texture id as a maskTextureId
   const std::size_t size = mTextureCacheManager.size();
 
+  // Keep notify observer required textureIds.
+  // Note : NotifyObservers can change mTextureCacheManager cache struct. We should check id's validation before notify.
+  std::vector<TextureId> notifyRequiredTextureIds;
+
   // TODO : Refactorize here to not iterate whole cached image.
   for(TextureCacheIndex cacheIndex = TextureCacheIndex(TextureManagerType::TEXTURE_CACHE_INDEX_TYPE_LOCAL, 0u); cacheIndex.GetIndex() < size; ++cacheIndex.detailValue.index)
   {
@@ -1156,8 +1149,7 @@ void TextureManager::CheckForWaitingTexture(TextureManager::TextureInfo& maskTex
           pixelBuffers.push_back(textureInfo.pixelBuffer);
           UploadTextures(pixelBuffers, textureInfo);
 
-          // notify mask texture set.
-          NotifyObservers(textureInfo, true);
+          notifyRequiredTextureIds.push_back(textureInfo.textureId);
         }
       }
       else
@@ -1167,10 +1159,22 @@ void TextureManager::CheckForWaitingTexture(TextureManager::TextureInfo& maskTex
         std::vector<Devel::PixelBuffer> pixelBuffers;
         pixelBuffers.push_back(textureInfo.pixelBuffer);
         UploadTextures(pixelBuffers, textureInfo);
-        NotifyObservers(textureInfo, true);
+
+        notifyRequiredTextureIds.push_back(textureInfo.textureId);
       }
     }
   }
+
+  // Notify textures are masked
+  for(const auto textureId : notifyRequiredTextureIds)
+  {
+    TextureCacheIndex textureCacheIndex = mTextureCacheManager.GetCacheIndexFromId(textureId);
+    if(textureCacheIndex != INVALID_CACHE_INDEX)
+    {
+      TextureInfo& textureInfo(mTextureCacheManager[textureCacheIndex]);
+      NotifyObservers(textureInfo, true);
+    }
+  }
 }
 
 void TextureManager::ApplyMask(TextureManager::TextureInfo& textureInfo, const TextureManager::TextureId& maskTextureId)
@@ -1317,7 +1321,8 @@ void TextureManager::ObserverDestroyed(TextureUploadObserver* observer)
   {
     if(element.mObserver == observer)
     {
-      element.mObserver = nullptr;
+      element.mTextureId = INVALID_TEXTURE_ID;
+      element.mObserver  = nullptr;
     }
   }
 }