Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#8854 closed defect (Fixed)

Locking problem in videobuffers WaitForAvailable()

Reported by: tomi.orava@… Owned by: danielk
Priority: minor Milestone: 0.24
Component: MythTV - General Version: Master Head
Severity: medium Keywords: Timed out waiting for free video buffers.
Cc: Ticket locked: no

Description

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

Change History (7)

comment:1 Changed 9 years ago by robertm

Owner: set to markk
Status: newassigned

Tomi,

Thanks a lot for the ticket and detailed analysis.

comment:2 Changed 9 years ago by markk

Owner: changed from markk to danielk

comment:3 Changed 9 years ago by danielk

Milestone: unknown0.25

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.

comment:4 Changed 9 years ago by beirdo

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

Last edited 9 years ago by beirdo (previous) (diff)

comment:5 Changed 9 years ago by markk

Resolution: Fixed
Status: assignedclosed

(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

comment:6 Changed 9 years ago by stuartm

Milestone: 0.25

Milestone 0.25 deleted

comment:7 Changed 9 years ago by stuartm

Milestone: 0.250.24
Note: See TracTickets for help on using tickets.