Fix ChunkDemuxer seek deadlock

Fix ChunkDemuxer seek deadlock

BUG=109879
TEST=PipelineIntegrationTest.ChunkDemuxerAbortRead_*,FFmpegAudioDecoderTest.AbortRead, AudioRendererBaseTest.AbortPendingRead_*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119851

[acolwell GONE FROM CHROMIUM, 2012-01-27 20:12:16]

I don't have the FFmpegVideoDecoder & VideoRendererBase tests finished yet, but
I wanted to get the review started on the rest of the changes. Everything shown
here fixes the seek deadlock. I just need to add a few more unit tests to make
sure we catch any regression in the new semantics.

[scherkus (not reviewing), 2012-01-27 22:02:17]

drive by: is it possible to get a layout test written that triggers the
deadlock?

[acolwell GONE FROM CHROMIUM, 2012-01-27 22:03:41]

On 2012/01/27 22:02:17, scherkus wrote:
> drive by: is it possible to get a layout test written that triggers the
> deadlock?

The media source layout tests trigger it sometimes. That is how I found it. This
bug causes them to be flaky.

[scherkus (not reviewing), 2012-01-27 22:05:39]

On 2012/01/27 22:03:41, acolwell wrote:
> On 2012/01/27 22:02:17, scherkus wrote:
> > drive by: is it possible to get a layout test written that triggers the
> > deadlock?
> 
> The media source layout tests trigger it sometimes. That is how I found it.
This
> bug causes them to be flaky.

woot! carry on!

side note: it seems that as the age of a new class approaches infinity we add
locks and state machines. I'm not sure why that is.

[Ami GONE FROM CHROMIUM, 2012-01-27 22:06:21]

On 2012/01/27 22:02:17, scherkus wrote:
> drive by: is it possible to get a layout test written that triggers the
> deadlock?

http://crbug.com/109879 says: The goal is to remove this deadlock situation from
the pipeline code. Once this is done, the LayoutTests for the Media Source API
can be turned on. The deadlock causes flakiness in these tests.

[Ami GONE FROM CHROMIUM, 2012-01-27 23:44:58]

http://codereview.chromium.org/9295020/diff/2001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9295020/diff/2001/media/base/filters.h#newcode141
media/base/filters.h:141: // An aborted read indicates a seek will likely happen
shortly.
I don't like the "likely" in the last sentence here for an API contract.  Drop
the sentence or convert to something that's a specification?

(ditto for the audio version below)

http://codereview.chromium.org/9295020/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:155: if (buffer) {
that you can't reverse the test and early-return (b/c of the code from l.175
downwards) smells wrong.

I wonder whether this and the next big if block can be unified into a switch
(state).  Want to take a crack at it?

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:44: virtual void Read(const ReadCallback&
read_callback);
How is this not breaking the clang bots, missing OVERRIDEs as it is?

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:54: FLUSHED_END_OF_STREAM,
These names are not self-explanatory as to what the relevant differences between
them are.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:60: void ChangeState_Locked(State state);
Commentary here and below please.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:100: std::deque<base::Closure> callbacks;
If you changed this to:
ReadCBQueue read_cbs;
and then under lock:
read_cbs.swap(read_cbs_);
and then outside the lock:
for (ReadCBQueue::iterator it = read_cbs.begin(); it != read_cbs.end(); ++i)
  read_cbs.Run(scoped_refptr<Buffer>());

