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

@@ -19,26 +19,33 @@
 
 namespace shaka {
 namespace {
-bool ValidateFormatTag(const std::string& format_tag) {
+Status ValidateFormatTag(const std::string& format_tag) {
-  DCHECK(!format_tag.empty());
+  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))
+    if (base::StringToUint(format_tag.substr(2, format_tag.size() - 3), &out)) {
-      return true;
+      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) {
+Status ValidateSegmentTemplate(const std::string& segment_template) {
-  if (segment_template.empty())
+  if (segment_template.empty()) {
-    return false;
+    return Status(error::INVALID_ARGUMENT,
+                  "Segment template should not be empty.");
+  }
 
   std::vector<std::string> 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 Status(error::INVALID_ARGUMENT,
-    return false;
+                  "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)))
+      Status tag_check = ValidateFormatTag(splits[i].substr(format_pos));
-        return false;
+      if (!tag_check.ok()) {
+        return tag_check;
+      }
     }
 
     // TODO(kqyang): Support "RepresentationID".
     if (identifier == "RepresentationID") {
-      NOTIMPLEMENTED() << "SegmentTemplate: $RepresentationID$ is not supported "
+      return Status(
-                          "yet.";
+          error::UNIMPLEMENTED,
-      return false;
+          "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 Status(error::INVALID_ARGUMENT,
-        return false;
+                      "'$$' should not have any format tags.");
       }
     } else if (identifier != "Bandwidth") {
-      LOG(ERROR) << "SegmentTemplate: '$" << splits[i]
+      return Status(error::INVALID_ARGUMENT,
-                 << "$' is not a valid identifier.";
+                    "'$" + splits[i] + "$' is not a valid identifier.");
-      return false;
     }
   }
   if (has_number && has_time) {
-    LOG(ERROR) << "SegmentTemplate: $Number$ and $Time$ should not co-exist.";
+    return Status(
-    return false;
+        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 Status(error::INVALID_ARGUMENT,
-    return false;
+                  "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<std::string> 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 {

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
+/// @return OK if the segment template complies with
-//          ISO/IEC 23009-1:2012 5.3.9.4.4, false otherwise.
+//          ISO/IEC 23009-1:2012 5.3.9.4.4.
-bool ValidateSegmentTemplate(const std::string& segment_template);
+Status ValidateSegmentTemplate(const std::string& segment_template);
 
 /// Build the segment name from provided input.
 /// @param segment_template is the segment template pattern, which should

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_EQ(Status::OK, ValidateSegmentTemplate("$Number$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Time$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Time$$Time$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time$$Time$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("foo$Time$goo"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("foo$Time$goo"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Number$_$Number$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Number$_$Number$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Number$$Bandwidth$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Number$$Bandwidth$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Time$$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_EQ(Status::OK, ValidateSegmentTemplate("foo$Time$__$$loo"));
-  EXPECT_TRUE(ValidateSegmentTemplate("foo$Time$$$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("foo$Time$$$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$$$Time$$$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$$$Time$$$"));
 
   // Missing $Number$ / $Time$.
-  EXPECT_FALSE(ValidateSegmentTemplate("$$"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("$$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("foo$$goo"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("foo$$goo"));
 
   // $Number$, $Time$ should not co-exist.
-  EXPECT_FALSE(ValidateSegmentTemplate("$Number$$Time$"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Number$$Time$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("foo$Number$_$Time$loo"));
+  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_EQ(Status::OK, ValidateSegmentTemplate("$Time%01d$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Time%05d$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time%05d$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("$Time%1d$"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%1d$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("$Time%$"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("$Time%01$"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%01$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("$Time%0xd$"));
+  EXPECT_NE(Status::OK, ValidateSegmentTemplate("$Time%0xd$"));
-  EXPECT_FALSE(ValidateSegmentTemplate("$Time%03xd$"));
+  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_EQ(Status::OK, ValidateSegmentTemplate("$Time%00d$"));
-  EXPECT_TRUE(ValidateSegmentTemplate("$Time%005d$"));
+  EXPECT_EQ(Status::OK, ValidateSegmentTemplate("$Time%005d$"));
 }
 
 TEST(MuxerUtilTest, GetSegmentName) {

packager/packager.cc

@@ -97,66 +97,84 @@ MediaContainerName GetOutputFormat(const StreamDescriptor& descriptor) {
   return output_format;
 }
 
-bool ValidateStreamDescriptor(bool dump_stream_info,
+Status ValidateStreamDescriptor(bool dump_stream_info,
-                              const StreamDescriptor& descriptor) {
+                                const StreamDescriptor& stream) {
-  // Validate and insert the descriptor
+  if (stream.input.empty()) {
-  if (descriptor.input.empty()) {
+    return Status(error::INVALID_ARGUMENT, "Stream input not specified.");
-    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;
   }
 
-  // We should have either output or segment_template specified.
+  // The only time a stream can have no outputs, is when dump stream info is
-  const bool output_specified =
+  // set.
-      !descriptor.output.empty() || !descriptor.segment_template.empty();
+  if (dump_stream_info && stream.output.empty() &&
-  if (!output_specified) {
+      stream.segment_template.empty()) {
-    if (!dump_stream_info) {
+    return Status::OK;
-      LOG(ERROR) << "Stream output not specified.";
+  }
-      return false;
+
+  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);
+    // For any other format, if there is a segment template, there must be an
-    if (output_format == CONTAINER_UNKNOWN)
+    // init segment provided.
-      return false;
+    if (stream.segment_template.length() && stream.output.empty()) {
-
+      return Status(error::INVALID_ARGUMENT,
-    if (output_format == MediaContainerName::CONTAINER_MPEG2TS) {
+                    "Please specify 'init_segment'. All non-TS multi-segment "
-      if (descriptor.segment_template.empty()) {
+                    "content must provide an init segment.");
-        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;
-      }
-    }
-  }
-  return true;
   }
 
-bool ValidateParams(const PackagingParams& packaging_params,
+  return Status::OK;
+}
+
+Status ValidateParams(const PackagingParams& packaging_params,
                       const std::vector<StreamDescriptor>& 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 "
+    return Status(error::INVALID_ARGUMENT,
-                  "subsegment_sap_aligned to true is not allowed.";
+                  "Setting segment_sap_aligned to false but "
-    return false;
+                  "subsegment_sap_aligned to true is not allowed.");
   }
 
   if (stream_descriptors.empty()) {
-    LOG(ERROR) << "Stream descriptors cannot be empty.";
+    return Status(error::INVALID_ARGUMENT,
-    return false;
+                  "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.";
+                    "stream descriptors.");
-      return false;
-    }
-    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;
+    Status stream_check = ValidateStreamDescriptor(
+        packaging_params.test_params.dump_stream_info, descriptor);
+
+    if (!stream_check.ok()) {
+      return stream_check;
+    }
+  }
+
+  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)) {
+      Status template_check =
-        return Status(
+          ValidateSegmentTemplate(stream_iter->segment_template);
-            error::INVALID_ARGUMENT,
+      if (!template_check.ok()) {
-            "Invalid segment template: " + stream_iter->segment_template);
+        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))
+  Status param_check =
-    return Status(error::INVALID_ARGUMENT, "Invalid packaging params.");
+      media::ValidateParams(packaging_params, stream_descriptors);
+  if (!param_check.ok()) {
+    return param_check;
+  }
 
   if (!packaging_params.test_params.injected_library_version.empty()) {
     SetPackagerVersionForTesting(