Fix ChunkDemuxer seek and init callback dispatch.

Fix ChunkDemuxer seek and init callback dispatch.

This patch ensures that seek and init callbacks are always
dispatched from the message loop that initiated the Seek()
or Initialize() operation. This prevents reentrancy problems
and allows the code to jump through less locking hoops.

TESTS=All existing unittests and LayoutTests still pass.

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

[acolwell GONE FROM CHROMIUM, 2013-06-20 16:06:22]

https://codereview.chromium.org/17261029/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (left):

https://codereview.chromium.org/17261029/diff/1/media/filters/chunk_demuxer.c...
media/filters/chunk_demuxer.cc:1066: base::AutoUnlock auto_unlock(lock_);
No longer needed because init_cb_ & seek_cb_ post to the message loop now.

https://codereview.chromium.org/17261029/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/17261029/diff/1/media/filters/chunk_demuxer.c...
media/filters/chunk_demuxer.cc:219: void OnSetDuration(TimeDelta duration);
The removal of base:: here and below is just to make this code consistant so it
all takes advantage of the using directive above.

[xhwang, 2013-06-20 18:02:40]

I have some questions about the relation b/w different ways to check if we have
a pending seek.

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

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:667: DCHECK(seek_cb_.is_null() ||
IsSeekPending_Locked());
When IsSeekPending_Locked(), if seek_cb.is_null() always false? If so we can
probably make more cleanup here.

e.g.

DCHECK(!seek_cb_.is_null()) or

if (seek_cb_.is_null()) {
  DCHECK(!IsSeekPending_Locked());
  return;
}

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:863: RunSeekCallback_Locked(PIPELINE_OK);
ditto about the relation between IsSeekPending_Locked() and !seek_cb_.is_null().
Also, doesn't !seek_cb_.is_null() mean old_seek_pending?

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:1282: return;
I am not a fan of hiding checks in the function. By looking at the calling code,
people have no idea that RunSeekCallback may actually not run the callback.

[acolwell GONE FROM CHROMIUM, 2013-06-20 18:52:21]

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

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:667: DCHECK(seek_cb_.is_null() ||
IsSeekPending_Locked());
On 2013/06/20 18:02:41, xhwang wrote:
> When IsSeekPending_Locked(), if seek_cb.is_null() always false? If so we can
> probably make more cleanup here.
> 
> e.g.
> 
> DCHECK(!seek_cb_.is_null()) or
> 
> if (seek_cb_.is_null()) {
>   DCHECK(!IsSeekPending_Locked());
>   return;
> }
We still need to make the CancelPendingSeek() calls because the Seek() may be in
flight. This can happen if a second seek occurs before the pipeline has a chance
to call Seek() on the demuxer for the first seek. The CancelPendingSeek() calls
make sure that IsSeekPending_Locked() equals false when the demuxer Seek()
eventually gets called. This ensures that the seek callback gets immediately
dispatched.

This is basically enforcing the precondition that if we have a callback we must
be waiting for data on at least one of the streams. I've renamed
IsSeekPending_Locked() to IsSeekWaitingForData_Locked() to make this a little
more clear.

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:863: RunSeekCallback_Locked(PIPELINE_OK);
On 2013/06/20 18:02:41, xhwang wrote:
> ditto about the relation between IsSeekPending_Locked() and
!seek_cb_.is_null().

This is basically saying "If the seek was waiting for data before the data was
appended and now it has enough data to complete the seek, then dispatch the seek
callback."

> Also, doesn't !seek_cb_.is_null() mean old_seek_pending?
Yes. Technically this is waiting for the last successful Seek() call on the
demuxer. We should never hit this branch if CancelPendingSeek() gets called
because that method triggers the seek callback to get called & cleared in that
method.

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:1282: return;
On 2013/06/20 18:02:41, xhwang wrote:
> I am not a fan of hiding checks in the function. By looking at the calling
code,
> people have no idea that RunSeekCallback may actually not run the callback.

Done.

[xhwang, 2013-06-20 19:36:25]

Hmm, I guess I didn't get the point yet. More questions.

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

https://codereview.chromium.org/17261029/diff/7001/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:667: DCHECK(seek_cb_.is_null() ||
IsSeekPending_Locked());
On 2013/06/20 18:52:22, acolwell wrote:
> On 2013/06/20 18:02:41, xhwang wrote:
> > When IsSeekPending_Locked(), if seek_cb.is_null() always false? If so we can
> > probably make more cleanup here.
> > 
> > e.g.
> > 
> > DCHECK(!seek_cb_.is_null()) or
> > 
> > if (seek_cb_.is_null()) {
> >   DCHECK(!IsSeekPending_Locked());
> >   return;
> > }
> We still need to make the CancelPendingSeek() calls because the Seek() may be
in
> flight. 

If Seek() is in flight, shouldn't seek_cb_ be set previously?

> This can happen if a second seek occurs before the pipeline has a chance
> to call Seek() on the demuxer for the first seek. 

What will happen in this case? Will the pipeline call Seek() twice?

> The CancelPendingSeek() calls
> make sure that IsSeekPending_Locked() equals false when the demuxer Seek()
> eventually gets called. This ensures that the seek callback gets immediately
> dispatched.
> 
> This is basically enforcing the precondition that if we have a callback we
must
> be waiting for data on at least one of the streams. I've renamed
> IsSeekPending_Locked() to IsSeekWaitingForData_Locked() to make this a little
> more clear.

Yes, my point was if we don't have a seek callback then we are not waiting for
data on any of the streams. In that case, do we still need to call
CancelPendingSeek() on those streams?

[acolwell GONE FROM CHROMIUM, 2013-06-20 21:59:28]

Made DCHECK in CancelPendingSeek() more restrictive. Based on our offline
discussion, I'll move the CANCELED and WAITING_FOR_SEEK logic from
ChunkDemuxerStream to ChunkDemuxer in a separate CL.

[xhwang, 2013-06-20 22:06:47]

lgtm % one last comment

https://codereview.chromium.org/17261029/diff/15002/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/17261029/diff/15002/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:667: DCHECK(seek_cb_.is_null() ^
IsSeekWaitingForData_Locked());
s/^/!=/ which may be a little easier to understand?

[acolwell GONE FROM CHROMIUM, 2013-06-20 23:09:37]

https://codereview.chromium.org/17261029/diff/15002/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/17261029/diff/15002/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:667: DCHECK(seek_cb_.is_null() ^
IsSeekWaitingForData_Locked());
On 2013/06/20 22:06:47, xhwang wrote:
> s/^/!=/ which may be a little easier to understand?

Done.

[commit-bot: I haz the power, 2013-06-20 23:10:33]

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

[commit-bot: I haz the power, 2013-06-21 09:41:45]

Change committed as 207767