Fix bug in VPx codec configuration.

Before, we converted the WebM configuration to MP4 in the video
client, however this lead to a bug when fields were missing.  So
now don't convert until the cluster parser so the extra info from
the stream can be added.

This also fixes a bug where the value was not printed in the warning
logs.

b/29580732

Change-Id: If0a1a4d135f98292cdaae15a11027f42d844e85d

packager/media/codecs/vp_codec_configuration_record.cc

@@ -38,6 +38,23 @@ std::string VPCodecAsString(VideoCodec codec) {
   }
 }
 
+template <typename T>
+void MergeField(const std::string& name,
+                T source_value,
+                bool source_is_set,
+                T* dest_value,
+                bool* dest_is_set) {
+  if (!*dest_is_set || source_is_set) {
+    if (*dest_is_set && source_value != *dest_value) {
+      LOG(WARNING) << "VPx " << name << " is inconsistent, "
+                   << static_cast<uint32_t>(*dest_value) << " vs "
+                   << static_cast<uint32_t>(source_value);
+    }
+    *dest_value = source_value;
+    *dest_is_set = true;
+  }
+}
+
 }  // namespace
 
 VPCodecConfigurationRecord::VPCodecConfigurationRecord() {}
@@ -153,29 +170,36 @@ void VPCodecConfigurationRecord::WriteMP4(std::vector<uint8_t>* data) const {
 void VPCodecConfigurationRecord::WriteWebM(std::vector<uint8_t>* data) const {
   BufferWriter writer;
 
+  if (profile_is_set_) {
     writer.AppendInt(static_cast<uint8_t>(kFeatureProfile));  // ID = 1
     writer.AppendInt(static_cast<uint8_t>(1));                // Length = 1
     writer.AppendInt(static_cast<uint8_t>(profile_));
+  }
 
-  if (level_ != 0) {
+  if (level_is_set_ && level_ != 0) {
     writer.AppendInt(static_cast<uint8_t>(kFeatureLevel));  // ID = 2
     writer.AppendInt(static_cast<uint8_t>(1));  // Length = 1
     writer.AppendInt(static_cast<uint8_t>(level_));
   }
 
+  if (bit_depth_is_set_) {
     writer.AppendInt(static_cast<uint8_t>(kFeatureBitDepth));  // ID = 3
     writer.AppendInt(static_cast<uint8_t>(1));  // Length = 1
     writer.AppendInt(static_cast<uint8_t>(bit_depth_));
+  }
 
+  if (chroma_subsampling_is_set_) {
     // WebM doesn't differentiate whether it is vertical or collocated with luma
     // for 4:2:0.
     const uint8_t subsampling =
         chroma_subsampling_ == CHROMA_420_COLLOCATED_WITH_LUMA
             ? CHROMA_420_VERTICAL
             : chroma_subsampling_;
-  writer.AppendInt(static_cast<uint8_t>(kFeatureChromaSubsampling));  // ID = 4
+    // ID = 4, Length = 1
-  writer.AppendInt(static_cast<uint8_t>(1));  // Length = 1
+    writer.AppendInt(static_cast<uint8_t>(kFeatureChromaSubsampling));
+    writer.AppendInt(static_cast<uint8_t>(1));
     writer.AppendInt(subsampling);
+  }
 
   writer.SwapBuffer(data);
 }
@@ -203,64 +227,24 @@ std::string VPCodecConfigurationRecord::GetCodecString(VideoCodec codec) const {
 
 void VPCodecConfigurationRecord::MergeFrom(
     const VPCodecConfigurationRecord& other) {
-  if (!profile_is_set_ || other.profile_is_set_) {
+  MergeField("profile", other.profile_, other.profile_is_set_, &profile_,
-    profile_ = other.profile();
+             &profile_is_set_);
-    profile_is_set_ = true;
+  MergeField("level", other.level_, other.level_is_set_, &level_,
-  }
+             &level_is_set_);
-  if (!level_is_set_ || other.level_is_set_) {
+  MergeField("bit depth", other.bit_depth_, other.bit_depth_is_set_,
-    if (level_is_set_ && other.level() != level_) {
+             &bit_depth_, &bit_depth_is_set_);
-      LOG(WARNING) << "VPx level is inconsistent, " << level_ << " vs "
+  MergeField("color space", other.color_space_, other.color_space_is_set_,
-                   << other.level();
+             &color_space_, &color_space_is_set_);
-    }
+  MergeField("chroma subsampling", other.chroma_subsampling_,
-    level_ = other.level();
+             other.chroma_subsampling_is_set_, &chroma_subsampling_,
-    level_is_set_ = true;
+             &chroma_subsampling_is_set_);
-  }
+  MergeField("transfer function", other.transfer_function_,
-  if (!bit_depth_is_set_ || other.bit_depth_is_set_) {
+             other.transfer_function_is_set_, &transfer_function_,
-    if (bit_depth_is_set_ && bit_depth_ != other.bit_depth()) {
+             &transfer_function_is_set_);
-      LOG(WARNING) << "VPx bit depth is inconsistent, " << bit_depth_ << " vs "
+  MergeField("video full range flag", other.video_full_range_flag_,
-                   << other.bit_depth();
+             other.video_full_range_flag_is_set_, &video_full_range_flag_,
-    }
+             &video_full_range_flag_is_set_);
-    bit_depth_ = other.bit_depth();
+
-    bit_depth_is_set_ = true;
-  }
-  if (!color_space_is_set_ || other.color_space_is_set_) {
-    if (color_space_is_set_ && color_space_ != other.color_space()) {
-      LOG(WARNING) << "VPx color space is inconsistent, " << color_space_
-                   << " vs " << other.color_space();
-    }
-    color_space_ = other.color_space();
-    color_space_is_set_ = true;
-  }
-  if (!chroma_subsampling_is_set_ || other.chroma_subsampling_is_set_) {
-    if (chroma_subsampling_is_set_ &&
-        chroma_subsampling_ != other.chroma_subsampling_) {
-      LOG(WARNING) << "VPx chroma subsampling is inconsistent, "
-                   << chroma_subsampling_ << " vs "
-                   << other.chroma_subsampling();
-    }
-    chroma_subsampling_ = other.chroma_subsampling();
-    chroma_subsampling_is_set_ = true;
-  }
-  if (!transfer_function_is_set_ || other.transfer_function_is_set_) {
-    if (transfer_function_is_set_ &&
-        transfer_function_ != other.transfer_function_) {
-      LOG(WARNING) << "VPx transfer function is inconsistent, "
-                   << transfer_function_ << " vs "
-                   << other.transfer_function();
-    }
-    transfer_function_ = other.transfer_function();
-    transfer_function_is_set_ = true;
-  }
-  if (!video_full_range_flag_is_set_ || other.video_full_range_flag_is_set_) {
-    if (video_full_range_flag_is_set_ &&
-        video_full_range_flag_ != other.video_full_range_flag_) {
-      LOG(WARNING) << "VPx video full-range flag is inconsistent, "
-                   << video_full_range_flag_<< " vs "
-                   << other.video_full_range_flag();
-    }
-    video_full_range_flag_ = other.video_full_range_flag();
-    video_full_range_flag_is_set_ = true;
-  }
   if (codec_initialization_data_.empty() ||
       !other.codec_initialization_data_.empty()) {
     if (!codec_initialization_data_.empty() &&

packager/media/formats/webm/webm_cluster_parser.cc

@@ -458,7 +458,7 @@ bool WebMClusterParser::OnBlock(bool is_simple_block,
 
         VPCodecConfigurationRecord codec_config;
         if (!video_stream_info_->codec_config().empty())
-          codec_config.ParseMP4(video_stream_info_->codec_config());
+          codec_config.ParseWebM(video_stream_info_->codec_config());
         codec_config.MergeFrom(vpx_parser->codec_config());
 
         video_stream_info_->set_codec_string(

packager/media/formats/webm/webm_video_client.cc

@@ -51,22 +51,16 @@ void WebMVideoClient::Reset() {
 scoped_refptr<VideoStreamInfo> WebMVideoClient::GetVideoStreamInfo(
     int64_t track_num,
     const std::string& codec_id,
-    const std::vector<uint8_t>& codec_private_in,
+    const std::vector<uint8_t>& codec_private,
     bool is_encrypted) {
-  std::vector<uint8_t> codec_private = codec_private_in;
   VideoCodec video_codec = kUnknownVideoCodec;
   if (codec_id == "V_VP8") {
     video_codec = kCodecVP8;
   } else if (codec_id == "V_VP9") {
     video_codec = kCodecVP9;
-
+    // The codec private data is in WebM format, but needs to be converted to
-    // Need to parse and convert the codec private data to MP4 format.
+    // MP4 format.  Don't do it yet, it will be handled in
-    VPCodecConfigurationRecord vp_config;
+    // webm_cluster_parser.cc
-    if (!vp_config.ParseWebM(codec_private)) {
-      LOG(ERROR) << "Unable to parse VP9 codec configuration";
-      return scoped_refptr<VideoStreamInfo>();
-    }
-    vp_config.WriteMP4(&codec_private);
   } else if (codec_id == "V_VP10") {
     video_codec = kCodecVP10;
   } else {