Improving WebM video duration estimation.

media/formats/webm/webm_cluster_parser.cc

Index: media/formats/webm/webm_cluster_parser.cc
diff --git a/media/formats/webm/webm_cluster_parser.cc b/media/formats/webm/webm_cluster_parser.cc
index 7dacd7102c7b7f24d3fc6ba27de9df9a4c484504..aed07ef862648c77e234e2c4a855f565deb41a31 100644
--- a/media/formats/webm/webm_cluster_parser.cc
+++ b/media/formats/webm/webm_cluster_parser.cc
@@ -25,7 +25,10 @@ const uint16_t WebMClusterParser::kOpusFrameDurationsMu[] = {
 enum {
   // Limits the number of MEDIA_LOG() calls in the path of reading encoded
   // duration to avoid spamming for corrupted data.
-  kMaxDurationLogs = 10,
+  kMaxDurationErrorLogs = 10,
+  // Limits the number of MEDIA_LOG() calls warning the user that a buffer
+  // durations have been estimated.

[wolenetz, 2015/03/28 00:26:06]

nit: regrammarize "a buffer durations have" ;)

[chcunningham, 2015/04/13 23:25:18]

Done.

+  kMaxDurationEstimateLogs = 10,
 };
 
 WebMClusterParser::WebMClusterParser(
@@ -185,7 +188,7 @@ base::TimeDelta WebMClusterParser::ReadOpusDuration(const uint8_t* data,
       base::TimeDelta::FromMilliseconds(120);
 
   if (size < 1) {
-    LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationLogs)
+    LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationErrorLogs)
         << "Invalid zero-byte Opus packet; demuxed block duration may be "
            "imprecise.";
     return kNoTimestamp();
@@ -206,7 +209,7 @@ base::TimeDelta WebMClusterParser::ReadOpusDuration(const uint8_t* data,
     case 3:
       // Type 3 indicates an arbitrary frame count described in the next byte.
       if (size < 2) {
-        LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationLogs)
+        LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationErrorLogs)
             << "Second byte missing from 'Code 3' Opus packet; demuxed block "
                "duration may be imprecise.";
         return kNoTimestamp();
@@ -215,7 +218,7 @@ base::TimeDelta WebMClusterParser::ReadOpusDuration(const uint8_t* data,
       frame_count = data[1] & kFrameCountMask;
 
       if (frame_count == 0) {
-        LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationLogs)
+        LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationErrorLogs)
             << "Illegal 'Code 3' Opus packet with frame count zero; demuxed "
                "block duration may be imprecise.";
         return kNoTimestamp();
@@ -223,7 +226,7 @@ base::TimeDelta WebMClusterParser::ReadOpusDuration(const uint8_t* data,
 
       break;
     default:
-      LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationLogs)
+      LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationErrorLogs)
           << "Unexpected Opus frame count type: " << frame_count_type << "; "
           << "demuxed block duration may be imprecise.";
       return kNoTimestamp();
@@ -241,7 +244,7 @@ base::TimeDelta WebMClusterParser::ReadOpusDuration(const uint8_t* data,
     // Intentionally allowing packet to pass through for now. Decoder should
     // either handle or fail gracefully. MEDIA_LOG as breadcrumbs in case
     // things go sideways.
-    LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationLogs)
+    LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationErrorLogs)
         << "Warning, demuxed Opus packet with encoded duration: " << duration
         << ". Should be no greater than " << kPacketDurationMax;
   }
