From 981041a3a806a6c40c76b707c1e0278732037dc7 Mon Sep 17 00:00:00 2001 From: Rintaro Kuroiwa Date: Thu, 12 Nov 2015 13:40:52 -0800 Subject: [PATCH] Remove locks from MpdBuilder and lock at MpdNotifier level - Since most use cases use notifier implementations that already uses locks, removing locks from MpdBuilder. - Added locks to MpdNotifier::Flush() implementations. Multiple threads were able to write to the same file. Issue #45, #49 Change-Id: I6e39824485672f40e6c947da97f1743fac174167 --- packager/mpd/base/dash_iop_mpd_notifier.cc | 1 + packager/mpd/base/dash_iop_mpd_notifier.h | 1 + .../base/dash_iop_mpd_notifier_unittest.cc | 16 +++++++++++ packager/mpd/base/mock_mpd_builder.h | 1 + packager/mpd/base/mpd_builder.cc | 27 +++---------------- packager/mpd/base/mpd_builder.h | 6 ----- packager/mpd/base/simple_mpd_notifier.cc | 1 + .../mpd/base/simple_mpd_notifier_unittest.cc | 16 +++++++++++ 8 files changed, 39 insertions(+), 30 deletions(-) diff --git a/packager/mpd/base/dash_iop_mpd_notifier.cc b/packager/mpd/base/dash_iop_mpd_notifier.cc index d48b13e36e..f92664bd74 100644 --- a/packager/mpd/base/dash_iop_mpd_notifier.cc +++ b/packager/mpd/base/dash_iop_mpd_notifier.cc @@ -164,6 +164,7 @@ bool DashIopMpdNotifier::AddContentProtectionElement( } bool DashIopMpdNotifier::Flush() { + base::AutoLock auto_lock(lock_); return WriteMpdToFile(output_path_, mpd_builder_.get()); } diff --git a/packager/mpd/base/dash_iop_mpd_notifier.h b/packager/mpd/base/dash_iop_mpd_notifier.h index 9a13ddeede..d4dce450b8 100644 --- a/packager/mpd/base/dash_iop_mpd_notifier.h +++ b/packager/mpd/base/dash_iop_mpd_notifier.h @@ -14,6 +14,7 @@ #include #include +#include "packager/base/synchronization/lock.h" #include "packager/mpd/base/mpd_builder.h" #include "packager/mpd/base/mpd_notifier_util.h" #include "packager/mpd/base/mpd_options.h" diff --git a/packager/mpd/base/dash_iop_mpd_notifier_unittest.cc b/packager/mpd/base/dash_iop_mpd_notifier_unittest.cc index dd3b2b2e59..e1d7b6102d 100644 --- a/packager/mpd/base/dash_iop_mpd_notifier_unittest.cc +++ b/packager/mpd/base/dash_iop_mpd_notifier_unittest.cc @@ -760,6 +760,22 @@ TEST_P(DashIopMpdNotifierTest, UpdateEncryption) { container_id, "myuuid", std::vector(), kBogusNewPsshVector)); } +// This test is mainly for tsan. Using both the notifier and the MpdBuilder. +// Although locks in MpdBuilder have been removed, +// https://github.com/google/edash-packager/issues/45 +// This issue identified a bug where using SimpleMpdNotifier with multiple +// threads causes a deadlock. This tests with DashIopMpdNotifier. +TEST_F(DashIopMpdNotifierTest, NotifyNewContainerAndSampleDurationNoMock) { + DashIopMpdNotifier notifier(kOnDemandProfile, empty_mpd_option_, + empty_base_urls_, output_path_); + uint32_t container_id; + EXPECT_TRUE(notifier.NotifyNewContainer(ConvertToMediaInfo(kValidMediaInfo), + &container_id)); + const uint32_t kAnySampleDuration = 1000; + EXPECT_TRUE(notifier.NotifySampleDuration(container_id, kAnySampleDuration)); + EXPECT_TRUE(notifier.Flush()); +} + INSTANTIATE_TEST_CASE_P(StaticAndDynamic, DashIopMpdNotifierTest, ::testing::Values(MpdBuilder::kStatic, diff --git a/packager/mpd/base/mock_mpd_builder.h b/packager/mpd/base/mock_mpd_builder.h index 50f123ab3f..8753983d5a 100644 --- a/packager/mpd/base/mock_mpd_builder.h +++ b/packager/mpd/base/mock_mpd_builder.h @@ -10,6 +10,7 @@ #include #include "packager/base/compiler_specific.h" +#include "packager/base/synchronization/lock.h" #include "packager/mpd/base/content_protection_element.h" #include "packager/mpd/base/mpd_builder.h" diff --git a/packager/mpd/base/mpd_builder.cc b/packager/mpd/base/mpd_builder.cc index 795bb709a1..4ae15d98ff 100644 --- a/packager/mpd/base/mpd_builder.cc +++ b/packager/mpd/base/mpd_builder.cc @@ -401,16 +401,13 @@ MpdBuilder::MpdBuilder(MpdType type, const MpdOptions& mpd_options) MpdBuilder::~MpdBuilder() {} void MpdBuilder::AddBaseUrl(const std::string& base_url) { - base::AutoLock scoped_lock(lock_); base_urls_.push_back(base_url); } AdaptationSet* MpdBuilder::AddAdaptationSet(const std::string& lang) { - base::AutoLock scoped_lock(lock_); scoped_ptr adaptation_set( new AdaptationSet(adaptation_set_counter_.GetNext(), lang, mpd_options_, - type_, - &representation_counter_)); + type_, &representation_counter_)); DCHECK(adaptation_set); adaptation_sets_.push_back(adaptation_set.get()); @@ -418,13 +415,11 @@ AdaptationSet* MpdBuilder::AddAdaptationSet(const std::string& lang) { } bool MpdBuilder::WriteMpdToFile(media::File* output_file) { - base::AutoLock scoped_lock(lock_); DCHECK(output_file); return WriteMpdToOutput(output_file); } bool MpdBuilder::ToString(std::string* output) { - base::AutoLock scoped_lock(lock_); DCHECK(output); return WriteMpdToOutput(output); } @@ -669,7 +664,6 @@ AdaptationSet::AdaptationSet(uint32_t adaptation_set_id, AdaptationSet::~AdaptationSet() {} Representation* AdaptationSet::AddRepresentation(const MediaInfo& media_info) { - base::AutoLock scoped_lock(lock_); const uint32_t representation_id = representation_counter_->GetNext(); // Note that AdaptationSet outlive Representation, so this object // will die before AdaptationSet. @@ -715,14 +709,12 @@ Representation* AdaptationSet::AddRepresentation(const MediaInfo& media_info) { void AdaptationSet::AddContentProtectionElement( const ContentProtectionElement& content_protection_element) { - base::AutoLock scoped_lock(lock_); content_protection_elements_.push_back(content_protection_element); RemoveDuplicateAttributes(&content_protection_elements_.back()); } void AdaptationSet::UpdateContentProtectionPssh(const std::string& drm_uuid, const std::string& pssh) { - base::AutoLock scoped_lock(lock_); UpdateContentProtectionPsshHelper(drm_uuid, pssh, &content_protection_elements_); } @@ -734,7 +726,6 @@ void AdaptationSet::AddRole(Role role) { // Creates a copy of xml element, iterate thru all the // (child) elements and add them to the copy. xml::ScopedXmlPtr::type AdaptationSet::GetXml() { - base::AutoLock scoped_lock(lock_); AdaptationSetXmlNode adaptation_set; if (!adaptation_set.AddContentProtectionElements( @@ -827,8 +818,6 @@ int AdaptationSet::Group() const { void AdaptationSet::OnNewSegmentForRepresentation(uint32_t representation_id, uint64_t start_time, uint64_t duration) { - base::AutoLock scoped_lock(lock_); - if (mpd_type_ == MpdBuilder::kDynamic) { CheckLiveSegmentAlignment(representation_id, start_time, duration); } else { @@ -838,17 +827,15 @@ void AdaptationSet::OnNewSegmentForRepresentation(uint32_t representation_id, } void AdaptationSet::OnSetFrameRateForRepresentation( - uint32_t /* representation_id */, + uint32_t representation_id, uint32_t frame_duration, uint32_t timescale) { - base::AutoLock scoped_lock(lock_); RecordFrameRate(frame_duration, timescale); } bool AdaptationSet::GetEarliestTimestamp(double* timestamp_seconds) { DCHECK(timestamp_seconds); - base::AutoLock scoped_lock(lock_); double earliest_timestamp(-1); for (std::list::const_iterator iter = representations_.begin(); @@ -1064,14 +1051,12 @@ bool Representation::Init() { void Representation::AddContentProtectionElement( const ContentProtectionElement& content_protection_element) { - base::AutoLock scoped_lock(lock_); content_protection_elements_.push_back(content_protection_element); RemoveDuplicateAttributes(&content_protection_elements_.back()); } void Representation::UpdateContentProtectionPssh(const std::string& drm_uuid, const std::string& pssh) { - base::AutoLock scoped_lock(lock_); UpdateContentProtectionPsshHelper(drm_uuid, pssh, &content_protection_elements_); } @@ -1084,7 +1069,6 @@ void Representation::AddNewSegment(uint64_t start_time, return; } - base::AutoLock scoped_lock(lock_); if (state_change_listener_) state_change_listener_->OnNewSegmentForRepresentation(start_time, duration); if (IsContiguous(start_time, duration, size)) { @@ -1102,8 +1086,6 @@ void Representation::AddNewSegment(uint64_t start_time, } void Representation::SetSampleDuration(uint32_t sample_duration) { - base::AutoLock scoped_lock(lock_); - if (media_info_.has_video_info()) { media_info_.mutable_video_info()->set_frame_duration(sample_duration); if (state_change_listener_) { @@ -1120,8 +1102,6 @@ void Representation::SetSampleDuration(uint32_t sample_duration) { // AudioChannelConfig elements), AddContentProtectionElements*(), and // AddVODOnlyInfo() (Adds segment info). xml::ScopedXmlPtr::type Representation::GetXml() { - base::AutoLock scoped_lock(lock_); - if (!HasRequiredMediaInfoFields()) { LOG(ERROR) << "MediaInfo missing required fields."; return xml::ScopedXmlPtr::type(); @@ -1137,7 +1117,7 @@ xml::ScopedXmlPtr::type Representation::GetXml() { // Mandatory fields for Representation. representation.SetId(id_); representation.SetIntegerAttribute("bandwidth", bandwidth); - if (!codecs_.empty()) + if (!codecs_.empty()) representation.SetStringAttribute("codecs", codecs_); representation.SetStringAttribute("mimeType", mime_type_); @@ -1345,7 +1325,6 @@ std::string Representation::GetTextMimeType() const { bool Representation::GetEarliestTimestamp(double* timestamp_seconds) { DCHECK(timestamp_seconds); - base::AutoLock scoped_lock(lock_); if (segment_infos_.empty()) return false; diff --git a/packager/mpd/base/mpd_builder.h b/packager/mpd/base/mpd_builder.h index f96c718249..0ab972e095 100644 --- a/packager/mpd/base/mpd_builder.h +++ b/packager/mpd/base/mpd_builder.h @@ -25,7 +25,6 @@ #include "packager/base/atomic_sequence_num.h" #include "packager/base/gtest_prod_util.h" #include "packager/base/stl_util.h" -#include "packager/base/synchronization/lock.h" #include "packager/mpd/base/bandwidth_estimator.h" #include "packager/mpd/base/content_protection_element.h" #include "packager/mpd/base/media_info.pb.h" @@ -144,7 +143,6 @@ class MpdBuilder { std::list base_urls_; std::string availability_start_time_; - base::Lock lock_; base::AtomicSequenceNumber adaptation_set_counter_; base::AtomicSequenceNumber representation_counter_; @@ -333,8 +331,6 @@ class AdaptationSet { std::list representations_; ::STLElementDeleter > representations_deleter_; - base::Lock lock_; - base::AtomicSequenceNumber* const representation_counter_; const uint32_t id_; @@ -530,8 +526,6 @@ class Representation { std::list content_protection_elements_; std::list segment_infos_; - base::Lock lock_; - const uint32_t id_; std::string mime_type_; std::string codecs_; diff --git a/packager/mpd/base/simple_mpd_notifier.cc b/packager/mpd/base/simple_mpd_notifier.cc index 55202b6158..4049a33af6 100644 --- a/packager/mpd/base/simple_mpd_notifier.cc +++ b/packager/mpd/base/simple_mpd_notifier.cc @@ -129,6 +129,7 @@ bool SimpleMpdNotifier::AddContentProtectionElement( } bool SimpleMpdNotifier::Flush() { + base::AutoLock auto_lock(lock_); return WriteMpdToFile(output_path_, mpd_builder_.get()); } diff --git a/packager/mpd/base/simple_mpd_notifier_unittest.cc b/packager/mpd/base/simple_mpd_notifier_unittest.cc index 5a624c2d3d..fa4deceb9a 100644 --- a/packager/mpd/base/simple_mpd_notifier_unittest.cc +++ b/packager/mpd/base/simple_mpd_notifier_unittest.cc @@ -180,6 +180,22 @@ TEST_F(SimpleMpdNotifierTest, OnDemandNotifySampleDuration) { notifier.NotifySampleDuration(kRepresentationId, kSampleDuration)); } +// This test is mainly for tsan. Using both the notifier and the MpdBuilder. +// Although locks in MpdBuilder have been removed, +// https://github.com/google/edash-packager/issues/45 +// This issue identified a bug where using SimpleMpdNotifier with multiple +// threads causes a deadlock. +TEST_F(SimpleMpdNotifierTest, NotifyNewContainerAndSampleDurationNoMock) { + SimpleMpdNotifier notifier(kOnDemandProfile, empty_mpd_option_, + empty_base_urls_, output_path_); + uint32_t container_id; + EXPECT_TRUE(notifier.NotifyNewContainer(ConvertToMediaInfo(kValidMediaInfo), + &container_id)); + const uint32_t kAnySampleDuration = 1000; + EXPECT_TRUE(notifier.NotifySampleDuration(container_id, kAnySampleDuration)); + EXPECT_TRUE(notifier.Flush()); +} + // Verify that NotifyNewSegment() for live works. TEST_F(SimpleMpdNotifierTest, LiveNotifyNewSegment) { SimpleMpdNotifier notifier(kLiveProfile, empty_mpd_option_, empty_base_urls_,