Flush sample buffers when reading new segments in Media Source BMFF parser.

Flush sample buffers when reading new segments in Media Source BMFF parser.

BUG=134274
TEST=MP4StreamParserTest


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

[acolwell GONE FROM CHROMIUM, 2012-06-25 21:22:16]

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc
File media/mp4/mp4_stream_parser.cc (right):

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:79: // Flush any buffers we've gotten in this
chunk so that buffers don't
It might be clearer to place this logic in EnqueueSample() where you transition
to kParsingBoxes. WDYT? Doesn't that correspond to the case you are trying to
capture?

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:81: if (!audio_buffers.empty()) {
This isn't sufficient. You need to do all the same checks that are done below.
I'd suggest moving that logic into a helper function and call it from here and
below.

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:82: CHECK(!audio_cb_.is_null());
Use DCHECK here and below. Let the null pointer exception below cause the crash
in release.

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:118: return false;
You should have a ChangeState(kError) here and below since it means we've
received unexpected data and we shouldn't accept  data anymore.

[strobe_, 2012-06-26 00:37:59]

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc
File media/mp4/mp4_stream_parser.cc (right):

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:79: // Flush any buffers we've gotten in this
chunk so that buffers don't
On 2012/06/25 21:22:16, acolwell wrote:
> It might be clearer to place this logic in EnqueueSample() where you
transition
> to kParsingBoxes. WDYT? Doesn't that correspond to the case you are trying to
> capture? 

Done.

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:81: if (!audio_buffers.empty()) {
On 2012/06/25 21:22:16, acolwell wrote:
> This isn't sufficient. You need to do all the same checks that are done below.
> I'd suggest moving that logic into a helper function and call it from here and
> below.

What checks are missing here as compared to below?

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:82: CHECK(!audio_cb_.is_null());
On 2012/06/25 21:22:16, acolwell wrote:
> Use DCHECK here and below. Let the null pointer exception below cause the
crash
> in release.

Done. (well, moved into the shared error conditions)

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:118: return false;
On 2012/06/25 21:22:16, acolwell wrote:
> You should have a ChangeState(kError) here and below since it means we've
> received unexpected data and we shouldn't accept  data anymore.

Done.

[acolwell GONE FROM CHROMIUM, 2012-06-26 15:25:14]

LGTM % minor nit

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc
File media/mp4/mp4_stream_parser.cc (right):

http://codereview.chromium.org/10660005/diff/1/media/mp4/mp4_stream_parser.cc...
media/mp4/mp4_stream_parser.cc:81: if (!audio_buffers.empty()) {
On 2012/06/26 00:37:59, strobe wrote:
> On 2012/06/25 21:22:16, acolwell wrote:
> > This isn't sufficient. You need to do all the same checks that are done
below.
> > I'd suggest moving that logic into a helper function and call it from here
and
> > below.
> 
> What checks are missing here as compared to below?
Sorry. The CHECK/DCHECK here was actually inappropriate. It should have been an
RCHECK(). Calls from JavaScript should not trigger a tab crash. I need to pay
more attention. Now that you are using the common helper function this is taken
care of.

http://codereview.chromium.org/10660005/diff/4001/media/mp4/mp4_stream_parser.h
File media/mp4/mp4_stream_parser.h (right):

http://codereview.chromium.org/10660005/diff/4001/media/mp4/mp4_stream_parser...
media/mp4/mp4_stream_parser.h:56: bool FlushSamples(BufferQueue* audio_buffers,
nit: How about SendAndFlushQueues or SendAndFlushSamples? Just to convey that we
actually are sending the buffers out to the callbacks before the queues are
flushed.

[commit-bot: I haz the power, 2012-06-27 05:16:15]

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

[strobe_, 2012-06-27 05:16:31]

https://chromiumcodereview.appspot.com/10660005/diff/4001/media/mp4/mp4_strea...
File media/mp4/mp4_stream_parser.h (right):

https://chromiumcodereview.appspot.com/10660005/diff/4001/media/mp4/mp4_strea...
media/mp4/mp4_stream_parser.h:56: bool FlushSamples(BufferQueue* audio_buffers,
On 2012/06/26 15:25:14, acolwell wrote:
> nit: How about SendAndFlushQueues or SendAndFlushSamples? Just to convey that
we
> actually are sending the buffers out to the callbacks before the queues are
> flushed.

Done.

[commit-bot: I haz the power, 2012-06-27 07:09:21]

Change committed as 144410