Prevent VideoSync from setting up in Destructor

[peak3d]

Description

Race conditions can lead to setup VideoSync while destructing VideoReferenceClock.
This PR prevents setting up VideoSync in VideoReferenceClock::Process() when it is destructing.

Motivation and Context

Kodi stalls sometimes when stopping video playback in waiting for VRC::join().
Hard kill required.

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

[fritsch]

Process is waiting until m_vsyncStopEvent is triggered. Here it checks only for SetupSuccess but as m_bStop is still false, it just continues in the while loop and creates another vsync object.

Why not calling: StopThread(); which is not waiting when run without parameter and just calling m_vsyncStopEvent.Set() afterwards?

Bad idea?

[fritsch]

Answering myself:

xbmc/xbmc/cores/VideoPlayer/VideoReferenceClock.cpp

Line 76 in c277a9b

CSingleLock SingleLock(m_CritSection);

is holding the lock, so that the process is not destroyed as long as the videosync is active. Then I don't see another solution than misusing m_bStop.

Though the design is not good :-(

[peak3d]

Yes, the design is really bad. StopThread without waiting (false param) leads to the issue that dtor destroys mutexes which are still waited for (in VideoSync::Run()). There is a synchronization implementation missing to sync Process loop with other threads.

[fritsch]

I posted some code on slack.

a) Setting member
b) Signalling Eventloop
c) StopThread(true);

might work.

[fritsch]

https://paste.ubuntu.com/p/TgFQMfSgzM/

(One could argue if the m_askedtoStop should be checked at the beginning as well in the while loop)

[fritsch]

All proposed solutions are racey. Fireing of the even might be done when m_pVideoSync is not in Run state, therefore it won't fix the root cause, e.g. "synced stop of a potentiall running clock" and shutdown of the thread.

[fritsch]

And final try without redesign:

diff --git a/xbmc/cores/VideoPlayer/VideoReferenceClock.cpp b/xbmc/cores/VideoPlayer/VideoReferenceClock.cpp
index 5977ee4ace..0c17a6de3a 100644
--- a/xbmc/cores/VideoPlayer/VideoReferenceClock.cpp
+++ b/xbmc/cores/VideoPlayer/VideoReferenceClock.cpp
@@ -31,14 +31,16 @@ CVideoReferenceClock::CVideoReferenceClock() : CThread("RefClock")
   m_RefreshRate = 0.0;
   m_MissedVblanks = 0;
   m_VblankTime = 0;
+  m_vsyncStopEvent.Reset();
 
   Start();
 }
 
 CVideoReferenceClock::~CVideoReferenceClock()
 {
+  m_bStop = true;
   m_vsyncStopEvent.Set();
-  StopThread();
+  StopThread(true);
 }
 
 void CVideoReferenceClock::Start()
@@ -87,9 +89,13 @@ void CVideoReferenceClock::Process()
       m_VblankTime = Now;          //initialize the timestamp of the last vblank
       SingleLock.Leave();
 
-      m_vsyncStopEvent.Reset();
-      //run the clock
-      m_pVideoSync->Run(m_vsyncStopEvent);
+      // we might got signalled while we did not wait
+      if (!m_vsyncStopEvent.Signaled())
+      {
+        //run the clock
+        m_pVideoSync->Run(m_vsyncStopEvent);
+        m_vsyncStopEvent.Reset();
+      }
     }
     else
     {

[peak3d]

@fritsch StopThread default is "true", did you change it just for readability?

-  StopThread();
+  StopThread(true);

[fritsch]

Jep. Ist was late.