Locking problem in videobuffers WaitForAvailable()

[tomi.orava@…]

The following code block is flawed by design in videobuffers.h:

bool WaitForAvailable?(uint w)

{

Qt4 requires a QMutex as a parameter... not sure if this is the best solution. Mutex Must be locked before wait. QMutex mutex; mutex.lock(); return available_wait.wait(&mutex, w);

}

The problem is of course that the mutex is completely bogus when defined inside the method. Also none of the calls to the available_wait.WakeAll?() have the same mutex locked and therefore it is very easy to have a race condition between threads where the available_wait.WakeAll?() signal is being missed by this method completely thus allowing the mythfrontend to hang completely with the following error message repeating itself over and over again:

010-09-02 21:06:52.733 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:06:54.763 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:06:56.800 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:06:58.824 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:07:00.848 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:07:02.870 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:07:04.894 Player(1): Timed out waiting for free video buffers. 2010-09-02 21:07:06.917 Player(1): Timed out waiting for free video buffers.

A pretty easy way to reproduce this problem is to rewind quickly and then press enter to continue playback. If the rewinding is short enough the frontend has a high chance of hanging at least on all the machines I have here.

The problematic code block (and problem) has existed for ages even in trunk and the current version I have is: r26022

[robertm]

Tomi,

Thanks a lot for the ticket and detailed analysis.

[danielk]

This dates back to at least 0.22 so it must be fairly uncommon to hit it. But obviously it should be fixed. The locking for whole the class needs to be reviewed. There should be a locking order and no recursive locking so we can use one of the existing locks for the wait condition.

[beirdo]

See also comment:21:ticket:8872 for another reported example in ThreadPool?.

[markk]

(In [26653]) Remove VideoBuffers::WaitForAvailable?

I've confirmed this is causing deadlocks in DVD playback. Given that it is only used in one place (MythPlayer::DecoderGetFrame?), I've replaced that usage with a simple loop. VideoBuffers? no doubt could use some additional internal locking but EnoughFreeFrames? has been in use unprotected for years, so there is probably no real risk.

Closes #8854

[stuartm]

Milestone 0.25 deleted