Move pending seek cancellation logic from ChunkDemuxerStream to ChunkDemuxer.

Move pending seek cancellation logic from ChunkDemuxerStream to ChunkDemuxer.

TEST=All existing unittests still pass.

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

[acolwell GONE FROM CHROMIUM, 2013-06-21 16:33:33]

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:662: DCHECK(seek_cb_.is_null() ||
IsSeekWaitingForData_Locked());
Changed this back to the original DCHECK because there is an implicit seek on
the initial preroll. I added a unit test to make sure this case is covered.

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:1089: TimeDelta start_time = GetStartTime();
This is just to make it clear what these initial seeks are for.

[xhwang, 2013-06-21 20:44:13]

I see the point now. CancelPendingSeek() is called on the renderer thread
directly, while the Seek() call comes from the media thread, after a series of
calls/posts. So CancelPendingSeek() can be called before the Seek() to be
cancelled. Is that correct? 

I haven't reviewed this CL thoroughly yet. Just have some general questions for
now:

1, If I call:
Seek()
CancelPendingSeek()
Seek()
How do I know which Seek() will be cancelled?

2, Why can't we make the ChunkDemuxer live on the media thread to make things
more clear?

3, Can we make Seek() call cancel (aborts) the previous reads and seeks
automatically? This is somewhat similar to VideoDecoder::Stop() which cancels
previous reads and resets. This way, we don't need the CancelPendingSeek() and
StartWaitForSeek() any more.

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:507: break;
nit: no need to break after return.

[acolwell GONE FROM CHROMIUM, 2013-06-21 21:00:37]

On 2013/06/21 20:44:13, xhwang wrote:
> I see the point now. CancelPendingSeek() is called on the renderer thread
> directly, while the Seek() call comes from the media thread, after a series of
> calls/posts. So CancelPendingSeek() can be called before the Seek() to be
> cancelled. Is that correct? 

Yes.

> 
> I haven't reviewed this CL thoroughly yet. Just have some general questions
for
> now:
> 
> 1, If I call:
> Seek()
> CancelPendingSeek()
> Seek()
> How do I know which Seek() will be cancelled?

The sequence would actually have to be the following.
Seek() - media thread.
CancelPendingSeek() - render thread.
StartWaitingForSeek() - media thread
Seek() - render thread.

The second Seek() can only occur after the Pipeline has told WMPI that the seek
has completed. Basically CancelPendingSeek() means that WMPI knows there is an
outstanding seek. StartWaitingForSeek() indicates that there isn't a pending
seek in flight so it should NOT ignore the next Seek() that occurs.

> 
> 2, Why can't we make the ChunkDemuxer live on the media thread to make things
> more clear?

The render thread needs to have a way of poking the media thread so that the
pipeline doesn't block if the application doesn't append data. Multiple seeks
can be issued by JavaScript and really the application only needs to append data
for the LAST seek request to allow playback to progress. We never run into this
problem with FFmpegDemuxer because data is always arriving so we are always
guaranteed that a seek will complete "eventually". There is no guarentees that
data will ever arrive for a particular seek point in the MSE case. From the web
application's point of view it should only have to append data for the current
seek location to make playback progress.

> 
> 3, Can we make Seek() call cancel (aborts) the previous reads and seeks
> automatically? This is somewhat similar to VideoDecoder::Stop() which cancels
> previous reads and resets. This way, we don't need the CancelPendingSeek() and
> StartWaitForSeek() any more.

This would likely require significant changes to the pipeline seek logic. The
current code pauses & flushes the renderers before it calls Seek() on the
demuxer. To do those operations, it has to wait for any outstanding Reads() to
complete. In the ChunkDemuxer case, these Reads will never complete if the
current playback position is in an unbuffered area and the application isn't
appending data. CancelPendingSeek() and StartWaitingForSeek() unblock the Reads
so that the pause & flush operations will complete.

> 
>
https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
> media/filters/chunk_demuxer.cc:507: break;
> nit: no need to break after return.

[xhwang, 2013-06-21 21:08:43]