@@ -558,7 +561,7 @@ bool WebMClusterParser::OnBlock(bool is_simple_block,
       const auto kWarnDurationDiff =
           base::TimeDelta::FromMicroseconds(timecode_multiplier_ * 2);
       if (duration_difference.magnitude() > kWarnDurationDiff) {
-        LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationLogs)
+        LIMITED_MEDIA_LOG(log_cb_, num_duration_errors_, kMaxDurationErrorLogs)
             << "BlockDuration "
             << "(" << block_duration_time_delta << ") "
             << "differs significantly from encoded duration "
@@ -585,7 +588,8 @@ WebMClusterParser::Track::Track(int track_num,
                                 bool is_video,
                                 base::TimeDelta default_duration,
                                 const LogCB& log_cb)
-    : track_num_(track_num),
+    : num_duration_estimates_(0),
+      track_num_(track_num),
       is_video_(is_video),
       default_duration_(default_duration),
       estimated_next_frame_duration_(kNoTimestamp()),
@@ -678,7 +682,17 @@ void WebMClusterParser::Track::ApplyDurationEstimateIfNeeded() {
   if (!last_added_buffer_missing_duration_.get())
     return;
 
-  last_added_buffer_missing_duration_->set_duration(GetDurationEstimate());
+  base::TimeDelta estimated_duration = GetDurationEstimate();
+  last_added_buffer_missing_duration_->set_duration(estimated_duration);
+
+  // Exposing estimation so splicing/overlap frame processing can make informed
+  // decisions downstream.
+  last_added_buffer_missing_duration_->set_is_duration_estimated(true);
+
+  LIMITED_MEDIA_LOG(log_cb_, num_duration_estimates_, kMaxDurationEstimateLogs)
+      << "Estimating duration to be " << estimated_duration << " for the "
+      << "last (Simple)Block in the Cluster. Use BlockDuration at the end of "

[wolenetz, 2015/03/28 00:26:06]

nit: "in the Cluster for this Track". And "Instead of SimpleBlocks, use
BlockGroups with BlockDurations at the end of each Track in a Cluster to avoid
estimation."

[chcunningham, 2015/04/13 23:25:18]

Done.

+      << "Clusters to avoid estimation.";
 
   DVLOG(2) << "ApplyDurationEstimateIfNeeded() : new dur : "
            << " ts "
@@ -746,16 +760,35 @@ bool WebMClusterParser::Track::QueueBuffer(
     return false;
   }
 
-  // The estimated frame duration is the minimum non-zero duration since the
-  // last initialization segment.  The minimum is used to ensure frame durations
-  // aren't overestimated.
+  // The estimated frame duration is the minimum (for audio) or the maximum
+  // (for video) non-zero duration since the last initialization segment. The
+  // minimum is used for audio to ensure frame durations aren't overestimated,
+  // triggering unnecessary frame splicing. For video, splicing does not apply,
+  // so maximum is used and overlap is is simply resolved by showing the

[wolenetz, 2015/03/28 00:26:06]

nit: is is

[chcunningham, 2015/04/13 23:25:18]

Done.

+  // later of the overlapping frames at its given PTS, effectively trimming down
+  // the over-estimated duration of the previous frame.
+  // TODO(chcunningham): Use max for audio and disable splicing whenever
+  // estimated buffers are encountered.
   if (duration > base::TimeDelta()) {
+    base::TimeDelta orig_duration_estimate = estimated_next_frame_duration_;
     if (estimated_next_frame_duration_ == kNoTimestamp()) {
       estimated_next_frame_duration_ = duration;
+    } else if (is_video_) {
+      estimated_next_frame_duration_ =
+          std::max(duration, estimated_next_frame_duration_);
     } else {
       estimated_next_frame_duration_ =
           std::min(duration, estimated_next_frame_duration_);
     }
+
+    if (orig_duration_estimate != estimated_next_frame_duration_) {
+      DVLOG(3) << "Updated duration estimate:"
+               << orig_duration_estimate
+               << " -> "
+               << estimated_next_frame_duration_
+               << " at dts: "

[wolenetz, 2015/03/28 00:26:06]

nit: webm doesn't differentiate dts/pts. s/dts/timestamp/
aside: it's weird that even inside webm parser we sometimes use
buffer->timestamp() and other times use buffer->GetDecodeTimestamp(). Seems ripe
for some low-pri cleanup.

[chcunningham, 2015/04/13 23:25:18]

Done. Re-cleanup: so you'd like a bug to change all DecodeTimestamp -> Timestamp
? I don't mind the using DecodeTimestamp, since thats intuitively whats most
important to sequencing buffers in a parser and the fact that WebM doesn't
differentiate is not obvious. If it were me, I'd always use DecodeTimestmp

[wolenetz, 2015/04/15 02:55:23]

Thanks. IIRC, WebM container doesn't mention PTS or DTS, just timestamp.
Internally here, it would be better to be consistent in usage of DTS or PTS on
the |buffer| accessor, but that's for a really low pri cleanup :)

+               << buffer->GetDecodeTimestamp().InSecondsF();
+    }
   }
 
   buffers_.push_back(buffer);