Parsing of encoded duration for unencrypted opus streams.

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 6784776a0e029e0c21d168391532087b8a43fa6b..4e82c81272395e0ef77d6d912a93237557fdf728 100644
--- a/media/formats/webm/webm_cluster_parser.cc
+++ b/media/formats/webm/webm_cluster_parser.cc
@@ -17,6 +17,11 @@
 
 namespace media {
 
+const uint16 WebMClusterParser::kOpusFrameDurationsMu[] = {
+    10000, 20000, 40000, 60000, 10000, 20000, 40000, 60000, 10000, 20000, 40000,
+    60000, 10000, 20000, 10000, 20000, 2500,  5000,  10000, 20000, 2500,  5000,
+    10000, 20000, 2500,  5000,  10000, 20000, 2500,  5000,  10000, 20000};
+
 WebMClusterParser::WebMClusterParser(
     int64 timecode_scale,
     int audio_track_num,
@@ -27,11 +32,13 @@ WebMClusterParser::WebMClusterParser(
     const std::set<int64>& ignored_tracks,
     const std::string& audio_encryption_key_id,
     const std::string& video_encryption_key_id,
+    const AudioCodec audio_codec,
     const LogCB& log_cb)
     : timecode_multiplier_(timecode_scale / 1000.0),
       ignored_tracks_(ignored_tracks),
       audio_encryption_key_id_(audio_encryption_key_id),
       video_encryption_key_id_(video_encryption_key_id),
+      audio_codec_(audio_codec),
       parser_(kWebMIdCluster, this),
       last_block_timecode_(-1),
       block_data_size_(-1),
@@ -140,6 +147,67 @@ WebMClusterParser::GetTextBuffers() {
   return text_buffers_map_;
 }
 
+base::TimeDelta WebMClusterParser::TryGetEncodedAudioDuration(const uint8* data,
+                                                              int size) {
+  if (audio_codec_ == kCodecOpus) {
+    return ReadOpusDuration(data, size);
+  }
+  // TODO(wolenetz/chcunningham): Implement duration reading for Vorbis.

[wolenetz, 2015/02/03 22:47:02]

nits: let's start a new or reference an existing crbug here please. Also, blank
line after } and before full line comment is more common.

[chcunningham, 2015/02/05 02:48:21]

Done.

+
+  return kNoTimestamp();
+}
+
+base::TimeDelta WebMClusterParser::ReadOpusDuration(const uint8* data,
+                                                    int size) {
+  // Masks for Opus TOC and Frame Count bytes. See http://goo.gl/2RmoxA

[wolenetz, 2015/02/03 22:47:01]

ditto full URL... sorry

[chcunningham, 2015/02/05 02:48:21]

Done.

+  static const uint8 kTocConfigMask = 0xf8;

[wolenetz, 2015/02/03 22:47:01]

ditto: _t

[chcunningham, 2015/02/05 02:48:21]

Done.

+  static const uint8 kTocFrameCountCodeMask = 0x03;
+  static const uint8 kFrameCountMask = 0x3f;
+  static const int kFrameCountMin = 1;
+  static const int kFrameCountMax = 48;
+
+  if (size < 1) {
+    MEDIA_LOG(log_cb_) << "Invalid zero-byte Opus packet.";
+    return kNoTimestamp();
+  }
+
+  // Frame count type described by last 2 bits of Opus TOC byte.
+  int frame_count_type = data[0] & kTocFrameCountCodeMask;
+
+  int frame_count = 0;
+  switch (frame_count_type) {
+    case 0:
+      frame_count = 1;
+      break;
+    case 1:
+    case 2:
+      frame_count = 2;
+      break;
+    case 3:
+      // Type 3 indicates an arbitrary frame count described in the next byte.
+      if (size < 2) {
+        MEDIA_LOG(log_cb_) << "Second byte missing from 'Code 3' Opus packet.";
+        return kNoTimestamp();
+      }
+      frame_count = data[1] & kFrameCountMask;
+      DCHECK_GE(frame_count, 1);
+      if (frame_count < kFrameCountMin || frame_count > kFrameCountMax) {
+        MEDIA_LOG(log_cb_) << "Illegal Opus packet frame_count: " << frame_count

[wolenetz, 2015/02/03 22:47:01]

nit: comment that we are explicitly allowing these to pass through the check
with just a log message for now at least.

[wolenetz, 2015/02/03 22:47:01]

nit: s/frame_count:/frame count:/

[chcunningham, 2015/02/05 02:48:21]

Done.

[chcunningham, 2015/02/05 02:48:21]

Done.

+                           << " Should be in range [" << kFrameCountMin << ", "
+                           << kFrameCountMax << "]";
+      }
+      break;
+    default:
+      MEDIA_LOG(log_cb_) << "Unexpected Opus frame count type: "
+                         << frame_count_type;
+      return kNoTimestamp();
+  }
+
+  int opusConfig = (data[0] & kTocConfigMask) >> 3;
+  return base::TimeDelta::FromMicroseconds(kOpusFrameDurationsMu[opusConfig] *

[wolenetz, 2015/02/03 22:47:01]

nit: kFrameCountMax is lower for longer frame sizes (see rfc: "...lower limits
for longer frame sizes"). Consider adding log message specifically instead for
just the return timedelta being too large or 0 (see rfc: "MUST NOT exceed 120 ms
(or be 0)").
Take this comment modulo: I agree that we should emit the packet regardless and
let decoder fuss about it. But we should also log (perhaps at most once per
webm_cluster_parser instance) when extracted encoded packet duration >120ms or
<=0 occurs.

[chcunningham, 2015/02/05 02:48:21]

Done.

+                                           frame_count);
+}
+
 WebMParserClient* WebMClusterParser::OnListStart(int id) {
   if (id == kWebMIdCluster) {
     cluster_timecode_ = -1;
@@ -324,9 +392,13 @@ bool WebMClusterParser::OnBlock(bool is_simple_block, int track_num,
   Track* track = NULL;
   StreamParserBuffer::Type buffer_type = DemuxerStream::AUDIO;
   std::string encryption_key_id;
+  base::TimeDelta encoded_duration = kNoTimestamp();
   if (track_num == audio_.track_num()) {
     track = &audio_;
     encryption_key_id = audio_encryption_key_id_;
+    if (encryption_key_id.empty()) {
+      encoded_duration = TryGetEncodedAudioDuration(data, size);
+    }
   } else if (track_num == video_.track_num()) {
     track = &video_;
     encryption_key_id = video_encryption_key_id_;
@@ -407,9 +479,42 @@ bool WebMClusterParser::OnBlock(bool is_simple_block, int track_num,
   if (cluster_start_time_ == kNoTimestamp())
     cluster_start_time_ = timestamp;
 
+  base::TimeDelta block_duration_time_delta = kNoTimestamp();
   if (block_duration >= 0) {
-    buffer->set_duration(base::TimeDelta::FromMicroseconds(
-        block_duration * timecode_multiplier_));
+    block_duration_time_delta = base::TimeDelta::FromMicroseconds(
+        block_duration * timecode_multiplier_);
+  }
+
+  // Prefer encoded duration when available. This layering violatiaon is a

[wolenetz, 2015/02/03 22:47:01]

nit: s/when available/over BlockGroup->BlockDuration or
TrackEntry->DefaultDuration when available/.

[wolenetz, 2015/02/03 22:47:02]

nit: s/violatiaon/violation/

[chcunningham, 2015/02/05 02:48:21]

Done. Oh, and I found a sublime text spell checker :)

[chcunningham, 2015/02/05 02:48:22]

Done.

[wolenetz, 2015/02/05 23:04:59]

Please share link offline with me :)

+  // workaround for http://crbug.com/396634, decreasing the likelihood of
+  // fallback to rough estimation techniques for Blocks that lack a
+  // BlockDuration at the end of a cluster. Cross cluster durations are not
+  // feasabile given flexibility of cluster ordering and MSE APIs. Duration

[wolenetz, 2015/02/03 22:47:01]

nit: s/feasabile/feasible/

[chcunningham, 2015/02/05 02:48:21]

Done.

+  // estimation may still apply in cases of encryption and unsupported codecs.

[wolenetz, 2015/02/03 22:47:01]

nit: s/unsupported codecs/codecs for which we do not extract encoded block
duration/

[chcunningham, 2015/02/05 02:48:21]

Done.

+  // Estimates are applied at the end of parsing once the whole cluster is
+  // parsed. See ApplyDurationEstimateIfNeeded for more on estimation.

[wolenetz, 2015/02/03 22:47:01]

nit: add ()

[chcunningham, 2015/02/05 02:48:21]

Done.

+  if (encoded_duration != kNoTimestamp()) {
+    DCHECK(encoded_duration != kInfiniteDuration());
+    DCHECK(encoded_duration > base::TimeDelta());
+    buffer->set_duration(encoded_duration);
+
+    DVLOG(3) << __FUNCTION__ << " : "
+             << "Using encoded duartion " << encoded_duration.InSecondsF();

[wolenetz, 2015/02/03 22:47:01]

nit: s/duartion/duration/

[chcunningham, 2015/02/05 02:48:21]

Done.

+
+    if (block_duration_time_delta != kNoTimestamp()) {
+      base::TimeDelta duration_difference =
+          block_duration_time_delta - encoded_duration;
+
+      const auto kWarnDurationDiff = base::TimeDelta::FromMilliseconds(10);

[wolenetz, 2015/02/03 22:47:01]

Hmmm. 10ms seems a little arbitrary (and big).
Consider:
Can we do something like: precalculate the microsecond granularity allowed by
current timecodescale and warn if the encoded duration is >= 2x that
granularity? (e.g. if granularity is 1ms, then warn if abs_diff(encoded vs
blockgroup->blockduration) >= 2ms?
I think that might lead to too many media_log if we don't throttle it.
Also, some form of UMA to let us know the answers to something like "how many
webm streams containing opus are there" and "of these, how many had at least one
occurrence of >= 2x granularity mismatch of encoded vs blockgroup duration"..
AND "of all webm streams containing opus, how many had at least once occurrence
of >= 2x granularity mismatch of encoded vs intra-cluster-block-timecode-diff".
If we could obtain a bucketized distribution of count of granularity mismatches,
it would really help us understand the state of our assumptions about how
frequently we play badly muxed webm w.r.t. duration.
WDYT? This UMA stuff would be a separate CL, perhaps along with indicators like
how often an encrypted webm stream had a cluster without a trailing blockgroup
for every track.
For now (this CL), to at least prevent spamming log, I recommend 2x granularity
logic, and log if it's exceeded at most once per instance of
webm_cluster_parser. Then the log messages would be much less spammy, and more
informative.

[chcunningham, 2015/02/05 02:48:21]

Done.

+      if (duration_difference.magnitude() > kWarnDurationDiff) {
+        MEDIA_LOG(log_cb_) << "BlockDuration "
+                           << "(" << block_duration_time_delta << ") "

[wolenetz, 2015/02/03 22:47:01]

nit: Units. Is this in secondsF? (use .InSecondsF() ?)

[chcunningham, 2015/02/05 02:48:21]

It is in secondsF. I can be explicit, but I think its probably ok as is. I can't
see anyone changing that default without taking the proper precautions. LMK if
you still want explicit.

[wolenetz, 2015/02/05 23:04:59]

Oh. I didn't realize that. And it comes with nice "s" appended, too. Fine as-is
then :)

+                           << "differs signifcantly from encoded duration "

[wolenetz, 2015/02/03 22:47:01]

nit:s/signifcantly/significantly/

[chcunningham, 2015/02/05 02:48:21]

Done.

+                           << "(" << encoded_duration << ").";

[wolenetz, 2015/02/03 22:47:01]

nit ditto: InSecondsF()

[chcunningham, 2015/02/05 02:48:21]

See other reply

[wolenetz, 2015/02/05 23:04:59]

Acknowledged.

+      }
+    }
+  } else if (block_duration_time_delta != kNoTimestamp()) {
+    buffer->set_duration(block_duration_time_delta);
   } else {
     DCHECK_NE(buffer_type, DemuxerStream::TEXT);
     buffer->set_duration(track->default_duration());