Fix WebMStreamParser to handle Cues between Clusters correctly.

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 43 matching lines @@
   end_of_segment_cb_ = end_of_segment_cb;
   log_cb_ = log_cb;
 }
 
 void WebMStreamParser::Flush() {
   DCHECK_NE(state_, kWaitingForInit);
 
   byte_queue_.Reset();
   parsing_cluster_ = false;
 
   if (state_ != kParsingClusters)

[wolenetz, 2014/06/18 19:51:25]

Hmm. On second (third) look, it seems we should Reset() the parser if it is
not-null, even if |state_| isn't kParsingClusters, because it could have been
switched to kParsingHeaders due to cues, for instance, that followed previous
cluster parse. 

[Sergey Ulanov, 2014/06/18 21:47:55]

Done.

     return;
 
   cluster_parser_->Reset();
 }
 
 bool WebMStreamParser::Parse(const uint8* buf, int size) {
   DCHECK_NE(state_, kWaitingForInit);
 
   if (state_ == kError)
     return false;
@@ skipping 69 matching lines @@
     case kWebMIdCues:
     case kWebMIdChapters:
       // TODO(matthewjheaney): Implement support for chapters.
       if (cur_size < (result + element_size)) {
         // We don't have the whole element yet. Signal we need more data.
         return 0;
       }
       // Skip the element.
       return result + element_size;
       break;
+    case kWebMIdCluster:
+      if (!cluster_parser_) {
+        MEDIA_LOG(log_cb_) << "Found Cluster element before Info.";
+        return -1;
+      }
+      ChangeState(kParsingClusters);
+      return 0;
     case kWebMIdSegment:
       // Segment of unknown size indicates live stream.
       if (element_size == kWebMUnknownSize)
         unknown_segment_size_ = true;
       // Just consume the segment header.
       return result;
       break;
     case kWebMIdInfo:
       // We've found the element we are looking for.
       break;
@@ skipping 60 matching lines @@
       tracks_parser.audio_track_num(),
       tracks_parser.GetAudioDefaultDuration(timecode_scale_in_us),
       tracks_parser.video_track_num(),
       tracks_parser.GetVideoDefaultDuration(timecode_scale_in_us),
       tracks_parser.text_tracks(),
       tracks_parser.ignored_tracks(),
       tracks_parser.audio_encryption_key_id(),
       tracks_parser.video_encryption_key_id(),
       log_cb_));
 
   ChangeState(kParsingClusters);

[acolwell GONE FROM CHROMIUM, 2014/06/18 19:54:22]

You should probably remove this so that the same code path is always used to
transition to kParseClusters.

[Sergey Ulanov, 2014/06/18 21:47:55]

Done.

 
   if (!init_cb_.is_null())
     base::ResetAndReturn(&init_cb_).Run(true, params);
 
   return bytes_parsed;
 }
 
 int WebMStreamParser::ParseCluster(const uint8* data, int size) {
   if (!cluster_parser_)
     return -1;
@@ skipping 52 matching lines @@
       end_of_segment_cb_.Run();
     }
 
     result += bytes_parsed;
     data += bytes_parsed;
     size -= bytes_parsed;
 
     // WebMClusterParser returns 0 and |cluster_ended| is true if previously
     // parsing an unknown-size cluster and |data| does not continue that
     // cluster. Try parsing again in that case.
   } while (size > 0 && (bytes_parsed > 0 || cluster_ended));

[acolwell GONE FROM CHROMIUM, 2014/06/18 19:54:22]

Do we still need this loop? Can't you just transition to kParseHeaders after a
single cluster has been parsed and use the outer loop that calls this method?

[Sergey Ulanov, 2014/06/18 21:47:55]

see crbug.com/382807. We need to handle the case when a new buffer starts new
cluster that implicitly ends the previous cluster. In that case
cluster_parser_->Parse() will return 0, but if it's called again it will parse
more.
WebMStreamParser::Parse() will not call ParseCluster() again if ParseCluster()
returns 0.

Now that I look at it again, I think we don't need parsing_cluster_, and instead
can go back to the header-parsing start after end of each cluster. This
simplifies this code significantly. PTAL

 
   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