Fix MP4StreamParser discard behavior when retaining 'moof'.

Fix MP4StreamParser discard behavior when retaining 'moof'.

Adding support for retaining the 'moof' atom (revision 148982) broke an
assumption in ReadMDATsUntil(). This removes the assumption, consolidates
logic, and adds a comment and a test.

BUG=
TEST=MP4StreamParserTest, manual tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149499

[strobe_, 2012-08-01 06:22:52]

Fix for drm-segment-piece. Thanks!

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

looks good. Just have a few nits and questions.

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
File media/mp4/mp4_stream_parser.cc (left):

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
media/mp4/mp4_stream_parser.cc:442: DCHECK(tgt_offset <= queue_.tail());
Why remove this DCHECK?

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
File media/mp4/mp4_stream_parser.cc (right):

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
media/mp4/mp4_stream_parser.cc:458: queue_.Trim(std::min(mdat_tail_,
tgt_offset));
Why do you unconditionally trim now?

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
File media/mp4/mp4_stream_parser.h (right):

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
media/mp4/mp4_stream_parser.h:59: bool ReadAndDiscardMDATsUntil(const int64
tgt_tail);
nits: How about just using offset for the parameter name or spell out target
instead of tgt? Update comment to match the parameter name.

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

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
File media/mp4/mp4_stream_parser.cc (left):

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
media/mp4/mp4_stream_parser.cc:442: DCHECK(tgt_offset <= queue_.tail());
On 2012/08/01 16:39:02, acolwell wrote:
> Why remove this DCHECK?

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.

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
File media/mp4/mp4_stream_parser.cc (right):

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
media/mp4/mp4_stream_parser.cc:458: queue_.Trim(std::min(mdat_tail_,
tgt_offset));
On 2012/08/01 16:39:02, acolwell wrote:
> Why do you unconditionally trim now?

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.

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
File media/mp4/mp4_stream_parser.h (right):

http://chromiumcodereview.appspot.com/10834101/diff/1/media/mp4/mp4_stream_pa...
media/mp4/mp4_stream_parser.h:59: bool ReadAndDiscardMDATsUntil(const int64
tgt_tail);
On 2012/08/01 16:39:02, acolwell wrote:
> nits: How about just using offset for the parameter name or spell out target
> instead of tgt? Update comment to match the parameter name. 

Done.

[acolwell GONE FROM CHROMIUM, 2012-08-01 20:08:07]

LGTM

[commit-bot: I haz the power, 2012-08-01 20:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/strobe@google.com/10834101/7001

[commit-bot: I haz the power, 2012-08-01 21:45:04]

Change committed as 149499