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

media/formats/mp4/mp4_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/mp4/mp4_stream_parser.h"
 
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/time/time.h"
@@ skipping 59 matching lines @@
 
 void MP4StreamParser::Reset() {
   queue_.Reset();
   runs_.reset();
   moof_head_ = 0;
   mdat_tail_ = 0;
 }
 
 void MP4StreamParser::Flush() {
   DCHECK_NE(state_, kWaitingForInit);
   Reset();

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

Before Reset(), we now have the possibility we're still in
kWaitingForSampleData, and there is a partial segment enqueued but not yet
parsed or emitted that we should parse and emit on Flush() (maybe for even a
subset of tracks). EnqueueSample(...)[perhaps repeatedly?] +
SendAndFlushSamples() prior to Reset() here?

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

As discussed offline, I think we should just drop them since we run into the
same problems that partial appends have. We could end up appending a bunch of
data for a single track, but nothing from the other ones. I think that could be
real confusing since the pipeline would stall eventhough it looks like there is
buffered data.

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

I agree.

   ChangeState(kParsingBoxes);
 }
 
 bool MP4StreamParser::Parse(const uint8* buf, int size) {
   DCHECK_NE(state_, kWaitingForInit);
 
   if (state_ == kError)
     return false;
 
   queue_.Push(buf, 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);
 
   if (!err)
     err = !SendAndFlushSamples(&audio_buffers, &video_buffers);
 
   if (err) {
     DLOG(ERROR) << "Error while parsing MP4";
     moov_.reset();
     Reset();
@@ skipping 188 matching lines @@
   return true;
 }
 
 bool MP4StreamParser::ParseMoof(BoxReader* reader) {
   RCHECK(moov_.get());  // Must already have initialization segment
   MovieFragment moof;
   RCHECK(moof.Parse(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;
 }
 
 void MP4StreamParser::EmitNeedKeyIfNecessary(
     const std::vector<ProtectionSystemSpecificHeader>& headers) {
   // TODO(strobe): ensure that the value of init_data (all PSSH headers
   // concatenated in arbitrary order) matches the EME spec.
   // See https://www.w3.org/Bugs/Public/show_bug.cgi?id=17673.
   if (headers.empty())
     return;
@@ skipping 58 matching lines @@
     subsamples->push_back(entry);
   } else {
     (*subsamples)[0].clear_bytes += kADTSHeaderMinSize;
   }
   return true;
 }
 
 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
     *err = !SendAndFlushSamples(audio_buffers, video_buffers);
     if (*err)
       return false;
 
     // Remain in kEnqueueingSamples state, discarding data, until the end of

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

nit: seems a good time to fix this comment: s/kEnqueue/kEmitt/

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

Done.

     // the current 'mdat' box has been appended to the queue.
     if (!queue_.Trim(mdat_tail_))
       return false;
 
     ChangeState(kParsingBoxes);
     end_of_segment_cb_.Run();
     return true;
   }
 
   if (!runs_->IsSampleValid()) {
     runs_->AdvanceRun();
     return true;
   }
 
   DCHECK(!(*err));
 
   const uint8* buf;
   int buf_size;
   queue_.Peek(&buf, &buf_size);
   if (!buf_size) return false;
 
   bool audio = has_audio_ && audio_track_id_ == runs_->track_id();
   bool video = has_video_ && video_track_id_ == runs_->track_id();
 
   // Skip this entire track if it's not one we're interested in
   if (!audio && !video)
     runs_->AdvanceRun();

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

I'm confused. What if there isn't another run here? Seems we might need to check
!runs_->IsRunValid() before proceeding after this AdvanceRun() (by just
returning true and letting the caller re-call us).

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

Yeah. I saw this too. I was planning on fixing that in a different CL because I
didn't have a test case for exposing the bug yet.

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

nit: (sgtm) Maybe add a TODO?

 
   // Attempt to cache the auxiliary information first. Aux info is usually
   // placed in a contiguous block before the sample data, rather than being
   // interleaved. If we didn't cache it, this would require that we retain the
   // start of the segment buffer while reading samples. Aux info is typically
   // quite small compared to sample data, so this pattern is useful on
   // memory-constrained devices where the source buffer consumes a substantial
   // portion of the total system memory.
   if (runs_->AuxInfoNeedsToBeCached()) {
     queue_.PeekAt(runs_->aux_info_offset() + moof_head_, &buf, &buf_size);
@@ skipping 129 matching lines @@
   }
   queue_.Trim(std::min(mdat_tail_, offset));
   return !err;
 }
 
 void MP4StreamParser::ChangeState(State new_state) {
   DVLOG(2) << "Changing 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