it would be a lot shorter (and incidentally avoid a bunch of work).

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:155: std::deque<base::Closure> callbacks;
s/callbacks/closures/ ?

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:190: callbacks.pop_front();
If you replace the piecewise pop_front() with a final clear() you get to remove
the braces from the while loop (single-line body) saving a vertical line!
(not to mention the pop_front cycles)

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:201: while (!read_cbs_.empty()) {
std::swap()

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:213: callbacks.pop_front();
pop_front unnecessary

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:252: if (buffers_.empty() || !read_cbs_.empty())
{
This test is the opposite of the comment, which I find confusing.
I think it'd be best to reverse the comment, since you actually take advantage
of the coalesced immediately-satisfy path.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:262: case FLUSHED_END_OF_STREAM:
Can these not be fall-throughs to the PLAYING case above?  
(since in these cases there are never buffers?)
(or maybe just add DCHECKs for that assertion here)

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:321: while ((state_ == PLAYING || state_ ==
RECEIVED_END_OF_STREAM) &&
Having state_ in this while condition is strange (since it isn't going to change
during the life of the loop).

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:399: if (audio_.get())
.get() here and below are unnecessary (scoped_refptr auto-converts to pointer).

http://codereview.chromium.org/9295020/diff/2001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:32: static const int kDecodeThreads = 1;
??

http://codereview.chromium.org/9295020/diff/2001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:224: if (!buffer) {
GpuVideoDecoder::RequestBufferDecode needs to similarly change?

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:28: // Helper class for testing
ChunkDemuxer.
IMO it would be more helpful to describe what this class mimics (a decoder?).

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:103: : public testing::Test {
unnecessary newline

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:241: void
TestChunkDemuxerSeekDuringRead(const std::string& filename,
#damnson !

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:280: 
extra newline

http://codereview.chromium.org/9295020/diff/2001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:353: if (frame) {
reverse test & early-return?

http://codereview.chromium.org/9295020/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:367: base::TimeDelta duration =
host()->GetDuration();
IMO this is a worsening of readability, since there are two "durations" involved
here (frame & host).  Revert?

http://codereview.chromium.org/9295020/diff/2001/media/webm/webm_cluster_pars...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:14: int extra_bytes = 16;
Commentary?

[Ami GONE FROM CHROMIUM, 2012-01-27 23:47:05]

> side note: it seems that as the age of a new class approaches infinity we add
> locks and state machines. I'm not sure why that is.

FWIW, this patch adds no locks.  It does add a state type/variable, but that's
replacing a pair of bools that tracked state (that would have to be extended to
support the functionality here).
I definitely think the pipeline arch forces each of the things it works with to
implement a state machine, which initial implementations do w/ bools and more
mature ones do with state types/variables. 
IWBN to minimize this (as the various code-improvement bugs we have filed would
do).

[Ami GONE FROM CHROMIUM, 2012-01-28 00:19:59]

https://chromiumcodereview.appspot.com/9295020/diff/2001/media/filters/chunk_...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9295020/diff/2001/media/filters/chunk_...
media/filters/chunk_demuxer.cc:52: PLAYING,
Oh, also, PLAYING is a terrible name ;)
NORMAL/STEADY_STATE are better names, though I'm not really happy with either.

The real problem, I think, is that these state names conflate things that have
happened and things that may happen in the future.

Perhaps a good way out of this mess is to write down the state diagram you want
to support (with nodes & labeled edges) and then decide what names should be.

[acolwell GONE FROM CHROMIUM, 2012-01-29 03:00:41]

Addressed comments and added VideoRendererBase & FFmpegVideoDecoder unit tests.

http://codereview.chromium.org/9295020/diff/2001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/9295020/diff/2001/media/base/filters.h#newcode141
media/base/filters.h:141: // An aborted read indicates a seek will likely happen
shortly.
On 2012/01/27 23:44:58, Ami Fischman wrote:
> I don't like the "likely" in the last sentence here for an API contract.  Drop
> the sentence or convert to something that's a specification?
> 
> (ditto for the audio version below)

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:155: if (buffer) {
On 2012/01/27 23:44:58, Ami Fischman wrote:
> that you can't reverse the test and early-return (b/c of the code from l.175
> downwards) smells wrong.
> 
> I wonder whether this and the next big if block can be unified into a switch
> (state).  Want to take a crack at it?

Done. Cleaned things up nicely and uncovered an unused state!

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:44: virtual void Read(const ReadCallback&
read_callback);
On 2012/01/27 23:44:58, Ami Fischman wrote:
> How is this not breaking the clang bots, missing OVERRIDEs as it is?
Not sure. Fixed though.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:52: PLAYING,
On 2012/01/28 00:19:59, Ami Fischman wrote:
> Oh, also, PLAYING is a terrible name ;)
> NORMAL/STEADY_STATE are better names, though I'm not really happy with either.
> 
> The real problem, I think, is that these state names conflate things that have
> happened and things that may happen in the future.
>

Agreed. I've renamed all the states. I think they are a little clearer now.
 
> Perhaps a good way out of this mess is to write down the state diagram you
want
> to support (with nodes & labeled edges) and then decide what names should be.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:54: FLUSHED_END_OF_STREAM,
On 2012/01/27 23:44:58, Ami Fischman wrote:
> These names are not self-explanatory as to what the relevant differences
between
> them are.

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:60: void ChangeState_Locked(State state);
On 2012/01/27 23:44:58, Ami Fischman wrote:
> Commentary here and below please.

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:100: std::deque<base::Closure> callbacks;
On 2012/01/27 23:44:58, Ami Fischman wrote:
> If you changed this to:
> ReadCBQueue read_cbs;
> and then under lock:
> read_cbs.swap(read_cbs_);
> and then outside the lock:
> for (ReadCBQueue::iterator it = read_cbs.begin(); it != read_cbs.end(); ++i)
>   read_cbs.Run(scoped_refptr<Buffer>());
> 
> it would be a lot shorter (and incidentally avoid a bunch of work).

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:155: std::deque<base::Closure> callbacks;
On 2012/01/27 23:44:58, Ami Fischman wrote:
> s/callbacks/closures/ ?

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:190: callbacks.pop_front();
On 2012/01/27 23:44:58, Ami Fischman wrote:
> If you replace the piecewise pop_front() with a final clear() you get to
remove
> the braces from the while loop (single-line body) saving a vertical line!
> (not to mention the pop_front cycles)
I don't follow. Replaced this with a for loop like the one suggested above,
which I think is what you were getting at.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:201: while (!read_cbs_.empty()) {
On 2012/01/27 23:44:58, Ami Fischman wrote:
> std::swap()

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:213: callbacks.pop_front();
On 2012/01/27 23:44:58, Ami Fischman wrote:
> pop_front unnecessary
Replaced with for loop.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:252: if (buffers_.empty() || !read_cbs_.empty())
{
On 2012/01/27 23:44:58, Ami Fischman wrote:
> This test is the opposite of the comment, which I find confusing.
> I think it'd be best to reverse the comment, since you actually take advantage
> of the coalesced immediately-satisfy path.

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:262: case FLUSHED_END_OF_STREAM:
On 2012/01/27 23:44:58, Ami Fischman wrote:
> Can these not be fall-throughs to the PLAYING case above?  
> (since in these cases there are never buffers?)
> (or maybe just add DCHECKs for that assertion here)
This was a bug. NULLs need to be immediately returned here. Added comment
indicating why.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:321: while ((state_ == PLAYING || state_ ==
RECEIVED_END_OF_STREAM) &&
On 2012/01/27 23:44:58, Ami Fischman wrote:
> Having state_ in this while condition is strange (since it isn't going to
change
> during the life of the loop).

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:399: if (audio_.get())
On 2012/01/27 23:44:58, Ami Fischman wrote:
> .get() here and below are unnecessary (scoped_refptr auto-converts to
pointer).

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:32: static const int kDecodeThreads = 1;
On 2012/01/27 23:44:58, Ami Fischman wrote:
> ??
This was to temporarily make valgrind shutup about FFVP8. I've reverted this
change for now. Fixing valgrind issues is the next thing on my list
(http://crbug.com/109875).

http://codereview.chromium.org/9295020/diff/2001/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:224: if (!buffer) {
On 2012/01/27 23:44:58, Ami Fischman wrote:
> GpuVideoDecoder::RequestBufferDecode needs to similarly change?

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:28: // Helper class for testing
ChunkDemuxer.
On 2012/01/27 23:44:58, Ami Fischman wrote:
> IMO it would be more helpful to describe what this class mimics (a decoder?).

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:103: : public testing::Test {
On 2012/01/27 23:44:58, Ami Fischman wrote:
> unnecessary newline

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:241: void
TestChunkDemuxerSeekDuringRead(const std::string& filename,
On 2012/01/27 23:44:58, Ami Fischman wrote:
> #damnson !
removed ChunkDemuxer to make the name a little shorter. :)

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:280: 
On 2012/01/27 23:44:58, Ami Fischman wrote:
> extra newline

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:353: if (frame) {
On 2012/01/27 23:44:58, Ami Fischman wrote:
> reverse test & early-return?

Done.

http://codereview.chromium.org/9295020/diff/2001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:367: base::TimeDelta duration =
host()->GetDuration();
On 2012/01/27 23:44:58, Ami Fischman wrote:
> IMO this is a worsening of readability, since there are two "durations"
involved
> here (frame & host).  Revert?

Done.

http://codereview.chromium.org/9295020/diff/2001/media/webm/webm_cluster_pars...
File media/webm/webm_cluster_parser.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/webm/webm_cluster_pars...
media/webm/webm_cluster_parser.cc:14: int extra_bytes = 16;
On 2012/01/27 23:44:58, Ami Fischman wrote:
> Commentary?
Changed to FF_INPUT_BUFFER_PADDING_SIZE & added a comment.

[Ami GONE FROM CHROMIUM, 2012-01-29 22:12:38]

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/9295020/diff/2001/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:241: void
TestChunkDemuxerSeekDuringRead(const std::string& filename,
On 2012/01/29 03:00:41, acolwell wrote:
> On 2012/01/27 23:44:58, Ami Fischman wrote:
> > #damnson !
> removed ChunkDemuxer to make the name a little shorter. :)

Sorry, I didn't mean the name was too long; I was pointing out your commitment
to testing.
Since this (chunkdemuxer-specific) helper is contained in a
non-ChunkDemuxer-specific class, I think including ChunkDemuxer in the method
name makes sense.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:160: break;
s/break/return/ here and everywhere below, for uniformity with the few return's
already here.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:164: ResetAndRunCB(&pause_callback_);
Before your CL the code could handle receiving multiple buffers in kPaused (by
checking for pause_callback_.is_null()).  Is the change ok?

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:172: if (!buffer || buffer->IsEndOfStream()
|| algorithm_->IsQueueFull()) {
That the first two clauses repeat the test in EnqueueBuffer, and that
EnqueueBuffer's name is unconditional while its body is conditional, makes me
think it can be improved.
For example, make EB's body unconditional, but only call it in the right
condition.  Or else make it return a bool indicating successful enqueuing.

Either way, it smells funny that only some of the callsites to EnqueueBuffer in
this switch check buffer's condition.  Is it really expected/OK to get a NULL
buffer in kPaused/kPlaying/kUnderflow/kRebuffering/kStopped?

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:312: 
Extra \n

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:353:
.WillOnce(Return(base::TimeDelta()));
one line?

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:354: EXPECT_CALL(host_,
SetTime(base::TimeDelta::FromMilliseconds(754)));
754?  Why not _?  More generally, wat???

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:56: RECEIVED_END_OF_STREAM,
s/END_OF_STREAM/EOS/ here and below to match the previous value.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:69: // |buffers_|.
It also pops the respective queues, though, right?

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:310: ClosureQueue* closures) {
one line?

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:322: if (state_ == RECEIVED_END_OF_STREAM &&
reverse test and early return.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:324: 
extra \n

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:332: ChangeState_Locked(SENT_END_OF_STREAM);
It's a tad sketchy that this state change happens before the EOS buffers are
actually sent (they're only queued up for sending in *closures at this point).

http://codereview.chromium.org/9295020/diff/4003/media/filters/gpu_video_deco...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.cc:231: gvd_loop_proxy_->PostTask(FROM_HERE,
base::Bind(
Are you intentionally jumping the queue here?  I think what you really want is:
EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());

http://codereview.chromium.org/9295020/diff/4003/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/video_renderer...
media/filters/video_renderer_base.cc:353: if (!frame) {
I thought this test encompassed the entire remainder of the function.  But now I
see that's not the case.  Is it really correct to not look at the ready_frames
in this case?

[acolwell GONE FROM CHROMIUM, 2012-01-30 00:14:14]

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:160: break;
On 2012/01/29 22:12:38, Ami Fischman wrote:
> s/break/return/ here and everywhere below, for uniformity with the few
return's
> already here.

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:164: ResetAndRunCB(&pause_callback_);
On 2012/01/29 22:12:38, Ami Fischman wrote:
> Before your CL the code could handle receiving multiple buffers in kPaused (by
> checking for pause_callback_.is_null()).  Is the change ok?
That was because algorithm->EnqueueBuffer() could blindly call
ScheduleRead_Locked() to request another read. Now that I modified
ScheduleRead_Locked() to block such a request, we don't have to worry about more
buffers coming in.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:172: if (!buffer || buffer->IsEndOfStream()
|| algorithm_->IsQueueFull()) {
On 2012/01/29 22:12:38, Ami Fischman wrote:
> That the first two clauses repeat the test in EnqueueBuffer, and that
> EnqueueBuffer's name is unconditional while its body is conditional, makes me
> think it can be improved.
> For example, make EB's body unconditional, but only call it in the right
> condition.  Or else make it return a bool indicating successful enqueuing.

Removed EB and just put the conditional checks in the appropriate place. 

> 
> Either way, it smells funny that only some of the callsites to EnqueueBuffer
in
> this switch check buffer's condition.  Is it really expected/OK to get a NULL
> buffer in kPaused/kPlaying/kUnderflow/kRebuffering/kStopped?

Yes. The demuxer doesn't have a Pause() so it doesn't know that one is in
progress. kPlaying/kUnderflow/kRebuffering are all "playing states" so the NULL
could come in before the renderer get's its Pause() call. We don't do anything
for kStopped so that sound be fine as well. 

The ChunkDemuxer gets the FlushData() call before the Pipeline is even notified
of the seek so we can be in any of these states when the demuxer sends the
NULLs.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:312: 
On 2012/01/29 22:12:38, Ami Fischman wrote:
> Extra \n

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:353:
.WillOnce(Return(base::TimeDelta()));
On 2012/01/29 22:12:38, Ami Fischman wrote:
> one line?

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:354: EXPECT_CALL(host_,
SetTime(base::TimeDelta::FromMilliseconds(754)));
On 2012/01/29 22:12:38, Ami Fischman wrote:
> 754?  Why not _?  More generally, wat???
Changed to _ since I don't actually care what the time was set to. 
This change became necessary when I added actual timestamps and durations to the
buffers passed to the renderer. It caused the clock time updating code to
actually execute. More test coverage.. woo hoo?!

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:56: RECEIVED_END_OF_STREAM,
On 2012/01/29 22:12:38, Ami Fischman wrote:
> s/END_OF_STREAM/EOS/ here and below to match the previous value.

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:69: // |buffers_|.
On 2012/01/29 22:12:38, Ami Fischman wrote:
> It also pops the respective queues, though, right?

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:310: ClosureQueue* closures) {
On 2012/01/29 22:12:38, Ami Fischman wrote:
> one line?

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:322: if (state_ == RECEIVED_END_OF_STREAM &&
On 2012/01/29 22:12:38, Ami Fischman wrote:
> reverse test and early return.

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:324: 
On 2012/01/29 22:12:38, Ami Fischman wrote:
> extra \n

Done.

http://codereview.chromium.org/9295020/diff/4003/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:332: ChangeState_Locked(SENT_END_OF_STREAM);
On 2012/01/29 22:12:38, Ami Fischman wrote:
> It's a tad sketchy that this state change happens before the EOS buffers are
> actually sent (they're only queued up for sending in *closures at this point).

True. I think the state name is the problem. I've renamed it to
RETURNING_EOS_FOR_READS since that is what happens once we enter this state.

http://codereview.chromium.org/9295020/diff/4003/media/filters/gpu_video_deco...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.cc:231: gvd_loop_proxy_->PostTask(FROM_HERE,
base::Bind(
On 2012/01/29 22:12:38, Ami Fischman wrote:
> Are you intentionally jumping the queue here?  I think what you really want
is:
> EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
Yes. I don't want to send an empty frame because I don't want to trigger EOS
logic in the renderer. I was also avoiding a call to
EnqueueFrameAndTriggerFrameDelivery() because it looks like you have special
semantics for NULL and assume that there are already frames in the queue. Should
I remove the DCHECK() and queue the NULL if ready_video_frames_.empty() is true?

http://codereview.chromium.org/9295020/diff/4003/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/4003/media/filters/video_renderer...
media/filters/video_renderer_base.cc:353: if (!frame) {
On 2012/01/29 22:12:38, Ami Fischman wrote:
> I thought this test encompassed the entire remainder of the function.  But now
I
> see that's not the case.  Is it really correct to not look at the ready_frames
> in this case?
I believe so. Getting a NULL frame means that the demuxer has no more data to
give and continuing to ask will only give you more NULL responses. This means
that we will never exit a seek. The NULL frames are triggered by a new seek
requested at the WebMediaPlayerImpl level so finishing out a preroll wouldn't do
us any good because all the data will be immediately flushed anyways.

Added a comment indicating why we are early terminating the seek.

[Ami GONE FROM CHROMIUM, 2012-01-30 17:37:42]

Almost there!

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:230: //    end of stream or schedule such
rendering.
What does "render end of stream" mean??
I think you mean SignalEOS, but again, not sure.

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
File media/filters/audio_renderer_base.h (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base.h:65: // FillBuffer() cannot immediately
signal end of stream event because browser
The second sentence of this para should be an explanation of what subclasses
might/should do in this case (I believe that is: drain output buffers and call
SignalEOS() back on this class).

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base.h:101: void SignalEndOfStream();
I am unhappy about the names OnRenderEOS / SignalEOS.

IIUC, rendered_end_of_stream_ is really not about EOS at all, but rather about
the Pipeline concept of "ended".

WDYT of:
s/SignalEndOfStream/NotifyEndedDone/  ("Done" to include buffers flushing; open
to better names)
s/OnRenderEndOfStream/OnEndOfStream/
s/rendered_end_of_stream_/ended_/
?

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:43:
MOCK_METHOD0(OnRenderEndOfStream, void());
This doesn't add logspam to the test (b/c of "default action (none) taken", does
it?

http://codereview.chromium.org/9295020/diff/1039/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:306: // TODO(acolwell): Fix flakiness
http://crbug.com/109875
Is this only flaky for valgrind/tsan?  Can you just disable it for those bots?
Better yet, can you add tools/valgrind/tsan/ignores.txt entries to make this
test not flaky (at the cost of not covering the relevant code) and leave the bug
for tracking removing those ignore.txt entries?

[acolwell GONE FROM CHROMIUM, 2012-01-30 22:01:17]

Most of the comments are the result of a rebase and I'll address in another CL.

I did make a few changes to fix some flakiness in VideoRendererBase unittests
and changed a DCHECK to a EXPECT in PipelineIntegrationTest so that a test will
fail instead of abort.

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base.cc:230: //    end of stream or schedule such
rendering.
On 2012/01/30 17:37:42, Ami Fischman wrote:
> What does "render end of stream" mean??
> I think you mean SignalEOS, but again, not sure.
This is from a rebase. I'll fix it in a separate CL.

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
File media/filters/audio_renderer_base.h (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base.h:65: // FillBuffer() cannot immediately
signal end of stream event because browser
On 2012/01/30 17:37:42, Ami Fischman wrote:
> The second sentence of this para should be an explanation of what subclasses
> might/should do in this case (I believe that is: drain output buffers and call
> SignalEOS() back on this class).
This is from a rebase. I'll fix it in a separate CL.

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base.h:101: void SignalEndOfStream();
On 2012/01/30 17:37:42, Ami Fischman wrote:
> I am unhappy about the names OnRenderEOS / SignalEOS.
> 
> IIUC, rendered_end_of_stream_ is really not about EOS at all, but rather about
> the Pipeline concept of "ended".
> 
> WDYT of:
> s/SignalEndOfStream/NotifyEndedDone/  ("Done" to include buffers flushing;
open
> to better names)
> s/OnRenderEndOfStream/OnEndOfStream/
> s/rendered_end_of_stream_/ended_/
> ?
This is from a rebase. I'll fix it in a separate CL.

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/audio_renderer...
media/filters/audio_renderer_base_unittest.cc:43:
MOCK_METHOD0(OnRenderEndOfStream, void());
On 2012/01/30 17:37:42, Ami Fischman wrote:
> This doesn't add logspam to the test (b/c of "default action (none) taken",
does
> it?
This is from a rebase. I'll fix it in a separate CL.

http://codereview.chromium.org/9295020/diff/1039/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/9295020/diff/1039/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:306: // TODO(acolwell): Fix flakiness
http://crbug.com/109875
On 2012/01/30 17:37:42, Ami Fischman wrote:
> Is this only flaky for valgrind/tsan?  Can you just disable it for those bots?
> Better yet, can you add tools/valgrind/tsan/ignores.txt entries to make this
> test not flaky (at the cost of not covering the relevant code) and leave the
bug
> for tracking removing those ignore.txt entries?
Unfortunately the flakiness isn't valgrind/tsan only. For some reason
FFmpegVideoDecoder just fails to decode a frame sometimes and errors out. I
haven't tracked down the cause yet, but it happens on the normal bots as well as
the valgrind/tsan ones. :(

[Ami GONE FROM CHROMIUM, 2012-01-30 22:08:32]

LGTM