Fix muxed MP4 parsing so it won't crash on partial media segment appends.

media/formats/mp4/mp4_stream_parser.cc

Index: media/formats/mp4/mp4_stream_parser.cc
diff --git a/media/formats/mp4/mp4_stream_parser.cc b/media/formats/mp4/mp4_stream_parser.cc
index 4d56a8c561c0ae37200ea391b41b0009523719ac..d2e4d44b15e078a615948e53e509fae505067f64 100644
--- a/media/formats/mp4/mp4_stream_parser.cc
+++ b/media/formats/mp4/mp4_stream_parser.cc
@@ -92,18 +92,33 @@ bool MP4StreamParser::Parse(const uint8* buf, int size) {
   BufferQueue audio_buffers;
   BufferQueue video_buffers;
 
-  bool result, err = false;
+  bool result = false;
+  bool err = false;
 
   do {
-    if (state_ == kParsingBoxes) {
-      result = ParseBox(&err);
-    } else {
-      DCHECK_EQ(kEmittingSamples, state_);
-      result = EnqueueSample(&audio_buffers, &video_buffers, &err);
-      if (result) {
-        int64 max_clear = runs_->GetMaxClearOffset() + moof_head_;
-        err = !ReadAndDiscardMDATsUntil(max_clear);
-      }
+    switch(state_) {

[wolenetz, 2014/06/19 22:01:14]

lint nit: insert space before (

[acolwell GONE FROM CHROMIUM, 2014/06/19 23:46:46]

Done.

+      case kWaitingForInit:
+      case kError:
+        NOTREACHED();
+        return false;
+

[wolenetz, 2014/06/19 22:01:14]

nit: add break; ? I'm not sure of code style here (guides don't clarify).

[acolwell GONE FROM CHROMIUM, 2014/06/19 23:46:46]

I don't believe it is necessary if all paths in the case return.

[wolenetz, 2014/06/20 00:17:29]

Yeah, it was just a style question, not a correctness question. I'm fine without
break; added in this case.

+      case kParsingBoxes:
+        result = ParseBox(&err);
+        break;
+
+      case kWaitingForSampleData:
+        result = HaveEnoughDataToEnqueueSamples();
+        if (result)
+          ChangeState(kEmittingSamples);
+        break;
+
+      case kEmittingSamples:
+        result = EnqueueSample(&audio_buffers, &video_buffers, &err);
+        if (result) {
+          int64 max_clear = runs_->GetMaxClearOffset() + moof_head_;
+          err = !ReadAndDiscardMDATsUntil(max_clear);
+        }
+        break;
     }
   } while (result && !err);
 
@@ -312,9 +327,10 @@ bool MP4StreamParser::ParseMoof(BoxReader* reader) {
   if (!runs_)
     runs_.reset(new TrackRunIterator(moov_.get(), log_cb_));
   RCHECK(runs_->Init(moof));
+  RCHECK(ComputeHighestEndOffset(moof));
   EmitNeedKeyIfNecessary(moof.pssh);
   new_segment_cb_.Run();
-  ChangeState(kEmittingSamples);
+  ChangeState(kWaitingForSampleData);
   return true;
 }
 
@@ -393,6 +409,8 @@ bool MP4StreamParser::PrepareAACBuffer(
 bool MP4StreamParser::EnqueueSample(BufferQueue* audio_buffers,
                                     BufferQueue* video_buffers,
                                     bool* err) {
+  DCHECK_EQ(state_, kEmittingSamples);
+
   if (!runs_->IsRunValid()) {
     // Flush any buffers we've gotten in this chunk so that buffers don't
     // cross NewSegment() calls
@@ -577,5 +595,40 @@ void MP4StreamParser::ChangeState(State new_state) {
   state_ = new_state;
 }
 
+bool MP4StreamParser::HaveEnoughDataToEnqueueSamples() {
+  DCHECK_EQ(state_, kWaitingForSampleData);
+  // For muxed content, make sure we have data up to |highest_end_offset_|
+  // so we can ensure proper equeuing behavior. Otherwise assume we have enough

[wolenetz, 2014/06/19 22:01:15]

nit: s/eq/enq

[acolwell GONE FROM CHROMIUM, 2014/06/19 23:46:46]

Done.

+  // data and allow per sample offset checks to meter sample enqueuing.
+  // TODO(acolwell): Fix trun box handling so we don't have to special case
+  // muxed content.
+  return !(has_audio_ && has_video_ &&
+           queue_.tail() < highest_end_offset_ + moof_head_);
+}
+
+bool MP4StreamParser::ComputeHighestEndOffset(const MovieFragment& moof) {
+  highest_end_offset_ = 0;

[wolenetz, 2014/06/19 22:01:14]

nit: Can we safely short-cut and return true after setting 0 here if there is
only one track_id for which we're interested in emitting samples (e.g.
audio-only or video-only)?

[acolwell GONE FROM CHROMIUM, 2014/06/19 23:46:47]

I suppose, but that would then mean that highest_end_offset_ would not have a
consistent definition. ISTM that would be rather confusing.

+
+  TrackRunIterator runs(moov_.get(), log_cb_);
+  RCHECK(runs.Init(moof));
+
+  while (runs.IsRunValid()) {
+    int64 aux_info_end_offset = runs.aux_info_offset() + runs.aux_info_size();
+    if (aux_info_end_offset > highest_end_offset_)
+      highest_end_offset_ = aux_info_end_offset;
+
+    while (runs.IsSampleValid()) {
+      int64 sample_end_offset = runs.sample_offset() + runs.sample_size();
+      if (sample_end_offset > highest_end_offset_)
+        highest_end_offset_ = sample_end_offset;
+
+      runs.AdvanceSample();
+    }
+    runs.AdvanceRun();
+  }
+
+  return true;
+}
+
 }  // namespace mp4
 }  // namespace media