Fix deadlock during content examination.
authorCarl Hetherington <cth@carlh.net>
Tue, 28 Jan 2020 22:23:47 +0000 (23:23 +0100)
committerCarl Hetherington <cth@carlh.net>
Sun, 16 Feb 2020 20:23:42 +0000 (21:23 +0100)
Before this fix, the following situation could happen in threads
A and B:

A: Some DONE signal happens; this triggers setup_pieces which
   takes a lock on the player mutex.

B: FFmpegContent::examine takes a lock on the content mutex.
B: FFmpegContent::examine adds a stream
B: That causes STREAMS PENDING to be emitted.
B: This tries to take a lock on the player mutex so it can update _suspended

A: setup_pieces tries to access some content information, hence
   tries to take a lock on the content mutex.

Now B is holding the CL and awaiting the PL and A is holding
the PL and awaiting the CL.

It feels like the root cause of this is that while setup_pieces
is happening another change (which would itself cause setup_pieces)
is announced, and this isn't dealt with properly.

There are two steps here; _suspended is protected with an atomic
rather than using _mutex, and also it can cope with being updated
recursively.

Backported from df48c75c38dd788835a93540aea243a2dac4bb10 in v2.15.x

src/lib/player.cc
src/lib/player.h

index 30b49d186db67b2492c333644f75dc03c07624d6..1d83b9b5a64122efc02b7e627c03e8f913d5d5dd 100644 (file)
@@ -85,7 +85,7 @@ int const PlayerProperty::DCP_DECODE_REDUCTION = 704;
 Player::Player (shared_ptr<const Film> film, shared_ptr<const Playlist> playlist)
        : _film (film)
        , _playlist (playlist)
-       , _suspended (false)
+       , _suspended (0)
        , _ignore_video (false)
        , _ignore_audio (false)
        , _ignore_text (false)
@@ -237,19 +237,17 @@ void
 Player::playlist_content_change (ChangeType type, int property, bool frequent)
 {
        if (type == CHANGE_TYPE_PENDING) {
-               boost::mutex::scoped_lock lm (_mutex);
                /* The player content is probably about to change, so we can't carry on
                   until that has happened and we've rebuilt our pieces.  Stop pass()
                   and seek() from working until then.
                */
-               _suspended = true;
+               ++_suspended;
        } else if (type == CHANGE_TYPE_DONE) {
                /* A change in our content has gone through.  Re-build our pieces. */
                setup_pieces ();
-               _suspended = false;
+               --_suspended;
        } else if (type == CHANGE_TYPE_CANCELLED) {
-               boost::mutex::scoped_lock lm (_mutex);
-               _suspended = false;
+               --_suspended;
        }
 
        Change (type, property, frequent);
index 8a81146c9547fb7d7612dc59e38d3f1b0d289880..65ab1279f7e3c986d528c61c3e8bdc4429575efe 100644 (file)
@@ -145,8 +145,8 @@ private:
        boost::shared_ptr<const Film> _film;
        boost::shared_ptr<const Playlist> _playlist;
 
-       /** true if we are suspended (i.e. pass() and seek() do nothing) */
-       bool _suspended;
+       /** > 0 if we are suspended (i.e. pass() and seek() do nothing) */
+       boost::atomic<int> _suspended;
        std::list<boost::shared_ptr<Piece> > _pieces;
 
        /** Size of the image in the DCP (e.g. 1990x1080 for flat) */