FFmpegDemuxerStream::Read() now functions properly while stopped.

FFmpegDemuxerStream::Read() now functions properly while stopped.

BUG=13907
TEST=some layout tests might start passing

[scherkus (not reviewing), 2009-06-19 02:32:13]

Depends on http://codereview.chromium.org/126306

[Alpha Left Google, 2009-06-19 07:56:58]

This fix can knock down so many crashes! good job!

I have a question about how you use the |stopped_| signal in the Read method and
EnqueuePacket. I have doubts that you are not using the flag to protect
FulFillPendingReads() that might put something to the read queue.

http://codereview.chromium.org/131092/diff/1/2
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/131092/diff/1/2#newcode106
Line 106: DCHECK(!stopped_) << "Attempted to enqueue packet on a stopped
stream.";
If this case really happens, it's better to do an if check. Because you can do
cleanup on the unwanted buffer, otherwise there'll be a leak.

http://codereview.chromium.org/131092/diff/1/2#newcode144
Line 144: demuxer_->PostDemuxTask();
If you have stopped, you still might enter this scope if you don't check for
stopped_ in FulfillPendingReads()

http://codereview.chromium.org/131092/diff/1/2#newcode149
Line 149: bool pending_reads = false;
Do you also need to check the stopped_ flag here? I'm not very sure about the
cause of crash, can you explain a bit more?

http://codereview.chromium.org/131092/diff/1/3
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/131092/diff/1/3#newcode162
Line 162: static AVPacket* ClonePacket(AVPacket* packet);
Is this method only used in this class? Can you put this method to an anonymous
namespace in the .cc file instead?

http://codereview.chromium.org/131092/diff/1/3#newcode179
Line 179: // Although it is safe to call vector methods (i.e., size()) on any
thread,
It's strange that you say it's safe to call vector methods on any thread...
maybe you should say it's safe to read from the vectors from any thread because
writing to the vectors only happen in initialization?

[scherkus (not reviewing), 2009-06-19 15:27:03]

http://codereview.chromium.org/131092/diff/1/2
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/131092/diff/1/2#newcode106
Line 106: DCHECK(!stopped_) << "Attempted to enqueue packet on a stopped
stream.";
On 2009/06/19 07:56:58, Alpha wrote:
> If this case really happens, it's better to do an if check. Because you can do
> cleanup on the unwanted buffer, otherwise there'll be a leak.

Done.

http://codereview.chromium.org/131092/diff/1/2#newcode144
Line 144: demuxer_->PostDemuxTask();
On 2009/06/19 07:56:58, Alpha wrote:
> If you have stopped, you still might enter this scope if you don't check for
> stopped_ in FulfillPendingReads()

I'm confused.. we do check for stop and we exit the function immediately so it
wouldn't be possible

http://codereview.chromium.org/131092/diff/1/2#newcode149
Line 149: bool pending_reads = false;
On 2009/06/19 07:56:58, Alpha wrote:
> Do you also need to check the stopped_ flag here? I'm not very sure about the
> cause of crash, can you explain a bit more?

this method is called in two places: Read() and EnqueuePacket()

In Read() we check for stop and discard the request so we'll never reach here

In EnqueuePacket() we DCHECK, however I'll add an if statement that handles stop
and discards the packet as well

The cause of the crash was Read() calling demuxer_->PostDemuxTask(), which
attempts to post a task to a NULL message loop.

http://codereview.chromium.org/131092/diff/1/3
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/131092/diff/1/3#newcode162
Line 162: static AVPacket* ClonePacket(AVPacket* packet);
On 2009/06/19 07:56:58, Alpha wrote:
> Is this method only used in this class? Can you put this method to an
anonymous
> namespace in the .cc file instead?

Done.

http://codereview.chromium.org/131092/diff/1/3#newcode179
Line 179: // Although it is safe to call vector methods (i.e., size()) on any
thread,
On 2009/06/19 07:56:58, Alpha wrote:
> It's strange that you say it's safe to call vector methods on any thread...
> maybe you should say it's safe to read from the vectors from any thread
because
> writing to the vectors only happen in initialization?

It is a little strange because technically the public methods on
FFmpegDemuxerStream are thread-safe, I'll figure out what to say here

[Alpha Left Google, 2009-06-19 17:19:52]

http://codereview.chromium.org/131092/diff/1/2
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/131092/diff/1/2#newcode144
Line 144: demuxer_->PostDemuxTask();
On 2009/06/19 15:27:03, scherkus wrote:
> On 2009/06/19 07:56:58, Alpha wrote:
> > If you have stopped, you still might enter this scope if you don't check for
> > stopped_ in FulfillPendingReads()
> 
> I'm confused.. we do check for stop and we exit the function immediately so it
> wouldn't be possible
> 

