Lock mutex when animation task check + completed task moving 72/315172/4
authorEunki, Hong <eunkiki.hong@samsung.com>
Fri, 26 Jul 2024 05:08:12 +0000 (14:08 +0900)
committerEunki, Hong <eunkiki.hong@samsung.com>
Mon, 29 Jul 2024 04:06:39 +0000 (13:06 +0900)
Since VectorAnimationThread::OnTaskCompleted() could be called
from various worker threads, we should not change the thread's member value.

To avoid this kind of thread issue,
 - Make mutex during animation tasks control (VectorAnimationThread vs EventThread)
 - Collect tasks from worker thread and move them at Rasterize() API (VectorAnimationThread vs WorkerThread)

Change-Id: Ib3dce6b98f6c66a3bd597a7b46fe8bbac60105b5
Signed-off-by: Eunki, Hong <eunkiki.hong@samsung.com>
dali-toolkit/internal/visuals/animated-vector-image/vector-animation-thread.cpp
dali-toolkit/internal/visuals/animated-vector-image/vector-animation-thread.h

index 5638ec3..a4992d9 100644 (file)
@@ -23,6 +23,7 @@
 #include <dali/devel-api/adaptor-framework/thread-settings.h>
 #include <dali/integration-api/adaptor-framework/adaptor.h>
 #include <dali/integration-api/debug.h>
+#include <dali/integration-api/trace.h>
 #include <thread>
 
 namespace Dali
@@ -37,6 +38,8 @@ namespace
 Debug::Filter* gVectorAnimationLogFilter = Debug::Filter::New(Debug::NoLogging, false, "LOG_VECTOR_ANIMATION");
 #endif
 
+DALI_INIT_TRACE_FILTER(gTraceFilter, DALI_TRACE_IMAGE_PERFORMANCE_MARKER, false);
+
 } // unnamed namespace
 
 VectorAnimationThread::VectorAnimationThread()
@@ -46,6 +49,8 @@ VectorAnimationThread::VectorAnimationThread()
   mSleepThread(MakeCallback(this, &VectorAnimationThread::OnAwakeFromSleep)),
   mConditionalWait(),
   mEventTriggerMutex(),
+  mAnimationTasksMutex(),
+  mTaskCompletedMutex(),
   mLogFactory(Dali::Adaptor::Get().GetLogFactory()),
   mTraceFactory(Dali::Adaptor::Get().GetTraceFactory()),
   mNeedToSleep(false),
@@ -59,14 +64,20 @@ VectorAnimationThread::VectorAnimationThread()
   mEventTrigger = std::unique_ptr<EventThreadCallback>(new EventThreadCallback(MakeCallback(this, &VectorAnimationThread::OnEventCallbackTriggered)));
 }
 
