From f4c07b9ce06a6d0b08be073bb793324d6117a905 Mon Sep 17 00:00:00 2001 From: Jacob Trimble Date: Tue, 20 Oct 2020 14:08:46 -0700 Subject: [PATCH] Work around non-deterministic tests. We currently have a bug about non-deterministic output in the MPD generator. This works around that bug by optionally doing everything in a single thread. This allows us to run manifest comparisons without making the major changes needed to add that feature. Issue #177 Change-Id: I10e1084dac77841220161fbd2575cdcb5c13c00e --- packager/app/job_manager.h | 8 ++- packager/app/packager_main.cc | 4 ++ packager/app/single_thread_job_manager.cc | 34 +++++++++++ packager/app/single_thread_job_manager.h | 32 ++++++++++ packager/app/test/packager_test.py | 47 ++------------- .../output.mpd | 20 +++---- .../output.mpd | 60 +++++++++---------- .../output.mpd | 24 ++++---- .../hls-only-dash-only-captions/output.mpd | 24 ++++---- .../output.mpd | 40 ++++++------- .../testdata/video-audio-webvtt/output.mpd | 20 +++---- .../vtt-text-to-mp4-with-ad-cues/output.mpd | 50 ++++++++-------- packager/packager.cc | 9 ++- packager/packager.gyp | 2 + packager/packager.h | 3 + 15 files changed, 213 insertions(+), 164 deletions(-) create mode 100644 packager/app/single_thread_job_manager.cc create mode 100644 packager/app/single_thread_job_manager.h diff --git a/packager/app/job_manager.h b/packager/app/job_manager.h index 09a7c20554..1f40e7116f 100644 --- a/packager/app/job_manager.h +++ b/packager/app/job_manager.h @@ -60,6 +60,8 @@ class JobManager { // fails or is cancelled. It can be NULL. explicit JobManager(std::unique_ptr sync_points); + virtual ~JobManager() = default; + // Create a new job entry by specifying the origin handler at the top of the // chain and a name for the thread. This will only register the job. To start // the job, you need to call |RunJobs|. @@ -68,12 +70,12 @@ class JobManager { // Initialize all registered jobs. If any job fails to initialize, this will // return the error and it will not be safe to call |RunJobs| as not all jobs // will be properly initialized. - Status InitializeJobs(); + virtual Status InitializeJobs(); // Run all registered jobs. Before calling this make sure that // |InitializedJobs| returned |Status::OK|. This call is blocking and will // block until all jobs exit. - Status RunJobs(); + virtual Status RunJobs(); // Ask all jobs to stop running. This call is non-blocking and can be used to // unblock a call to |RunJobs|. @@ -81,7 +83,7 @@ class JobManager { SyncPointQueue* sync_points() { return sync_points_.get(); } - private: + protected: JobManager(const JobManager&) = delete; JobManager& operator=(const JobManager&) = delete; diff --git a/packager/app/packager_main.cc b/packager/app/packager_main.cc index 9d5c8b7af4..b6f1add2e0 100644 --- a/packager/app/packager_main.cc +++ b/packager/app/packager_main.cc @@ -48,6 +48,9 @@ DEFINE_bool(use_fake_clock_for_muxer, DEFINE_string(test_packager_version, "", "Packager version for testing. Should be used for testing only."); +DEFINE_bool(single_threaded, + false, + "If enabled, only use one thread when generating content."); namespace shaka { namespace { @@ -311,6 +314,7 @@ base::Optional GetPackagingParams() { PackagingParams packaging_params; packaging_params.temp_dir = FLAGS_temp_dir; + packaging_params.single_threaded = FLAGS_single_threaded; AdCueGeneratorParams& ad_cue_generator_params = packaging_params.ad_cue_generator_params; diff --git a/packager/app/single_thread_job_manager.cc b/packager/app/single_thread_job_manager.cc new file mode 100644 index 0000000000..0d0c3a8b85 --- /dev/null +++ b/packager/app/single_thread_job_manager.cc @@ -0,0 +1,34 @@ +// Copyright 2020 Google LLLC All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +#include "packager/app/single_thread_job_manager.h" + +#include "packager/media/chunking/sync_point_queue.h" +#include "packager/media/origin/origin_handler.h" + +namespace shaka { +namespace media { + +SingleThreadJobManager::SingleThreadJobManager( + std::unique_ptr sync_points) + : JobManager(std::move(sync_points)) {} + +Status SingleThreadJobManager::InitializeJobs() { + Status status; + for (const JobEntry& job_entry : job_entries_) + status.Update(job_entry.worker->Initialize()); + return status; +} + +Status SingleThreadJobManager::RunJobs() { + Status status; + for (const JobEntry& job_entry : job_entries_) + status.Update(job_entry.worker->Run()); + return status; +} + +} // namespace media +} // namespace shaka diff --git a/packager/app/single_thread_job_manager.h b/packager/app/single_thread_job_manager.h new file mode 100644 index 0000000000..fa99e89428 --- /dev/null +++ b/packager/app/single_thread_job_manager.h @@ -0,0 +1,32 @@ +// Copyright 2020 Google LLLC All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +#ifndef PACKAGER_APP_SINGLE_THREAD_JOB_MANAGER_H_ +#define PACKAGER_APP_SINGLE_THREAD_JOB_MANAGER_H_ + +#include + +#include "packager/app/job_manager.h" + +namespace shaka { +namespace media { + +// A subclass of JobManager that runs all the jobs in a single thread. +class SingleThreadJobManager : public JobManager { + public: + // @param sync_points is an optional SyncPointQueue used to synchronize and + // align cue points. JobManager cancels @a sync_points when any job + // fails or is cancelled. It can be NULL. + explicit SingleThreadJobManager(std::unique_ptr sync_points); + + Status InitializeJobs() override; + Status RunJobs() override; +}; + +} // namespace media +} // namespace shaka + +#endif // PACKAGER_APP_SINGLE_THREAD_JOB_MANAGER_H_ diff --git a/packager/app/test/packager_test.py b/packager/app/test/packager_test.py index 3c83be4eb2..7ceb670fcf 100755 --- a/packager/app/test/packager_test.py +++ b/packager/app/test/packager_test.py @@ -477,7 +477,7 @@ class PackagerAppTest(unittest.TestCase): segment_duration=1.0, use_fake_clock=True, allow_codec_switching=False): - flags = [] + flags = ['--single_threaded'] if not strip_parameter_set_nalus: flags += ['--strip_parameter_set_nalus=false'] @@ -767,14 +767,7 @@ class PackagerFunctionalTest(PackagerAppTest): streams = audio_video_streams + dash_text_stream + hls_text_stream self.assertPackageSuccess(streams, self._GetFlags(output_dash=True, output_hls=True)) - # Mpd cannot be validated right now since we don't generate deterministic - # mpd with multiple inputs due to thread racing. - # TODO(b/73349711): Generate deterministic mpd or at least validate mpd - # schema. - self._CheckTestResults( - 'hls-only-dash-only-captions', - diff_files_policy=DiffFilesPolicy( - allowed_diff_files=['output.mpd'], exact=False)) + self._CheckTestResults('hls-only-dash-only-captions') def testDashOnlyAndHlsOnly(self): streams = [ @@ -1241,14 +1234,7 @@ class PackagerFunctionalTest(PackagerAppTest): flags = self._GetFlags(output_dash=True, output_hls=True, generate_static_live_mpd=True, ad_cues='1.5') self.assertPackageSuccess(streams, flags) - # Mpd cannot be validated right now since we don't generate determinstic - # mpd with multiple inputs due to thread racing. - # TODO(b/73349711): Generate determinstic mpd or at least validate mpd - # schema. - self._CheckTestResults( - 'vtt-text-to-mp4-with-ad-cues', - diff_files_policy=DiffFilesPolicy( - allowed_diff_files=['output.mpd'], exact=False)) + self._CheckTestResults('vtt-text-to-mp4-with-ad-cues') def testWebmSubsampleEncryption(self): streams = [ @@ -1621,15 +1607,7 @@ class PackagerFunctionalTest(PackagerAppTest): self.assertPackageSuccess(streams, self._GetFlags(output_dash=True, allow_codec_switching=True)) - # Mpd cannot be validated right now since we don't generate determinstic - # mpd with multiple inputs due to thread racing. - # TODO(b/73349711): Generate determinstic mpd or at least validate mpd - # schema. - # See also https://github.com/google/shaka-packager/issues/177. - self._CheckTestResults( - 'audio-video-with-codec-switching', - diff_files_policy=DiffFilesPolicy( - allowed_diff_files=['output.mpd'], exact=False)) + self._CheckTestResults('audio-video-with-codec-switching') def testAllowCodecSwitchingWithEncryptionAndTrickplay(self): streams = [ @@ -1645,15 +1623,8 @@ class PackagerFunctionalTest(PackagerAppTest): self._GetFlags(output_dash=True, allow_codec_switching=True, encryption=True)) - # Mpd cannot be validated right now since we don't generate determinstic - # mpd with multiple inputs due to thread racing. - # TODO(b/73349711): Generate determinstic mpd or at least validate mpd - # schema. - # See also https://github.com/google/shaka-packager/issues/177. self._CheckTestResults( - 'audio-video-with-codec-switching-encryption-trick-play', - diff_files_policy=DiffFilesPolicy( - allowed_diff_files=['output.mpd'], exact=False)) + 'audio-video-with-codec-switching-encryption-trick-play') def testLiveProfileAndEncryption(self): self.assertPackageSuccess( @@ -1675,14 +1646,8 @@ class PackagerFunctionalTest(PackagerAppTest): test_files=['bear-1280x720.mp4', 'bear-640x360.mp4', 'bear-320x180.mp4']), self._GetFlags(encryption=True, output_dash=True)) - # Mpd cannot be validated right now since we don't generate determinstic - # mpd with multiple inputs due to thread racing. - # TODO(b/73349711): Generate determinstic mpd or at least validate mpd - # schema. self._CheckTestResults( - 'live-profile-and-encryption-and-mult-files', - diff_files_policy=DiffFilesPolicy( - allowed_diff_files=['output.mpd'], exact=False)) + 'live-profile-and-encryption-and-mult-files') def testLiveProfileAndKeyRotation(self): self.assertPackageSuccess( diff --git a/packager/app/test/testdata/audio-video-with-accessibilities-and-roles/output.mpd b/packager/app/test/testdata/audio-video-with-accessibilities-and-roles/output.mpd index 4cc940fcee..c6de0c9286 100644 --- a/packager/app/test/testdata/audio-video-with-accessibilities-and-roles/output.mpd +++ b/packager/app/test/testdata/audio-video-with-accessibilities-and-roles/output.mpd @@ -2,24 +2,18 @@ - - - - bear-english-text.vtt - - - - + + bear-640x360-video.mp4 - + - + bear-640x360-audio.mp4 @@ -27,5 +21,11 @@ + + + + bear-english-text.vtt + + diff --git a/packager/app/test/testdata/audio-video-with-codec-switching-encryption-trick-play/output.mpd b/packager/app/test/testdata/audio-video-with-codec-switching-encryption-trick-play/output.mpd index 2893b62da0..e9c2a74c72 100644 --- a/packager/app/test/testdata/audio-video-with-codec-switching-encryption-trick-play/output.mpd +++ b/packager/app/test/testdata/audio-video-with-codec-switching-encryption-trick-play/output.mpd @@ -2,50 +2,50 @@ - + AAAANHBzc2gBAAAAEHfv7MCyTQKs4zweUuL7SwAAAAExMjM0NTY3ODkwMTIzNDU2AAAAAA== - + - - bear-640x360-hevc-video.mp4 - - + + bear-1280x720-video.mp4 + + + + + + bear-640x360-video.mp4 + + - + + + + AAAANHBzc2gBAAAAEHfv7MCyTQKs4zweUuL7SwAAAAExMjM0NTY3ODkwMTIzNDU2AAAAAA== + + + + bear-1280x720-video-trick_play_factor_1.mp4 + + + + + + AAAANHBzc2gBAAAAEHfv7MCyTQKs4zweUuL7SwAAAAExMjM0NTY3ODkwMTIzNDU2AAAAAA== - - bear-640x360-video.mp4 - - - - - - bear-1280x720-video.mp4 - - - - - - - - - AAAANHBzc2gBAAAAEHfv7MCyTQKs4zweUuL7SwAAAAExMjM0NTY3ODkwMTIzNDU2AAAAAA== - - - - bear-1280x720-video-trick_play_factor_1.mp4 - - + + bear-640x360-hevc-video.mp4 + + diff --git a/packager/app/test/testdata/audio-video-with-codec-switching/output.mpd b/packager/app/test/testdata/audio-video-with-codec-switching/output.mpd index 9241a6c2c8..f399d13106 100644 --- a/packager/app/test/testdata/audio-video-with-codec-switching/output.mpd +++ b/packager/app/test/testdata/audio-video-with-codec-switching/output.mpd @@ -2,20 +2,10 @@ - + - - bear-640x360-hevc-video.mp4 - - - - - - - - - + bear-1280x720-video.mp4 @@ -28,6 +18,16 @@ + + + + + bear-640x360-hevc-video.mp4 + + + + + diff --git a/packager/app/test/testdata/hls-only-dash-only-captions/output.mpd b/packager/app/test/testdata/hls-only-dash-only-captions/output.mpd index 6bb2ccbbd3..1644042a48 100644 --- a/packager/app/test/testdata/hls-only-dash-only-captions/output.mpd +++ b/packager/app/test/testdata/hls-only-dash-only-captions/output.mpd @@ -12,18 +12,8 @@ - - - - - - - - - - - - + + @@ -34,5 +24,15 @@ + + + + + + + + + + diff --git a/packager/app/test/testdata/live-profile-and-encryption-and-mult-files/output.mpd b/packager/app/test/testdata/live-profile-and-encryption-and-mult-files/output.mpd index bdf40745c9..c0180e6e66 100644 --- a/packager/app/test/testdata/live-profile-and-encryption-and-mult-files/output.mpd +++ b/packager/app/test/testdata/live-profile-and-encryption-and-mult-files/output.mpd @@ -15,14 +15,6 @@ - - - - - - - - @@ -31,13 +23,21 @@ + + + + + + + + AAAANHBzc2gBAAAAEHfv7MCyTQKs4zweUuL7SwAAAAExMjM0NTY3ODkwMTIzNDU2AAAAAA== - + @@ -47,17 +47,7 @@ - - - - - - - - - - - + @@ -67,6 +57,16 @@ + + + + + + + + + + diff --git a/packager/app/test/testdata/video-audio-webvtt/output.mpd b/packager/app/test/testdata/video-audio-webvtt/output.mpd index c9506e753f..2e57b85d86 100644 --- a/packager/app/test/testdata/video-audio-webvtt/output.mpd +++ b/packager/app/test/testdata/video-audio-webvtt/output.mpd @@ -2,22 +2,16 @@ - - - - bear-english-text.vtt - - - - + + bear-640x360-video.mp4 - - + + bear-640x360-audio.mp4 @@ -25,5 +19,11 @@ + + + + bear-english-text.vtt + + diff --git a/packager/app/test/testdata/vtt-text-to-mp4-with-ad-cues/output.mpd b/packager/app/test/testdata/vtt-text-to-mp4-with-ad-cues/output.mpd index 6e663cde8b..0f9f7cc45f 100644 --- a/packager/app/test/testdata/vtt-text-to-mp4-with-ad-cues/output.mpd +++ b/packager/app/test/testdata/vtt-text-to-mp4-with-ad-cues/output.mpd @@ -2,19 +2,8 @@ - - - - - - - - - - - - - + + @@ -22,8 +11,8 @@ - - + + @@ -33,20 +22,21 @@ - - - + - - + + - + + - - + + + + @@ -54,8 +44,8 @@ - - + + @@ -64,5 +54,15 @@ + + + + + + + + + + diff --git a/packager/packager.cc b/packager/packager.cc index 8d66640b97..43e0a0de71 100644 --- a/packager/packager.cc +++ b/packager/packager.cc @@ -12,6 +12,7 @@ #include "packager/app/libcrypto_threading.h" #include "packager/app/muxer_factory.h" #include "packager/app/packager_util.h" +#include "packager/app/single_thread_job_manager.h" #include "packager/app/stream_descriptor.h" #include "packager/base/at_exit.h" #include "packager/base/files/file_path.h" @@ -56,6 +57,7 @@ using media::Demuxer; using media::JobManager; using media::KeySource; using media::MuxerOptions; +using media::SingleThreadJobManager; using media::SyncPointQueue; namespace media { @@ -837,7 +839,12 @@ Status Packager::Initialize( sync_points.reset( new SyncPointQueue(packaging_params.ad_cue_generator_params)); } - internal->job_manager.reset(new JobManager(std::move(sync_points))); + if (packaging_params.single_threaded) { + internal->job_manager.reset( + new SingleThreadJobManager(std::move(sync_points))); + } else { + internal->job_manager.reset(new JobManager(std::move(sync_points))); + } std::vector streams_for_jobs; diff --git a/packager/packager.gyp b/packager/packager.gyp index 1ca81fa6bf..f3b20bf8fa 100644 --- a/packager/packager.gyp +++ b/packager/packager.gyp @@ -22,6 +22,8 @@ 'app/libcrypto_threading.h', 'app/packager_util.cc', 'app/packager_util.h', + 'app/single_thread_job_manager.cc', + 'app/single_thread_job_manager.h', 'packager.cc', 'packager.h', ], diff --git a/packager/packager.h b/packager/packager.h index 19d041ebd9..f4a9a6f502 100644 --- a/packager/packager.h +++ b/packager/packager.h @@ -53,6 +53,9 @@ struct PackagingParams { /// Create a human readable format of MediaInfo. The output file name will be /// the name specified by output flag, suffixed with `.media_info`. bool output_media_info = false; + /// Only use a single thread to generate output. This is useful in tests to + /// avoid non-deterministic outputs. + bool single_threaded = false; /// DASH MPD related parameters. MpdParams mpd_params; /// HLS related parameters.