Possible deadlock with packet feeding

[Ceylo]

The player can get in the following situation:

• main thread wants to preload the first image, which asks the video stream to fetch raw packets (video stream packet queue get locked) although the Demuxer is already fulfilling another stream's request (so waiting for the mutex to unlock)
• audio stream is buffering in its own thread (and locks its packet queue), ask for the demuxer to fetch data (demuxer gets locked). The demuxer reads packets, but it finds one that belong to the video stream, so it tries to deliver the packet, but the queue of the video stream is already locked (blocked).

So:

• main thread has locked the video stream queue and can't continue because the Demuxer is locked
• second thread has locked the Demuxer and can't continue because the video stream queue is locked

[bluekirby0]

If you aren't trying to avoid C++11 then consider the use of atomic ring buffers. As long as you only have one producer thread and one consumer thread for each buffer this should allow you to remove the need for any mutex on the stream buffers.

Turning the demuxer access into a lock-free operation could be a bit trickier...but would probably be unnecessary if this is the only deadlock condition you are coming across. By making all buffer i/o (for all intents and purposes) atomic you've removed the possibility of hitting this particular problem.

As a bonus, this method produces far less latency than mutexes which can be helpful for smooth playback of high bitrate (or lossless) video compressed into small chunks such as can be found in huffman, lagarith, or ffv1 codecs.

[Ceylo]

Can you explain how the ring buffer would avoid the deadlock?

As for C++11, for now I'm indeed avoiding it, but I wish to switch to it. However I don't want to do so on the fly: it must be done in an independent task that it doesn't introduce issues with dependencies like SFML and that it is nicely supported by our target platforms and compilers. Again I don't mind if we requires VS2013 for that though.

Apart from that, the idea I had in mind to fix the issue was to remove a level of locking. It would be done by sending packets only to the stream that requested them. The other packets found by the demuxer would be saved by the demuxer instead of being delivered to the other relevant streams.

[bluekirby0]

The key is with atomic operations. An atomic operation is one that must be executed from start to finish without surrendering any CPU cycles (generally because the operation is simple enough to be performed in a single instruction, but there are some more complex atomic operations). The advantage here is that atomic operations cannot be interrupted, thus negating the need for a mutex.

Since you would be going from 3 mutexes to 1, the conditions that made this deadlock possible could no longer occur, because reads and writes to the buffers would become atomic, lock-free operations.

The downside here is without using C++11's std::atomic<> it is impossible to guarantee that any piece of code will be compiled in a way that makes access to any particular type atomic. Generally, reads and writes for integer types up to the size of the CPU's register are atomic when not compiled with debugging options enabled, but even that is not guaranteed without atomic types.

That said, std::atomic<> can be used to declare ANYTHING as an atomic type...but that does not mean that everything can be given atomic access. What happens when you assign something that cannot be made atomic with std::atomic<> is it removes the burden of mutex handling from the coder and gives it to the preprocessor...thus giving you "automatic" thread-safe handling of any non-pointer type (pointers are safe too unless you dereference them of course).

[Ceylo]

I'm not sure I understand. At which level would you put the atomic ring buffer(s)? In Stream or Demuxer? Can you describe the execution flow that would occur between the Streams and the Demuxer?

[Ceylo]

Replacing VideoStream::update() implementation with the following piece of code:

void VideoStream::update()
{
    if (getStatus() == Playing)
    {
        while (getSynchronizationGap() < sf::Time::Zero)
        {
            if (!onGetData(m_texture))
            {
                setStatus(Stopped);
            }
            else
            {
                if (getSynchronizationGap() >= sf::Time::Zero)
                    m_delegate.didUpdateVideo(*this, m_texture);
            }
        }
    }
}

makes this deadlock appear almost every time. Although the purpose of the change is only not to display frames when video playback is late.

Edit: actually the deadlock is only triggered on the seeking branch with this …

[Ceylo]

Fix confirmed to work in the the seeking branch.

[Ceylo]

Merged to master

[Ceylo]

This fix prevents subtitles from showing up. As packets are only distributed to their requester, and as the subtitle stream never requests packets, it never receives packets.

Passive streams should be handled in a different way: as they are never requesting packets, distributing packets to them even if they did not request them cannot cause deadlock.

Reopening this issue.

[Ceylo]

Now fixed and merged