From 8987f133295e352c44e05ef338eacc801c61a629 Mon Sep 17 00:00:00 2001 From: Carl Hetherington Date: Thu, 26 May 2022 20:04:33 +0200 Subject: [PATCH] Fix race between the Butler thread starting and audio (perhaps) being disabled. This could cause Butler::audio to be called with _audio_channels = 0 and _disable_audio = false, causing an exception in AudioBuffers when remap() tried to make an AudioBuffers object with a channel count of 0. --- src/lib/butler.cc | 13 +++---------- src/lib/butler.h | 11 ++++++++--- src/lib/ffmpeg_encoder.cc | 11 ++++++++++- src/wx/film_viewer.cc | 7 ++----- test/butler_test.cc | 24 ++++++++++++++++++++++-- test/dcp_playback_test.cc | 3 ++- test/player_test.cc | 16 ++++++++++------ test/threed_test.cc | 4 +++- 8 files changed, 60 insertions(+), 29 deletions(-) diff --git a/src/lib/butler.cc b/src/lib/butler.cc index 27b39ba4f..ce35b1f39 100644 --- a/src/lib/butler.cc +++ b/src/lib/butler.cc @@ -69,7 +69,8 @@ Butler::Butler ( VideoRange video_range, Image::Alignment alignment, bool fast, - bool prepare_only_proxy + bool prepare_only_proxy, + Audio audio ) : _film (film) , _player (player) @@ -81,7 +82,7 @@ Butler::Butler ( , _stop_thread (false) , _audio_mapping (audio_mapping) , _audio_channels (audio_channels) - , _disable_audio (false) + , _disable_audio (audio == Audio::DISABLED) , _pixel_format (pixel_format) , _video_range (video_range) , _alignment (alignment) @@ -394,14 +395,6 @@ Butler::get_audio (Behaviour behaviour, float* out, Frame frames) } -void -Butler::disable_audio () -{ - boost::mutex::scoped_lock lm (_mutex); - _disable_audio = true; -} - - pair Butler::memory_used () const { diff --git a/src/lib/butler.h b/src/lib/butler.h index c7e71658d..79701d370 100644 --- a/src/lib/butler.h +++ b/src/lib/butler.h @@ -38,6 +38,12 @@ class PlayerVideo; class Butler : public ExceptionStore { public: + enum class Audio + { + ENABLED, + DISABLED + }; + Butler ( std::weak_ptr film, std::shared_ptr player, @@ -47,7 +53,8 @@ public: VideoRange video_range, Image::Alignment alignment, bool fast, - bool prepare_only_proxy + bool prepare_only_proxy, + Audio audio ); ~Butler (); @@ -81,8 +88,6 @@ public: boost::optional get_audio (Behaviour behaviour, float* out, Frame frames); boost::optional get_closed_caption (); - void disable_audio (); - std::pair memory_used () const; private: diff --git a/src/lib/ffmpeg_encoder.cc b/src/lib/ffmpeg_encoder.cc index d4f0b4b47..bcf2b82de 100644 --- a/src/lib/ffmpeg_encoder.cc +++ b/src/lib/ffmpeg_encoder.cc @@ -108,7 +108,16 @@ FFmpegEncoder::FFmpegEncoder ( } _butler = std::make_shared( - _film, _player, map, _output_audio_channels, bind(&PlayerVideo::force, FFmpegFileEncoder::pixel_format(format)), VideoRange::VIDEO, Image::Alignment::PADDED, false, false + _film, + _player, + map, + _output_audio_channels, + bind(&PlayerVideo::force, FFmpegFileEncoder::pixel_format(format)), + VideoRange::VIDEO, + Image::Alignment::PADDED, + false, + false, + Butler::Audio::ENABLED ); } diff --git a/src/wx/film_viewer.cc b/src/wx/film_viewer.cc index 510269793..b5b2ca972 100644 --- a/src/wx/film_viewer.cc +++ b/src/wx/film_viewer.cc @@ -228,13 +228,10 @@ FilmViewer::recreate_butler () VideoRange::FULL, j2k_gl_optimised ? Image::Alignment::COMPACT : Image::Alignment::PADDED, true, - j2k_gl_optimised + j2k_gl_optimised, + (Config::instance()->sound() && _audio.isStreamOpen()) ? Butler::Audio::ENABLED : Butler::Audio::DISABLED ); - if (!Config::instance()->sound() && !_audio.isStreamOpen()) { - _butler->disable_audio (); - } - _closed_captions_dialog->set_butler (_butler); resume (); diff --git a/test/butler_test.cc b/test/butler_test.cc index 0422b89ad..7fd5ea571 100644 --- a/test/butler_test.cc +++ b/test/butler_test.cc @@ -60,7 +60,18 @@ BOOST_AUTO_TEST_CASE (butler_test1) map.set (i, i, 1); } - Butler butler (film, make_shared(film, Image::Alignment::COMPACT), map, 6, boost::bind(&PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::COMPACT, false, false); + Butler butler ( + film, + make_shared(film, Image::Alignment::COMPACT), + map, + 6, + boost::bind(&PlayerVideo::force, AV_PIX_FMT_RGB24), + VideoRange::FULL, + Image::Alignment::COMPACT, + false, + false, + Butler::Audio::ENABLED + ); BOOST_CHECK (butler.get_video(Butler::Behaviour::BLOCKING, 0).second == DCPTime()); BOOST_CHECK (butler.get_video(Butler::Behaviour::BLOCKING, 0).second == DCPTime::from_frames(1, 24)); @@ -95,7 +106,16 @@ BOOST_AUTO_TEST_CASE (butler_test2) } Butler butler ( - film, make_shared(film, Image::Alignment::COMPACT), map, 6, boost::bind(&PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::COMPACT, false, false + film, + make_shared(film, Image::Alignment::COMPACT), + map, + 6, + boost::bind(&PlayerVideo::force, AV_PIX_FMT_RGB24), + VideoRange::FULL, + Image::Alignment::COMPACT, + false, + false, + Butler::Audio::ENABLED ); int const audio_frames_per_video_frame = 48000 / 25; diff --git a/test/dcp_playback_test.cc b/test/dcp_playback_test.cc index 4cdb9f897..62c72cc84 100644 --- a/test/dcp_playback_test.cc +++ b/test/dcp_playback_test.cc @@ -52,7 +52,8 @@ BOOST_AUTO_TEST_CASE (dcp_playback_test) VideoRange::FULL, Image::Alignment::PADDED, true, - false + false, + Butler::Audio::ENABLED ); std::vector audio_buffer(2000 * 6); diff --git a/test/player_test.cc b/test/player_test.cc index 03f2eb6e6..e78e0ee35 100644 --- a/test/player_test.cc +++ b/test/player_test.cc @@ -233,8 +233,9 @@ BOOST_AUTO_TEST_CASE (player_seek_test) player->set_always_burn_open_subtitles (); player->set_play_referenced (); - auto butler = std::make_shared(film, player, AudioMapping(), 2, bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false); - butler->disable_audio(); + auto butler = std::make_shared( + film, player, AudioMapping(), 2, bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false, Butler::Audio::DISABLED + ); for (int i = 0; i < 10; ++i) { auto t = DCPTime::from_frames (i, 24); @@ -265,8 +266,9 @@ BOOST_AUTO_TEST_CASE (player_seek_test2) player->set_always_burn_open_subtitles (); player->set_play_referenced (); - auto butler = std::make_shared(film, player, AudioMapping(), 2, bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false); - butler->disable_audio(); + auto butler = std::make_shared + (film, player, AudioMapping(), 2, bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false, Butler::Audio::DISABLED + ); butler->seek(DCPTime::from_seconds(5), true); @@ -356,7 +358,9 @@ BOOST_AUTO_TEST_CASE (player_trim_crash) auto player = std::make_shared(film, Image::Alignment::COMPACT); player->set_fast (); - auto butler = std::make_shared(film, player, AudioMapping(), 6, bind(&PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::COMPACT, true, false); + auto butler = std::make_shared( + film, player, AudioMapping(), 6, bind(&PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::COMPACT, true, false, Butler::Audio::ENABLED + ); /* Wait for the butler to fill */ dcpomatic_sleep_seconds (5); @@ -486,7 +490,7 @@ BOOST_AUTO_TEST_CASE (encrypted_dcp_with_no_kdm_gives_no_butler_error) auto film2 = new_test_film2 ("encrypted_dcp_with_no_kdm_gives_no_butler_error2", { content2 }); auto player = std::make_shared(film2, Image::Alignment::COMPACT); - Butler butler(film2, player, AudioMapping(), 2, bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false); + Butler butler(film2, player, AudioMapping(), 2, bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false, Butler::Audio::ENABLED); float buffer[2000 * 6]; for (int i = 0; i < length; ++i) { diff --git a/test/threed_test.cc b/test/threed_test.cc index b4599cf80..4413ff01b 100644 --- a/test/threed_test.cc +++ b/test/threed_test.cc @@ -272,7 +272,9 @@ BOOST_AUTO_TEST_CASE (threed_test_butler_overfill) auto player = std::make_shared(film, Image::Alignment::COMPACT); int const audio_channels = 2; - auto butler = std::make_shared(film, player, AudioMapping(), audio_channels, boost::bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false); + auto butler = std::make_shared( + film, player, AudioMapping(), audio_channels, boost::bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false, Butler::Audio::ENABLED + ); int const audio_frames = 1920; std::vector audio(audio_frames * audio_channels); -- 2.30.2