Handle end of stream for media...

Handle end of stream for media
When FFmpegDemuxer failed to decode a raw packet, the signal of
end of stream should bubble up to the renderers. It is done in
this CL by creating fake buffers.
This change also fixes a bug with video of only 1 frame.

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

[Alpha Left Google, 2009-05-20 02:36:08]

Handle EOS and bubble the signal up to renderers.
So the flow of the EOS signal is:

In case of video:
FFmpegDemuxer -> FFmpegDemuxerStream(s) -> DecoderBase -> FFmpegVideoDecoder ->
VideoThread -> ...

In case of audio:
FFmpegDemuxer -> FFmpegDemuxerStream(s) -> DecoderBase -> FFmpegAudioDecoder ->
AudioRendererBase -> ...

Unfortunately each layer has to respect the EOS buffer and don't anything with
it, now it is handled in:
FFmpegVideoDecoder and VideoThread
FFmpegAudioDecoder and AudioRendererBase

[scherkus (not reviewing), 2009-05-22 20:35:10]

general thought:

what if EOF was handled by having buffers/frames with NULL data and zero size?

right now we have to maintain the assertion that if IsEndOfStream() == true,
there is no valid data attached to the buffer... but what if there is?

just thinking if we want end of stream imply no data, perhaps we should say no
data implies end of stream (less confusion)

