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..7e4b7d595e27c12b0d240d22c7483888fe48a7c5 100644
--- a/media/formats/webm/webm_cluster_parser.cc
+++ b/media/formats/webm/webm_cluster_parser.cc
@@ -27,11 +27,13 @@ WebMClusterParser::WebMClusterParser(
     const std::set<int64>& ignored_tracks,
     const std::string& audio_encryption_key_id,
     const std::string& video_encryption_key_id,
+    const AudioDecoderConfig& audio_config,
     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_config_(audio_config),

[chcunningham, 2015/01/29 22:14:20]

I'm not doing anything with this in ::Reset. Should I?

[wolenetz, 2015/01/30 01:48:08]

::Reset() is only for "let's parse another cluster from that track that we were
constructed with", so the audio_config_ from ctor should be left alone in
::Reset().

[chcunningham, 2015/02/03 20:34:43]

Acknowledged.

       parser_(kWebMIdCluster, this),
       last_block_timecode_(-1),
       block_data_size_(-1),
@@ -140,6 +142,57 @@ WebMClusterParser::GetTextBuffers() {
   return text_buffers_map_;
 }
 
+bool WebMClusterParser::TryGetEncodedAudioDuration(const uint8* data,

[wolenetz, 2015/01/30 01:48:07]

I suggest using a base::TimeDelta duration as retval and kNoTimestamp magic
value to indicate lack of success? This reduces # of params. caller should
DCHECK that the retval in that case is never otherwise negative, zero, or
kInfiniteDuration (which maps to negative, but inclusion in DCEHCK improves
readability).

[chcunningham, 2015/02/03 20:34:42]

Done, but a question about asserts. I haven't yet added the assert for duration
> 0. I notice in OnBlock that the existing code that sets the duration from
BlockDuration allows that duraiton to be == 0 (check is >= 0). The assert > 0
for the encoded duration seems inconsistent with this existing check. Is the
existing check wrong? Should we relax the assert you proposed to >= 0? While
Opus can't give a duration of 0, it may be too strong to assume for this API
which will one day serve Vorbis and potentially other codecs.

I'm personally a fan of letting WebMClusterParser allow for zero duration
packets - I'm not aware of this being in violation of WebM spec. If MSE chokes
on this, can we handle it downstream in a centralized place?

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

For now, yeah, let frame processor/etc deal with 0 duration coded frames.
Targeted strengthening of the requirements may come in later CL(s).

+                                                   int size,
+                                                   int64* duration) {
+  if (audio_config_.codec() == kCodecOpus) {
+    return ReadOpusDuration(data, size, duration);
+  }
+
+  return false;
+}
+
+bool WebMClusterParser::ReadOpusDuration(const uint8* data,

[wolenetz, 2015/01/30 01:48:08]

ditto on retval comment.

[chcunningham, 2015/02/03 20:34:42]

Done.

+                                         int size,
+                                         int64* duration) {
+  if (size < 1) {
+    MEDIA_LOG(log_cb_) << "Invalid zero-byte Opus packet.";

[wolenetz, 2015/01/30 01:48:07]

Could it be possible that an append one-byte-at-a-time logs this on 0 byte data
first time parse is attempted, but not once another byte is appended? If so,
need to protect against spurious log?

[wolenetz, 2015/01/30 20:46:12]

nm. OnBlock is done on complete block, not partial.

[chcunningham, 2015/02/03 20:34:42]

Acknowledged.

+    return false;
+  }
+
+  // Frame count type described by last 2 bits of Opus TOC byte.
+  int frame_count_type = data[0] & 0x03;
+
+  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.";

[wolenetz, 2015/01/30 01:48:08]

ditto: protect against spurious log?

[wolenetz, 2015/01/30 20:46:12]

ditto nm

[chcunningham, 2015/02/03 20:34:42]

Acknowledged.

+        return false;
+      }
+      frame_count = data[1] & 0x3f;
+      DCHECK_GE(frame_count, 1);
+      break;
+    default:
+      MEDIA_LOG(log_cb_) << "Unexpected Opus frame count type: "
+                         << frame_count_type;
+      return false;
+  }
+
+  int opusConfig = (data[0] & 0xf8) >> 3;

[wolenetz, 2015/01/30 20:46:12]

nit: put "magic" 0xf8 and others like that into an enum with clear naming to
help reader.

[chcunningham, 2015/02/03 20:34:43]

Done, sort of. Did it right here in the .cc - header file has enough Opus mess I
think. Also, rather than use an enum I just made some static const types. This
allows me to save a little memory. WDYT? I can also do an enum right here in the
cc function...

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

I'm not sure how an enum consumes more mem that a static const. However, static
const is fine with me for now. If vorbis CL splits out an abstraction for this
interface, then enums in the concrete impl can be done at that time.

+  *duration = kOpusFrameDurationsMu[opusConfig] * frame_count;

[wolenetz, 2015/01/30 01:48:08]