+/// Event thread called
 VectorAnimationThread::~VectorAnimationThread()
 {
+  DALI_TRACE_SCOPE(gTraceFilter, "VECTOR_ANIMATION_THREAD_DESTROY");
+
   // Stop the thread
   {
     ConditionalWait::ScopedLock lock(mConditionalWait);
     // Wait until some event thread trigger relative job finished.
     {
-      Mutex::ScopedLock lock(mEventTriggerMutex);
+      Mutex::ScopedLock eventTriggerLock(mEventTriggerMutex);
+      Mutex::ScopedLock taskCompletedLock(mTaskCompletedMutex);
+      Mutex::ScopedLock animationTasksLock(mAnimationTasksMutex);
+      DALI_LOG_DEBUG_INFO("Mark VectorAnimationThread destroyed\n");
       mDestroyThread = true;
     }
     mNeedToSleep = false;
@@ -78,82 +89,46 @@ VectorAnimationThread::~VectorAnimationThread()
 
   DALI_LOG_INFO(gVectorAnimationLogFilter, Debug::Verbose, "VectorAnimationThread::~VectorAnimationThread: Join [%p]\n", this);
 
+  DALI_LOG_DEBUG_INFO("VectorAnimationThread Join request\n");
+
   Join();
 }
 
+/// Event thread called
 void VectorAnimationThread::AddTask(VectorAnimationTaskPtr task)
 {
   ConditionalWait::ScopedLock lock(mConditionalWait);
 
-  // Find if the task is already in the list except loading task
-  auto iter = std::find_if(mAnimationTasks.begin(), mAnimationTasks.end(), [task](VectorAnimationTaskPtr& element) { return (element == task && !element->IsLoadRequested()); });
-  if(iter == mAnimationTasks.end())
+  // Rasterize as soon as possible
+  if(MoveTasksToAnimation(task, true))
   {
-    auto currentTime = task->CalculateNextFrameTime(true); // Rasterize as soon as possible
-
-    bool inserted = false;
-    for(auto iter = mAnimationTasks.begin(); iter != mAnimationTasks.end(); ++iter)
-    {
-      auto nextTime = (*iter)->GetNextFrameTime();
-      if(nextTime > currentTime)
-      {
-        mAnimationTasks.insert(iter, task);
-        inserted = true;
-        break;
-      }
-    }
-
-    if(!inserted)
-    {
-      mAnimationTasks.push_back(task);
-    }
-
     mNeedToSleep = false;
     // wake up the animation thread
     mConditionalWait.Notify(lock);
   }
 }
 
+/// Worker thread called
 void VectorAnimationThread::OnTaskCompleted(VectorAnimationTaskPtr task, bool success, bool keepAnimation)
 {
-  if(!mDestroyThread)
-  {
-    ConditionalWait::ScopedLock lock(mConditionalWait);
-    bool                        needRasterize = false;
+  ConditionalWait::ScopedLock lock(mConditionalWait);
 
-    auto workingTask = std::find(mWorkingTasks.begin(), mWorkingTasks.end(), task);
-    if(workingTask != mWorkingTasks.end())
-    {
-      mWorkingTasks.erase(workingTask);
-    }
+  Mutex::ScopedLock taskCompletedLock(mTaskCompletedMutex);
 
-    // Check pending task
-    if(mAnimationTasks.end() != std::find(mAnimationTasks.begin(), mAnimationTasks.end(), task))
-    {
-      needRasterize = true;
-    }
-
-    if(keepAnimation && success)
-    {
-      if(mCompletedTasks.end() == std::find(mCompletedTasks.begin(), mCompletedTasks.end(), task))
-      {
-        mCompletedTasks.push_back(task);
-        needRasterize = true;
-      }
-    }
+  if(DALI_LIKELY(!mDestroyThread))
+  {
+    mCompletedTasksQueue.emplace_back(task, success && keepAnimation);
 
-    if(needRasterize)
-    {
-      mNeedToSleep = false;
-      // wake up the animation thread
-      mConditionalWait.Notify(lock);
-    }
+    // wake up the animation thread.
+    // Note that we should not make mNeedToSleep as false now.
+    mConditionalWait.Notify(lock);
   }
 }
 
+/// VectorAnimationThread::SleepThread called
 void VectorAnimationThread::OnAwakeFromSleep()
 {
-  if(!mDestroyThread)
+  if(DALI_LIKELY(!mDestroyThread))
   {
     mNeedToSleep = false;
     // wake up the animation thread
@@ -161,10 +136,11 @@ void VectorAnimationThread::OnAwakeFromSleep()
   }
 }
 
+/// Worker thread called
 void VectorAnimationThread::AddEventTriggerCallback(CallbackBase* callback, uint32_t argument)
 {
   Mutex::ScopedLock lock(mEventTriggerMutex);
-  if(!mDestroyThread)
+  if(DALI_LIKELY(!mDestroyThread))
   {
     mTriggerEventCallbacks.emplace_back(callback, argument);
 
@@ -176,20 +152,22 @@ void VectorAnimationThread::AddEventTriggerCallback(CallbackBase* callback, uint
   }
 }
 
+/// Event thread called
 void VectorAnimationThread::RemoveEventTriggerCallbacks(CallbackBase* callback)
 {
   Mutex::ScopedLock lock(mEventTriggerMutex);
-  if(!mDestroyThread)
+  if(DALI_LIKELY(!mDestroyThread))
   {
     auto iter = std::remove_if(mTriggerEventCallbacks.begin(), mTriggerEventCallbacks.end(), [&callback](std::pair<CallbackBase*, uint32_t>& item) { return item.first == callback; });
     mTriggerEventCallbacks.erase(iter, mTriggerEventCallbacks.end());
   }
 }
 
+/// Worker thread called
 void VectorAnimationThread::RequestForceRenderOnce()
 {
   Mutex::ScopedLock lock(mEventTriggerMutex);
-  if(!mDestroyThread)
+  if(DALI_LIKELY(!mDestroyThread))
   {
     mForceRenderOnce = true;
 
@@ -201,6 +179,7 @@ void VectorAnimationThread::RequestForceRenderOnce()
   }
 }
 
+/// VectorAnimationThread called
 void VectorAnimationThread::Run()
 {
   SetThreadName("VectorAnimationThread");
@@ -213,32 +192,34 @@ void VectorAnimationThread::Run()
   }
 }
 
-void VectorAnimationThread::Rasterize()
+/// Event & VectorAnimationThread called
+bool VectorAnimationThread::MoveTasksToAnimation(VectorAnimationTaskPtr task, bool useCurrentTime)
 {
-  // Lock while popping task out from the queue
-  ConditionalWait::ScopedLock lock(mConditionalWait);
+  Mutex::ScopedLock animationTasksLock(mAnimationTasksMutex);
 
-  // conditional wait
-  if(mNeedToSleep)
-  {
-    mConditionalWait.Wait(lock);
-  }
-
-  mNeedToSleep = true;
-
-  // Process completed tasks
-  for(auto&& task : mCompletedTasks)
+  if(DALI_LIKELY(!mDestroyThread))
   {
-    if(mAnimationTasks.end() == std::find(mAnimationTasks.begin(), mAnimationTasks.end(), task))
+    DALI_TRACE_BEGIN_WITH_MESSAGE_GENERATOR(gTraceFilter, "VECTOR_ANIMATION_THREAD_ANIMATION_TASK", [&](std::ostringstream& oss) {
+      oss << "[" << mAnimationTasks.size() << "," << useCurrentTime << "]";
+    });
+
+    // Find if the task is already in the list except loading task
+    auto iter = std::find_if(mAnimationTasks.begin(), mAnimationTasks.end(), [task, useCurrentTime](VectorAnimationTaskPtr& element) {
+      return (element == task) &&
+             (!useCurrentTime ||            // If we don't need to use current time (i.e. CompletedTasks)
+              !element->IsLoadRequested()); // Or we need to use current time And loading completed.
+    });
+
+    if(iter == mAnimationTasks.end())
     {
-      // Should use the frame rate of the animation file
-      auto nextFrameTime = task->CalculateNextFrameTime(false);
+      // Use the frame rate of the animation file, or use current time.
+      auto nextFrameTime = task->CalculateNextFrameTime(useCurrentTime);
 
       bool inserted = false;
       for(auto iter = mAnimationTasks.begin(); iter != mAnimationTasks.end(); ++iter)
       {
-        auto time = (*iter)->GetNextFrameTime();
-        if(time > nextFrameTime)
+        auto nextTime = (*iter)->GetNextFrameTime();
+        if(nextTime > nextFrameTime)
         {
           mAnimationTasks.insert(iter, task);
           inserted = true;
@@ -250,47 +231,156 @@ void VectorAnimationThread::Rasterize()
       {
         mAnimationTasks.push_back(task);
       }
+
+      DALI_TRACE_END_WITH_MESSAGE_GENERATOR(gTraceFilter, "VECTOR_ANIMATION_THREAD_ANIMATION_TASK", [&](std::ostringstream& oss) {
+        oss << "[" << mAnimationTasks.size() << "]";
+      });
+
+      return true;
     }
+    DALI_TRACE_END(gTraceFilter, "VECTOR_ANIMATION_THREAD_ANIMATION_TASK");
   }
-  mCompletedTasks.clear();
+  return false;
+}
 
-  // pop out the next task from the queue
-  for(auto it = mAnimationTasks.begin(); it != mAnimationTasks.end();)
+/// VectorAnimationThread called
+void VectorAnimationThread::MoveTasksToCompleted(VectorAnimationTaskPtr task, bool keepAnimation)
+{
+  if(DALI_LIKELY(!mDestroyThread))
   {
-    VectorAnimationTaskPtr nextTask = *it;
-
-    auto currentTime   = std::chrono::steady_clock::now();
-    auto nextFrameTime = nextTask->GetNextFrameTime();
+    DALI_TRACE_BEGIN_WITH_MESSAGE_GENERATOR(gTraceFilter, "VECTOR_ANIMATION_THREAD_COMPLETED_TASK", [&](std::ostringstream& oss) {
+      oss << "[w:" << mWorkingTasks.size() << ",c:" << mCompletedTasks.size() << "]";
+    });
+    bool needRasterize = false;
 
-#if defined(DEBUG_ENABLED)
-//    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(nextFrameTime - currentTime);
+    auto workingTask = std::find(mWorkingTasks.begin(), mWorkingTasks.end(), task);
+    if(workingTask != mWorkingTasks.end())
+    {
+      mWorkingTasks.erase(workingTask);
+    }
 
-//    DALI_LOG_INFO(gVectorAnimationLogFilter, Debug::Verbose, "VectorAnimationThread::Rasterize: [next time = %lld]\n", duration.count());
-#endif
-    if(nextFrameTime <= currentTime)
+    // Check pending task
     {
-      // If the task is not in the working list
-      if(std::find(mWorkingTasks.begin(), mWorkingTasks.end(), nextTask) == mWorkingTasks.end())
+      Mutex::ScopedLock animationTasksLock(mAnimationTasksMutex);
+      if(mAnimationTasks.end() != std::find(mAnimationTasks.begin(), mAnimationTasks.end(), task))
       {
-        it = mAnimationTasks.erase(it);
+        needRasterize = true;
+      }
+    }
 
-        // Add it to the working list
-        mWorkingTasks.push_back(nextTask);
-        mAsyncTaskManager.AddTask(nextTask);
+    if(keepAnimation)
+    {
+      if(mCompletedTasks.end() == std::find(mCompletedTasks.begin(), mCompletedTasks.end(), task))
+      {
+        mCompletedTasks.push_back(task);
+        needRasterize = true;
       }
-      else
+    }
+
+    if(needRasterize)
+    {
+      mNeedToSleep = false;
+    }
+
+    DALI_TRACE_END_WITH_MESSAGE_GENERATOR(gTraceFilter, "VECTOR_ANIMATION_THREAD_COMPLETED_TASK", [&](std::ostringstream& oss) {
+      oss << "[w:" << mWorkingTasks.size() << ",c:" << mCompletedTasks.size() << ",r?" << needRasterize << ",s?" << mNeedToSleep << "]";
+    });
+  }
+}
+
+/// VectorAnimationThread called
+void VectorAnimationThread::Rasterize()
+{
+  // Lock while popping task out from the queue
+  ConditionalWait::ScopedLock lock(mConditionalWait);
+
+  // conditional wait
+  while(DALI_LIKELY(!mDestroyThread) && mNeedToSleep)
+  {
+    // ConditionalWait notifyed when sleep done, or task complete.
+    // If task complete, then we need to re-validate the tasks container, and then sleep again.
+    decltype(mCompletedTasksQueue) completedTasksQueue;
+    {
+      Mutex::ScopedLock taskCompletedLock(mTaskCompletedMutex);
+      completedTasksQueue.swap(mCompletedTasksQueue);
+    }
+    if(completedTasksQueue.empty())
+    {
+      mConditionalWait.Wait(lock);
+      // Task completed may have been added to the queue while we were waiting.
       {
-        it++;
+        Mutex::ScopedLock taskCompletedLock(mTaskCompletedMutex);
+        completedTasksQueue.swap(mCompletedTasksQueue);
       }
     }
-    else
+
+    for(auto&& taskPair : completedTasksQueue)
     {
-      mSleepThread.SleepUntil(nextFrameTime);
-      break;
+      auto& task          = taskPair.first;
+      bool  keepAnimation = taskPair.second;
+      MoveTasksToCompleted(task, keepAnimation);
+    }
+  }
+
+  mNeedToSleep = true;
+
+  // Process completed tasks
+  for(auto&& task : mCompletedTasks)
+  {
+    // Should use the frame rate of the animation file
+    MoveTasksToAnimation(task, false);
+  }
+  mCompletedTasks.clear();
+
+  {
+    Mutex::ScopedLock animationTasksLock(mAnimationTasksMutex);
+    if(DALI_LIKELY(!mDestroyThread))
+    {
+      if(mAnimationTasks.size() > 0u)
+      {
+        DALI_TRACE_BEGIN_WITH_MESSAGE_GENERATOR(gTraceFilter, "VECTOR_ANIMATION_THREAD_ANIMATION_TASK2", [&](std::ostringstream& oss) {
+          oss << "[" << mAnimationTasks.size() << "]";
+        });
+
+        // pop out the next task from the queue
+        for(auto it = mAnimationTasks.begin(); it != mAnimationTasks.end();)
+        {
+          VectorAnimationTaskPtr nextTask = *it;
+
+          auto currentTime   = std::chrono::steady_clock::now();
+          auto nextFrameTime = nextTask->GetNextFrameTime();
+
+          if(nextFrameTime <= currentTime)
+          {
+            // If the task is not in the working list
+            if(std::find(mWorkingTasks.begin(), mWorkingTasks.end(), nextTask) == mWorkingTasks.end())
+            {
+              it = mAnimationTasks.erase(it);
+
+              // Add it to the working list
+              mWorkingTasks.push_back(nextTask);
+              mAsyncTaskManager.AddTask(nextTask);
+            }
+            else
+            {
+              it++;
+            }
+          }
+          else
+          {
+            mSleepThread.SleepUntil(nextFrameTime);
+            break;
+          }
+        }
+        DALI_TRACE_END_WITH_MESSAGE_GENERATOR(gTraceFilter, "VECTOR_ANIMATION_THREAD_ANIMATION_TASK2", [&](std::ostringstream& oss) {
+          oss << "[" << mAnimationTasks.size() << "]";
+        });
+      }
     }
   }
 }
 
+/// Event thread called (Due to mTrigger triggered)
 void VectorAnimationThread::OnEventCallbackTriggered()
 {
   while(true)
@@ -316,6 +406,7 @@ void VectorAnimationThread::OnEventCallbackTriggered()
   }
 }
 
+/// Event thread called
 std::pair<CallbackBase*, uint32_t> VectorAnimationThread::GetNextEventCallback()
 {
   Mutex::ScopedLock lock(mEventTriggerMutex);
@@ -346,16 +437,20 @@ VectorAnimationThread::SleepThread::SleepThread(CallbackBase* callback)
 
 VectorAnimationThread::SleepThread::~SleepThread()
 {
+  DALI_TRACE_SCOPE(gTraceFilter, "VECTOR_ANIMATION_SLEEP_THREAD_DESTROY");
+
   // Stop the thread
   {
     ConditionalWait::ScopedLock lock(mConditionalWait);
     mDestroyThread = true;
+    mAwakeCallback.reset();
     mConditionalWait.Notify(lock);
   }
 
   Join();
 }
 
+/// VectorAnimationThread called
 void VectorAnimationThread::SleepThread::SleepUntil(std::chrono::time_point<std::chrono::steady_clock> timeToSleepUntil)
 {
   ConditionalWait::ScopedLock lock(mConditionalWait);
@@ -386,11 +481,7 @@ void VectorAnimationThread::SleepThread::Run()
 
     if(needToSleep)
     {
-#if defined(DEBUG_ENABLED)
-//      auto sleepDuration = std::chrono::duration_cast<std::chrono::milliseconds>(mSleepTimePoint - std::chrono::steady_clock::now());
-
-//      DALI_LOG_INFO(gVectorAnimationLogFilter, Debug::Verbose, "VectorAnimationThread::SleepThread::Run: [sleep duration = %lld]\n", sleepDuration.count());
-#endif
+      DALI_TRACE_SCOPE(gTraceFilter, "VECTOR_ANIMATION_SLEEP_THREAD");
 
       std::this_thread::sleep_until(sleepTimePoint);
 
@@ -404,6 +495,7 @@ void VectorAnimationThread::SleepThread::Run()
       ConditionalWait::ScopedLock lock(mConditionalWait);
       if(!mDestroyThread && !mNeedToSleep)
       {
+        DALI_TRACE_SCOPE(gTraceFilter, "VECTOR_ANIMATION_SLEEP_THREAD_WAIT");
         mConditionalWait.Wait(lock);
       }
     }
index 8b820cc..ac5053b 100644 (file)
@@ -103,6 +103,18 @@ protected:
 
 private:
   /**
+   * @brief Move given tasks to mAnimationTasks if required.
+   * @return True if task added. False if not.
+   */
+  bool MoveTasksToAnimation(VectorAnimationTaskPtr task, bool useCurrentTime);
+
+  /**
+   * @brief Move given tasks to mCompletedTasks if required.
+   */
+  void MoveTasksToCompleted(VectorAnimationTaskPtr task, bool keepAnimation);
+
+private:
+  /**
    * @brief Rasterizes the tasks.
    */
   void Rasterize();
@@ -167,13 +179,19 @@ private:
   VectorAnimationThread& operator=(const VectorAnimationThread& thread) = delete;
 
 private:
-  std::vector<VectorAnimationTaskPtr>             mAnimationTasks;
-  std::vector<VectorAnimationTaskPtr>             mCompletedTasks;
-  std::vector<VectorAnimationTaskPtr>             mWorkingTasks;
+  std::vector<VectorAnimationTaskPtr> mAnimationTasks; ///< Animation processing tasks, ordered by next frame time.
+  std::vector<VectorAnimationTaskPtr> mCompletedTasks; ///< Temperal storage for completed tasks.
+  std::vector<VectorAnimationTaskPtr> mWorkingTasks;
+
+  std::vector<std::pair<VectorAnimationTaskPtr, bool>> mCompletedTasksQueue; ///< Queue of completed tasks from worker thread. pair of task, and rasterize required.
+                                                                             ///< It will be moved at begin of Rasterize().
+
   std::vector<std::pair<CallbackBase*, uint32_t>> mTriggerEventCallbacks{}; // Callbacks are not owned
   SleepThread                                     mSleepThread;
   ConditionalWait                                 mConditionalWait;
   Mutex                                           mEventTriggerMutex;
+  Mutex                                           mAnimationTasksMutex; ///< Mutex to change + get mAnimationTasks from event thread
+  Mutex                                           mTaskCompletedMutex;  ///< Mutex to collect completed tasks to mCompletedTasksQueue from worker threads
   std::unique_ptr<EventThreadCallback>            mEventTrigger{};
   const Dali::LogFactoryInterface&                mLogFactory;
   const Dali::TraceFactoryInterface&              mTraceFactory;