Fix WebMStreamParser to continue parsing after a cluster end

media/formats/webm/webm_stream_parser.cc

Index: media/formats/webm/webm_stream_parser.cc
diff --git a/media/formats/webm/webm_stream_parser.cc b/media/formats/webm/webm_stream_parser.cc
index a90a623a2bdf9aa729206e66cc53574d6453b95e..06ee499360549fd33ac4f6bb36beae248b155106 100644
--- a/media/formats/webm/webm_stream_parser.cc
+++ b/media/formats/webm/webm_stream_parser.cc
@@ -242,61 +242,86 @@ int WebMStreamParser::ParseCluster(const uint8* data, int size) {
   if (!cluster_parser_)
     return -1;
 
-  int id;
-  int64 element_size;
-  int result = WebMParseElementHeader(data, size, &id, &element_size);
-
-  if (result <= 0)
-    return result;
-
-  // TODO(matthewjheaney): implement support for chapters
-  if (id == kWebMIdCues || id == kWebMIdChapters) {
-    // TODO(wolenetz): Handle unknown-sized cluster parse completion correctly.
-    // See http://crbug.com/335676.
-    if (size < (result + element_size)) {
-      // We don't have the whole element yet. Signal we need more data.
-      return 0;
+  int result = 0;
+  int bytes_parsed;
+  bool cluster_ended;
+  do {
+    cluster_ended = false;
+
+    // If we are not parsing a cluster then handle the case when the next
+    // element is not a cluster.
+    if (!parsing_cluster_) {
+      int id;
+      int64 element_size;
+      bytes_parsed = WebMParseElementHeader(data, size, &id, &element_size);
+
+      if (bytes_parsed < 0)
+        return bytes_parsed;
+
+      if (bytes_parsed == 0)
+        return result;
+
+      switch (id) {
+        // Cues and Chapters are skipped.
+        // TODO(matthewjheaney): Implement support for chapters.
+        case kWebMIdCues:
+        case kWebMIdChapters:
+          if (size < (bytes_parsed + element_size)) {
+            // We don't have the whole element yet. Signal we need more data.
+            return result;
+          }
+
+          // Skip the element.
+          bytes_parsed += element_size;
+          result += bytes_parsed;
+          data += bytes_parsed;
+          size -= bytes_parsed;
+          continue;
+
+        case kWebMIdEBMLHeader:
+          ChangeState(kParsingHeaders);
+          return result;
+      }

[wolenetz, 2014/06/16 18:39:35]

Switch should have default case if not on enum. Also, ParseInfoAndTracks also
skips Cues and Chapters. To keep this ParseCluster() cleaner, ISTM better to
just replace this switch with:
if (id != kWebMIdCluster) {
  // Let ParseInfoAndTracks() handle all other level 1 element types.
  ChangeState(kParsingHeaders);
  return result;
}
and move matthewjheaney's TODO to the switch in ParseInfoAndTracks for now. This
allows similar coherent handling in one place of other level 0 and 1 elements
like SeekHead. Overall, it seems like there is really a fourth state that is
implicit beyond kWaitingForInit, kParsingHeaders and kParsingClusters, like
kFindingNextHeaderOrCluster. I'm fine with not including such an additional
state in this CL.

[Sergey Ulanov, 2014/06/16 22:17:41]

Done.

     }
-    // Skip the element.
-    return result + element_size;
-  }
 
-  if (id == kWebMIdEBMLHeader) {
-    // TODO(wolenetz): Handle unknown-sized cluster parse completion correctly.
-    // See http://crbug.com/335676.
-    ChangeState(kParsingHeaders);
-    return 0;
-  }
+    bytes_parsed = cluster_parser_->Parse(data, size);
 
-  int bytes_parsed = cluster_parser_->Parse(data, size);
+    if (bytes_parsed < 0)
+      return bytes_parsed;
 
-  if (bytes_parsed <= 0)
-    return bytes_parsed;
+    // If cluster detected, immediately notify new segment if we have not
+    // already done this.
+    if (!parsing_cluster_ && bytes_parsed > 0) {
+      parsing_cluster_ = true;
+      new_segment_cb_.Run();
+    }
 
-  // If cluster detected, immediately notify new segment if we have not already
-  // done this.
-  if (id == kWebMIdCluster && !parsing_cluster_) {
-    parsing_cluster_ = true;
-    new_segment_cb_.Run();
-  }
+    const BufferQueue& audio_buffers = cluster_parser_->GetAudioBuffers();
+    const BufferQueue& video_buffers = cluster_parser_->GetVideoBuffers();
+    const TextBufferQueueMap& text_map = cluster_parser_->GetTextBuffers();
 
-  const BufferQueue& audio_buffers = cluster_parser_->GetAudioBuffers();
-  const BufferQueue& video_buffers = cluster_parser_->GetVideoBuffers();
-  const TextBufferQueueMap& text_map = cluster_parser_->GetTextBuffers();
+    cluster_ended = cluster_parser_->cluster_ended();
 
-  bool cluster_ended = cluster_parser_->cluster_ended();
+    if ((!audio_buffers.empty() || !video_buffers.empty() ||
+         !text_map.empty()) &&
+        !new_buffers_cb_.Run(audio_buffers, video_buffers, text_map)) {
+      return -1;
+    }
 
-  if ((!audio_buffers.empty() || !video_buffers.empty() || !text_map.empty()) &&
-      !new_buffers_cb_.Run(audio_buffers, video_buffers, text_map)) {
-    return -1;
-  }
+    if (cluster_ended) {
+      parsing_cluster_ = false;
+      end_of_segment_cb_.Run();
+    }
 
-  if (cluster_ended) {
-    parsing_cluster_ = false;
-    end_of_segment_cb_.Run();
-  }
+    result += bytes_parsed;
+    data += bytes_parsed;
+    size -= bytes_parsed;
 
-  return bytes_parsed;
+    // WebMClusterParser returns 0 if |data| starts with a beginning of a new
+    // cluster. Try parsing again in that case.

[wolenetz, 2014/06/16 18:39:35]

nit: s/"returns 0"..."cluster."/"returns 0 and |cluster_ended| is true if
previously parsing an unknown-size cluster and |data| does not continue that
cluster."/

[Sergey Ulanov, 2014/06/16 22:17:41]

Done.

+  } while (size > 0 && (bytes_parsed > 0 || cluster_ended));
+
+  return result;
 }
 
 void WebMStreamParser::FireNeedKey(const std::string& key_id) {