Fix MP4StreamParser discard behavior when retaining 'moof'.

media/mp4/mp4_stream_parser.cc

 // Copyright (c) 2012 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/mp4/mp4_stream_parser.h"
 
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/time.h"
@@ skipping 68 matching lines @@
   bool result, 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_;
-        DCHECK(max_clear <= queue_.tail());
-        err = !(ReadMDATsUntil(max_clear) && queue_.Trim(max_clear));
+        err = !ReadAndDiscardMDATsUntil(max_clear);
       }
     }
   } while (result && !err);
 
   if (!err)
     err = !SendAndFlushSamples(&audio_buffers, &video_buffers);
 
   if (err) {
     DLOG(ERROR) << "Error while parsing MP4";
     queue_.Reset();
@@ skipping 330 matching lines @@
     err |= (audio_cb_.is_null() || !audio_cb_.Run(*audio_buffers));
     audio_buffers->clear();
   }
   if (!video_buffers->empty()) {
     err |= (video_cb_.is_null() || !video_cb_.Run(*video_buffers));
     video_buffers->clear();
   }
   return !err;
 }
 
-bool MP4StreamParser::ReadMDATsUntil(const int64 tgt_offset) {
-  DCHECK(tgt_offset <= queue_.tail());

[acolwell GONE FROM CHROMIUM, 2012/08/01 16:39:02]

Why remove this DCHECK?

[strobe_, 2012/08/01 19:01:33]

It was in fact always too narrow. It worked before because samples were stored
contiguously in runs (as mandated by BMFF) *and* runs were stored contiguously
in the 'mdat' (which isn't mandated). When aux info was moved to the 'moof', the
gap between aux info and the first sample would cause this to fire for any
append range that included all aux info but not the first frame in the sequence.

-
+bool MP4StreamParser::ReadAndDiscardMDATsUntil(const int64 tgt_offset) {
+  bool err = false;
   while (mdat_tail_ < tgt_offset) {
     const uint8* buf;
     int size;
     queue_.PeekAt(mdat_tail_, &buf, &size);
 
     FourCC type;
     int box_sz;
-    bool err;
     if (!BoxReader::StartTopLevelBox(buf, size, &type, &box_sz, &err))
-      return false;
+      break;
 
     if (type != FOURCC_MDAT) {
       DLOG(WARNING) << "Unexpected type while parsing MDATs: "
                     << FourCCToString(type);
     }
-
     mdat_tail_ += box_sz;
   }
-
-  return true;
+  queue_.Trim(std::min(mdat_tail_, tgt_offset));

[acolwell GONE FROM CHROMIUM, 2012/08/01 16:39:02]

Why do you unconditionally trim now?

[strobe_, 2012/08/01 19:01:33]

If multiple boxes are consumed in this loop, but the appended data ends in the
middle of a box header, we still want to clear all boxes up to the start of that
box header. Also, if user appends data on segment boundaries, mdat_tail_ will
equal tgt_offset on the last frame, so we want to clear  that data even though
the loop doesn't run.

A false return from queue_.Trim() is also not an error anymore for the same
reason that the DCHECK was removed.

+  return !err;
 }
 
 void MP4StreamParser::ChangeState(State new_state) {
   DVLOG(2) << "Changing state: " << new_state;
   state_ = new_state;
 }
 
 }  // namespace mp4
 }  // namespace media