Fix WebMStreamParser to continue parsing after a cluster end

media/formats/webm/webm_stream_parser.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/formats/webm/webm_stream_parser.h"
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/callback_helpers.h"
@@ skipping 226 matching lines @@
 
   return bytes_parsed;
 }
 
 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);
+  int bytes_parsed = WebMParseElementHeader(data, size, &id, &element_size);
 
-  if (result <= 0)
-    return result;
+  if (bytes_parsed <= 0)
+    return bytes_parsed;
 
   // 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)) {
+    if (size < (bytes_parsed + element_size)) {
       // We don't have the whole element yet. Signal we need more data.
       return 0;
     }
     // Skip the element.
-    return result + element_size;
+    return bytes_parsed + element_size;
   }
 
   if (id == kWebMIdEBMLHeader) {
     // TODO(wolenetz): Handle unknown-sized cluster parse completion correctly.
     // See http://crbug.com/335676.
     ChangeState(kParsingHeaders);
     return 0;
   }
 
-  int bytes_parsed = cluster_parser_->Parse(data, size);
+  int result = 0;
+  bool cluster_ended;
+  do {
+    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 (id == kWebMIdCluster && !parsing_cluster_) {
-    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();
 
-  bool cluster_ended = cluster_parser_->cluster_ended();
+    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();
+    }
 
-  return bytes_parsed;
+    result += bytes_parsed;
+    data += bytes_parsed;
+    size -= bytes_parsed;
+
+    // WebMClusterParser returns 0 if |data| starts with a beginning of a new
+    // cluster. Try parsing again in that case.
+  } while (bytes_parsed > 0 || cluster_ended);

[wolenetz, 2014/06/12 21:32:48]

This looks wrong. Yes, in the case of a new append allowing the cluster parser
to detect the end of a previously 'kWebMUnknownSize' cluster, cluster_ended will
be true, (though bytes_parsed might not be 0, see below) and a single repeat
here would work. But the new append could be a sequence of at least one cluster
followed by an init segment, all at once. Some further peek-ahead to prevent
attempt to parse init segment as a cluster is needed, I think, instead of
blindly assuming in this loop that all of *data[0]..*data[size-1] is all media
segment (cluster) data.

bytes_parsed might not be 0 if the previous append did not completely send all
of an unknown-sized cluster, and the next append includes both the completion of
the first cluster and then some other data.

[Sergey Ulanov, 2014/06/13 00:26:08]

Done.
Yes. I think this code should handle that case correctly.

+
+  return result;
 }
 
 void WebMStreamParser::FireNeedKey(const std::string& key_id) {
   std::vector<uint8> key_id_vector(key_id.begin(), key_id.end());
   need_key_cb_.Run(kWebMEncryptInitDataType, key_id_vector);
 }
 
 }  // namespace media