If cause of crash is at this line, then there still is a race condition.. Since
the whole function is not under a critical section so it may happens that:

if (FulfillPendingReads()) {
  // ... <stop> is called so the demuxer thread message loop
  // is no more.
  demuxer_->PostDemuxTask(); // .. bang!
}

Because of how we use decoders, that is they are running on pipeline thread so
this case won't happen. But this is still a potential risk here, since decoders
are "allowed" to run on their own thread. I think either we remove the decoder
thread capability from decoder_base.h or we make sure we can handle this race
condition case (a cheap way might be lock down PostDemuxTask so it won't post a
task when it's stopped)

[awong, 2009-06-19 18:16:07]

I wasn't quite able to evaluate the threading semantics since I don't fully
understand the invocation/sharing model + invaraints we expect in our code.

In general though, I'm concerned by our use of thread-safe objects, but don't
have enough context yet to say "this looks okay or not okay."

http://codereview.chromium.org/131092/diff/1002/11
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/131092/diff/1002/11#newcode145
Line 145: STLDeleteElements(&read_queue_);
I'm now confused about the threading semantics for this class.  Who can call
which varaibles?

Ideally, if we do everything via message passing, we can kill most of the
locks...

Anyways, how big is the queue?  Do we need to worry about blocking due to the
memory release?  An alternate pattern could be

BufferQueue tmp_buffer_queue;
ReadQueue tmp_read_queue;

{
  AutoLock l(lock_);
  buffer_queue_.swap(tmp_buffer_queue);
  read_queue_.swap(tmp_read_queue);
  stopped_ = true;
}

tmp_buffer_queue.clear();
STLDeleteElements(&tmp_read_queue);

Normally I wouldn't care too much about optimizing the locks before we know
there's an issue, but the code change is simple enough here that I think it's a
nice to have.

...btw, if I haven't mentioned it before, I hate ref counts...especially
refcounts in containers cause the clear/copy/pop tasks all become O(m) or O(m+n)
instead of O(1) or O(n) :-/

http://codereview.chromium.org/131092/diff/1002/11#newcode159
Line 159: delete read_callback;
Do we want to call inline with a eos buffer or something?  Or does that call a
problem?

Also, is it improper to call read on a stopped stream?  I'm not sure how the
filter synchronization works, but I could imagine that the read is in the middle
of being invoked when this thread gets "stopped"...

http://codereview.chromium.org/131092/diff/1002/11#newcode204
Line 204: 
remove extra newline.

http://codereview.chromium.org/131092/diff/1002/12
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/131092/diff/1002/12#newcode16
Line 16: // excessive memory consumption.
Add explanation of semantics when the stream is stopped.

http://codereview.chromium.org/131092/diff/1002/13
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/131092/diff/1002/13#newcode628
Line 628: OnDelete();
cleaver.

http://codereview.chromium.org/131092/diff/1002/13#newcode645
Line 645: MockReadCallback* callback = new MockReadCallback();
Just to be safe, since you don't hand this off imediately, use scoped_ptr and
then when passing to demuxer, pass it via calling .release()

http://codereview.chromium.org/131092/diff/1002/13#newcode659
Line 659: EXPECT_CALL(*callback, OnDelete());
Do we need to use a StrictMock with the callback above?  I think currently,
RunWithParams can be called, and this mock will just warn, not fail the test.

[scherkus (not reviewing), 2009-06-19 18:20:38]

On 2009/06/19 17:19:52, Alpha wrote:
> http://codereview.chromium.org/131092/diff/1/2
> File media/filters/ffmpeg_demuxer.cc (right):
> 
> http://codereview.chromium.org/131092/diff/1/2#newcode144
> Line 144: demuxer_->PostDemuxTask();
> On 2009/06/19 15:27:03, scherkus wrote:
> > On 2009/06/19 07:56:58, Alpha wrote:
> > > If you have stopped, you still might enter this scope if you don't check
for
> > > stopped_ in FulfillPendingReads()
> > 
> > I'm confused.. we do check for stop and we exit the function immediately so
it
> > wouldn't be possible
> > 
> 
> If cause of crash is at this line, then there still is a race condition..
Since
> the whole function is not under a critical section so it may happens that:
> 
> if (FulfillPendingReads()) {
>   // ... <stop> is called so the demuxer thread message loop
>   // is no more.
>   demuxer_->PostDemuxTask(); // .. bang!
> }
> 
> Because of how we use decoders, that is they are running on pipeline thread so
> this case won't happen. But this is still a potential risk here, since
decoders
> are "allowed" to run on their own thread. I think either we remove the decoder
> thread capability from decoder_base.h or we make sure we can handle this race
> condition case (a cheap way might be lock down PostDemuxTask so it won't post
a
> task when it's stopped)

Funny enough.. I recently changed decoders to run on their own dedicated
threads.  For reasons I won't get into here, having a shared pipeline thread
turned out to be a bad idea (response time vs. throughput).

But yes I thought I was safe here because all stream operations are serialized
by demuxer thread, but Read() is the exception -- good catch!

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

http://codereview.chromium.org/131092/diff/1002/11
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/131092/diff/1002/11#newcode145
Line 145: STLDeleteElements(&read_queue_);
On 2009/06/19 18:16:07, awong wrote:
> I'm now confused about the threading semantics for this class.  Who can call
> which varaibles?

The queues and stopped_ are touched by both the demuxer thread and the
downstream thread inside of Read().

> 
> Ideally, if we do everything via message passing, we can kill most of the
> locks...
> 
> Anyways, how big is the queue?  Do we need to worry about blocking due to the
> memory release?  An alternate pattern could be
> 
> BufferQueue tmp_buffer_queue;
> ReadQueue tmp_read_queue;
> 
> {
>   AutoLock l(lock_);
>   buffer_queue_.swap(tmp_buffer_queue);
>   read_queue_.swap(tmp_read_queue);
>   stopped_ = true;
> }
> 
> tmp_buffer_queue.clear();
> STLDeleteElements(&tmp_read_queue);
> 
> Normally I wouldn't care too much about optimizing the locks before we know
> there's an issue, but the code change is simple enough here that I think it's
a
> nice to have.
> 
> ...btw, if I haven't mentioned it before, I hate ref counts...especially
> refcounts in containers cause the clear/copy/pop tasks all become O(m) or
O(m+n)
> instead of O(1) or O(n) :-/

Ahh forgot about this trick... MessageLoop uses a similar pattern to minimize
lock contention.

http://codereview.chromium.org/131092/diff/1002/11#newcode159
Line 159: delete read_callback;
On 2009/06/19 18:16:07, awong wrote:
> Do we want to call inline with a eos buffer or something?  Or does that call a
> problem?
> 
> Also, is it improper to call read on a stopped stream?  I'm not sure how the
> filter synchronization works, but I could imagine that the read is in the
middle
> of being invoked when this thread gets "stopped"...

Pipeline thread calls Stop() downstream-wardly (data source -> demuxer ->
decoders -> renderers).

What's happening here is Stop() has been called on demuxer and is being called
on the decoders.

Technically there's a window where the decoders are still running but won't be
running for much longer (pipeline is working down the chain).

Reading on a stopped filter would only happen in a teardown situation, so even
if we replied saying "hey I'm stopped!" there's not much the downstream filter
could do other than delete the buffer.

Now this makes sense when you consider the pipeline as a whole (every filter is
getting stopped + destroyed), but perhaps individually it's not as nice.  For
example in our unit test we have to test that the callback is instantly
destroyed, as opposed to receiving an error reply that the filter is stopped.

I'm inclined to leave it as is for now (but will add a TODO) until the design
crystallizes a bit more.

http://codereview.chromium.org/131092/diff/1002/11#newcode204
Line 204: 
On 2009/06/19 18:16:07, awong wrote:
> remove extra newline.

Done.

http://codereview.chromium.org/131092/diff/1002/12
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/131092/diff/1002/12#newcode16
Line 16: // excessive memory consumption.
On 2009/06/19 18:16:07, awong wrote:
> Add explanation of semantics when the stream is stopped.

Done.

http://codereview.chromium.org/131092/diff/1002/13
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/131092/diff/1002/13#newcode645
Line 645: MockReadCallback* callback = new MockReadCallback();
On 2009/06/19 18:16:07, awong wrote:
> Just to be safe, since you don't hand this off imediately, use scoped_ptr and
> then when passing to demuxer, pass it via calling .release()

Done.

http://codereview.chromium.org/131092/diff/1002/13#newcode659
Line 659: EXPECT_CALL(*callback, OnDelete());
On 2009/06/19 18:16:07, awong wrote:
> Do we need to use a StrictMock with the callback above?  I think currently,
> RunWithParams can be called, and this mock will just warn, not fail the test.

ahhh right.  I know gmock prints out "uninteresting method call" (or something
like that) but I've only ever seen those at the same time as other (real) test
failures, so I assumed mocks were strict by default.

good catch!

[awong, 2009-06-19 19:46:00]

LGTM

[Alpha Left Google, 2009-06-19 19:58:04]

looks good to me too.