From df48c75c38dd788835a93540aea243a2dac4bb10 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Tue, 28 Jan 2020 23:23:47 +0100 Subject: [PATCH] Fix deadlock during content examination. 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. --- src/lib/player.cc | 10 ++++------ src/lib/player.h | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/lib/player.cc b/src/lib/player.cc index 657c1a8b4..62527e3eb 100644 --- a/src/lib/player.cc +++ b/src/lib/player.cc @@ -87,7 +87,7 @@ int const PlayerProperty::DCP_DECODE_REDUCTION = 704; Player::Player (shared_ptr film, shared_ptr playlist) : _film (film) , _playlist (playlist) - , _suspended (false) + , _suspended (0) , _ignore_video (false) , _ignore_audio (false) , _ignore_text (false) @@ -252,19 +252,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); diff --git a/src/lib/player.h b/src/lib/player.h index 2fd7c8668..68531cfb3 100644 --- a/src/lib/player.h +++ b/src/lib/player.h @@ -149,8 +149,8 @@ private: boost::shared_ptr _film; boost::shared_ptr _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 _suspended; std::list > _pieces; /** Size of the image in the DCP (e.g. 1990x1080 for flat) */ -- 2.30.2