http://codereview.chromium.org/113611/diff/59/1033
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/113611/diff/59/1033#newcode122
Line 122: void FFmpegDemuxerStream::StreamHasEnded() {
I think it'd be better handle this in EnqueuePacket by passing in NULL

http://codereview.chromium.org/113611/diff/59/1033#newcode124
Line 124: duration_,
this should be 0

you can also just pass in the default constructor:
base::TimeDelta()

http://codereview.chromium.org/113611/diff/59/1033#newcode327
Line 327: if (result < 0) {
should be:
if (result == AVERROR_EOF) {
  StreamHasEnded();
  return;
} else if (result != 0) {
  // error
  return;
}

http://codereview.chromium.org/113611/diff/59/1033#newcode381
Line 381: void FFmpegDemuxer::StreamHasEnded() {
you can just inline this code and add a comment saying we're EOF

http://codereview.chromium.org/113611/diff/1046/1053
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/113611/diff/1046/1053#newcode62
Line 62: if (!stopped_ && !buffer_in->IsEndOfStream()) {
add comment here as well
"Don't enqueue end of stream buffers since they don't contain data."

http://codereview.chromium.org/113611/diff/1046/1047
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1046/1047#newcode78
Line 78: HandleEOS(input);
if you look at what ffmpeg.c does, they actually call avcodec_decode_audio/video
with NULL data and 0 size

we may have to do the same (i.e., flush decoders) to make sure we get every last
bit of audio/video

if you want just add a TODO comment saying if we should flush the decoders

http://codereview.chromium.org/113611/diff/1046/1047#newcode94
Line 94: if (/*result < 0 ||
commented out code?

http://codereview.chromium.org/113611/diff/1046/1047#newcode121
Line 121: void FFmpegAudioDecoder::HandleEOS(Buffer* buffer) {
again I'd say inline this code so we can see that both branches end with
EnqueueResult

http://codereview.chromium.org/113611/diff/1046/1057
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1046/1057#newcode223
Line 223: // If this is an EOS frame, don't enqueue it.
should explain why we don't want to enqueue, like:
"Don't enqueue end of stream frames since they don't contain data."

[Alpha Left Google, 2009-05-22 21:01:35]

On 2009/05/22 20:35:10, scherkus wrote:
> general thought:
> 
> what if EOF was handled by having buffers/frames with NULL data and zero size?
> 
> right now we have to maintain the assertion that if IsEndOfStream() == true,
> there is no valid data attached to the buffer... but what if there is?
> 
> just thinking if we want end of stream imply no data, perhaps we should say no
> data implies end of stream (less confusion)

Agreed, we have currently two possible ways to signal EOS, we really should just
use one method, I'll use data == NULL then.

> 
> http://codereview.chromium.org/113611/diff/59/1033
> File media/filters/ffmpeg_demuxer.cc (right):
> 
> http://codereview.chromium.org/113611/diff/59/1033#newcode122
> Line 122: void FFmpegDemuxerStream::StreamHasEnded() {
> I think it'd be better handle this in EnqueuePacket by passing in NULL
> 
> http://codereview.chromium.org/113611/diff/59/1033#newcode124
> Line 124: duration_,
> this should be 0
> 
> you can also just pass in the default constructor:
> base::TimeDelta()
> 
> http://codereview.chromium.org/113611/diff/59/1033#newcode327
> Line 327: if (result < 0) {
> should be:
> if (result == AVERROR_EOF) {
>   StreamHasEnded();
>   return;
> } else if (result != 0) {
>   // error
>   return;
> }
> 
> http://codereview.chromium.org/113611/diff/59/1033#newcode381
> Line 381: void FFmpegDemuxer::StreamHasEnded() {
> you can just inline this code and add a comment saying we're EOF
> 
> http://codereview.chromium.org/113611/diff/1046/1053
> File media/filters/audio_renderer_base.cc (right):
> 
> http://codereview.chromium.org/113611/diff/1046/1053#newcode62
> Line 62: if (!stopped_ && !buffer_in->IsEndOfStream()) {
> add comment here as well
> "Don't enqueue end of stream buffers since they don't contain data."
> 
> http://codereview.chromium.org/113611/diff/1046/1047
> File media/filters/ffmpeg_audio_decoder.cc (right):
> 
> http://codereview.chromium.org/113611/diff/1046/1047#newcode78
> Line 78: HandleEOS(input);
> if you look at what ffmpeg.c does, they actually call
avcodec_decode_audio/video
> with NULL data and 0 size
> 
> we may have to do the same (i.e., flush decoders) to make sure we get every
last
> bit of audio/video
> 
> if you want just add a TODO comment saying if we should flush the decoders
> 
> http://codereview.chromium.org/113611/diff/1046/1047#newcode94
> Line 94: if (/*result < 0 ||
> commented out code?
> 
> http://codereview.chromium.org/113611/diff/1046/1047#newcode121
> Line 121: void FFmpegAudioDecoder::HandleEOS(Buffer* buffer) {
> again I'd say inline this code so we can see that both branches end with
> EnqueueResult
> 
> http://codereview.chromium.org/113611/diff/1046/1057
> File media/filters/video_thread.cc (right):
> 
> http://codereview.chromium.org/113611/diff/1046/1057#newcode223
> Line 223: // If this is an EOS frame, don't enqueue it.
> should explain why we don't want to enqueue, like:
> "Don't enqueue end of stream frames since they don't contain data."

[Alpha Left Google, 2009-05-23 00:49:38]

There was also a in avcodec_decode_video that we should pass in empty data and 0
size after the last valid packet to get the last frame out. This particularly is
a bug for video with single frame. This CL should fix this problem, but this is
only tested with video.

http://codereview.chromium.org/113611/diff/1046/1053
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/113611/diff/1046/1053#newcode62
Line 62: if (!stopped_ && !buffer_in->IsEndOfStream()) {
On 2009/05/22 20:35:11, scherkus wrote:
> add comment here as well
> "Don't enqueue end of stream buffers since they don't contain data."

Done.

http://codereview.chromium.org/113611/diff/1046/1047
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1046/1047#newcode78
Line 78: HandleEOS(input);
On 2009/05/22 20:35:11, scherkus wrote:
> if you look at what ffmpeg.c does, they actually call
avcodec_decode_audio/video
> with NULL data and 0 size
> 
> we may have to do the same (i.e., flush decoders) to make sure we get every
last
> bit of audio/video
> 
> if you want just add a TODO comment saying if we should flush the decoders

Done.

http://codereview.chromium.org/113611/diff/1046/1047#newcode94
Line 94: if (/*result < 0 ||
On 2009/05/22 20:35:11, scherkus wrote:
> commented out code?
my bad, some debugging code..

http://codereview.chromium.org/113611/diff/1046/1057
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1046/1057#newcode223
Line 223: // If this is an EOS frame, don't enqueue it.
On 2009/05/22 20:35:11, scherkus wrote:
> should explain why we don't want to enqueue, like:
> "Don't enqueue end of stream frames since they don't contain data."

Done.

[scherkus (not reviewing), 2009-05-27 22:49:07]

http://codereview.chromium.org/113611/diff/1078/105
File media/base/video_frame_impl.h (right):

http://codereview.chromium.org/113611/diff/1078/105#newcode24
Line 24: static void CreateEmptyFrame(scoped_refptr<VideoFrame>* frame_out);
could you add a quick comment for this?

http://codereview.chromium.org/113611/diff/1078/100
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/113611/diff/1078/100#newcode63
Line 63: // it has not data.
nit: "has not" -> "has no"

http://codereview.chromium.org/113611/diff/1078/100#newcode79
Line 79: initialized_ = true;
I think we have the same issues as the video renderer, ie.
DCHECK(!queue_.empty());

http://codereview.chromium.org/113611/diff/1078/96
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1078/96#newcode79
Line 79: if (input->IsEndOfStream()) {
the thing is we DO want to decode but pass in NULL to flush the decoder

We'll need some investigating to see how much FFmpeg can emit when flushing the
decoder.  If it's only ever 0 or 1 frames, then we can safely always enqueue an
end-of-stream buffer whenever input->IsEndOfStream() is true after whatever
FFmpeg outputs.

If it can be 2 frames or more... that can be a little scary and I'm not very
sure how to handle that :\

http://codereview.chromium.org/113611/diff/1078/96#newcode86
Line 86: // avcodec_decode_audio with empty data.
just noticed your comment here.. I'd go all the way and just implement this now
(frank just uploaded a similar patch for bench.cc)

http://codereview.chromium.org/113611/diff/1078/96#newcode100
Line 100: if (result < 0 || output_buffer_size < 0)
curious why this was added if you're handling end of stream by returning
immediately

http://codereview.chromium.org/113611/diff/1078/98
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/113611/diff/1078/98#newcode33
Line 33: if (packet_.get())
On EOF av_read_frame() returns a packet with NULL data and 0 size.  Instead of
worrying about a NULL packet, just pass in the packet with the correct fields
set

http://codereview.chromium.org/113611/diff/1078/98#newcode126
Line 126: void FFmpegDemuxerStream::StreamHasEnded() {
again this function isn't needed if you pass in an AVPacket with NULL data and 0
size.

http://codereview.chromium.org/113611/diff/1078/98#newcode330
Line 330: if (result < 0) {
check for AVERROR_EOF for EOF

all other negative result codes are an error, so I would signal the pipeline in
that scenario (or just have a NOTIMPLEMENTED or DCHECK for now)

http://codereview.chromium.org/113611/diff/1078/98#newcode387
Line 387: (*iter)->StreamHasEnded();
here I'd construct an EOF packet for each stream

it seems silly but we should work with AVPacket as much as possible.

Important values to set:
size = 0
data = NULL
pts = 0
dts = 0
duration = 0

...or just memset to 0 :)

http://codereview.chromium.org/113611/diff/1078/97
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1078/97#newcode103
Line 103: if (buffer->IsEndOfStream()) {
does this section of the code actually get hit?

I'm worrying about if we inject a NULL frame to flush the decoder, but then we
never follow up enqueuing our own end of stream packet

would it make more sense to flush the decoder, enqueue anything that comes out,
then always enqueue an EOF frame?

http://codereview.chromium.org/113611/diff/1078/97#newcode112
Line 112: // Queue the incoming timestamp only if can decode the frame
successfully.
nit: "only if can" -> "only if we can"

http://codereview.chromium.org/113611/diff/1078/104
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1078/104#newcode223
Line 223: // If this is an EOS frame, don't enqueue it since it has no data.
readability: try to use "end of stream" instead of EOS (I don't think that's a
standard abbreviation other people would be used to)

http://codereview.chromium.org/113611/diff/1078/104#newcode236
Line 236: // frame in the queue.
hmm the only time this be true if we get an EOF frame as the very first frame we
receive

we should check if it's possible to create such a video (i.e., a video with 1
audio packet and 0 video packets)  maybe frank could help

http://codereview.chromium.org/113611/diff/1078/104#newcode245
Line 245: frame_available_.Signal();
since we hold onto the lock the entire time you can signal this up when we
push_back the frame

[scherkus (not reviewing), 2009-05-27 22:54:59]

also... UNIT TESTS :)

ffmpeg_demuxer_unittest.cc already sets up an EOF scenario.. you can add in
asserts that packets are generated for every stream that have no data, zero size
and IsEndOfStream() == true

[Alpha Left Google, 2009-06-02 19:13:02]

http://codereview.chromium.org/113611/diff/1078/105
File media/base/video_frame_impl.h (right):

http://codereview.chromium.org/113611/diff/1078/105#newcode24
Line 24: static void CreateEmptyFrame(scoped_refptr<VideoFrame>* frame_out);
On 2009/05/27 22:49:08, scherkus wrote:
> could you add a quick comment for this?

Done.

http://codereview.chromium.org/113611/diff/1078/100
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/113611/diff/1078/100#newcode63
Line 63: // it has not data.
On 2009/05/27 22:49:08, scherkus wrote:
> nit: "has not" -> "has no"

Done.

http://codereview.chromium.org/113611/diff/1078/100#newcode79
Line 79: initialized_ = true;
On 2009/05/27 22:49:08, scherkus wrote:
> I think we have the same issues as the video renderer, ie.
> DCHECK(!queue_.empty());

If the queue is empty then we should better raise a pipeline error instead of
proceeding with initialization complete.

http://codereview.chromium.org/113611/diff/1078/96
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1078/96#newcode79
Line 79: if (input->IsEndOfStream()) {
On 2009/05/27 22:49:08, scherkus wrote:
> the thing is we DO want to decode but pass in NULL to flush the decoder
> 
> We'll need some investigating to see how much FFmpeg can emit when flushing
the
> decoder.  If it's only ever 0 or 1 frames, then we can safely always enqueue
an
> end-of-stream buffer whenever input->IsEndOfStream() is true after whatever
> FFmpeg outputs.
> 
> If it can be 2 frames or more... that can be a little scary and I'm not very
> sure how to handle that :\

Just tried to do an experiment, in fact doing multiple runs of
avcodec_decode_audio with NULL went through and returned me 0, so I'm moving
this down with if (Result == 0).

If we really want to be extra careful we can set a state so that we won't pass
in two NULL packets in a row to FFmpeg. Reset this state whenever we receive a
non-NULL packet again.

http://codereview.chromium.org/113611/diff/1078/96#newcode86
Line 86: // avcodec_decode_audio with empty data.
On 2009/05/27 22:49:08, scherkus wrote:
> just noticed your comment here.. I'd go all the way and just implement this
now
> (frank just uploaded a similar patch for bench.cc)

Done.

http://codereview.chromium.org/113611/diff/1078/96#newcode100
Line 100: if (result < 0 || output_buffer_size < 0)
On 2009/05/27 22:49:08, scherkus wrote:
> curious why this was added if you're handling end of stream by returning
> immediately

I think this is merge problem.. need to fix this.

http://codereview.chromium.org/113611/diff/1078/98
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/113611/diff/1078/98#newcode33
Line 33: if (packet_.get())
On 2009/05/27 22:49:08, scherkus wrote:
> On EOF av_read_frame() returns a packet with NULL data and 0 size.  Instead of
> worrying about a NULL packet, just pass in the packet with the correct fields
> set

Done.

http://codereview.chromium.org/113611/diff/1078/98#newcode126
Line 126: void FFmpegDemuxerStream::StreamHasEnded() {
On 2009/05/27 22:49:08, scherkus wrote:
> again this function isn't needed if you pass in an AVPacket with NULL data and
0
> size.

Done.

http://codereview.chromium.org/113611/diff/1078/98#newcode330
Line 330: if (result < 0) {
On 2009/05/27 22:49:08, scherkus wrote:
> check for AVERROR_EOF for EOF
> 
> all other negative result codes are an error, so I would signal the pipeline
in

Interestingly AVERROR_EOF is -32 but a real end of stream is -1, we are actually
safe to call for end of stream in case of any error.
> that scenario (or just have a NOTIMPLEMENTED or DCHECK for now)

Done.

http://codereview.chromium.org/113611/diff/1078/98#newcode387
Line 387: (*iter)->StreamHasEnded();
On 2009/05/27 22:49:08, scherkus wrote:
> here I'd construct an EOF packet for each stream
> 
> it seems silly but we should work with AVPacket as much as possible.
> 
> Important values to set:
> size = 0
> data = NULL
> pts = 0
> dts = 0
> duration = 0
> 
> ...or just memset to 0 :)

Done.

http://codereview.chromium.org/113611/diff/1078/97
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1078/97#newcode103
Line 103: if (buffer->IsEndOfStream()) {
On 2009/05/27 22:49:08, scherkus wrote:
> does this section of the code actually get hit?
> 
> I'm worrying about if we inject a NULL frame to flush the decoder, but then we
> never follow up enqueuing our own end of stream packet
> 
> would it make more sense to flush the decoder, enqueue anything that comes
out,
> then always enqueue an EOF frame?

I made this condition even harder to reach and it worked. There's several
reasons I'm doing this:
1. We may receive a packet here but decoded == 0 (FFmpeg caches the input)
2. FFmpeg didn't read anything but outputs a frame (It's being flushed)

So we can't rely on the above conditions to declare end of stream. We can let
the above conditions pass through until we have three conditions met. If we
output nothing from this method, DecoderBase will try to read again from the
demuxer and passes us the buffer to decode. We will keeping receiving
buffer->IsEndOfStream() since av_read_XXX continues to give us errors, for that
we enqueue end of stream buffer. So the end result is that we get errors and
these errors trigger flushing of decoders until we really have nothing left. The
same applies to audio too.

http://codereview.chromium.org/113611/diff/1078/97#newcode112
Line 112: // Queue the incoming timestamp only if can decode the frame
successfully.
On 2009/05/27 22:49:08, scherkus wrote:
> nit: "only if can" -> "only if we can"

Done.

http://codereview.chromium.org/113611/diff/1078/104
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1078/104#newcode223
Line 223: // If this is an EOS frame, don't enqueue it since it has no data.
On 2009/05/27 22:49:08, scherkus wrote:
> readability: try to use "end of stream" instead of EOS (I don't think that's a
> standard abbreviation other people would be used to)

Done.

http://codereview.chromium.org/113611/diff/1078/104#newcode236
Line 236: // frame in the queue.
On 2009/05/27 22:49:08, scherkus wrote:
> hmm the only time this be true if we get an EOF frame as the very first frame
we
> receive
> 
> we should check if it's possible to create such a video (i.e., a video with 1
> audio packet and 0 video packets)  maybe frank could help

Or we receive a badly made file that only has the container without content. ;-)

http://codereview.chromium.org/113611/diff/1078/104#newcode245
Line 245: frame_available_.Signal();
On 2009/05/27 22:49:08, scherkus wrote:
> since we hold onto the lock the entire time you can signal this up when we
> push_back the frame

Done.

[scherkus (not reviewing), 2009-06-02 19:20:52]

few comments.. rest to come later

http://codereview.chromium.org/113611/diff/1125/181
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/113611/diff/1125/181#newcode31
Line 31: EXPECT_TRUE(buffer->IsEndOfStream());
we should also test that GetData() is NULL
EXPECT_FALSE(buffer->GetData());

http://codereview.chromium.org/113611/diff/1125/181#newcode38
Line 38: EXPECT_TRUE(buffer->IsEndOfStream());
this line isn't needed anymore

http://codereview.chromium.org/113611/diff/1125/183
File media/base/pipeline.h (right):

http://codereview.chromium.org/113611/diff/1125/183#newcode40
Line 40: PIPELINE_ERROR_DEMUX,
do the demuxer errors listed below not cover your situation?

http://codereview.chromium.org/113611/diff/1125/174
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/113611/diff/1125/174#newcode98
Line 98: host_->Error(PIPELINE_ERROR_NO_DATA);
This might not actually be an error... I imagine we'd play no sound, similar to
if the audio ended early.

[Alpha Left Google, 2009-06-02 23:03:17]

http://codereview.chromium.org/113611/diff/1125/181
File media/base/data_buffer_unittest.cc (right):

http://codereview.chromium.org/113611/diff/1125/181#newcode31
Line 31: EXPECT_TRUE(buffer->IsEndOfStream());
On 2009/06/02 19:20:52, scherkus wrote:
> we should also test that GetData() is NULL
> EXPECT_FALSE(buffer->GetData());

Done.

http://codereview.chromium.org/113611/diff/1125/181#newcode38
Line 38: EXPECT_TRUE(buffer->IsEndOfStream());
On 2009/06/02 19:20:52, scherkus wrote:
> this line isn't needed anymore

Done.

http://codereview.chromium.org/113611/diff/1125/183
File media/base/pipeline.h (right):

http://codereview.chromium.org/113611/diff/1125/183#newcode40
Line 40: PIPELINE_ERROR_DEMUX,
On 2009/06/02 19:20:52, scherkus wrote:
> do the demuxer errors listed below not cover your situation?

PIPELINE_ERROR_DEMUX is not used any more, what about NO_DATA, it is when we
have hit end of stream but there's no data in the renderer..

http://codereview.chromium.org/113611/diff/1125/174
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/113611/diff/1125/174#newcode98
Line 98: host_->Error(PIPELINE_ERROR_NO_DATA);
On 2009/06/02 19:20:52, scherkus wrote:
> This might not actually be an error... I imagine we'd play no sound, similar
to
> if the audio ended early.

But is this a bit strange you have an audio stream but there's no valid decoded
data? It sounds suspicious to me..

[scherkus (not reviewing), 2009-06-03 22:37:39]

few nits and questions

http://codereview.chromium.org/113611/diff/1078/97
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1078/97#newcode103
Line 103: if (buffer->IsEndOfStream()) {
On 2009/06/02 19:13:02, Alpha wrote:
> On 2009/05/27 22:49:08, scherkus wrote:
> > does this section of the code actually get hit?
> > 
> > I'm worrying about if we inject a NULL frame to flush the decoder, but then
we
> > never follow up enqueuing our own end of stream packet
> > 
> > would it make more sense to flush the decoder, enqueue anything that comes
> out,
> > then always enqueue an EOF frame?
> 
> I made this condition even harder to reach and it worked. There's several
> reasons I'm doing this:
> 1. We may receive a packet here but decoded == 0 (FFmpeg caches the input)
> 2. FFmpeg didn't read anything but outputs a frame (It's being flushed)
> 
> So we can't rely on the above conditions to declare end of stream. We can let
> the above conditions pass through until we have three conditions met. If we
> output nothing from this method, DecoderBase will try to read again from the
> demuxer and passes us the buffer to decode. We will keeping receiving
> buffer->IsEndOfStream() since av_read_XXX continues to give us errors, for
that
> we enqueue end of stream buffer. So the end result is that we get errors and
> these errors trigger flushing of decoders until we really have nothing left.
The
> same applies to audio too.
> 

Hmmm OK.  Ideally there shouldn't be multiple end of stream packets being sent. 
DecoderBase needs some tweaking :)

http://codereview.chromium.org/113611/diff/1135/1136
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/113611/diff/1135/1136#newcode122
Line 122: void FFmpegDemuxerStream::StreamHasEnded(AVPacket* packet) {
You don't need this function:
 - FFmpegDemuxer::StreamHasEnded() memsets an AVPacket
 - Pass it to EnqueuePacket() and you have the same result

http://codereview.chromium.org/113611/diff/1135/1136#newcode390
Line 390: memset(packet, 0, sizeof(AVPacket));
sizeof(AVPacket) -> sizeof(*packet)

http://codereview.chromium.org/113611/diff/1135/1146
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/113611/diff/1135/1146#newcode625
Line 625: EXPECT_TRUE(reader->buffer());
change to an ASSERT_TRUE and then remove the if statement

http://codereview.chromium.org/113611/diff/1135/1146#newcode627
Line 627: EXPECT_TRUE(reader->buffer()->IsEndOfStream());
future reference: potentially dangerous code because you don't have { } with the
if and you're calling a macro

http://codereview.chromium.org/113611/diff/1135/1137
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1135/1137#newcode251
Line 251: frame_available_.Signal();
in a previous changelist you only called this if it wasn't an end-of-stream
buffer, but now that condition is gone

my original suggestion was to move the signal call up by
frames_.push_back(frame)... is it ok to call this every time even if we don't
actually push a frame?

[Alpha Left Google, 2009-06-03 23:24:37]

http://codereview.chromium.org/113611/diff/1078/97
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/113611/diff/1078/97#newcode103
Line 103: if (buffer->IsEndOfStream()) {
On 2009/06/03 22:37:39, scherkus wrote:
> On 2009/06/02 19:13:02, Alpha wrote:
> > On 2009/05/27 22:49:08, scherkus wrote:
> > > does this section of the code actually get hit?
> > > 
> > > I'm worrying about if we inject a NULL frame to flush the decoder, but
then
> we
> > > never follow up enqueuing our own end of stream packet
> > > 
> > > would it make more sense to flush the decoder, enqueue anything that comes
> > out,
> > > then always enqueue an EOF frame?
> > 
> > I made this condition even harder to reach and it worked. There's several
> > reasons I'm doing this:
> > 1. We may receive a packet here but decoded == 0 (FFmpeg caches the input)
> > 2. FFmpeg didn't read anything but outputs a frame (It's being flushed)
> > 
> > So we can't rely on the above conditions to declare end of stream. We can
let
> > the above conditions pass through until we have three conditions met. If we
> > output nothing from this method, DecoderBase will try to read again from the
> > demuxer and passes us the buffer to decode. We will keeping receiving
> > buffer->IsEndOfStream() since av_read_XXX continues to give us errors, for
> that
> > we enqueue end of stream buffer. So the end result is that we get errors and
> > these errors trigger flushing of decoders until we really have nothing left.
> The
> > same applies to audio too.
> > 
> 
> Hmmm OK.  Ideally there shouldn't be multiple end of stream packets being
sent. 
> DecoderBase needs some tweaking :)

There's a little fear here though, that we don't know whether doing
av_decode_XXX once with NULL parameters can actually flush everything from it.
So ideally we want to do one and only one flush, but giving some flexibility to
handle multiple flush is nice.

http://codereview.chromium.org/113611/diff/1135/1136
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/113611/diff/1135/1136#newcode122
Line 122: void FFmpegDemuxerStream::StreamHasEnded(AVPacket* packet) {
On 2009/06/03 22:37:39, scherkus wrote:
> You don't need this function:
>  - FFmpegDemuxer::StreamHasEnded() memsets an AVPacket
>  - Pass it to EnqueuePacket() and you have the same result

Done.

http://codereview.chromium.org/113611/diff/1135/1136#newcode390
Line 390: memset(packet, 0, sizeof(AVPacket));
On 2009/06/03 22:37:39, scherkus wrote:
> sizeof(AVPacket) -> sizeof(*packet)

Done.

http://codereview.chromium.org/113611/diff/1135/1146
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/113611/diff/1135/1146#newcode625
Line 625: EXPECT_TRUE(reader->buffer());
On 2009/06/03 22:37:39, scherkus wrote:
> change to an ASSERT_TRUE and then remove the if statement

Done.

http://codereview.chromium.org/113611/diff/1135/1146#newcode627
Line 627: EXPECT_TRUE(reader->buffer()->IsEndOfStream());
On 2009/06/03 22:37:39, scherkus wrote:
> future reference: potentially dangerous code because you don't have { } with
the
> if and you're calling a macro

Done.

http://codereview.chromium.org/113611/diff/1135/1137
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1135/1137#newcode251
Line 251: frame_available_.Signal();
On 2009/06/03 22:37:39, scherkus wrote:
> in a previous changelist you only called this if it wasn't an end-of-stream
> buffer, but now that condition is gone
> 
> my original suggestion was to move the signal call up by
> frames_.push_back(frame)... is it ok to call this every time even if we don't
> actually push a frame?

Watch for the order with the state change, the signal should come last. I can
return when there's an error.

[scherkus (not reviewing), 2009-06-04 00:27:21]

http://codereview.chromium.org/113611/diff/1135/1137
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1135/1137#newcode251
Line 251: frame_available_.Signal();
On 2009/06/03 23:24:38, Alpha wrote:
> On 2009/06/03 22:37:39, scherkus wrote:
> > in a previous changelist you only called this if it wasn't an end-of-stream
> > buffer, but now that condition is gone
> > 
> > my original suggestion was to move the signal call up by
> > frames_.push_back(frame)... is it ok to call this every time even if we
don't
> > actually push a frame?
> 
> Watch for the order with the state change, the signal should come last. I can
> return when there's an error.

But because state_ is protected by lock_, no one (should) wake up or be able to
access state_ until you release the lock.

http://codereview.chromium.org/113611/diff/1167/243
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/113611/diff/1167/243#newcode626
Line 626: ASSERT_TRUE(reader->buffer()->IsEndOfStream());
nit: sorry I wasn't clear, you only need to assert on thing that will otherwise
cause following tests to crash.  The test should be:

EXPECT_TRUE(reader->WaitForRead());
EXPECT_TRUE(reader->called());
ASSERT_TRUE(reader->buffer());
EXPECT_TRUE(reader->buffer()->IsEndOfStream());

[Alpha Left Google, 2009-06-04 17:42:12]

http://codereview.chromium.org/113611/diff/1135/1137
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1135/1137#newcode251
Line 251: frame_available_.Signal();
On 2009/06/04 00:27:21, scherkus wrote:
> On 2009/06/03 23:24:38, Alpha wrote:
> > On 2009/06/03 22:37:39, scherkus wrote:
> > > in a previous changelist you only called this if it wasn't an
end-of-stream
> > > buffer, but now that condition is gone
> > > 
> > > my original suggestion was to move the signal call up by
> > > frames_.push_back(frame)... is it ok to call this every time even if we
> don't
> > > actually push a frame?
> > 
> > Watch for the order with the state change, the signal should come last. I
can
> > return when there's an error.
> 
> But because state_ is protected by lock_, no one (should) wake up or be able
to
> access state_ until you release the lock.

Done.

http://codereview.chromium.org/113611/diff/1167/243
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/113611/diff/1167/243#newcode626
Line 626: ASSERT_TRUE(reader->buffer()->IsEndOfStream());
On 2009/06/04 00:27:21, scherkus wrote:
> nit: sorry I wasn't clear, you only need to assert on thing that will
otherwise
> cause following tests to crash.  The test should be:
> 
> EXPECT_TRUE(reader->WaitForRead());
> EXPECT_TRUE(reader->called());
> ASSERT_TRUE(reader->buffer());
> EXPECT_TRUE(reader->buffer()->IsEndOfStream());

Done.

[scherkus (not reviewing), 2009-06-04 20:11:08]

LGTM with one nit (remove an empty line)

http://codereview.chromium.org/113611/diff/1178/257
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1178/257#newcode166
Line 166: 
remove empty line

[Alpha Left Google, 2009-06-04 20:13:34]

http://codereview.chromium.org/113611/diff/1178/257
File media/filters/video_thread.cc (right):

http://codereview.chromium.org/113611/diff/1178/257#newcode166
Line 166: 
On 2009/06/04 20:11:08, scherkus wrote:
> remove empty line

Done.