From 7c5508555c3196052ac506acd7f5cfeb7aaa5197 Mon Sep 17 00:00:00 2001 From: Aaron Vaage Date: Tue, 29 Aug 2017 10:54:29 -0700 Subject: [PATCH] Made Verify Functions Return Status There are multiple functions we use to verify input from the user and must of them would return a boolean expect for the top level function which would return a Status. Each function would log a message but to the user the error was not clear. This change makes each verify function return a status so that the actual problem will be surfaced easier. Change-Id: I12ab43f8ca3a4d379b4309336644a70eb8cbbbe0 --- packager/media/base/muxer_util.cc | 67 +++++---- packager/media/base/muxer_util.h | 6 +- packager/media/base/muxer_util_unittest.cc | 56 ++++---- packager/packager.cc | 158 ++++++++++++--------- 4 files changed, 161 insertions(+), 126 deletions(-) diff --git a/packager/media/base/muxer_util.cc b/packager/media/base/muxer_util.cc index 77e72664fd..86571cb062 100644 --- a/packager/media/base/muxer_util.cc +++ b/packager/media/base/muxer_util.cc @@ -19,26 +19,33 @@ namespace shaka { namespace { -bool ValidateFormatTag(const std::string& format_tag) { - DCHECK(!format_tag.empty()); +Status ValidateFormatTag(const std::string& format_tag) { + if (format_tag.empty()) { + return Status(error::INVALID_ARGUMENT, "Format tag should not be empty"); + } + // Format tag should follow this prototype: %0[width]d. if (format_tag.size() > 3 && format_tag[0] == '%' && format_tag[1] == '0' && format_tag[format_tag.size() - 1] == 'd') { unsigned out; - if (base::StringToUint(format_tag.substr(2, format_tag.size() - 3), &out)) - return true; + if (base::StringToUint(format_tag.substr(2, format_tag.size() - 3), &out)) { + return Status::OK; + } } - LOG(ERROR) << "SegmentTemplate: Format tag should follow this prototype: " - << "%0[width]d if exist."; - return false; + + return Status( + error::INVALID_ARGUMENT, + "Format tag should follow this prototype: %0[width]d if exist."); } } // namespace namespace media { -bool ValidateSegmentTemplate(const std::string& segment_template) { - if (segment_template.empty()) - return false; +Status ValidateSegmentTemplate(const std::string& segment_template) { + if (segment_template.empty()) { + return Status(error::INVALID_ARGUMENT, + "Segment template should not be empty."); + } std::vector splits = base::SplitString( segment_template, "$", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); @@ -47,8 +54,8 @@ bool ValidateSegmentTemplate(const std::string& segment_template) { // Allowed identifiers: $$, $RepresentationID$, $Number$, $Bandwidth$, $Time$. // "$" always appears in pairs, so there should be odd number of splits. if (splits.size() % 2 == 0) { - LOG(ERROR) << "SegmentTemplate: '$' should appear in pairs."; - return false; + return Status(error::INVALID_ARGUMENT, + "In segment templates, '$' should appear in pairs."); } bool has_number = false; @@ -61,49 +68,51 @@ bool ValidateSegmentTemplate(const std::string& segment_template) { size_t format_pos = splits[i].find('%'); std::string identifier = splits[i].substr(0, format_pos); if (format_pos != std::string::npos) { - if (!ValidateFormatTag(splits[i].substr(format_pos))) - return false; + Status tag_check = ValidateFormatTag(splits[i].substr(format_pos)); + if (!tag_check.ok()) { + return tag_check; + } } // TODO(kqyang): Support "RepresentationID". if (identifier == "RepresentationID") { - NOTIMPLEMENTED() << "SegmentTemplate: $RepresentationID$ is not supported " - "yet."; - return false; + return Status( + error::UNIMPLEMENTED, + "Segment template flag $RepresentationID$ is not supported yet."); } else if (identifier == "Number") { has_number = true; } else if (identifier == "Time") { has_time = true; } else if (identifier == "") { if (format_pos != std::string::npos) { - LOG(ERROR) << "SegmentTemplate: $$ should not have any format tags."; - return false; + return Status(error::INVALID_ARGUMENT, + "'$$' should not have any format tags."); } } else if (identifier != "Bandwidth") { - LOG(ERROR) << "SegmentTemplate: '$" << splits[i] - << "$' is not a valid identifier."; - return false; + return Status(error::INVALID_ARGUMENT, + "'$" + splits[i] + "$' is not a valid identifier."); } } if (has_number && has_time) { - LOG(ERROR) << "SegmentTemplate: $Number$ and $Time$ should not co-exist."; - return false; + return Status( + error::INVALID_ARGUMENT, + "In segment templates $Number$ and $Time$ should not co-exist."); } if (!has_number && !has_time) { - LOG(ERROR) << "SegmentTemplate: $Number$ or $Time$ should exist."; - return false; + return Status(error::INVALID_ARGUMENT, + "In segment templates $Number$ or $Time$ should exist."); } // Note: The below check is skipped. // Strings outside identifiers shall only contain characters that are // permitted within URLs according to RFC 1738. - return true; + return Status::OK; } std::string GetSegmentName(const std::string& segment_template, uint64_t segment_start_time, uint32_t segment_index, uint32_t bandwidth) { - DCHECK(ValidateSegmentTemplate(segment_template)); + DCHECK_EQ(Status::OK, ValidateSegmentTemplate(segment_template)); std::vector splits = base::SplitString( segment_template, "$", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL); @@ -131,7 +140,7 @@ std::string GetSegmentName(const std::string& segment_template, std::string format_tag; if (format_pos != std::string::npos) { format_tag = splits[i].substr(format_pos); - DCHECK(ValidateFormatTag(format_tag)); + DCHECK_EQ(Status::OK, ValidateFormatTag(format_tag)); // Replace %d formatting to correctly format uint64_t. format_tag = format_tag.substr(0, format_tag.size() - 1) + PRIu64; } else { diff --git a/packager/media/base/muxer_util.h b/packager/media/base/muxer_util.h index 08b4caa8a2..49d6fb2bb5 100644 --- a/packager/media/base/muxer_util.h +++ b/packager/media/base/muxer_util.h @@ -23,9 +23,9 @@ class StreamInfo; /// Validates the segment template against segment URL construction rule /// specified in ISO/IEC 23009-1:2012 5.3.9.4.4. /// @param segment_template is the template to be validated. -/// @return true if the segment template complies with -// ISO/IEC 23009-1:2012 5.3.9.4.4, false otherwise. -bool ValidateSegmentTemplate(const std::string& segment_template); +/// @return OK if the segment template complies with +// ISO/IEC 23009-1:2012 5.3.9.4.4. +Status ValidateSegmentTemplate(const std::string& segment_template); /// Build the segment name from provided input. /// @param segment_template is the segment template pattern, which should diff --git a/packager/media/base/muxer_util_unittest.cc b/packager/media/base/muxer_util_unittest.cc index 152396d3d7..7e0c14616f 100644 --- a/packager/media/base/muxer_util_unittest.cc +++ b/packager/media/base/muxer_util_unittest.cc @@ -12,52 +12,52 @@ namespace shaka { namespace media { TEST(MuxerUtilTest, ValidateSegmentTemplate) { - EXPECT_FALSE(ValidateSegmentTemplate("")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("")); - EXPECT_TRUE(ValidateSegmentTemplate("$Number$")); - EXPECT_TRUE(ValidateSegmentTemplate("$Time$")); - EXPECT_TRUE(ValidateSegmentTemplate("$Time$$Time$")); - EXPECT_TRUE(ValidateSegmentTemplate("foo$Time$goo")); - EXPECT_TRUE(ValidateSegmentTemplate("$Number$_$Number$")); - EXPECT_TRUE(ValidateSegmentTemplate("$Number$$Bandwidth$")); - EXPECT_TRUE(ValidateSegmentTemplate("$Time$$Bandwidth$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Number$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time$$Time$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("foo$Time$goo")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Number$_$Number$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Number$$Bandwidth$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time$$Bandwidth$")); // Bandwidth without Number or Time should not be valid. - EXPECT_FALSE(ValidateSegmentTemplate("$Bandwidth$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Bandwidth$")); // Escape sequence "$$". - EXPECT_TRUE(ValidateSegmentTemplate("foo$Time$__$$loo")); - EXPECT_TRUE(ValidateSegmentTemplate("foo$Time$$$")); - EXPECT_TRUE(ValidateSegmentTemplate("$$$Time$$$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("foo$Time$__$$loo")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("foo$Time$$$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$$$Time$$$")); // Missing $Number$ / $Time$. - EXPECT_FALSE(ValidateSegmentTemplate("$$")); - EXPECT_FALSE(ValidateSegmentTemplate("foo$$goo")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("foo$$goo")); // $Number$, $Time$ should not co-exist. - EXPECT_FALSE(ValidateSegmentTemplate("$Number$$Time$")); - EXPECT_FALSE(ValidateSegmentTemplate("foo$Number$_$Time$loo")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Number$$Time$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("foo$Number$_$Time$loo")); // $RepresentationID$ not implemented yet. - EXPECT_FALSE(ValidateSegmentTemplate("$RepresentationID$__$Time$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$RepresentationID$__$Time$")); // Unknown identifier. - EXPECT_FALSE(ValidateSegmentTemplate("$foo$$Time$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$foo$$Time$")); } TEST(MuxerUtilTest, ValidateSegmentTemplateWithFormatTag) { - EXPECT_TRUE(ValidateSegmentTemplate("$Time%01d$")); - EXPECT_TRUE(ValidateSegmentTemplate("$Time%05d$")); - EXPECT_FALSE(ValidateSegmentTemplate("$Time%1d$")); - EXPECT_FALSE(ValidateSegmentTemplate("$Time%$")); - EXPECT_FALSE(ValidateSegmentTemplate("$Time%01$")); - EXPECT_FALSE(ValidateSegmentTemplate("$Time%0xd$")); - EXPECT_FALSE(ValidateSegmentTemplate("$Time%03xd$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time%01d$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time%05d$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%1d$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%01$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%0xd$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%03xd$")); // $$ should not have any format tag. - EXPECT_FALSE(ValidateSegmentTemplate("$%01d$$Time$")); + EXPECT_NE(Status::OK, ValidateSegmentTemplate("$%01d$$Time$")); // Format specifier edge cases. - EXPECT_TRUE(ValidateSegmentTemplate("$Time%00d$")); - EXPECT_TRUE(ValidateSegmentTemplate("$Time%005d$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time%00d$")); + EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time%005d$")); } TEST(MuxerUtilTest, GetSegmentName) { diff --git a/packager/packager.cc b/packager/packager.cc index 82056d920e..69a78eb5c4 100644 --- a/packager/packager.cc +++ b/packager/packager.cc @@ -97,66 +97,84 @@ MediaContainerName GetOutputFormat(const StreamDescriptor& descriptor) { return output_format; } -bool ValidateStreamDescriptor(bool dump_stream_info, - const StreamDescriptor& descriptor) { - // Validate and insert the descriptor - if (descriptor.input.empty()) { - LOG(ERROR) << "Stream input not specified."; - return false; - } - if (!dump_stream_info && descriptor.stream_selector.empty()) { - LOG(ERROR) << "Stream stream_selector not specified."; - return false; +Status ValidateStreamDescriptor(bool dump_stream_info, + const StreamDescriptor& stream) { + if (stream.input.empty()) { + return Status(error::INVALID_ARGUMENT, "Stream input not specified."); } - // We should have either output or segment_template specified. - const bool output_specified = - !descriptor.output.empty() || !descriptor.segment_template.empty(); - if (!output_specified) { - if (!dump_stream_info) { - LOG(ERROR) << "Stream output not specified."; - return false; + // The only time a stream can have no outputs, is when dump stream info is + // set. + if (dump_stream_info && stream.output.empty() && + stream.segment_template.empty()) { + return Status::OK; + } + + if (stream.output.empty() && stream.segment_template.empty()) { + return Status(error::INVALID_ARGUMENT, + "Streams must specify 'output' or 'segment template'."); + } + + // Whenever there is output, a stream must be selected. + if (stream.stream_selector.empty()) { + return Status(error::INVALID_ARGUMENT, + "Stream stream_selector not specified."); + } + + // If a segment template is provided, it must be valid. + if (stream.segment_template.length()) { + Status template_check = ValidateSegmentTemplate(stream.segment_template); + if (!template_check.ok()) { + return template_check; + } + } + + // There are some specifics that must be checked based on which format + // we are writing to. + const MediaContainerName output_format = GetOutputFormat(stream); + + if (output_format == CONTAINER_UNKNOWN) { + return Status(error::INVALID_ARGUMENT, "Unsupported output format."); + } else if (output_format == MediaContainerName::CONTAINER_MPEG2TS) { + if (stream.segment_template.empty()) { + return Status(error::INVALID_ARGUMENT, + "Please specify segment_template. Single file TS output is " + "not supported."); + } + + // Right now the init segment is saved in |output| for multi-segment + // content. However, for TS all segments must be self-initializing so + // there cannot be an init segment. + if (stream.output.length()) { + return Status(error::INVALID_ARGUMENT, + "All TS segments must be self-initializing. Stream " + "descriptors 'output' or 'init_segment' are not allowed."); } } else { - const MediaContainerName output_format = GetOutputFormat(descriptor); - if (output_format == CONTAINER_UNKNOWN) - return false; - - if (output_format == MediaContainerName::CONTAINER_MPEG2TS) { - if (descriptor.segment_template.empty()) { - LOG(ERROR) << "Please specify segment_template. Single file TS output " - "is not supported."; - return false; - } - // Note that MPEG2 TS doesn't need a separate initialization segment, so - // output field is not needed. - if (!descriptor.output.empty()) { - LOG(WARNING) << "TS init_segment '" << descriptor.output - << "' ignored. TS muxer does not support initialization " - "segment generation."; - } - } else { - if (descriptor.output.empty()) { - LOG(ERROR) << "init_segment is required for format " << output_format; - return false; - } + // For any other format, if there is a segment template, there must be an + // init segment provided. + if (stream.segment_template.length() && stream.output.empty()) { + return Status(error::INVALID_ARGUMENT, + "Please specify 'init_segment'. All non-TS multi-segment " + "content must provide an init segment."); } } - return true; + + return Status::OK; } -bool ValidateParams(const PackagingParams& packaging_params, - const std::vector& stream_descriptors) { +Status ValidateParams(const PackagingParams& packaging_params, + const std::vector& stream_descriptors) { if (!packaging_params.chunking_params.segment_sap_aligned && packaging_params.chunking_params.subsegment_sap_aligned) { - LOG(ERROR) << "Setting segment_sap_aligned to false but " - "subsegment_sap_aligned to true is not allowed."; - return false; + return Status(error::INVALID_ARGUMENT, + "Setting segment_sap_aligned to false but " + "subsegment_sap_aligned to true is not allowed."); } if (stream_descriptors.empty()) { - LOG(ERROR) << "Stream descriptors cannot be empty."; - return false; + return Status(error::INVALID_ARGUMENT, + "Stream descriptors cannot be empty."); } // On demand profile generates single file segment while live profile @@ -165,23 +183,28 @@ bool ValidateParams(const PackagingParams& packaging_params, stream_descriptors.begin()->segment_template.empty(); for (const auto& descriptor : stream_descriptors) { if (on_demand_dash_profile != descriptor.segment_template.empty()) { - LOG(ERROR) << "Inconsistent stream descriptor specification: " + return Status(error::INVALID_ARGUMENT, + "Inconsistent stream descriptor specification: " "segment_template should be specified for none or all " - "stream descriptors."; - return false; + "stream descriptors."); + } + + Status stream_check = ValidateStreamDescriptor( + packaging_params.test_params.dump_stream_info, descriptor); + + if (!stream_check.ok()) { + return stream_check; } - if (!ValidateStreamDescriptor(packaging_params.test_params.dump_stream_info, - descriptor)) - return false; - } - if (packaging_params.output_media_info && !on_demand_dash_profile) { - // TODO(rkuroiwa, kqyang): Support partial media info dump for live. - NOTIMPLEMENTED() << "ERROR: --output_media_info is only supported for " - "on-demand profile (not using segment_template)."; - return false; } - return true; + if (packaging_params.output_media_info && !on_demand_dash_profile) { + // TODO(rkuroiwa, kqyang): Support partial media info dump for live. + return Status(error::UNIMPLEMENTED, + "--output_media_info is only supported for on-demand profile " + "(not using segment_template)."); + } + + return Status::OK; } class StreamDescriptorCompareFn { @@ -364,10 +387,10 @@ Status CreateRemuxJobs(const StreamDescriptorList& stream_descriptors, stream_muxer_options.temp_dir = packaging_params.temp_dir; stream_muxer_options.output_file_name = stream_iter->output; if (!stream_iter->segment_template.empty()) { - if (!ValidateSegmentTemplate(stream_iter->segment_template)) { - return Status( - error::INVALID_ARGUMENT, - "Invalid segment template: " + stream_iter->segment_template); + Status template_check = + ValidateSegmentTemplate(stream_iter->segment_template); + if (!template_check.ok()) { + return template_check; } stream_muxer_options.segment_template = stream_iter->segment_template; } @@ -605,8 +628,11 @@ Status Packager::Initialize( if (internal_) return Status(error::INVALID_ARGUMENT, "Already initialized."); - if (!media::ValidateParams(packaging_params, stream_descriptors)) - return Status(error::INVALID_ARGUMENT, "Invalid packaging params."); + Status param_check = + media::ValidateParams(packaging_params, stream_descriptors); + if (!param_check.ok()) { + return param_check; + } if (!packaging_params.test_params.injected_library_version.empty()) { SetPackagerVersionForTesting(