From 26334f2808089c62889f9a880914e5905cefbbd1 Mon Sep 17 00:00:00 2001 From: Jacob Trimble Date: Mon, 9 Nov 2020 16:32:58 -0800 Subject: [PATCH] Refactor XmlNode and libxml usages. - Use std::move to transfer ownership. - Avoid libxml primitives outside XmlNode. - Have attribute setting return errors. - Mark bool returns with WARN_UNUSED_RESULTS and use return value. - Use std::string over char*. Change-Id: Ia00bb29c690025275e270f10e5c065722481c0c5 --- packager/mpd/base/adaptation_set.cc | 99 ++++--- packager/mpd/base/adaptation_set.h | 8 +- packager/mpd/base/adaptation_set_unittest.cc | 175 ++++++----- packager/mpd/base/mpd_builder.cc | 153 +++++----- packager/mpd/base/mpd_builder.h | 21 +- packager/mpd/base/mpd_utils_unittest.cc | 36 +-- packager/mpd/base/period.cc | 25 +- packager/mpd/base/period.h | 8 +- packager/mpd/base/period_unittest.cc | 10 +- packager/mpd/base/representation.cc | 28 +- packager/mpd/base/representation.h | 18 +- packager/mpd/base/representation_unittest.cc | 94 +++--- packager/mpd/base/xml/xml_node.cc | 293 ++++++++++--------- packager/mpd/base/xml/xml_node.h | 109 ++++--- packager/mpd/base/xml/xml_node_unittest.cc | 167 +++++------ packager/mpd/test/xml_compare.cc | 49 ++-- packager/mpd/test/xml_compare.h | 33 ++- 17 files changed, 678 insertions(+), 648 deletions(-) diff --git a/packager/mpd/base/adaptation_set.cc b/packager/mpd/base/adaptation_set.cc index e5f6b0fbb0..e0257940fe 100644 --- a/packager/mpd/base/adaptation_set.cc +++ b/packager/mpd/base/adaptation_set.cc @@ -237,41 +237,55 @@ void AdaptationSet::AddRole(Role role) { // can be passed to Representation to avoid setting redundant attributes. For // example, if AdaptationSet@width is set, then Representation@width is // redundant and should not be set. -xml::scoped_xml_ptr AdaptationSet::GetXml() { +base::Optional AdaptationSet::GetXml() { xml::AdaptationSetXmlNode adaptation_set; bool suppress_representation_width = false; bool suppress_representation_height = false; bool suppress_representation_frame_rate = false; - if (id_) - adaptation_set.SetId(id_.value()); - adaptation_set.SetStringAttribute("contentType", content_type_); - if (!language_.empty() && language_ != "und") { - adaptation_set.SetStringAttribute("lang", language_); + if (id_ && !adaptation_set.SetId(id_.value())) + return base::nullopt; + if (!adaptation_set.SetStringAttribute("contentType", content_type_)) + return base::nullopt; + if (!language_.empty() && language_ != "und" && + !adaptation_set.SetStringAttribute("lang", language_)) { + return base::nullopt; } // Note that std::{set,map} are ordered, so the last element is the max value. if (video_widths_.size() == 1) { suppress_representation_width = true; - adaptation_set.SetIntegerAttribute("width", *video_widths_.begin()); + if (!adaptation_set.SetIntegerAttribute("width", *video_widths_.begin())) + return base::nullopt; } else if (video_widths_.size() > 1) { - adaptation_set.SetIntegerAttribute("maxWidth", *video_widths_.rbegin()); + if (!adaptation_set.SetIntegerAttribute("maxWidth", + *video_widths_.rbegin())) { + return base::nullopt; + } } if (video_heights_.size() == 1) { suppress_representation_height = true; - adaptation_set.SetIntegerAttribute("height", *video_heights_.begin()); + if (!adaptation_set.SetIntegerAttribute("height", *video_heights_.begin())) + return base::nullopt; } else if (video_heights_.size() > 1) { - adaptation_set.SetIntegerAttribute("maxHeight", *video_heights_.rbegin()); + if (!adaptation_set.SetIntegerAttribute("maxHeight", + *video_heights_.rbegin())) { + return base::nullopt; + } } if (video_frame_rates_.size() == 1) { suppress_representation_frame_rate = true; - adaptation_set.SetStringAttribute("frameRate", - video_frame_rates_.begin()->second); + if (!adaptation_set.SetStringAttribute( + "frameRate", video_frame_rates_.begin()->second)) { + return base::nullopt; + } } else if (video_frame_rates_.size() > 1) { - adaptation_set.SetStringAttribute("maxFrameRate", - video_frame_rates_.rbegin()->second); + if (!adaptation_set.SetStringAttribute( + "maxFrameRate", video_frame_rates_.rbegin()->second)) { + return base::nullopt; + } } // Note: must be checked before checking segments_aligned_ (below). So that @@ -281,19 +295,24 @@ xml::scoped_xml_ptr AdaptationSet::GetXml() { } if (segments_aligned_ == kSegmentAlignmentTrue) { - adaptation_set.SetStringAttribute( - mpd_options_.dash_profile == DashProfile::kOnDemand - ? "subsegmentAlignment" - : "segmentAlignment", - "true"); + if (!adaptation_set.SetStringAttribute( + mpd_options_.dash_profile == DashProfile::kOnDemand + ? "subsegmentAlignment" + : "segmentAlignment", + "true")) { + return base::nullopt; + } } - if (picture_aspect_ratio_.size() == 1) - adaptation_set.SetStringAttribute("par", *picture_aspect_ratio_.begin()); + if (picture_aspect_ratio_.size() == 1 && + !adaptation_set.SetStringAttribute("par", + *picture_aspect_ratio_.begin())) { + return base::nullopt; + } if (!adaptation_set.AddContentProtectionElements( content_protection_elements_)) { - return xml::scoped_xml_ptr(); + return base::nullopt; } std::string trick_play_reference_ids; @@ -304,9 +323,10 @@ xml::scoped_xml_ptr AdaptationSet::GetXml() { CHECK(adaptation_set->has_id()); trick_play_reference_ids += std::to_string(adaptation_set->id()); } - if (!trick_play_reference_ids.empty()) { - adaptation_set.AddEssentialProperty( - "http://dashif.org/guidelines/trickmode", trick_play_reference_ids); + if (!trick_play_reference_ids.empty() && + !adaptation_set.AddEssentialProperty( + "http://dashif.org/guidelines/trickmode", trick_play_reference_ids)) { + return base::nullopt; } std::string switching_ids; @@ -317,18 +337,25 @@ xml::scoped_xml_ptr AdaptationSet::GetXml() { CHECK(adaptation_set->has_id()); switching_ids += std::to_string(adaptation_set->id()); } - if (!switching_ids.empty()) { - adaptation_set.AddSupplementalProperty( - "urn:mpeg:dash:adaptation-set-switching:2016", switching_ids); + if (!switching_ids.empty() && + !adaptation_set.AddSupplementalProperty( + "urn:mpeg:dash:adaptation-set-switching:2016", switching_ids)) { + return base::nullopt; } for (const AdaptationSet::Accessibility& accessibility : accessibilities_) { - adaptation_set.AddAccessibilityElement(accessibility.scheme, - accessibility.value); + if (!adaptation_set.AddAccessibilityElement(accessibility.scheme, + accessibility.value)) { + return base::nullopt; + } } - for (AdaptationSet::Role role : roles_) - adaptation_set.AddRoleElement("urn:mpeg:dash:role:2011", RoleToText(role)); + for (AdaptationSet::Role role : roles_) { + if (!adaptation_set.AddRoleElement("urn:mpeg:dash:role:2011", + RoleToText(role))) { + return base::nullopt; + } + } for (const auto& representation_pair : representation_map_) { const auto& representation = representation_pair.second; @@ -338,12 +365,12 @@ xml::scoped_xml_ptr AdaptationSet::GetXml() { representation->SuppressOnce(Representation::kSuppressHeight); if (suppress_representation_frame_rate) representation->SuppressOnce(Representation::kSuppressFrameRate); - xml::scoped_xml_ptr child(representation->GetXml()); - if (!child || !adaptation_set.AddChild(std::move(child))) - return xml::scoped_xml_ptr(); + auto child = representation->GetXml(); + if (!child || !adaptation_set.AddChild(std::move(*child))) + return base::nullopt; } - return adaptation_set.PassScopedPtr(); + return std::move(adaptation_set); } void AdaptationSet::ForceSetSegmentAlignment(bool segment_alignment) { diff --git a/packager/mpd/base/adaptation_set.h b/packager/mpd/base/adaptation_set.h index a8fc70605e..dfc299a34a 100644 --- a/packager/mpd/base/adaptation_set.h +++ b/packager/mpd/base/adaptation_set.h @@ -18,7 +18,7 @@ #include #include "packager/base/optional.h" -#include "packager/mpd/base/xml/scoped_xml_ptr.h" +#include "packager/mpd/base/xml/xml_node.h" namespace shaka { @@ -28,10 +28,6 @@ class Representation; struct ContentProtectionElement; struct MpdOptions; -namespace xml { -class XmlNode; -} // namespace xml - /// AdaptationSet class provides methods to add Representations and /// elements to the AdaptationSet element. class AdaptationSet { @@ -112,7 +108,7 @@ class AdaptationSet { /// and ContentProtection elements. /// @return On success returns a non-NULL scoped_xml_ptr. Otherwise returns a /// NULL scoped_xml_ptr. - xml::scoped_xml_ptr GetXml(); + base::Optional GetXml(); /// Forces the (sub)segmentAlignment field to be set to @a segment_alignment. /// Use this if you are certain that the (sub)segments are alinged/unaligned diff --git a/packager/mpd/base/adaptation_set_unittest.cc b/packager/mpd/base/adaptation_set_unittest.cc index 0d6e37d3b8..62c9e78526 100644 --- a/packager/mpd/base/adaptation_set_unittest.cc +++ b/packager/mpd/base/adaptation_set_unittest.cc @@ -69,7 +69,7 @@ TEST_F(AdaptationSetTest, AddAdaptationSetSwitching) { " schemeIdUri=\"urn:mpeg:dash:adaptation-set-switching:2016\" " " value=\"1,2,8\"/>" ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput)); } // Verify that content type is set correctly if video info is present in @@ -89,8 +89,7 @@ TEST_F(AdaptationSetTest, CheckAdaptationSetVideoContentType) { auto adaptation_set = CreateAdaptationSet(kNoLanguage); adaptation_set->AddRepresentation(ConvertToMediaInfo(kVideoMediaInfo)); - EXPECT_THAT(adaptation_set->GetXml().get(), - AttributeEqual("contentType", "video")); + EXPECT_THAT(adaptation_set->GetXml(), AttributeEqual("contentType", "video")); } // Verify that content type is set correctly if audio info is present in @@ -107,8 +106,7 @@ TEST_F(AdaptationSetTest, CheckAdaptationSetAudioContentType) { auto adaptation_set = CreateAdaptationSet(kNoLanguage); adaptation_set->AddRepresentation(ConvertToMediaInfo(kAudioMediaInfo)); - EXPECT_THAT(adaptation_set->GetXml().get(), - AttributeEqual("contentType", "audio")); + EXPECT_THAT(adaptation_set->GetXml(), AttributeEqual("contentType", "audio")); } // Verify that content type is set correctly if text info is present in @@ -123,8 +121,7 @@ TEST_F(AdaptationSetTest, CheckAdaptationSetTextContentType) { auto adaptation_set = CreateAdaptationSet("en"); adaptation_set->AddRepresentation(ConvertToMediaInfo(kTextMediaInfo)); - EXPECT_THAT(adaptation_set->GetXml().get(), - AttributeEqual("contentType", "text")); + EXPECT_THAT(adaptation_set->GetXml(), AttributeEqual("contentType", "text")); } TEST_F(AdaptationSetTest, CopyRepresentation) { @@ -152,14 +149,14 @@ TEST_F(AdaptationSetTest, CopyRepresentation) { // Verify that language passed to the constructor sets the @lang field is set. TEST_F(AdaptationSetTest, CheckLanguageAttributeSet) { auto adaptation_set = CreateAdaptationSet("en"); - EXPECT_THAT(adaptation_set->GetXml().get(), AttributeEqual("lang", "en")); + EXPECT_THAT(adaptation_set->GetXml(), AttributeEqual("lang", "en")); } TEST_F(AdaptationSetTest, CheckAdaptationSetId) { auto adaptation_set = CreateAdaptationSet(kNoLanguage); const uint32_t kAdaptationSetId = 42; adaptation_set->set_id(kAdaptationSetId); - EXPECT_THAT(adaptation_set->GetXml().get(), + EXPECT_THAT(adaptation_set->GetXml(), AttributeEqual("id", std::to_string(kAdaptationSetId))); } @@ -176,7 +173,7 @@ TEST_F(AdaptationSetTest, AddAccessibilityElement) { " \n" ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput)); } // Verify AdaptationSet::AddRole() works for "main" role. @@ -190,7 +187,7 @@ TEST_F(AdaptationSetTest, AdaptationAddRoleElementMain) { "\n" " \n" ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput)); } // Add Role, ContentProtection, and Representation elements. Verify that @@ -211,7 +208,6 @@ TEST_F(AdaptationSetTest, CheckContentProtectionRoleRepresentationOrder) { "container_type: 1\n"; adaptation_set->AddRepresentation(ConvertToMediaInfo(kAudioMediaInfo)); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); const char kExpectedOutput[] = "\n" " \n" @@ -224,7 +220,7 @@ TEST_F(AdaptationSetTest, CheckContentProtectionRoleRepresentationOrder) { " value=\"2\"/>\n" " \n" ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput)); } // Verify that if all video Representations in an AdaptationSet have the same @@ -254,9 +250,9 @@ TEST_F(AdaptationSetTest, AdapatationSetFrameRate) { ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(kVideoMediaInfo2))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("frameRate", "10/3")); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("maxFrameRate"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("frameRate", "10/3")); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("maxFrameRate"))); } // Verify that if there are videos with different frame rates, the maxFrameRate @@ -287,10 +283,9 @@ TEST_F(AdaptationSetTest, AdapatationSetMaxFrameRate) { ASSERT_TRUE(adaptation_set->AddRepresentation( ConvertToMediaInfo(kVideoMediaInfo15fps))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), - AttributeEqual("maxFrameRate", "3000/100")); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("frameRate"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("maxFrameRate", "3000/100")); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("frameRate"))); } // Verify that (max)FrameRate can be set by calling @@ -331,9 +326,9 @@ TEST_F(AdaptationSetTest, // First, make sure that maxFrameRate nor frameRate are set because // frame durations were not provided in the MediaInfo. - xml::scoped_xml_ptr no_frame_rate(adaptation_set->GetXml()); - EXPECT_THAT(no_frame_rate.get(), Not(AttributeSet("maxFrameRate"))); - EXPECT_THAT(no_frame_rate.get(), Not(AttributeSet("frameRate"))); + auto no_frame_rate = adaptation_set->GetXml(); + EXPECT_THAT(no_frame_rate, Not(AttributeSet("maxFrameRate"))); + EXPECT_THAT(no_frame_rate, Not(AttributeSet("frameRate"))); // Then set same frame duration for the representations. (Given that the // time scales match). @@ -341,9 +336,9 @@ TEST_F(AdaptationSetTest, representation_480p->SetSampleDuration(kSameFrameDuration); representation_360p->SetSampleDuration(kSameFrameDuration); - xml::scoped_xml_ptr same_frame_rate(adaptation_set->GetXml()); - EXPECT_THAT(same_frame_rate.get(), Not(AttributeSet("maxFrameRate"))); - EXPECT_THAT(same_frame_rate.get(), AttributeEqual("frameRate", "10/3")); + auto same_frame_rate = adaptation_set->GetXml(); + EXPECT_THAT(same_frame_rate, Not(AttributeSet("maxFrameRate"))); + EXPECT_THAT(same_frame_rate, AttributeEqual("frameRate", "10/3")); // Then set 480p to be 5fps (10/2) so that maxFrameRate is set. const uint32_t k5FPSFrameDuration = 2; @@ -351,9 +346,9 @@ TEST_F(AdaptationSetTest, "frame_duration_must_be_shorter_for_max_frame_rate"); representation_480p->SetSampleDuration(k5FPSFrameDuration); - xml::scoped_xml_ptr max_frame_rate(adaptation_set->GetXml()); - EXPECT_THAT(max_frame_rate.get(), AttributeEqual("maxFrameRate", "10/2")); - EXPECT_THAT(max_frame_rate.get(), Not(AttributeSet("frameRate"))); + auto max_frame_rate = adaptation_set->GetXml(); + EXPECT_THAT(max_frame_rate, AttributeEqual("maxFrameRate", "10/2")); + EXPECT_THAT(max_frame_rate, Not(AttributeSet("frameRate"))); } // Verify that if the picture aspect ratio of all the Representations are the @@ -417,8 +412,8 @@ TEST_F(AdaptationSetTest, AdaptationSetParAllSame) { ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(k360pVideoInfo))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("par", "16:9")); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("par", "16:9")); } // Verify that adding Representations with different par will generate @@ -454,8 +449,8 @@ TEST_F(AdaptationSetTest, AdaptationSetParDifferent) { ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(k2by1VideoInfo))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("par"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("par"))); } // Verify that adding Representation without pixel_width and pixel_height will @@ -475,8 +470,8 @@ TEST_F(AdaptationSetTest, AdaptationSetParUnknown) { ASSERT_TRUE(adaptation_set->AddRepresentation( ConvertToMediaInfo(kUknownPixelWidthAndHeight))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("par"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("par"))); } // Catch the case where it ends up wrong if integer division is used to check @@ -509,9 +504,9 @@ TEST_F(AdaptationSetTest, AdapatationSetMaxFrameRateIntegerDivisionEdgeCase) { ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(kVideoMediaInfo2))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("maxFrameRate", "11/3")); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("frameRate"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("maxFrameRate", "11/3")); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("frameRate"))); } // Attribute values that are common to all the children Representations should @@ -571,35 +566,34 @@ TEST_F(AdaptationSetTest, BubbleUpAttributesToAdaptationSet) { auto adaptation_set = CreateAdaptationSet(kNoLanguage); ASSERT_TRUE(adaptation_set->AddRepresentation(ConvertToMediaInfo(k1080p))); - xml::scoped_xml_ptr all_attributes_on_adaptation_set( - adaptation_set->GetXml()); - EXPECT_THAT(all_attributes_on_adaptation_set.get(), + auto all_attributes_on_adaptation_set = adaptation_set->GetXml(); + EXPECT_THAT(all_attributes_on_adaptation_set, AttributeEqual("width", "1920")); - EXPECT_THAT(all_attributes_on_adaptation_set.get(), + EXPECT_THAT(all_attributes_on_adaptation_set, AttributeEqual("height", "1080")); - EXPECT_THAT(all_attributes_on_adaptation_set.get(), + EXPECT_THAT(all_attributes_on_adaptation_set, AttributeEqual("frameRate", "30/1")); ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(kDifferentWidth))); - xml::scoped_xml_ptr width_not_set(adaptation_set->GetXml()); - EXPECT_THAT(width_not_set.get(), Not(AttributeSet("width"))); - EXPECT_THAT(width_not_set.get(), AttributeEqual("height", "1080")); - EXPECT_THAT(width_not_set.get(), AttributeEqual("frameRate", "30/1")); + auto width_not_set = adaptation_set->GetXml(); + EXPECT_THAT(width_not_set, Not(AttributeSet("width"))); + EXPECT_THAT(width_not_set, AttributeEqual("height", "1080")); + EXPECT_THAT(width_not_set, AttributeEqual("frameRate", "30/1")); ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(kDifferentHeight))); - xml::scoped_xml_ptr width_height_not_set(adaptation_set->GetXml()); - EXPECT_THAT(width_height_not_set.get(), Not(AttributeSet("width"))); - EXPECT_THAT(width_height_not_set.get(), Not(AttributeSet("height"))); - EXPECT_THAT(width_height_not_set.get(), AttributeEqual("frameRate", "30/1")); + auto width_height_not_set = adaptation_set->GetXml(); + EXPECT_THAT(width_height_not_set, Not(AttributeSet("width"))); + EXPECT_THAT(width_height_not_set, Not(AttributeSet("height"))); + EXPECT_THAT(width_height_not_set, AttributeEqual("frameRate", "30/1")); ASSERT_TRUE(adaptation_set->AddRepresentation( ConvertToMediaInfo(kDifferentFrameRate))); - xml::scoped_xml_ptr no_common_attributes(adaptation_set->GetXml()); - EXPECT_THAT(no_common_attributes.get(), Not(AttributeSet("width"))); - EXPECT_THAT(no_common_attributes.get(), Not(AttributeSet("height"))); - EXPECT_THAT(no_common_attributes.get(), Not(AttributeSet("frameRate"))); + auto no_common_attributes = adaptation_set->GetXml(); + EXPECT_THAT(no_common_attributes, Not(AttributeSet("width"))); + EXPECT_THAT(no_common_attributes, Not(AttributeSet("height"))); + EXPECT_THAT(no_common_attributes, Not(AttributeSet("frameRate"))); } TEST_F(AdaptationSetTest, GetRepresentations) { @@ -694,21 +688,20 @@ TEST_F(OnDemandAdaptationSetTest, SubsegmentAlignment) { adaptation_set->AddRepresentation(ConvertToMediaInfo(k360pMediaInfo)); representation_360p->AddNewSegment(kStartTime, kDuration, kAnySize); - xml::scoped_xml_ptr aligned(adaptation_set->GetXml()); - EXPECT_THAT(aligned.get(), AttributeEqual("subsegmentAlignment", "true")); + auto aligned = adaptation_set->GetXml(); + EXPECT_THAT(aligned, AttributeEqual("subsegmentAlignment", "true")); // Unknown because 480p has an extra subsegments. representation_480p->AddNewSegment(11, 20, kAnySize); - xml::scoped_xml_ptr alignment_unknown(adaptation_set->GetXml()); - EXPECT_THAT(alignment_unknown.get(), - Not(AttributeSet("subsegmentAlignment"))); + auto alignment_unknown = adaptation_set->GetXml(); + EXPECT_THAT(alignment_unknown, Not(AttributeSet("subsegmentAlignment"))); // Add segments that make them not aligned. representation_360p->AddNewSegment(10, 1, kAnySize); representation_360p->AddNewSegment(11, 19, kAnySize); - xml::scoped_xml_ptr unaligned(adaptation_set->GetXml()); - EXPECT_THAT(unaligned.get(), Not(AttributeSet("subsegmentAlignment"))); + auto unaligned = adaptation_set->GetXml(); + EXPECT_THAT(unaligned, Not(AttributeSet("subsegmentAlignment"))); } // Verify that subsegmentAlignment can be force set to true. @@ -749,13 +742,13 @@ TEST_F(OnDemandAdaptationSetTest, ForceSetsubsegmentAlignment) { const uint64_t kAnySize = 19834u; representation_480p->AddNewSegment(kStartTime1, kDuration, kAnySize); representation_360p->AddNewSegment(kStartTime2, kDuration, kAnySize); - xml::scoped_xml_ptr unaligned(adaptation_set->GetXml()); - EXPECT_THAT(unaligned.get(), Not(AttributeSet("subsegmentAlignment"))); + auto unaligned = adaptation_set->GetXml(); + EXPECT_THAT(unaligned, Not(AttributeSet("subsegmentAlignment"))); // Then force set the segment alignment attribute to true. adaptation_set->ForceSetSegmentAlignment(true); - xml::scoped_xml_ptr aligned(adaptation_set->GetXml()); - EXPECT_THAT(aligned.get(), AttributeEqual("subsegmentAlignment", "true")); + auto aligned = adaptation_set->GetXml(); + EXPECT_THAT(aligned, AttributeEqual("subsegmentAlignment", "true")); } // Verify that segmentAlignment is set to true if all the Representations @@ -800,16 +793,16 @@ TEST_F(LiveAdaptationSetTest, SegmentAlignmentDynamicMpd) { representation_480p->AddNewSegment(kStartTime, kDuration, kAnySize); representation_360p->AddNewSegment(kStartTime, kDuration, kAnySize); - xml::scoped_xml_ptr aligned(adaptation_set->GetXml()); - EXPECT_THAT(aligned.get(), AttributeEqual("segmentAlignment", "true")); + auto aligned = adaptation_set->GetXml(); + EXPECT_THAT(aligned, AttributeEqual("segmentAlignment", "true")); // Add segments that make them not aligned. representation_480p->AddNewSegment(11, 20, kAnySize); representation_360p->AddNewSegment(10, 1, kAnySize); representation_360p->AddNewSegment(11, 19, kAnySize); - xml::scoped_xml_ptr unaligned(adaptation_set->GetXml()); - EXPECT_THAT(unaligned.get(), Not(AttributeSet("segmentAlignment"))); + auto unaligned = adaptation_set->GetXml(); + EXPECT_THAT(unaligned, Not(AttributeSet("segmentAlignment"))); } // Verify that segmentAlignment is set to true if all the Representations @@ -862,8 +855,8 @@ TEST_F(LiveAdaptationSetTest, SegmentAlignmentStaticMpd) { representation_360p->AddNewSegment(kStartTime + kDuration, kDuration, kAnySize); - xml::scoped_xml_ptr aligned(adaptation_set->GetXml()); - EXPECT_THAT(aligned.get(), AttributeEqual("segmentAlignment", "true")); + auto aligned = adaptation_set->GetXml(); + EXPECT_THAT(aligned, AttributeEqual("segmentAlignment", "true")); } // Verify that the width and height attribute are set if all the video @@ -894,11 +887,11 @@ TEST_F(OnDemandAdaptationSetTest, AdapatationSetWidthAndHeight) { ASSERT_TRUE( adaptation_set->AddRepresentation(ConvertToMediaInfo(kVideoMediaInfo2))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("width", "1280")); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("height", "720")); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("maxWidth"))); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("maxHeight"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("width", "1280")); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("height", "720")); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("maxWidth"))); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("maxHeight"))); } // Verify that the maxWidth and maxHeight attribute are set if there are @@ -928,11 +921,11 @@ TEST_F(OnDemandAdaptationSetTest, AdaptationSetMaxWidthAndMaxHeight) { ASSERT_TRUE(adaptation_set->AddRepresentation( ConvertToMediaInfo(kVideoMediaInfo720p))); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("maxWidth", "1920")); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("maxHeight", "1080")); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("width"))); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("height"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("maxWidth", "1920")); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("maxHeight", "1080")); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("width"))); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("height"))); } // Verify that Representation::SetSampleDuration() works by checking that @@ -955,12 +948,12 @@ TEST_F(AdaptationSetTest, SetSampleDuration) { adaptation_set->AddRepresentation(video_media_info); EXPECT_TRUE(representation->Init()); - xml::scoped_xml_ptr adaptation_set_xml(adaptation_set->GetXml()); - EXPECT_THAT(adaptation_set_xml.get(), Not(AttributeSet("frameRate"))); + auto adaptation_set_xml = adaptation_set->GetXml(); + EXPECT_THAT(adaptation_set_xml, Not(AttributeSet("frameRate"))); representation->SetSampleDuration(2u); adaptation_set_xml = adaptation_set->GetXml(); - EXPECT_THAT(adaptation_set_xml.get(), AttributeEqual("frameRate", "3000/2")); + EXPECT_THAT(adaptation_set_xml, AttributeEqual("frameRate", "3000/2")); } // Verify that AdaptationSet::AddContentProtection() and @@ -1000,7 +993,7 @@ TEST_F(AdaptationSetTest, AdaptationSetAddContentProtectionAndUpdate) { " " ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput1)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput1)); adaptation_set->UpdateContentProtectionPssh( "edef8ba9-79d6-4ace-a3c8-27dcd51d21ed", "new pssh value"); @@ -1018,7 +1011,7 @@ TEST_F(AdaptationSetTest, AdaptationSetAddContentProtectionAndUpdate) { " " ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput2)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput2)); } // Verify that if the ContentProtection element for the DRM without @@ -1055,7 +1048,7 @@ TEST_F(AdaptationSetTest, UpdateToRemovePsshElement) { " " ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput1)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput1)); adaptation_set->UpdateContentProtectionPssh( "edef8ba9-79d6-4ace-a3c8-27dcd51d21ed", "added pssh value"); @@ -1073,7 +1066,7 @@ TEST_F(AdaptationSetTest, UpdateToRemovePsshElement) { " " ""; - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput2)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput2)); } // MPD schema has strict ordering. AudioChannelConfiguration must appear before @@ -1132,7 +1125,7 @@ TEST_F(OnDemandAdaptationSetTest, adaptation_set->AddRepresentation(ConvertToMediaInfo(kTestMediaInfo)); ASSERT_TRUE(audio_representation); audio_representation->AddContentProtectionElement(content_protection); - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput)); } // Verify that a text path works. @@ -1163,7 +1156,7 @@ TEST_F(OnDemandAdaptationSetTest, Text) { adaptation_set->AddRepresentation(ConvertToMediaInfo(kTextMediaInfo)); ASSERT_TRUE(text_representation); - EXPECT_THAT(adaptation_set->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set->GetXml(), XmlNodeEqual(kExpectedOutput)); } } // namespace shaka diff --git a/packager/mpd/base/mpd_builder.cc b/packager/mpd/base/mpd_builder.cc index f515586563..f09381601e 100644 --- a/packager/mpd/base/mpd_builder.cc +++ b/packager/mpd/base/mpd_builder.cc @@ -16,11 +16,11 @@ #include "packager/base/synchronization/lock.h" #include "packager/base/time/default_clock.h" #include "packager/base/time/time.h" +#include "packager/media/base/rcheck.h" #include "packager/mpd/base/adaptation_set.h" #include "packager/mpd/base/mpd_utils.h" #include "packager/mpd/base/period.h" #include "packager/mpd/base/representation.h" -#include "packager/mpd/base/xml/xml_node.h" #include "packager/version/version.h" namespace shaka { @@ -30,7 +30,7 @@ using xml::XmlNode; namespace { -void AddMpdNameSpaceInfo(XmlNode* mpd) { +bool AddMpdNameSpaceInfo(XmlNode* mpd) { DCHECK(mpd); const std::set namespaces = mpd->ExtractReferencedNamespaces(); @@ -41,9 +41,9 @@ void AddMpdNameSpaceInfo(XmlNode* mpd) { static const char kDashSchemaMpd2011[] = "urn:mpeg:dash:schema:mpd:2011 DASH-MPD.xsd"; - mpd->SetStringAttribute("xmlns", kXmlNamespace); - mpd->SetStringAttribute("xmlns:xsi", kXmlNamespaceXsi); - mpd->SetStringAttribute("xsi:schemaLocation", kDashSchemaMpd2011); + RCHECK(mpd->SetStringAttribute("xmlns", kXmlNamespace)); + RCHECK(mpd->SetStringAttribute("xmlns:xsi", kXmlNamespaceXsi)); + RCHECK(mpd->SetStringAttribute("xsi:schemaLocation", kDashSchemaMpd2011)); static const char kCencNamespace[] = "urn:mpeg:cenc:2013"; static const char kMarlinNamespace[] = @@ -62,10 +62,11 @@ void AddMpdNameSpaceInfo(XmlNode* mpd) { auto iter = uris.find(namespace_name); CHECK(iter != uris.end()) << " unexpected namespace " << namespace_name; - mpd->SetStringAttribute( + RCHECK(mpd->SetStringAttribute( base::StringPrintf("xmlns:%s", namespace_name.c_str()).c_str(), - iter->second); + iter->second)); } + return true; } bool Positive(double d) { @@ -88,10 +89,9 @@ std::string XmlDateTimeNowWithOffset( time_exploded.second); } -void SetIfPositive(const char* attr_name, double value, XmlNode* mpd) { - if (Positive(value)) { - mpd->SetStringAttribute(attr_name, SecondsToXmlDuration(value)); - } +bool SetIfPositive(const char* attr_name, double value, XmlNode* mpd) { + return !Positive(value) || + mpd->SetStringAttribute(attr_name, SecondsToXmlDuration(value)); } std::string MakePathRelative(const std::string& media_path, @@ -160,27 +160,21 @@ bool MpdBuilder::ToString(std::string* output) { DCHECK(output); static LibXmlInitializer lib_xml_initializer; - xml::scoped_xml_ptr doc(GenerateMpd()); - if (!doc) + auto mpd = GenerateMpd(); + if (!mpd) return false; - static const int kNiceFormat = 1; - int doc_str_size = 0; - xmlChar* doc_str = nullptr; - xmlDocDumpFormatMemoryEnc(doc.get(), &doc_str, &doc_str_size, "UTF-8", - kNiceFormat); - output->assign(doc_str, doc_str + doc_str_size); - xmlFree(doc_str); - - // Cleanup, free the doc. - doc.reset(); + std::string version = GetPackagerVersion(); + if (!version.empty()) { + version = + base::StringPrintf("Generated with %s version %s", + GetPackagerProjectUrl().c_str(), version.c_str()); + } + *output = mpd->ToString(version); return true; } -xmlDocPtr MpdBuilder::GenerateMpd() { - // Setup nodes. - static const char kXmlVersion[] = "1.0"; - xml::scoped_xml_ptr doc(xmlNewDoc(BAD_CAST kXmlVersion)); +base::Optional MpdBuilder::GenerateMpd() { XmlNode mpd("MPD"); // Add baseurls to MPD. @@ -188,8 +182,8 @@ xmlDocPtr MpdBuilder::GenerateMpd() { XmlNode xml_base_url("BaseURL"); xml_base_url.SetContent(base_url); - if (!mpd.AddChild(xml_base_url.PassScopedPtr())) - return nullptr; + if (!mpd.AddChild(std::move(xml_base_url))) + return base::nullopt; } bool output_period_duration = false; @@ -202,13 +196,13 @@ xmlDocPtr MpdBuilder::GenerateMpd() { } for (const auto& period : periods_) { - xml::scoped_xml_ptr period_node( - period->GetXml(output_period_duration)); - if (!period_node || !mpd.AddChild(std::move(period_node))) - return nullptr; + auto period_node = period->GetXml(output_period_duration); + if (!period_node || !mpd.AddChild(std::move(*period_node))) + return base::nullopt; } - AddMpdNameSpaceInfo(&mpd); + if (!AddMpdNameSpaceInfo(&mpd)) + return base::nullopt; static const char kOnDemandProfile[] = "urn:mpeg:dash:profile:isoff-on-demand:2011"; @@ -216,10 +210,12 @@ xmlDocPtr MpdBuilder::GenerateMpd() { "urn:mpeg:dash:profile:isoff-live:2011"; switch (mpd_options_.dash_profile) { case DashProfile::kOnDemand: - mpd.SetStringAttribute("profiles", kOnDemandProfile); + if (!mpd.SetStringAttribute("profiles", kOnDemandProfile)) + return base::nullopt; break; case DashProfile::kLive: - mpd.SetStringAttribute("profiles", kLiveProfile); + if (!mpd.SetStringAttribute("profiles", kLiveProfile)) + return base::nullopt; break; default: NOTREACHED() << "Unknown DASH profile: " @@ -227,69 +223,61 @@ xmlDocPtr MpdBuilder::GenerateMpd() { break; } - AddCommonMpdInfo(&mpd); + if (!AddCommonMpdInfo(&mpd)) + return base::nullopt; switch (mpd_options_.mpd_type) { case MpdType::kStatic: - AddStaticMpdInfo(&mpd); + if (!AddStaticMpdInfo(&mpd)) + return base::nullopt; break; case MpdType::kDynamic: - AddDynamicMpdInfo(&mpd); + if (!AddDynamicMpdInfo(&mpd)) + return base::nullopt; // Must be after Period element. - AddUtcTiming(&mpd); + if (!AddUtcTiming(&mpd)) + return base::nullopt; break; default: NOTREACHED() << "Unknown MPD type: " << static_cast(mpd_options_.mpd_type); break; } - - DCHECK(doc); - const std::string version = GetPackagerVersion(); - if (!version.empty()) { - std::string version_string = - base::StringPrintf("Generated with %s version %s", - GetPackagerProjectUrl().c_str(), version.c_str()); - xml::scoped_xml_ptr comment( - xmlNewDocComment(doc.get(), BAD_CAST version_string.c_str())); - xmlDocSetRootElement(doc.get(), comment.get()); - xmlAddSibling(comment.release(), mpd.Release()); - } else { - xmlDocSetRootElement(doc.get(), mpd.Release()); - } - return doc.release(); + return mpd; } -void MpdBuilder::AddCommonMpdInfo(XmlNode* mpd_node) { +bool MpdBuilder::AddCommonMpdInfo(XmlNode* mpd_node) { if (Positive(mpd_options_.mpd_params.min_buffer_time)) { - mpd_node->SetStringAttribute( + RCHECK(mpd_node->SetStringAttribute( "minBufferTime", - SecondsToXmlDuration(mpd_options_.mpd_params.min_buffer_time)); + SecondsToXmlDuration(mpd_options_.mpd_params.min_buffer_time))); } else { LOG(ERROR) << "minBufferTime value not specified."; - // TODO(tinskip): Propagate error. + return false; } + return true; } -void MpdBuilder::AddStaticMpdInfo(XmlNode* mpd_node) { +bool MpdBuilder::AddStaticMpdInfo(XmlNode* mpd_node) { DCHECK(mpd_node); DCHECK_EQ(MpdType::kStatic, mpd_options_.mpd_type); static const char kStaticMpdType[] = "static"; - mpd_node->SetStringAttribute("type", kStaticMpdType); - mpd_node->SetStringAttribute("mediaPresentationDuration", - SecondsToXmlDuration(GetStaticMpdDuration())); + return mpd_node->SetStringAttribute("type", kStaticMpdType) && + mpd_node->SetStringAttribute( + "mediaPresentationDuration", + SecondsToXmlDuration(GetStaticMpdDuration())); } -void MpdBuilder::AddDynamicMpdInfo(XmlNode* mpd_node) { +bool MpdBuilder::AddDynamicMpdInfo(XmlNode* mpd_node) { DCHECK(mpd_node); DCHECK_EQ(MpdType::kDynamic, mpd_options_.mpd_type); static const char kDynamicMpdType[] = "dynamic"; - mpd_node->SetStringAttribute("type", kDynamicMpdType); + RCHECK(mpd_node->SetStringAttribute("type", kDynamicMpdType)); // No offset from NOW. - mpd_node->SetStringAttribute("publishTime", - XmlDateTimeNowWithOffset(0, clock_.get())); + RCHECK(mpd_node->SetStringAttribute( + "publishTime", XmlDateTimeNowWithOffset(0, clock_.get()))); // 'availabilityStartTime' is required for dynamic profile. Calculate if // not already calculated. @@ -304,36 +292,41 @@ void MpdBuilder::AddDynamicMpdInfo(XmlNode* mpd_node) { // TODO(tinskip). Propagate an error. } } - if (!availability_start_time_.empty()) - mpd_node->SetStringAttribute("availabilityStartTime", - availability_start_time_); + if (!availability_start_time_.empty()) { + RCHECK(mpd_node->SetStringAttribute("availabilityStartTime", + availability_start_time_)); + } if (Positive(mpd_options_.mpd_params.minimum_update_period)) { - mpd_node->SetStringAttribute( + RCHECK(mpd_node->SetStringAttribute( "minimumUpdatePeriod", - SecondsToXmlDuration(mpd_options_.mpd_params.minimum_update_period)); + SecondsToXmlDuration(mpd_options_.mpd_params.minimum_update_period))); } else { LOG(WARNING) << "The profile is dynamic but no minimumUpdatePeriod " "specified."; } - SetIfPositive("timeShiftBufferDepth", - mpd_options_.mpd_params.time_shift_buffer_depth, mpd_node); - SetIfPositive("suggestedPresentationDelay", - mpd_options_.mpd_params.suggested_presentation_delay, mpd_node); + return SetIfPositive("timeShiftBufferDepth", + mpd_options_.mpd_params.time_shift_buffer_depth, + mpd_node) && + SetIfPositive("suggestedPresentationDelay", + mpd_options_.mpd_params.suggested_presentation_delay, + mpd_node); } -void MpdBuilder::AddUtcTiming(XmlNode* mpd_node) { +bool MpdBuilder::AddUtcTiming(XmlNode* mpd_node) { DCHECK(mpd_node); DCHECK_EQ(MpdType::kDynamic, mpd_options_.mpd_type); for (const MpdParams::UtcTiming& utc_timing : mpd_options_.mpd_params.utc_timings) { XmlNode utc_timing_node("UTCTiming"); - utc_timing_node.SetStringAttribute("schemeIdUri", utc_timing.scheme_id_uri); - utc_timing_node.SetStringAttribute("value", utc_timing.value); - mpd_node->AddChild(utc_timing_node.PassScopedPtr()); + RCHECK(utc_timing_node.SetStringAttribute("schemeIdUri", + utc_timing.scheme_id_uri)); + RCHECK(utc_timing_node.SetStringAttribute("value", utc_timing.value)); + RCHECK(mpd_node->AddChild(std::move(utc_timing_node))); } + return true; } float MpdBuilder::GetStaticMpdDuration() { diff --git a/packager/mpd/base/mpd_builder.h b/packager/mpd/base/mpd_builder.h index cba41001c5..306cd3c609 100644 --- a/packager/mpd/base/mpd_builder.h +++ b/packager/mpd/base/mpd_builder.h @@ -17,8 +17,11 @@ #include #include +#include "packager/base/compiler_specific.h" +#include "packager/base/optional.h" #include "packager/base/time/clock.h" #include "packager/mpd/base/mpd_options.h" +#include "packager/mpd/base/xml/xml_node.h" // TODO(rkuroiwa): For classes with |id_|, consider removing the field and let // the MPD (XML) generation functions take care of assigning an ID to each @@ -29,10 +32,6 @@ class AdaptationSet; class MediaInfo; class Period; -namespace xml { -class XmlNode; -} // namespace xml - /// This class generates DASH MPDs (Media Presentation Descriptions). class MpdBuilder { public: @@ -57,7 +56,7 @@ class MpdBuilder { /// @param[out] output is an output string where the MPD gets written. /// @return true on success, false otherwise. // TODO(kqyang): Handle file IO in this class as in HLS media_playlist? - virtual bool ToString(std::string* output); + virtual bool ToString(std::string* output) WARN_UNUSED_RESULT; /// Adjusts the fields of MediaInfo so that paths are relative to the /// specified MPD path. @@ -86,21 +85,21 @@ class MpdBuilder { // Returns the document pointer to the MPD. This must be freed by the caller // using appropriate xmlDocPtr freeing function. // On failure, this returns NULL. - xmlDocPtr GenerateMpd(); + base::Optional GenerateMpd(); // Set MPD attributes common to all profiles. Uses non-zero |mpd_options_| to // set attributes for the MPD. - void AddCommonMpdInfo(xml::XmlNode* mpd_node); + bool AddCommonMpdInfo(xml::XmlNode* mpd_node) WARN_UNUSED_RESULT; // Adds 'static' MPD attributes and elements to |mpd_node|. This assumes that // the first child element is a Period element. - void AddStaticMpdInfo(xml::XmlNode* mpd_node); + bool AddStaticMpdInfo(xml::XmlNode* mpd_node) WARN_UNUSED_RESULT; // Same as AddStaticMpdInfo() but for 'dynamic' MPDs. - void AddDynamicMpdInfo(xml::XmlNode* mpd_node); + bool AddDynamicMpdInfo(xml::XmlNode* mpd_node) WARN_UNUSED_RESULT; // Add UTCTiming element if utc timing is provided. - void AddUtcTiming(xml::XmlNode* mpd_node); + bool AddUtcTiming(xml::XmlNode* mpd_node) WARN_UNUSED_RESULT; float GetStaticMpdDuration(); @@ -110,7 +109,7 @@ class MpdBuilder { // Gets the earliest, normalized segment timestamp. Returns true if // successful, false otherwise. - bool GetEarliestTimestamp(double* timestamp_seconds); + bool GetEarliestTimestamp(double* timestamp_seconds) WARN_UNUSED_RESULT; // Update Period durations and presentation timestamps. void UpdatePeriodDurationAndPresentationTimestamp(); diff --git a/packager/mpd/base/mpd_utils_unittest.cc b/packager/mpd/base/mpd_utils_unittest.cc index 122d6ee19a..0eee598ca6 100644 --- a/packager/mpd/base/mpd_utils_unittest.cc +++ b/packager/mpd/base/mpd_utils_unittest.cc @@ -72,7 +72,7 @@ TEST_F(MpdUtilsTest, ContentProtectionGeneral) { " " ""; - EXPECT_THAT(adaptation_set_.GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set_.GetXml(), XmlNodeEqual(kExpectedOutput)); } TEST_F(MpdUtilsTest, ContentProtectionMarlin) { @@ -114,7 +114,7 @@ TEST_F(MpdUtilsTest, ContentProtectionMarlin) { " " ""; - EXPECT_THAT(adaptation_set_.GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set_.GetXml(), XmlNodeEqual(kExpectedOutput)); } TEST_F(MpdUtilsTest, ContentProtectionPlayReadyCencMspr) { @@ -154,22 +154,24 @@ TEST_F(MpdUtilsTest, ContentProtectionPlayReadyCencMspr) { ASSERT_TRUE(adaptation_set_.AddRepresentation(media_info)); const char kExpectedOutput[] = - "" - " " - " " - " " - "AAAAOHBzc2gBAAAAmgTweZhAQoarkuZb4IhflQAAAAERIjNEVWZ3iJkAqrvM3e7/AAAABDAxMjM=" - " " - " MDEyMw==" - " " - " " + "" + " " + " " + " " + "AAAAOHBzc2gBAAAAmgTweZhAQoarkuZb4IhflQAAAAERIjNEVWZ3iJkAqrvM3e7/" + "AAAABDAxMjM=" + " " + " MDEyMw==" + " " + " " ""; - EXPECT_THAT(adaptation_set_.GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set_.GetXml(), XmlNodeEqual(kExpectedOutput)); } TEST_F(MpdUtilsTest, ContentProtectionPlayReadyCenc) { @@ -223,7 +225,7 @@ TEST_F(MpdUtilsTest, ContentProtectionPlayReadyCenc) { " " ""; - EXPECT_THAT(adaptation_set_.GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(adaptation_set_.GetXml(), XmlNodeEqual(kExpectedOutput)); } } // namespace shaka diff --git a/packager/mpd/base/period.cc b/packager/mpd/base/period.cc index f86902dd96..73e656cd11 100644 --- a/packager/mpd/base/period.cc +++ b/packager/mpd/base/period.cc @@ -120,7 +120,7 @@ AdaptationSet* Period::GetOrCreateAdaptationSet( return adaptation_set_ptr; } -xml::scoped_xml_ptr Period::GetXml(bool output_period_duration) { +base::Optional Period::GetXml(bool output_period_duration) { adaptation_sets_.sort( [](const std::unique_ptr& adaptation_set_a, const std::unique_ptr& adaptation_set_b) { @@ -134,22 +134,27 @@ xml::scoped_xml_ptr Period::GetXml(bool output_period_duration) { xml::XmlNode period("Period"); // Required for 'dynamic' MPDs. - period.SetId(id_); + if (!period.SetId(id_)) + return base::nullopt; // Iterate thru AdaptationSets and add them to one big Period element. for (const auto& adaptation_set : adaptation_sets_) { - xml::scoped_xml_ptr child(adaptation_set->GetXml()); - if (!child || !period.AddChild(std::move(child))) - return nullptr; + auto child = adaptation_set->GetXml(); + if (!child || !period.AddChild(std::move(*child))) + return base::nullopt; } if (output_period_duration) { - period.SetStringAttribute("duration", - SecondsToXmlDuration(duration_seconds_)); + if (!period.SetStringAttribute("duration", + SecondsToXmlDuration(duration_seconds_))) { + return base::nullopt; + } } else if (mpd_options_.mpd_type == MpdType::kDynamic) { - period.SetStringAttribute("start", - SecondsToXmlDuration(start_time_in_seconds_)); + if (!period.SetStringAttribute( + "start", SecondsToXmlDuration(start_time_in_seconds_))) { + return base::nullopt; + } } - return period.PassScopedPtr(); + return period; } const std::list Period::GetAdaptationSets() const { diff --git a/packager/mpd/base/period.h b/packager/mpd/base/period.h index 047e788745..8e6c190661 100644 --- a/packager/mpd/base/period.h +++ b/packager/mpd/base/period.h @@ -15,16 +15,12 @@ #include "packager/base/optional.h" #include "packager/mpd/base/adaptation_set.h" #include "packager/mpd/base/media_info.pb.h" -#include "packager/mpd/base/xml/scoped_xml_ptr.h" +#include "packager/mpd/base/xml/xml_node.h" namespace shaka { struct MpdOptions; -namespace xml { -class XmlNode; -} // namespace xml - /// Period class maps to element and provides methods to add /// AdaptationSets. class Period { @@ -50,7 +46,7 @@ class Period { /// Generates xml element with its child AdaptationSet elements. /// @return On success returns a non-NULL scoped_xml_ptr. Otherwise returns a /// NULL scoped_xml_ptr. - xml::scoped_xml_ptr GetXml(bool output_period_duration); + base::Optional GetXml(bool output_period_duration); /// @return The list of AdaptationSets in this Period. const std::list GetAdaptationSets() const; diff --git a/packager/mpd/base/period_unittest.cc b/packager/mpd/base/period_unittest.cc index a166e17a86..151e041257 100644 --- a/packager/mpd/base/period_unittest.cc +++ b/packager/mpd/base/period_unittest.cc @@ -137,7 +137,7 @@ TEST_F(PeriodTest, GetXml) { // Representation::Init() is called. " " ""; - EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration).get(), + EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration), XmlNodeEqual(kExpectedXml)); } @@ -169,7 +169,7 @@ TEST_F(PeriodTest, DynamicMpdGetXml) { // Representation::Init() is called. " " ""; - EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration).get(), + EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration), XmlNodeEqual(kExpectedXml)); } @@ -202,7 +202,7 @@ TEST_F(PeriodTest, SetDurationAndGetXml) { // Representation::Init() is called. " " ""; - EXPECT_THAT(testable_period_.GetXml(kOutputPeriodDuration).get(), + EXPECT_THAT(testable_period_.GetXml(kOutputPeriodDuration), XmlNodeEqual(kExpectedXml)); const char kExpectedXmlSuppressDuration[] = "" @@ -210,7 +210,7 @@ TEST_F(PeriodTest, SetDurationAndGetXml) { // Representation::Init() is called. " " ""; - EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration).get(), + EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration), XmlNodeEqual(kExpectedXmlSuppressDuration)); } @@ -551,7 +551,7 @@ TEST_F(PeriodTest, OrderedByAdaptationSetId) { R"( )" R"( )" R"()"; - EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration).get(), + EXPECT_THAT(testable_period_.GetXml(!kOutputPeriodDuration), XmlNodeEqual(kExpectedXml)); } diff --git a/packager/mpd/base/representation.cc b/packager/mpd/base/representation.cc index b1b46c8c2e..3d17472fcb 100644 --- a/packager/mpd/base/representation.cc +++ b/packager/mpd/base/representation.cc @@ -218,10 +218,10 @@ const MediaInfo& Representation::GetMediaInfo() const { // AddVideoInfo() (possibly adds FramePacking elements), AddAudioInfo() (Adds // AudioChannelConfig elements), AddContentProtectionElements*(), and // AddVODOnlyInfo() (Adds segment info). -xml::scoped_xml_ptr Representation::GetXml() { +base::Optional Representation::GetXml() { if (!HasRequiredMediaInfoFields()) { LOG(ERROR) << "MediaInfo missing required fields."; - return xml::scoped_xml_ptr(); + return base::nullopt; } const uint64_t bandwidth = media_info_.has_bandwidth() @@ -232,11 +232,13 @@ xml::scoped_xml_ptr Representation::GetXml() { xml::RepresentationXmlNode representation; // Mandatory fields for Representation. - representation.SetId(id_); - representation.SetIntegerAttribute("bandwidth", bandwidth); - if (!codecs_.empty()) - representation.SetStringAttribute("codecs", codecs_); - representation.SetStringAttribute("mimeType", mime_type_); + if (!representation.SetId(id_) || + !representation.SetIntegerAttribute("bandwidth", bandwidth) || + !(codecs_.empty() || + representation.SetStringAttribute("codecs", codecs_)) || + !representation.SetStringAttribute("mimeType", mime_type_)) { + return base::nullopt; + } const bool has_video_info = media_info_.has_video_info(); const bool has_audio_info = media_info_.has_audio_info(); @@ -248,37 +250,37 @@ xml::scoped_xml_ptr Representation::GetXml() { !(output_suppression_flags_ & kSuppressHeight), !(output_suppression_flags_ & kSuppressFrameRate))) { LOG(ERROR) << "Failed to add video info to Representation XML."; - return xml::scoped_xml_ptr(); + return base::nullopt; } if (has_audio_info && !representation.AddAudioInfo(media_info_.audio_info())) { LOG(ERROR) << "Failed to add audio info to Representation XML."; - return xml::scoped_xml_ptr(); + return base::nullopt; } if (!representation.AddContentProtectionElements( content_protection_elements_)) { - return xml::scoped_xml_ptr(); + return base::nullopt; } if (HasVODOnlyFields(media_info_) && !representation.AddVODOnlyInfo(media_info_)) { LOG(ERROR) << "Failed to add VOD info."; - return xml::scoped_xml_ptr(); + return base::nullopt; } if (HasLiveOnlyFields(media_info_) && !representation.AddLiveOnlyInfo(media_info_, segment_infos_, start_number_)) { LOG(ERROR) << "Failed to add Live info."; - return xml::scoped_xml_ptr(); + return base::nullopt; } // TODO(rkuroiwa): It is likely that all representations have the exact same // SegmentTemplate. Optimize and propagate the tag up to AdaptationSet level. output_suppression_flags_ = 0; - return representation.PassScopedPtr(); + return std::move(representation); } void Representation::SuppressOnce(SuppressFlag flag) { diff --git a/packager/mpd/base/representation.h b/packager/mpd/base/representation.h index 813d43edc5..942612845e 100644 --- a/packager/mpd/base/representation.h +++ b/packager/mpd/base/representation.h @@ -9,26 +9,22 @@ #ifndef PACKAGER_MPD_BASE_REPRESENTATION_H_ #define PACKAGER_MPD_BASE_REPRESENTATION_H_ -#include "packager/mpd/base/bandwidth_estimator.h" -#include "packager/mpd/base/media_info.pb.h" -#include "packager/mpd/base/segment_info.h" -#include "packager/mpd/base/xml/scoped_xml_ptr.h" - #include #include #include +#include "packager/base/optional.h" +#include "packager/mpd/base/bandwidth_estimator.h" +#include "packager/mpd/base/media_info.pb.h" +#include "packager/mpd/base/segment_info.h" +#include "packager/mpd/base/xml/xml_node.h" + namespace shaka { struct ContentProtectionElement; struct MpdOptions; -namespace xml { -class XmlNode; -class RepresentationXmlNode; -} // namespace xml - class RepresentationStateChangeListener { public: RepresentationStateChangeListener() {} @@ -116,7 +112,7 @@ class Representation { virtual const MediaInfo& GetMediaInfo() const; /// @return Copy of . - xml::scoped_xml_ptr GetXml(); + base::Optional GetXml(); /// By calling this methods, the next time GetXml() is /// called, the corresponding attributes will not be set. diff --git a/packager/mpd/base/representation_unittest.cc b/packager/mpd/base/representation_unittest.cc index c9a9f97684..832f931e22 100644 --- a/packager/mpd/base/representation_unittest.cc +++ b/packager/mpd/base/representation_unittest.cc @@ -169,7 +169,7 @@ TEST_F(RepresentationTest, CheckVideoInfoReflectedInXml) { " codecs=\"avc1\" mimeType=\"video/mp4\" " " sar=\"1:1\" width=\"1280\" height=\"720\" " " frameRate=\"10/10\"/>"; - EXPECT_THAT(representation->GetXml().get(), XmlNodeEqual(kExpectedOutput)); + EXPECT_THAT(representation->GetXml(), XmlNodeEqual(kExpectedOutput)); } TEST_F(RepresentationTest, CheckVideoInfoVp8CodecInMp4) { @@ -188,7 +188,7 @@ TEST_F(RepresentationTest, CheckVideoInfoVp8CodecInMp4) { CreateRepresentation(ConvertToMediaInfo(kTestMediaInfoCodecVp8), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), + EXPECT_THAT(representation->GetXml(), AttributeEqual("codecs", "vp08.00.00.08.01.01.00.00")); } @@ -210,7 +210,7 @@ TEST_F(RepresentationTest, CheckVideoInfoVp8CodecInWebm) { CreateRepresentation(ConvertToMediaInfo(kTestMediaInfoCodecVp8), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), AttributeEqual("codecs", "vp8")); + EXPECT_THAT(representation->GetXml(), AttributeEqual("codecs", "vp8")); } TEST_F(RepresentationTest, CheckVideoInfoVp9CodecInWebm) { @@ -229,7 +229,7 @@ TEST_F(RepresentationTest, CheckVideoInfoVp9CodecInWebm) { CreateRepresentation(ConvertToMediaInfo(kTestMediaInfoCodecVp9), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), + EXPECT_THAT(representation->GetXml(), AttributeEqual("codecs", "vp09.00.00.08.01.01.00.00")); } @@ -251,7 +251,7 @@ TEST_F(RepresentationTest, CheckVideoInfoLegacyVp9CodecInWebm) { CreateRepresentation(ConvertToMediaInfo(kTestMediaInfoCodecVp9), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), AttributeEqual("codecs", "vp9")); + EXPECT_THAT(representation->GetXml(), AttributeEqual("codecs", "vp9")); } // Make sure RepresentationStateChangeListener::OnNewSegmentForRepresentation() @@ -325,7 +325,7 @@ TEST_F(RepresentationTest, TtmlXmlMimeType) { CreateRepresentation(ConvertToMediaInfo(kTtmlXmlMediaInfo), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), + EXPECT_THAT(representation->GetXml(), AttributeEqual("mimeType", "application/ttml+xml")); } @@ -340,7 +340,7 @@ TEST_F(RepresentationTest, TtmlMp4MimeType) { CreateRepresentation(ConvertToMediaInfo(kTtmlMp4MediaInfo), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), + EXPECT_THAT(representation->GetXml(), AttributeEqual("mimeType", "application/mp4")); } @@ -354,8 +354,7 @@ TEST_F(RepresentationTest, WebVttMimeType) { auto representation = CreateRepresentation( ConvertToMediaInfo(kWebVttMediaInfo), kAnyRepresentationId, NoListener()); ASSERT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), - AttributeEqual("mimeType", "text/vtt")); + EXPECT_THAT(representation->GetXml(), AttributeEqual("mimeType", "text/vtt")); } // Verify that Suppress*() methods work. @@ -376,22 +375,22 @@ TEST_F(RepresentationTest, SuppressRepresentationAttributes) { ConvertToMediaInfo(kTestMediaInfo), kAnyRepresentationId, NoListener()); representation->SuppressOnce(Representation::kSuppressWidth); - xml::scoped_xml_ptr no_width(representation->GetXml()); - EXPECT_THAT(no_width.get(), Not(AttributeSet("width"))); - EXPECT_THAT(no_width.get(), AttributeEqual("height", "480")); - EXPECT_THAT(no_width.get(), AttributeEqual("frameRate", "10/10")); + auto no_width = representation->GetXml(); + EXPECT_THAT(no_width, Not(AttributeSet("width"))); + EXPECT_THAT(no_width, AttributeEqual("height", "480")); + EXPECT_THAT(no_width, AttributeEqual("frameRate", "10/10")); representation->SuppressOnce(Representation::kSuppressHeight); - xml::scoped_xml_ptr no_height(representation->GetXml()); - EXPECT_THAT(no_height.get(), Not(AttributeSet("height"))); - EXPECT_THAT(no_height.get(), AttributeEqual("width", "720")); - EXPECT_THAT(no_height.get(), AttributeEqual("frameRate", "10/10")); + auto no_height = representation->GetXml(); + EXPECT_THAT(no_height, Not(AttributeSet("height"))); + EXPECT_THAT(no_height, AttributeEqual("width", "720")); + EXPECT_THAT(no_height, AttributeEqual("frameRate", "10/10")); representation->SuppressOnce(Representation::kSuppressFrameRate); - xml::scoped_xml_ptr no_frame_rate(representation->GetXml()); - EXPECT_THAT(no_frame_rate.get(), Not(AttributeSet("frameRate"))); - EXPECT_THAT(no_frame_rate.get(), AttributeEqual("width", "720")); - EXPECT_THAT(no_frame_rate.get(), AttributeEqual("height", "480")); + auto no_frame_rate = representation->GetXml(); + EXPECT_THAT(no_frame_rate, Not(AttributeSet("frameRate"))); + EXPECT_THAT(no_frame_rate, AttributeEqual("width", "720")); + EXPECT_THAT(no_frame_rate, AttributeEqual("height", "480")); } TEST_F(RepresentationTest, CheckRepresentationId) { @@ -401,7 +400,7 @@ TEST_F(RepresentationTest, CheckRepresentationId) { auto representation = CreateRepresentation(video_media_info, kRepresentationId, NoListener()); EXPECT_TRUE(representation->Init()); - EXPECT_THAT(representation->GetXml().get(), + EXPECT_THAT(representation->GetXml(), AttributeEqual("id", std::to_string(kRepresentationId))); } @@ -508,7 +507,7 @@ TEST_F(SegmentTemplateTest, OneSegmentNormal) { AddSegments(kStartTime, kDuration, kSize, 0); expected_s_elements_ = ""; - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } TEST_F(SegmentTemplateTest, RepresentationClone) { @@ -533,8 +532,7 @@ TEST_F(SegmentTemplateTest, RepresentationClone) { " media=\"$Number$.mp4\" startNumber=\"2\">\n" " \n" "\n"; - EXPECT_THAT(cloned_representation->GetXml().get(), - XmlNodeEqual(kExpectedXml)); + EXPECT_THAT(cloned_representation->GetXml(), XmlNodeEqual(kExpectedXml)); } TEST_F(SegmentTemplateTest, PresentationTimeOffset) { @@ -557,7 +555,7 @@ TEST_F(SegmentTemplateTest, PresentationTimeOffset) { " \n" " \n" "\n"; - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(kExpectedXml)); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(kExpectedXml)); } TEST_F(SegmentTemplateTest, GetStartAndEndTimestamps) { @@ -597,7 +595,7 @@ TEST_F(SegmentTemplateTest, NormalRepeatedSegmentDuration) { repeat = 0; AddSegments(start_time, duration, kSize, repeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } TEST_F(SegmentTemplateTest, RepeatedSegmentsFromNonZeroStartTime) { @@ -617,7 +615,7 @@ TEST_F(SegmentTemplateTest, RepeatedSegmentsFromNonZeroStartTime) { repeat = 3; AddSegments(start_time, duration, kSize, repeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } // Segments not starting from 0. @@ -629,7 +627,7 @@ TEST_F(SegmentTemplateTest, NonZeroStartTime) { const int kRepeat = 1; AddSegments(kStartTime, kDuration, kSize, kRepeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } // There is a gap in the segments, but still valid. @@ -643,7 +641,7 @@ TEST_F(SegmentTemplateTest, NonContiguousLiveInfo) { const int64_t kStartTimeOffset = 100; AddSegments(kDuration + kStartTimeOffset, kDuration, kSize, kRepeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } // Add segments out of order. Segments that start before the previous segment @@ -658,7 +656,7 @@ TEST_F(SegmentTemplateTest, OutOfOrder) { AddSegments(kLaterStartTime, kDuration, kSize, kRepeat); AddSegments(kEarlierStartTime, kDuration, kSize, kRepeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } // No segments should be overlapping. @@ -674,7 +672,7 @@ TEST_F(SegmentTemplateTest, OverlappingSegments) { AddSegments(kEarlierStartTime, kDuration, kSize, kRepeat); AddSegments(kOverlappingSegmentStartTime, kDuration, kSize, kRepeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } // Some segments can be overlapped due to rounding errors. As long as it falls @@ -692,7 +690,7 @@ TEST_F(SegmentTemplateTest, OverlappingSegmentsWithinErrorRange) { AddSegments(kEarlierStartTime, kDuration, kSize, kRepeat); AddSegments(kOverlappingSegmentStartTime, kDuration, kSize, kRepeat); - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(ExpectedXml())); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml())); } class SegmentTimelineTestBase : public SegmentTemplateTest { @@ -782,7 +780,7 @@ TEST_P(ApproximateSegmentTimelineTest, SegmentDurationAdjusted) { expected_s_elements = base::StringPrintf(kSElementTemplateWithoutR, kStartTime, kDurationSmaller); } - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_elements))); } @@ -803,7 +801,7 @@ TEST_P(ApproximateSegmentTimelineTest, expected_s_elements = base::StringPrintf(kSElementTemplateWithoutR, kStartTime, kDurationSmaller); } - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_elements))); } @@ -835,7 +833,7 @@ TEST_P(ApproximateSegmentTimelineTest, SegmentsWithSimilarDurations) { kStartTime + kDurationSmaller + kDurationLarger, kDurationSmaller); } - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_elements))); } @@ -861,7 +859,7 @@ TEST_P(ApproximateSegmentTimelineTest, SegmentsWithSimilarDurations2) { expected_s_elements = base::StringPrintf(kSElementTemplate, kStartTime, kDurationLarger, kNumSegments - 1); } - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_elements))); } @@ -885,7 +883,7 @@ TEST_P(ApproximateSegmentTimelineTest, FillSmallGap) { base::StringPrintf(kSElementTemplate, kStartTime + kDuration + kGap, kDuration, 1 /* repeat */); } - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_elements))); } @@ -909,7 +907,7 @@ TEST_P(ApproximateSegmentTimelineTest, FillSmallOverlap) { base::StringPrintf(kSElementTemplate, kStartTime + kDuration - kOverlap, kDuration, 1 /* repeat */); } - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_elements))); } @@ -946,7 +944,7 @@ TEST_P(ApproximateSegmentTimelineTest, NoSampleDuration) { " \n" " \n" "\n"; - EXPECT_THAT(representation_->GetXml().get(), XmlNodeEqual(kExpectedXml)); + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(kExpectedXml)); } INSTANTIATE_TEST_CASE_P(ApproximateSegmentTimelineTest, @@ -996,7 +994,7 @@ TEST_P(TimeShiftBufferDepthTest, Normal) { initial_start_time_ + kDuration * (kRepeat - kExpectedRepeatsLeft), kDuration, kExpectedRepeatsLeft); EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_element, kExpectedStartNumber))); } @@ -1020,7 +1018,7 @@ TEST_P(TimeShiftBufferDepthTest, TimeShiftBufferDepthShorterThanSegmentLength) { const std::string expected_s_element = base::StringPrintf( kSElementTemplate, initial_start_time_, kDuration, kRepeat); EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_element, kDefaultStartNumber))); } @@ -1053,7 +1051,7 @@ TEST_P(TimeShiftBufferDepthTest, Generic) { const int kExpectedRemovedSegments = kRepeat + 1; EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml( expected_s_element, kDefaultStartNumber + kExpectedRemovedSegments))); } @@ -1095,7 +1093,7 @@ TEST_P(TimeShiftBufferDepthTest, MoreThanOneS) { kTwoSecondDuration, kTwoSecondSegmentRepeat); EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml( expected_s_element, kDefaultStartNumber + kExpectedRemovedSegments))); } @@ -1136,7 +1134,7 @@ TEST_P(TimeShiftBufferDepthTest, UseLastSegmentInS) { expected_s_element += base::StringPrintf(kSElementTemplate, first_s_element_end_time, kTwoSecondDuration, kTwoSecondSegmentRepeat); - EXPECT_THAT(representation_->GetXml().get(), + EXPECT_THAT(representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_element, 2))); } @@ -1168,7 +1166,7 @@ TEST_P(TimeShiftBufferDepthTest, NormalGap) { gap_s_element_start_time, kDuration); EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_element, kDefaultStartNumber))); } @@ -1204,7 +1202,7 @@ TEST_P(TimeShiftBufferDepthTest, HugeGap) { kSecondSElementRepeat); const int kExpectedRemovedSegments = kRepeat; EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml( expected_s_element, kDefaultStartNumber + kExpectedRemovedSegments))); } @@ -1233,7 +1231,7 @@ TEST_P(TimeShiftBufferDepthTest, ManySegments) { initial_start_time_ + kExpectedRemovedSegments * kDuration, kDuration, kExpectedSegmentsRepeat); EXPECT_THAT( - representation_->GetXml().get(), + representation_->GetXml(), XmlNodeEqual(ExpectedXml(expected_s_element, kExpectedStartNumber))); } diff --git a/packager/mpd/base/xml/xml_node.cc b/packager/mpd/base/xml/xml_node.cc index f7c908400e..720d0b2786 100644 --- a/packager/mpd/base/xml/xml_node.cc +++ b/packager/mpd/base/xml/xml_node.cc @@ -7,6 +7,7 @@ #include "packager/mpd/base/xml/xml_node.h" #include +#include #include #include @@ -15,9 +16,11 @@ #include "packager/base/macros.h" #include "packager/base/strings/string_number_conversions.h" #include "packager/base/sys_byteorder.h" +#include "packager/media/base/rcheck.h" #include "packager/mpd/base/media_info.pb.h" #include "packager/mpd/base/mpd_utils.h" #include "packager/mpd/base/segment_info.h" +#include "packager/mpd/base/xml/scoped_xml_ptr.h" DEFINE_bool(segment_template_constant_duration, false, @@ -77,12 +80,12 @@ bool PopulateSegmentTimeline(const std::list& segment_infos, XmlNode* segment_timeline) { for (const SegmentInfo& segment_info : segment_infos) { XmlNode s_element("S"); - s_element.SetIntegerAttribute("t", segment_info.start_time); - s_element.SetIntegerAttribute("d", segment_info.duration); + RCHECK(s_element.SetIntegerAttribute("t", segment_info.start_time)); + RCHECK(s_element.SetIntegerAttribute("d", segment_info.duration)); if (segment_info.repeat > 0) - s_element.SetIntegerAttribute("r", segment_info.repeat); + RCHECK(s_element.SetIntegerAttribute("r", segment_info.repeat)); - CHECK(segment_timeline->AddChild(s_element.PassScopedPtr())); + RCHECK(segment_timeline->AddChild(std::move(s_element))); } return true; @@ -118,22 +121,30 @@ void TraverseNodesAndCollectNamespaces(const xmlNode* node, namespace xml { -XmlNode::XmlNode(const char* name) : node_(xmlNewNode(NULL, BAD_CAST name)) { - DCHECK(name); - DCHECK(node_); +class XmlNode::Impl { + public: + scoped_xml_ptr node; +}; + +XmlNode::XmlNode(const std::string& name) : impl_(new Impl) { + impl_->node.reset(xmlNewNode(NULL, BAD_CAST name.c_str())); + DCHECK(impl_->node); } +XmlNode::XmlNode(XmlNode&&) = default; + XmlNode::~XmlNode() {} -bool XmlNode::AddChild(scoped_xml_ptr child) { - DCHECK(node_); - DCHECK(child); - if (!xmlAddChild(node_.get(), child.get())) - return false; +XmlNode& XmlNode::operator=(XmlNode&&) = default; - // Reaching here means the ownership of |child| transfered to |node_|. +bool XmlNode::AddChild(XmlNode child) { + DCHECK(impl_->node); + DCHECK(child.impl_->node); + RCHECK(xmlAddChild(impl_->node.get(), child.impl_->node.get())); + + // Reaching here means the ownership of |child| transfered to |node|. // Release the pointer so that it doesn't get destructed in this scope. - ignore_result(child.release()); + ignore_result(child.impl_->node.release()); return true; } @@ -141,12 +152,12 @@ bool XmlNode::AddElements(const std::vector& elements) { for (size_t element_index = 0; element_index < elements.size(); ++element_index) { const Element& child_element = elements[element_index]; - XmlNode child_node(child_element.name.c_str()); + XmlNode child_node(child_element.name); for (std::map::const_iterator attribute_it = child_element.attributes.begin(); attribute_it != child_element.attributes.end(); ++attribute_it) { - child_node.SetStringAttribute(attribute_it->first.c_str(), - attribute_it->second); + RCHECK(child_node.SetStringAttribute(attribute_it->first, + attribute_it->second)); } // Note that somehow |SetContent| needs to be called before |AddElements| @@ -154,114 +165,128 @@ bool XmlNode::AddElements(const std::vector& elements) { child_node.SetContent(child_element.content); // Recursively set children for the child. - if (!child_node.AddElements(child_element.subelements)) - return false; + RCHECK(child_node.AddElements(child_element.subelements)); - if (!xmlAddChild(node_.get(), child_node.GetRawPtr())) { + if (!xmlAddChild(impl_->node.get(), child_node.impl_->node.get())) { LOG(ERROR) << "Failed to set child " << child_element.name << " to parent element " - << reinterpret_cast(node_->name); + << reinterpret_cast(impl_->node->name); return false; } - // Reaching here means the ownership of |child_node| transfered to |node_|. + // Reaching here means the ownership of |child_node| transfered to |node|. // Release the pointer so that it doesn't get destructed in this scope. - ignore_result(child_node.Release()); + child_node.impl_->node.release(); } return true; } -void XmlNode::SetStringAttribute(const char* attribute_name, +bool XmlNode::SetStringAttribute(const std::string& attribute_name, const std::string& attribute) { - DCHECK(node_); - DCHECK(attribute_name); - xmlSetProp(node_.get(), BAD_CAST attribute_name, BAD_CAST attribute.c_str()); + DCHECK(impl_->node); + return xmlSetProp(impl_->node.get(), BAD_CAST attribute_name.c_str(), + BAD_CAST attribute.c_str()) != nullptr; } -void XmlNode::SetIntegerAttribute(const char* attribute_name, uint64_t number) { - DCHECK(node_); - DCHECK(attribute_name); - xmlSetProp(node_.get(), - BAD_CAST attribute_name, - BAD_CAST (base::Uint64ToString(number).c_str())); +bool XmlNode::SetIntegerAttribute(const std::string& attribute_name, + uint64_t number) { + DCHECK(impl_->node); + return xmlSetProp(impl_->node.get(), BAD_CAST attribute_name.c_str(), + BAD_CAST(base::Uint64ToString(number).c_str())) != nullptr; } -void XmlNode::SetFloatingPointAttribute(const char* attribute_name, +bool XmlNode::SetFloatingPointAttribute(const std::string& attribute_name, double number) { - DCHECK(node_); - DCHECK(attribute_name); - xmlSetProp(node_.get(), BAD_CAST attribute_name, - BAD_CAST(base::DoubleToString(number).c_str())); + DCHECK(impl_->node); + return xmlSetProp(impl_->node.get(), BAD_CAST attribute_name.c_str(), + BAD_CAST(base::DoubleToString(number).c_str())) != nullptr; } -void XmlNode::SetId(uint32_t id) { - SetIntegerAttribute("id", id); +bool XmlNode::SetId(uint32_t id) { + return SetIntegerAttribute("id", id); } void XmlNode::SetContent(const std::string& content) { - DCHECK(node_); - xmlNodeSetContent(node_.get(), BAD_CAST content.c_str()); + DCHECK(impl_->node); + xmlNodeSetContent(impl_->node.get(), BAD_CAST content.c_str()); } -std::set XmlNode::ExtractReferencedNamespaces() { +std::set XmlNode::ExtractReferencedNamespaces() const { std::set namespaces; - TraverseNodesAndCollectNamespaces(node_.get(), &namespaces); + TraverseNodesAndCollectNamespaces(impl_->node.get(), &namespaces); return namespaces; } -scoped_xml_ptr XmlNode::PassScopedPtr() { - DVLOG(2) << "Passing node_."; - DCHECK(node_); - return std::move(node_); +std::string XmlNode::ToString(const std::string& comment) const { + // Create an xmlDoc from xmlNodePtr. The node is copied so ownership does not + // transfer. + xml::scoped_xml_ptr doc(xmlNewDoc(BAD_CAST "1.0")); + if (comment.empty()) { + xmlDocSetRootElement(doc.get(), xmlCopyNode(impl_->node.get(), true)); + } else { + xml::scoped_xml_ptr comment_xml( + xmlNewDocComment(doc.get(), BAD_CAST comment.c_str())); + xmlDocSetRootElement(doc.get(), comment_xml.get()); + xmlAddSibling(comment_xml.release(), xmlCopyNode(impl_->node.get(), true)); + } + + // Format the xmlDoc to string. + static const int kNiceFormat = 1; + int doc_str_size = 0; + xmlChar* doc_str = nullptr; + xmlDocDumpFormatMemoryEnc(doc.get(), &doc_str, &doc_str_size, "UTF-8", + kNiceFormat); + std::string output(doc_str, doc_str + doc_str_size); + xmlFree(doc_str); + return output; } -xmlNodePtr XmlNode::Release() { - DVLOG(2) << "Releasing node_."; - DCHECK(node_); - return node_.release(); +bool XmlNode::GetAttribute(const std::string& name, std::string* value) const { + xml::scoped_xml_ptr str( + xmlGetProp(impl_->node.get(), BAD_CAST name.c_str())); + if (!str) + return false; + *value = reinterpret_cast(str.get()); + return true; } -xmlNodePtr XmlNode::GetRawPtr() { - return node_.get(); +xmlNode* XmlNode::GetRawPtr() const { + return impl_->node.get(); } -RepresentationBaseXmlNode::RepresentationBaseXmlNode(const char* name) +RepresentationBaseXmlNode::RepresentationBaseXmlNode(const std::string& name) : XmlNode(name) {} RepresentationBaseXmlNode::~RepresentationBaseXmlNode() {} bool RepresentationBaseXmlNode::AddContentProtectionElements( const std::list& content_protection_elements) { - std::list::const_iterator content_protection_it = - content_protection_elements.begin(); - for (; content_protection_it != content_protection_elements.end(); - ++content_protection_it) { - if (!AddContentProtectionElement(*content_protection_it)) - return false; + for (const auto& elem : content_protection_elements) { + RCHECK(AddContentProtectionElement(elem)); } return true; } -void RepresentationBaseXmlNode::AddSupplementalProperty( +bool RepresentationBaseXmlNode::AddSupplementalProperty( const std::string& scheme_id_uri, const std::string& value) { - AddDescriptor("SupplementalProperty", scheme_id_uri, value); + return AddDescriptor("SupplementalProperty", scheme_id_uri, value); } -void RepresentationBaseXmlNode::AddEssentialProperty( +bool RepresentationBaseXmlNode::AddEssentialProperty( const std::string& scheme_id_uri, const std::string& value) { - AddDescriptor("EssentialProperty", scheme_id_uri, value); + return AddDescriptor("EssentialProperty", scheme_id_uri, value); } bool RepresentationBaseXmlNode::AddDescriptor( const std::string& descriptor_name, const std::string& scheme_id_uri, const std::string& value) { - XmlNode descriptor(descriptor_name.c_str()); - descriptor.SetStringAttribute("schemeIdUri", scheme_id_uri); + XmlNode descriptor(descriptor_name); + RCHECK(descriptor.SetStringAttribute("schemeIdUri", scheme_id_uri)); if (!value.empty()) - descriptor.SetStringAttribute("value", value); - return AddChild(descriptor.PassScopedPtr()); + RCHECK(descriptor.SetStringAttribute("value", value)); + return AddChild(std::move(descriptor)); } bool RepresentationBaseXmlNode::AddContentProtectionElement( @@ -270,43 +295,34 @@ bool RepresentationBaseXmlNode::AddContentProtectionElement( // @value is an optional attribute. if (!content_protection_element.value.empty()) { - content_protection_node.SetStringAttribute( - "value", content_protection_element.value); + RCHECK(content_protection_node.SetStringAttribute( + "value", content_protection_element.value)); } - content_protection_node.SetStringAttribute( - "schemeIdUri", content_protection_element.scheme_id_uri); + RCHECK(content_protection_node.SetStringAttribute( + "schemeIdUri", content_protection_element.scheme_id_uri)); - typedef std::map AttributesMapType; - const AttributesMapType& additional_attributes = - content_protection_element.additional_attributes; - - AttributesMapType::const_iterator attributes_it = - additional_attributes.begin(); - for (; attributes_it != additional_attributes.end(); ++attributes_it) { - content_protection_node.SetStringAttribute(attributes_it->first.c_str(), - attributes_it->second); + for (const auto& pair : content_protection_element.additional_attributes) { + RCHECK(content_protection_node.SetStringAttribute(pair.first, pair.second)); } - if (!content_protection_node.AddElements( - content_protection_element.subelements)) { - return false; - } - return AddChild(content_protection_node.PassScopedPtr()); + RCHECK(content_protection_node.AddElements( + content_protection_element.subelements)); + return AddChild(std::move(content_protection_node)); } AdaptationSetXmlNode::AdaptationSetXmlNode() : RepresentationBaseXmlNode("AdaptationSet") {} AdaptationSetXmlNode::~AdaptationSetXmlNode() {} -void AdaptationSetXmlNode::AddAccessibilityElement( +bool AdaptationSetXmlNode::AddAccessibilityElement( const std::string& scheme_id_uri, const std::string& value) { - AddDescriptor("Accessibility", scheme_id_uri, value); + return AddDescriptor("Accessibility", scheme_id_uri, value); } -void AdaptationSetXmlNode::AddRoleElement(const std::string& scheme_id_uri, +bool AdaptationSetXmlNode::AddRoleElement(const std::string& scheme_id_uri, const std::string& value) { - AddDescriptor("Role", scheme_id_uri, value); + return AddDescriptor("Role", scheme_id_uri, value); } RepresentationXmlNode::RepresentationXmlNode() @@ -323,39 +339,36 @@ bool RepresentationXmlNode::AddVideoInfo(const VideoInfo& video_info, } if (video_info.has_pixel_width() && video_info.has_pixel_height()) { - SetStringAttribute("sar", base::IntToString(video_info.pixel_width()) + - ":" + - base::IntToString(video_info.pixel_height())); + RCHECK(SetStringAttribute( + "sar", base::IntToString(video_info.pixel_width()) + ":" + + base::IntToString(video_info.pixel_height()))); } if (set_width) - SetIntegerAttribute("width", video_info.width()); + RCHECK(SetIntegerAttribute("width", video_info.width())); if (set_height) - SetIntegerAttribute("height", video_info.height()); + RCHECK(SetIntegerAttribute("height", video_info.height())); if (set_frame_rate) { - SetStringAttribute("frameRate", - base::IntToString(video_info.time_scale()) + "/" + - base::IntToString(video_info.frame_duration())); + RCHECK(SetStringAttribute( + "frameRate", base::IntToString(video_info.time_scale()) + "/" + + base::IntToString(video_info.frame_duration()))); } if (video_info.has_playback_rate()) { - SetStringAttribute("maxPlayoutRate", - base::IntToString(video_info.playback_rate())); + RCHECK(SetStringAttribute("maxPlayoutRate", + base::IntToString(video_info.playback_rate()))); // Since the trick play stream contains only key frames, there is no coding // dependency on the main stream. Simply set the codingDependency to false. // TODO(hmchen): propagate this attribute up to the AdaptationSet, since // all are set to false. - SetStringAttribute("codingDependency", "false"); + RCHECK(SetStringAttribute("codingDependency", "false")); } return true; } bool RepresentationXmlNode::AddAudioInfo(const AudioInfo& audio_info) { - if (!AddAudioChannelInfo(audio_info)) - return false; - - AddAudioSamplingRateInfo(audio_info); - return true; + return AddAudioChannelInfo(audio_info) && + AddAudioSamplingRateInfo(audio_info); } bool RepresentationXmlNode::AddVODOnlyInfo(const MediaInfo& media_info) { @@ -363,8 +376,7 @@ bool RepresentationXmlNode::AddVODOnlyInfo(const MediaInfo& media_info) { XmlNode base_url("BaseURL"); base_url.SetContent(media_info.media_file_url()); - if (!AddChild(base_url.PassScopedPtr())) - return false; + RCHECK(AddChild(std::move(base_url))); } const bool need_segment_base = @@ -374,31 +386,29 @@ bool RepresentationXmlNode::AddVODOnlyInfo(const MediaInfo& media_info) { if (need_segment_base) { XmlNode segment_base("SegmentBase"); if (media_info.has_index_range()) { - segment_base.SetStringAttribute("indexRange", - RangeToString(media_info.index_range())); + RCHECK(segment_base.SetStringAttribute( + "indexRange", RangeToString(media_info.index_range()))); } if (media_info.has_reference_time_scale()) { - segment_base.SetIntegerAttribute("timescale", - media_info.reference_time_scale()); + RCHECK(segment_base.SetIntegerAttribute( + "timescale", media_info.reference_time_scale())); } if (media_info.has_presentation_time_offset()) { - segment_base.SetIntegerAttribute("presentationTimeOffset", - media_info.presentation_time_offset()); + RCHECK(segment_base.SetIntegerAttribute( + "presentationTimeOffset", media_info.presentation_time_offset())); } if (media_info.has_init_range()) { XmlNode initialization("Initialization"); - initialization.SetStringAttribute("range", - RangeToString(media_info.init_range())); + RCHECK(initialization.SetStringAttribute( + "range", RangeToString(media_info.init_range()))); - if (!segment_base.AddChild(initialization.PassScopedPtr())) - return false; + RCHECK(segment_base.AddChild(std::move(initialization))); } - if (!AddChild(segment_base.PassScopedPtr())) - return false; + RCHECK(AddChild(std::move(segment_base))); } return true; @@ -410,50 +420,48 @@ bool RepresentationXmlNode::AddLiveOnlyInfo( uint32_t start_number) { XmlNode segment_template("SegmentTemplate"); if (media_info.has_reference_time_scale()) { - segment_template.SetIntegerAttribute("timescale", - media_info.reference_time_scale()); + RCHECK(segment_template.SetIntegerAttribute( + "timescale", media_info.reference_time_scale())); } if (media_info.has_presentation_time_offset()) { - segment_template.SetIntegerAttribute("presentationTimeOffset", - media_info.presentation_time_offset()); + RCHECK(segment_template.SetIntegerAttribute( + "presentationTimeOffset", media_info.presentation_time_offset())); } if (media_info.has_init_segment_url()) { - segment_template.SetStringAttribute("initialization", - media_info.init_segment_url()); + RCHECK(segment_template.SetStringAttribute("initialization", + media_info.init_segment_url())); } if (media_info.has_segment_template_url()) { - segment_template.SetStringAttribute("media", - media_info.segment_template_url()); - segment_template.SetIntegerAttribute("startNumber", start_number); + RCHECK(segment_template.SetStringAttribute( + "media", media_info.segment_template_url())); + RCHECK(segment_template.SetIntegerAttribute("startNumber", start_number)); } if (!segment_infos.empty()) { // Don't use SegmentTimeline if all segments except the last one are of // the same duration. if (IsTimelineConstantDuration(segment_infos, start_number)) { - segment_template.SetIntegerAttribute("duration", - segment_infos.front().duration); + RCHECK(segment_template.SetIntegerAttribute( + "duration", segment_infos.front().duration)); if (FLAGS_dash_add_last_segment_number_when_needed) { uint32_t last_segment_number = start_number - 1; for (const auto& segment_info_element : segment_infos) last_segment_number += segment_info_element.repeat + 1; - AddSupplementalProperty( + RCHECK(AddSupplementalProperty( "http://dashif.org/guidelines/last-segment-number", - std::to_string(last_segment_number)); + std::to_string(last_segment_number))); } } else { XmlNode segment_timeline("SegmentTimeline"); - if (!PopulateSegmentTimeline(segment_infos, &segment_timeline) || - !segment_template.AddChild(segment_timeline.PassScopedPtr())) { - return false; - } + RCHECK(PopulateSegmentTimeline(segment_infos, &segment_timeline)); + RCHECK(segment_template.AddChild(std::move(segment_timeline))); } } - return AddChild(segment_template.PassScopedPtr()); + return AddChild(std::move(segment_template)); } bool RepresentationXmlNode::AddAudioChannelInfo(const AudioInfo& audio_info) { @@ -549,10 +557,11 @@ bool RepresentationXmlNode::AddAudioChannelInfo(const AudioInfo& audio_info) { // MPD expects one number for sampling frequency, or if it is a range it should // be space separated. -void RepresentationXmlNode::AddAudioSamplingRateInfo( +bool RepresentationXmlNode::AddAudioSamplingRateInfo( const AudioInfo& audio_info) { - if (audio_info.has_sampling_frequency()) - SetIntegerAttribute("audioSamplingRate", audio_info.sampling_frequency()); + return !audio_info.has_sampling_frequency() || + SetIntegerAttribute("audioSamplingRate", + audio_info.sampling_frequency()); } } // namespace xml diff --git a/packager/mpd/base/xml/xml_node.h b/packager/mpd/base/xml/xml_node.h index 976730d4d3..4aec56a065 100644 --- a/packager/mpd/base/xml/xml_node.h +++ b/packager/mpd/base/xml/xml_node.h @@ -10,21 +10,32 @@ #ifndef MPD_BASE_XML_XML_NODE_H_ #define MPD_BASE_XML_XML_NODE_H_ -#include #include #include #include +#include +#include +#include "packager/base/compiler_specific.h" #include "packager/base/macros.h" #include "packager/mpd/base/content_protection_element.h" #include "packager/mpd/base/media_info.pb.h" -#include "packager/mpd/base/xml/scoped_xml_ptr.h" + +typedef struct _xmlNode xmlNode; namespace shaka { +class MpdBuilder; struct SegmentInfo; +namespace xml { +class XmlNode; +} // namespace xml + +// Defined in tests under mpd/test/xml_compare.h +bool XmlEqual(const std::string& xml1, const xml::XmlNode& xml2); + namespace xml { /// These classes are wrapper classes for XML elements for generating MPD. @@ -34,37 +45,41 @@ class XmlNode { public: /// Make an XML element. /// @param name is the name of the element, which should not be NULL. - explicit XmlNode(const char* name); + explicit XmlNode(const std::string& name); + XmlNode(XmlNode&&); virtual ~XmlNode(); + XmlNode& operator=(XmlNode&&); + /// Add a child element to this element. - /// @param child is a xmlNode to add as a child for this element. Ownership - /// of the child node is transferred. + /// @param child is an XmlNode to add as a child for this element. /// @return true on success, false otherwise. - bool AddChild(scoped_xml_ptr child); + bool AddChild(XmlNode child) WARN_UNUSED_RESULT; /// Adds Elements to this node using the Element struct. - bool AddElements(const std::vector& elements); + bool AddElements(const std::vector& elements) WARN_UNUSED_RESULT; /// Set a string attribute. /// @param attribute_name The name (lhs) of the attribute. /// @param attribute The value (rhs) of the attribute. - void SetStringAttribute(const char* attribute_name, - const std::string& attribute); + bool SetStringAttribute(const std::string& attribute_name, + const std::string& attribute) WARN_UNUSED_RESULT; /// Sets an integer attribute. /// @param attribute_name The name (lhs) of the attribute. /// @param number The value (rhs) of the attribute. - void SetIntegerAttribute(const char* attribute_name, uint64_t number); + bool SetIntegerAttribute(const std::string& attribute_name, + uint64_t number) WARN_UNUSED_RESULT; /// Set a floating point number attribute. /// @param attribute_name is the name of the attribute to set. /// @param number is the value (rhs) of the attribute. - void SetFloatingPointAttribute(const char* attribute_name, double number); + bool SetFloatingPointAttribute(const std::string& attribute_name, + double number) WARN_UNUSED_RESULT; /// Sets 'id=@a id' attribute. /// @param id is the ID for this element. - void SetId(uint32_t id); + bool SetId(uint32_t id) WARN_UNUSED_RESULT; /// Set the contents of an XML element using a string. /// This cannot set child elements because <> will become < and &rt; @@ -75,22 +90,28 @@ class XmlNode { void SetContent(const std::string& content); /// @return namespaces used in the node and its descendents. - std::set ExtractReferencedNamespaces(); + std::set ExtractReferencedNamespaces() const; - /// Transfer the ownership of the xmlNodePtr. After calling this method, the - /// behavior of any methods, except the destructor, is undefined. - /// @return The resource of this object. - scoped_xml_ptr PassScopedPtr(); + /// @param comment The body of a comment to add to the top of the XML. + /// @return A string containing the XML. + std::string ToString(const std::string& comment) const; - /// Release the xmlNodePtr of this object. After calling this method, the - /// behavior of any methods, except the destructor, is undefined. - xmlNodePtr Release(); - - /// @return Raw pointer to the element. - xmlNodePtr GetRawPtr(); + /// Gets the attribute with the given name. + /// @param name The name of the attribute to get. + /// @param value [OUT] where to put the resulting value. + /// @return True if the attribute exists, false if not. + bool GetAttribute(const std::string& name, std::string* value) const; private: - scoped_xml_ptr node_; + friend bool shaka::XmlEqual(const std::string& xml1, + const xml::XmlNode& xml2); + xmlNode* GetRawPtr() const; + + // Don't use xmlNode directly so we don't have to forward-declare a bunch of + // libxml types to define the scoped_xml_ptr type. This allows us to only + // include libxml headers in a few source files. + class Impl; + std::unique_ptr impl_; DISALLOW_COPY_AND_ASSIGN(XmlNode); }; @@ -102,20 +123,21 @@ class RepresentationBaseXmlNode : public XmlNode { public: ~RepresentationBaseXmlNode() override; bool AddContentProtectionElements( - const std::list& content_protection_elements); + const std::list& content_protection_elements) + WARN_UNUSED_RESULT; /// @param scheme_id_uri is content of the schemeIdUri attribute. /// @param value is the content of value attribute. - void AddSupplementalProperty(const std::string& scheme_id_uri, - const std::string& value); + bool AddSupplementalProperty(const std::string& scheme_id_uri, + const std::string& value) WARN_UNUSED_RESULT; /// @param scheme_id_uri is content of the schemeIdUri attribute. /// @param value is the content of value attribute. - void AddEssentialProperty(const std::string& scheme_id_uri, - const std::string& value); + bool AddEssentialProperty(const std::string& scheme_id_uri, + const std::string& value) WARN_UNUSED_RESULT; protected: - explicit RepresentationBaseXmlNode(const char* name); + explicit RepresentationBaseXmlNode(const std::string& name); /// Add a Descriptor. /// @param descriptor_name is the name of the descriptor. @@ -123,11 +145,12 @@ class RepresentationBaseXmlNode : public XmlNode { /// @param value is the content of value attribute. bool AddDescriptor(const std::string& descriptor_name, const std::string& scheme_id_uri, - const std::string& value); + const std::string& value) WARN_UNUSED_RESULT; private: bool AddContentProtectionElement( - const ContentProtectionElement& content_protection_element); + const ContentProtectionElement& content_protection_element) + WARN_UNUSED_RESULT; DISALLOW_COPY_AND_ASSIGN(RepresentationBaseXmlNode); }; @@ -140,13 +163,13 @@ class AdaptationSetXmlNode : public RepresentationBaseXmlNode { /// @param scheme_id_uri is content of the schemeIdUri attribute. /// @param value is the content of value attribute. - void AddAccessibilityElement(const std::string& scheme_id_uri, - const std::string& value); + bool AddAccessibilityElement(const std::string& scheme_id_uri, + const std::string& value) WARN_UNUSED_RESULT; /// @param scheme_id_uri is content of the schemeIdUri attribute. /// @param value is the content of value attribute. - void AddRoleElement(const std::string& scheme_id_uri, - const std::string& value); + bool AddRoleElement(const std::string& scheme_id_uri, + const std::string& value) WARN_UNUSED_RESULT; private: DISALLOW_COPY_AND_ASSIGN(AdaptationSetXmlNode); @@ -168,33 +191,35 @@ class RepresentationXmlNode : public RepresentationBaseXmlNode { bool AddVideoInfo(const MediaInfo::VideoInfo& video_info, bool set_width, bool set_height, - bool set_frame_rate); + bool set_frame_rate) WARN_UNUSED_RESULT; /// Adds audio metadata to the MPD. /// @param audio_info constains the AudioInfos for a Representation. /// @return true if successfully set attributes and children elements (if /// applicable), false otherwise. - bool AddAudioInfo(const MediaInfo::AudioInfo& audio_info); + bool AddAudioInfo(const MediaInfo::AudioInfo& audio_info) WARN_UNUSED_RESULT; /// Adds fields that are specific to VOD. This ignores @a media_info fields /// for Live. /// @param media_info is a MediaInfo with VOD information. /// @return true on success, false otherwise. - bool AddVODOnlyInfo(const MediaInfo& media_info); + bool AddVODOnlyInfo(const MediaInfo& media_info) WARN_UNUSED_RESULT; /// @param segment_infos is a set of SegmentInfos. This method assumes that /// SegmentInfos are sorted by its start time. bool AddLiveOnlyInfo(const MediaInfo& media_info, const std::list& segment_infos, - uint32_t start_number); + uint32_t start_number) WARN_UNUSED_RESULT; private: // Add AudioChannelConfiguration element. Note that it is a required element // for audio Representations. - bool AddAudioChannelInfo(const MediaInfo::AudioInfo& audio_info); + bool AddAudioChannelInfo(const MediaInfo::AudioInfo& audio_info) + WARN_UNUSED_RESULT; // Add audioSamplingRate attribute to this element, if present. - void AddAudioSamplingRateInfo(const MediaInfo::AudioInfo& audio_info); + bool AddAudioSamplingRateInfo(const MediaInfo::AudioInfo& audio_info) + WARN_UNUSED_RESULT; DISALLOW_COPY_AND_ASSIGN(RepresentationXmlNode); }; diff --git a/packager/mpd/base/xml/xml_node_unittest.cc b/packager/mpd/base/xml/xml_node_unittest.cc index 2376c21cc9..c24aac35b4 100644 --- a/packager/mpd/base/xml/xml_node_unittest.cc +++ b/packager/mpd/base/xml/xml_node_unittest.cc @@ -145,14 +145,14 @@ TEST(XmlNodeTest, ExtractReferencedNamespaces) { XmlNode child("child1"); child.SetContent("child1 content"); - child.AddChild(grand_child_with_namespace.PassScopedPtr()); + ASSERT_TRUE(child.AddChild(std::move(grand_child_with_namespace))); XmlNode child_with_namespace("child_ns:child2"); child_with_namespace.SetContent("child2 content"); XmlNode root("root"); - root.AddChild(child.PassScopedPtr()); - root.AddChild(child_with_namespace.PassScopedPtr()); + ASSERT_TRUE(root.AddChild(std::move(child))); + ASSERT_TRUE(root.AddChild(std::move(child_with_namespace))); EXPECT_THAT(root.ExtractReferencedNamespaces(), ElementsAre("child_ns", "grand_ns")); @@ -160,13 +160,13 @@ TEST(XmlNodeTest, ExtractReferencedNamespaces) { TEST(XmlNodeTest, ExtractReferencedNamespacesFromAttributes) { XmlNode child("child"); - child.SetStringAttribute("child_attribute_ns:attribute", - "child attribute value"); + ASSERT_TRUE(child.SetStringAttribute("child_attribute_ns:attribute", + "child attribute value")); XmlNode root("root"); - root.AddChild(child.PassScopedPtr()); - root.SetStringAttribute("root_attribute_ns:attribute", - "root attribute value"); + ASSERT_TRUE(root.AddChild(std::move(child))); + ASSERT_TRUE(root.SetStringAttribute("root_attribute_ns:attribute", + "root attribute value")); EXPECT_THAT(root.ExtractReferencedNamespaces(), ElementsAre("child_attribute_ns", "root_attribute_ns")); @@ -195,9 +195,9 @@ TEST(XmlNodeTest, AddContentProtectionElements) { content_protections.push_back(content_protection_clearkey); RepresentationXmlNode representation; - representation.AddContentProtectionElements(content_protections); + ASSERT_TRUE(representation.AddContentProtectionElements(content_protections)); EXPECT_THAT( - representation.GetRawPtr(), + representation, XmlNodeEqual( "\n" " \n" " set_channel_mpeg_value(6); RepresentationXmlNode representation; - representation.AddAudioInfo(audio_info); - EXPECT_THAT( - representation.GetRawPtr(), - XmlNodeEqual( - "\n" - " \n" - "\n")); + ASSERT_TRUE(representation.AddAudioInfo(audio_info)); + EXPECT_THAT(representation, + XmlNodeEqual("\n" + " \n" + "\n")); } TEST(XmlNodeTest, AddEC3AudioInfoMPEGSchemeJOC) { @@ -261,9 +259,9 @@ TEST(XmlNodeTest, AddEC3AudioInfoMPEGSchemeJOC) { audio_info.mutable_codec_specific_data()->set_ec3_joc_complexity(16); RepresentationXmlNode representation; - representation.AddAudioInfo(audio_info); + ASSERT_TRUE(representation.AddAudioInfo(audio_info)); EXPECT_THAT( - representation.GetRawPtr(), + representation, XmlNodeEqual( "\n" " set_ac4_cbi_flag(false); RepresentationXmlNode representation; - representation.AddAudioInfo(audio_info); + ASSERT_TRUE(representation.AddAudioInfo(audio_info)); EXPECT_THAT( - representation.GetRawPtr(), - XmlNodeEqual( - "\n" - " \n" - "\n")); + representation, + XmlNodeEqual( + "\n" + " \n" + "\n")); } TEST(XmlNodeTest, AddAC4AudioInfoMPEGScheme) { @@ -315,16 +313,14 @@ TEST(XmlNodeTest, AddAC4AudioInfoMPEGScheme) { codec_data->set_ac4_cbi_flag(false); RepresentationXmlNode representation; - representation.AddAudioInfo(audio_info); - EXPECT_THAT( - representation.GetRawPtr(), - XmlNodeEqual( - "\n" - " \n" - "\n")); + ASSERT_TRUE(representation.AddAudioInfo(audio_info)); + EXPECT_THAT(representation, + XmlNodeEqual("\n" + " \n" + "\n")); } TEST(XmlNodeTest, AddAC4AudioInfoMPEGSchemeIMS) { @@ -338,20 +334,19 @@ TEST(XmlNodeTest, AddAC4AudioInfoMPEGSchemeIMS) { codec_data->set_ac4_cbi_flag(false); RepresentationXmlNode representation; - representation.AddAudioInfo(audio_info); + ASSERT_TRUE(representation.AddAudioInfo(audio_info)); EXPECT_THAT( - representation.GetRawPtr(), - XmlNodeEqual( - "\n" - " \n" - " \n" - "\n")); + representation, + XmlNodeEqual("\n" + " \n" + " \n" + "\n")); } class LiveSegmentTimelineTest : public ::testing::Test { @@ -380,7 +375,7 @@ TEST_F(LiveSegmentTimelineTest, OneSegmentInfo) { representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); EXPECT_THAT( - representation.GetRawPtr(), + representation, XmlNodeEqual("" " " @@ -400,7 +395,7 @@ TEST_F(LiveSegmentTimelineTest, OneSegmentInfoNonZeroStartTime) { ASSERT_TRUE( representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); - EXPECT_THAT(representation.GetRawPtr(), + EXPECT_THAT(representation, XmlNodeEqual( "" " " @@ -425,7 +420,7 @@ TEST_F(LiveSegmentTimelineTest, OneSegmentInfoMatchingStartTimeAndNumber) { representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); EXPECT_THAT( - representation.GetRawPtr(), + representation, XmlNodeEqual("" " " @@ -452,7 +447,7 @@ TEST_F(LiveSegmentTimelineTest, AllSegmentsSameDurationExpectLastOne) { representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); EXPECT_THAT( - representation.GetRawPtr(), + representation, XmlNodeEqual("" " " @@ -478,7 +473,7 @@ TEST_F(LiveSegmentTimelineTest, SecondSegmentInfoNonZeroRepeat) { ASSERT_TRUE( representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); - EXPECT_THAT(representation.GetRawPtr(), + EXPECT_THAT(representation, XmlNodeEqual( "" " " @@ -510,7 +505,7 @@ TEST_F(LiveSegmentTimelineTest, TwoSegmentInfoWithGap) { ASSERT_TRUE( representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); - EXPECT_THAT(representation.GetRawPtr(), + EXPECT_THAT(representation, XmlNodeEqual( "" " " @@ -522,31 +517,31 @@ TEST_F(LiveSegmentTimelineTest, TwoSegmentInfoWithGap) { "")); } -TEST_F(LiveSegmentTimelineTest, LastSegmentNumberSupplementalProperty) { - const uint32_t kStartNumber = 1; - const uint64_t kStartTime = 0; - const uint64_t kDuration = 100; - const uint64_t kRepeat = 9; - - std::list segment_infos = { - {kStartTime, kDuration, kRepeat}, - }; - RepresentationXmlNode representation; - FLAGS_dash_add_last_segment_number_when_needed = true; - - ASSERT_TRUE( +TEST_F(LiveSegmentTimelineTest, LastSegmentNumberSupplementalProperty) { + const uint32_t kStartNumber = 1; + const uint64_t kStartTime = 0; + const uint64_t kDuration = 100; + const uint64_t kRepeat = 9; + + std::list segment_infos = { + {kStartTime, kDuration, kRepeat}, + }; + RepresentationXmlNode representation; + FLAGS_dash_add_last_segment_number_when_needed = true; + + ASSERT_TRUE( representation.AddLiveOnlyInfo(media_info_, segment_infos, kStartNumber)); - - EXPECT_THAT( - representation.GetRawPtr(), - XmlNodeEqual("" - "" - " " - "")); - FLAGS_dash_add_last_segment_number_when_needed = false; -} + + EXPECT_THAT( + representation, + XmlNodeEqual("" + "" + " " + "")); + FLAGS_dash_add_last_segment_number_when_needed = false; +} } // namespace xml } // namespace shaka diff --git a/packager/mpd/test/xml_compare.cc b/packager/mpd/test/xml_compare.cc index accbea628a..c8c8b57d37 100644 --- a/packager/mpd/test/xml_compare.cc +++ b/packager/mpd/test/xml_compare.cc @@ -144,54 +144,41 @@ bool CompareNodes(xmlNodePtr node1, xmlNodePtr node2) { bool XmlEqual(const std::string& xml1, const std::string& xml2) { xml::scoped_xml_ptr xml1_doc(GetDocFromString(xml1)); xml::scoped_xml_ptr xml2_doc(GetDocFromString(xml2)); - return XmlEqual(xml1_doc.get(), xml2_doc.get()); -} - -bool XmlEqual(const std::string& xml1, xmlDocPtr xml2) { - xml::scoped_xml_ptr xml1_doc(GetDocFromString(xml1)); - return XmlEqual(xml1_doc.get(), xml2); -} - -bool XmlEqual(xmlDocPtr xml1, xmlDocPtr xml2) { - if (!xml1 || !xml2) { - LOG(ERROR) << "xml1 and/or xml2 are not valid XML."; + if (!xml1_doc || !xml2_doc) { + LOG(ERROR) << "xml1/xml2 is not valid XML."; return false; } - xmlNodePtr xml1_root_element = xmlDocGetRootElement(xml1); - xmlNodePtr xml2_root_element = xmlDocGetRootElement(xml2); + xmlNodePtr xml1_root_element = xmlDocGetRootElement(xml1_doc.get()); + xmlNodePtr xml2_root_element = xmlDocGetRootElement(xml2_doc.get()); if (!xml1_root_element || !xml2_root_element) - return xml1_root_element == xml2_root_element; - + return false; return CompareNodes(xml1_root_element, xml2_root_element); } -bool XmlEqual(const std::string& xml1, xmlNodePtr xml2) { +bool XmlEqual(const std::string& xml1, + const base::Optional& xml2) { + return xml2 && XmlEqual(xml1, *xml2); +} + +bool XmlEqual(const std::string& xml1, const xml::XmlNode& xml2) { xml::scoped_xml_ptr xml1_doc(GetDocFromString(xml1)); if (!xml1_doc) { - LOG(ERROR) << "xml1 are not valid XML."; + LOG(ERROR) << "xml1 is not valid XML."; return false; } xmlNodePtr xml1_root_element = xmlDocGetRootElement(xml1_doc.get()); if (!xml1_root_element) return false; - return CompareNodes(xml1_root_element, xml2); + return CompareNodes(xml1_root_element, xml2.GetRawPtr()); } -std::string XmlNodeToString(xmlNodePtr xml_node) { - // Create an xmlDoc from xmlNodePtr. The node is copied so ownership does not - // transfer. - xml::scoped_xml_ptr doc(xmlNewDoc(BAD_CAST "")); - xmlDocSetRootElement(doc.get(), xmlCopyNode(xml_node, true)); +std::string XmlNodeToString(const base::Optional& xml_node) { + return xml_node ? XmlNodeToString(*xml_node) : "$ERROR$"; +} - // Format the xmlDoc to string. - static const int kNiceFormat = 1; - int doc_str_size = 0; - xmlChar* doc_str = nullptr; - xmlDocDumpFormatMemoryEnc(doc.get(), &doc_str, &doc_str_size, "UTF-8", - kNiceFormat); - std::string output(doc_str, doc_str + doc_str_size); - xmlFree(doc_str); +std::string XmlNodeToString(const xml::XmlNode& xml_node) { + std::string output = xml_node.ToString(/* comment= */ ""); // Remove the first line from the formatted string: // diff --git a/packager/mpd/test/xml_compare.h b/packager/mpd/test/xml_compare.h index 1759a81ac4..1f949897fe 100644 --- a/packager/mpd/test/xml_compare.h +++ b/packager/mpd/test/xml_compare.h @@ -6,7 +6,9 @@ #include +#include "packager/base/optional.h" #include "packager/mpd/base/xml/scoped_xml_ptr.h" +#include "packager/mpd/base/xml/xml_node.h" namespace shaka { @@ -19,13 +21,13 @@ namespace shaka { /// @param xml2 is compared against @a xml1. /// @return true if @a xml1 and @a xml2 are equivalent, false otherwise. bool XmlEqual(const std::string& xml1, const std::string& xml2); -bool XmlEqual(const std::string& xml1, xmlDocPtr xml2); -bool XmlEqual(xmlDocPtr xml1, xmlDocPtr xml2); -bool XmlEqual(const std::string& xml1, xmlNodePtr xml2); +bool XmlEqual(const std::string& xml1, const xml::XmlNode& xml2); +bool XmlEqual(const std::string& xml1, + const base::Optional& xml2); /// Get string representation of the xml node. -/// Note that the ownership is not transferred. -std::string XmlNodeToString(xmlNodePtr xml_node); +std::string XmlNodeToString(const xml::XmlNode& xml_node); +std::string XmlNodeToString(const base::Optional& xml_node); /// Match an xmlNodePtr with an xml in string representation. MATCHER_P(XmlNodeEqual, @@ -38,14 +40,16 @@ MATCHER_P(XmlNodeEqual, /// Match the attribute of an xmlNodePtr with expected value. /// Note that the ownership is not transferred. MATCHER_P2(AttributeEqual, attribute, expected_value, "") { - xml::scoped_xml_ptr attribute_xml_str( - xmlGetProp(arg, BAD_CAST attribute)); - if (!attribute_xml_str) { + if (!arg) { + *result_listener << "returned error"; + return false; + } + + std::string actual_value; + if (!arg->GetAttribute(attribute, &actual_value)) { *result_listener << "no attribute '" << attribute << "'"; return false; } - std::string actual_value = - reinterpret_cast(attribute_xml_str.get()); *result_listener << actual_value; return expected_value == actual_value; } @@ -53,9 +57,12 @@ MATCHER_P2(AttributeEqual, attribute, expected_value, "") { /// Check if the attribute is set in an xmlNodePtr. /// Note that the ownership is not transferred. MATCHER_P(AttributeSet, attribute, "") { - xml::scoped_xml_ptr attribute_xml_str( - xmlGetProp(arg, BAD_CAST attribute)); - return attribute_xml_str != nullptr; + if (!arg) { + *result_listener << "returned error"; + return false; + } + std::string unused; + return arg->GetAttribute(attribute, &unused); } } // namespace shaka