frame_count could be zero in a "code 3 packet". Guard against?
Likewise, frame_count could be zero even in a "code 2 packet" due to 0 == frame
length coding. Extract and guard against for code 2 packets, too?
Further, "Any Opus frame in any mode MAY have a length of 0". Does this mean we
should look at |size|-1 ?= 0 for "code 1 packets", too (assuming |size| is the
actual full size of the complete data in the webm block)? Would a length 0 in
this case also need to be disallowed?? Or just not emit a buffer? (I think the
latter is more tolerant with intent of Opus codec itself, though am not clear if
webm allows for it.) WDYT?
btw, that rfc is full-of-badness. Especially 3.2.1. talking about code 3 packet
interpretation of the frame length code that is completely conflicting versus
3.2.5. Did you find any reinforcement that 3.2.5. is indeed the one tru way of
interpreting code 3 packet frame count?
Next, code 3 packet (3.2.5): M MUST NOT be zero: where is guard against this
here?
Next, code 3 packet (3.2.5): audio duration contained within a packet MUST NOT
exceed 120ms. (and ancilliary text around max frame count for code 3 is 48):
where is guard against this here?

And finally, I'd like to understand exactly how bits 0-4 in TOC map to the "top
five bits". This wording is very strange, leading me to question whether or not
the most-significant bit in an Opus bitstream is first or last in a byte. I
would assume so, but still...

[wolenetz, 2015/01/30 20:46:12]

The strange wording is clarified elsewhere in that rfc. Thanks for pointing that
out.

[chcunningham, 2015/02/03 20:34:42]

Agree this is hard to read. I think I have an interpretation that makes it at
least mostly consistent with itself... 

I think we have to de-tangle the notions of VBR frame count (M) and VBR frame
byte length (noted in the 1|2 bytes before the frame).

When I read "M MUST NOT be zero" then later see that VBR frames can have zero
data, may take is that a frame with zero data is still a frame which is counted
by M. Also, we know its impossible to describe a duration of 0ms within the Opus
TOC byte. So I believe that frames with zero data still have a non-zero duration
(which ends up just being the duration of the discontinuity). 

Basically, in the case where a VBR frame's data is dropped by
transmission/encoder, the remaining parts of the packet are useful for you to
know that the frame was dropped and what that frame's duration would have been.

I don't think the parser should guard against these zero-data frames or change
what's emitted. This is still a valid Opus packet and the decoder should handle
it without issue. What this means for duration depends on how the decoder
reacts. Does it heal the gap? Play silence? If so, we're fine. If it just plays
the next frame and skips this duration, I guess the timeline may get just a
little off. Do we worry about this? We could do crazy things like remove any
mention of the frame from the packet and adjust the duration... but it seems
like way too much effort and layer violating for what is hopefully a rare edge
case. 

I vote we just emit the buffer and carry on. If we see Opus timeline bugs we can
re-consider. But I bet this dropped frame problem is not specific to just Opus
and seeing such bugs might motivate a decoder/architecture change instead of
additional complexity in layer violating parser code. WDYT?

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

I agree. See my comment on current PS.

+
+  return true;
+}
+
 WebMParserClient* WebMClusterParser::OnListStart(int id) {
   if (id == kWebMIdCluster) {
     cluster_timecode_ = -1;
@@ -324,9 +377,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;
+  int64 encoded_duration = -1;

[wolenetz, 2015/01/30 01:48:08]

ditto: base::TimeDelta please

[chcunningham, 2015/02/03 20:34:42]

Done.

   if (track_num == audio_.track_num()) {
     track = &audio_;
     encryption_key_id = audio_encryption_key_id_;
+    if (encryption_key_id.empty()) {
+      TryGetEncodedAudioDuration(data, size, &encoded_duration);

[wolenetz, 2015/01/30 01:48:08]

note the retval was ignored previously... not sure why it was there then.

[chcunningham, 2015/02/03 20:34:42]

Done.

+    }

[wolenetz, 2015/01/30 01:48:08]

nit: no {} necessary for one-line block here. I'm fine if you want it there for
readability though.

[wolenetz, 2015/01/30 01:48:08]

I'd like to see at least DVLOG for audio block if there is a
BlockGroup->BlockDuration and an encoded duration that significantly mismatches.
Perhaps always DVLOG here for unenc audio, but at high verbosity level (like
DVLOG(3,...))? If significantly mismatching, media_log() could be appropriate to
inform webdevs that something's wrong with content or parser.

[chcunningham, 2015/02/03 20:34:42]

It became a mutli-line block after adding the DVLOG. But also, my style is to
always do the {}. I was once bitten by a nasty bug that came back to this...
never again.

[chcunningham, 2015/02/03 20:34:42]

Done. How does 10ms for a threshold sound? 

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

See my comment on current PS.

   } else if (track_num == video_.track_num()) {
     track = &video_;
     encryption_key_id = video_encryption_key_id_;
@@ -407,7 +464,9 @@ bool WebMClusterParser::OnBlock(bool is_simple_block, int track_num,
   if (cluster_start_time_ == kNoTimestamp())
     cluster_start_time_ = timestamp;
 
-  if (block_duration >= 0) {
+  if (encoded_duration >= 0) {
+    buffer->set_duration(base::TimeDelta::FromMicroseconds(encoded_duration));
+  } else if (block_duration >= 0) {
     buffer->set_duration(base::TimeDelta::FromMicroseconds(
         block_duration * timecode_multiplier_));
   } else {