On 2013/06/21 21:00:37, acolwell wrote:
> On 2013/06/21 20:44:13, xhwang wrote:
> > I see the point now. CancelPendingSeek() is called on the renderer thread
> > directly, while the Seek() call comes from the media thread, after a series
of
> > calls/posts. So CancelPendingSeek() can be called before the Seek() to be
> > cancelled. Is that correct? 
> 
> Yes.
> 
> > 
> > I haven't reviewed this CL thoroughly yet. Just have some general questions
> for
> > now:
> > 
> > 1, If I call:
> > Seek()
> > CancelPendingSeek()
> > Seek()
> > How do I know which Seek() will be cancelled?
> 
> The sequence would actually have to be the following.
> Seek() - media thread.
> CancelPendingSeek() - render thread.
> StartWaitingForSeek() - media thread
> Seek() - render thread.
> 
> The second Seek() can only occur after the Pipeline has told WMPI that the
seek
> has completed. Basically CancelPendingSeek() means that WMPI knows there is an
> outstanding seek. StartWaitingForSeek() indicates that there isn't a pending
> seek in flight so it should NOT ignore the next Seek() that occurs.

I see. Can we add comments on CancelPendingSeek() and StartWaitingForSeek()
about these?

> 
> > 
> > 2, Why can't we make the ChunkDemuxer live on the media thread to make
things
> > more clear?
> 
> The render thread needs to have a way of poking the media thread so that the
> pipeline doesn't block if the application doesn't append data. Multiple seeks
> can be issued by JavaScript and really the application only needs to append
data
> for the LAST seek request to allow playback to progress. We never run into
this
> problem with FFmpegDemuxer because data is always arriving so we are always
> guaranteed that a seek will complete "eventually". There is no guarentees that
> data will ever arrive for a particular seek point in the MSE case. From the
web
> application's point of view it should only have to append data for the current
> seek location to make playback progress.
> 
> > 
> > 3, Can we make Seek() call cancel (aborts) the previous reads and seeks
> > automatically? This is somewhat similar to VideoDecoder::Stop() which
cancels
> > previous reads and resets. This way, we don't need the CancelPendingSeek()
and
> > StartWaitForSeek() any more.
> 
> This would likely require significant changes to the pipeline seek logic. The
> current code pauses & flushes the renderers before it calls Seek() on the
> demuxer. To do those operations, it has to wait for any outstanding Reads() to
> complete. In the ChunkDemuxer case, these Reads will never complete if the
> current playback position is in an unbuffered area and the application isn't
> appending data. CancelPendingSeek() and StartWaitingForSeek() unblock the
Reads
> so that the pause & flush operations will complete.

I see. Thanks for the explanation!
> 
> > 
> >
>
https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
> > File media/filters/chunk_demuxer.cc (right):
> > 
> >
>
https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
> > media/filters/chunk_demuxer.cc:507: break;
> > nit: no need to break after return.

[xhwang, 2013-06-21 21:21:36]

lgtm % a minor question

Thanks for the patience and explanation!

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:656: cancel_next_seek_ = false;
Can |cancel_next_seek_| be true here? If so, does that mean the caller didn't
wait for the previous Seek() to be cancelled to call StartWaitingForSeek(),
which shouldn't happen?

[acolwell GONE FROM CHROMIUM, 2013-06-21 22:08:37]

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:507: break;
On 2013/06/21 20:44:13, xhwang wrote:
> nit: no need to break after return.

Done.

https://codereview.chromium.org/16867010/diff/2001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:656: cancel_next_seek_ = false;
On 2013/06/21 21:21:36, xhwang wrote:
> Can |cancel_next_seek_| be true here? If so, does that mean the caller didn't
> wait for the previous Seek() to be cancelled to call StartWaitingForSeek(),
> which shouldn't happen?
This is needed to handle the case where the initial preroll gets cancelled. In
that case there is no Seek() call in flight. We are just waiting for the
pipeline initialization to complete. Once it completes, WMPI initiates the new
seek and calls StartWaitingForSeek(). In this case we want to clear the
cancellation state because we actually want to pay attention to the next Seek().

[commit-bot: I haz the power, 2013-06-21 22:08:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16867010/13001

[commit-bot: I haz the power, 2013-06-22 02:31:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16867010/13001

[commit-bot: I haz the power, 2013-06-22 02:59:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16867010/13001

[commit-bot: I haz the power, 2013-06-22 03:29:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16867010/13001

[commit-bot: I haz the power, 2013-06-22 03:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16867010/13001

[commit-bot: I haz the power, 2013-06-22 14:50:01]

Change committed as 208046