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

@@ -156,6 +156,7 @@ bool DashIopMpdNotifier::AddContentProtectionElement(
 }
 
 bool DashIopMpdNotifier::Flush() {
+  base::AutoLock auto_lock(lock_);
   return WriteMpdToFile(output_path_, mpd_builder_.get());
 }
 

packager/mpd/base/dash_iop_mpd_notifier.h

@@ -14,6 +14,7 @@
 #include <string>
 #include <vector>
 
+#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"

packager/mpd/base/dash_iop_mpd_notifier_unittest.cc

@@ -726,6 +726,22 @@ TEST_P(DashIopMpdNotifierTest, UpdateEncryption) {
       container_id, "myuuid", std::vector<uint8_t>(), 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,

packager/mpd/base/mock_mpd_builder.h

@@ -10,6 +10,7 @@
 #include <gmock/gmock.h>
 
 #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"
 

packager/mpd/base/mpd_builder.cc

@@ -385,30 +385,25 @@ 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<AdaptationSet> 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());
   return adaptation_set.release();
 }
 
 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);
 }
@@ -653,7 +648,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.
@@ -692,14 +686,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_);
 }
@@ -711,7 +703,6 @@ void AdaptationSet::AddRole(Role role) {
 // Creates a copy of <AdaptationSet> xml element, iterate thru all the
 // <Representation> (child) elements and add them to the copy.
 xml::ScopedXmlPtr<xmlNode>::type AdaptationSet::GetXml() {
-  base::AutoLock scoped_lock(lock_);
   AdaptationSetXmlNode adaptation_set;
 
   if (!adaptation_set.AddContentProtectionElements(
@@ -804,8 +795,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 {
@@ -815,17 +804,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<Representation*>::const_iterator iter =
            representations_.begin();
@@ -1037,14 +1024,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_);
 }
@@ -1057,7 +1042,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)) {
@@ -1075,8 +1059,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_) {
@@ -1093,8 +1075,6 @@ void Representation::SetSampleDuration(uint32_t sample_duration) {
 // AudioChannelConfig elements), AddContentProtectionElements*(), and
 // AddVODOnlyInfo() (Adds segment info).
 xml::ScopedXmlPtr<xmlNode>::type Representation::GetXml() {
-  base::AutoLock scoped_lock(lock_);
-
   if (!HasRequiredMediaInfoFields()) {
     LOG(ERROR) << "MediaInfo missing required fields.";
     return xml::ScopedXmlPtr<xmlNode>::type();
@@ -1110,7 +1090,8 @@ xml::ScopedXmlPtr<xmlNode>::type Representation::GetXml() {
   // Mandatory fields for Representation.
   representation.SetId(id_);
   representation.SetIntegerAttribute("bandwidth", bandwidth);
-  representation.SetStringAttribute("codecs", codecs_);
+  if (!codecs_.empty())
+    representation.SetStringAttribute("codecs", codecs_);
   representation.SetStringAttribute("mimeType", mime_type_);
 
   const bool has_video_info = media_info_.has_video_info();
@@ -1288,7 +1269,6 @@ std::string Representation::GetAudioMimeType() const {
 bool Representation::GetEarliestTimestamp(double* timestamp_seconds) {
   DCHECK(timestamp_seconds);
 
-  base::AutoLock scoped_lock(lock_);
   if (segment_infos_.empty())
     return false;
 

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<std::string> base_urls_;
   std::string availability_start_time_;
 
-  base::Lock lock_;
   base::AtomicSequenceNumber adaptation_set_counter_;
   base::AtomicSequenceNumber representation_counter_;
 
@@ -345,8 +343,6 @@ class AdaptationSet {
   std::list<Representation*> representations_;
   ::STLElementDeleter<std::list<Representation*> > representations_deleter_;
 
-  base::Lock lock_;
-
   base::AtomicSequenceNumber* const representation_counter_;
 
   const uint32_t id_;
@@ -551,8 +547,6 @@ class Representation {
   std::list<ContentProtectionElement> content_protection_elements_;
   std::list<SegmentInfo> segment_infos_;
 
-  base::Lock lock_;
-
   const uint32_t id_;
   std::string mime_type_;
   std::string codecs_;

packager/mpd/base/simple_mpd_notifier.cc

@@ -127,6 +127,7 @@ bool SimpleMpdNotifier::AddContentProtectionElement(
 }
 
 bool SimpleMpdNotifier::Flush() {
+  base::AutoLock auto_lock(lock_);
   return WriteMpdToFile(output_path_, mpd_builder_.get());
 }
 

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_,