Pipeline: Call clock_->SetTime() right before StartPlayingFrom().

Pipeline: Call clock_->SetTime() right before StartPlayingFrom().

Currently we call clock_->SetTime() before we flush the renderers. This CL
moves it to right before we call StartPlayingFrom(start_timestamp_). This
makes it easier in a later CL to hide all clock logic into a new Renderer
implementation.

This CL also fixed the PipelineTest.Seek test. Originally we AddTextStream()
in the text which causes a TextRenderer::Read(). But we never satisfy or
abort that Read(). This caused TextRenderer::Pause() to never return the
completion callback. Therefore, the pipeline seek process never finishes.
This test was passing because we SetTime() before we DoSeek(). Now I moved
SetTime() after DoSeek() and this issue is exposed.

BUG=392259
TEST=All existing tests pass.

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

[xhwang, 2014-07-08 18:22:32]

PTAL

[scherkus (not reviewing), 2014-07-08 18:37:20]

https://codereview.chromium.org/377003002/diff/20001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/377003002/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:59: base::AutoLock auto_lock(lock_);
we shouldn't have any synchronization issues inside the ctor

also .. what does the Clock ctor do? does it make sense to have it start at 0,0?

[xhwang, 2014-07-08 19:32:09]

comments addressed

[xhwang, 2014-07-08 19:32:16]

PTAL again.

https://codereview.chromium.org/377003002/diff/20001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/377003002/diff/20001/media/base/pipeline.cc#n...
media/base/pipeline.cc:59: base::AutoLock auto_lock(lock_);
On 2014/07/08 18:37:20, scherkus wrote:
> we shouldn't have any synchronization issues inside the ctor

Done.

> also .. what does the Clock ctor do? does it make sense to have it start at
0,0?

The media_time_ defaults to base::TimeDelta(). The max_time_ defaults to
kNoTimeStamp(). It seems the Clock actually uses the info about whether the
max_time_ has been set to calculate Eclapsed(). I think these can be simplified
but it definitely deserves a separate CL.

[scherkus (not reviewing), 2014-07-08 21:21:06]

lgtm w/ nit

https://codereview.chromium.org/377003002/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/377003002/diff/40001/media/base/pipeline.cc#n...
media/base/pipeline.cc:59: // base media time so our timestamp calculations will
be correct.
I'd just remove this comment -- it doesn't seem very relevant anymore

[xhwang, 2014-07-08 21:23:58]

comments addressed

[xhwang, 2014-07-08 21:24:36]

https://codereview.chromium.org/377003002/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/377003002/diff/40001/media/base/pipeline.cc#n...
media/base/pipeline.cc:59: // base media time so our timestamp calculations will
be correct.
On 2014/07/08 21:21:06, scherkus wrote:
> I'd just remove this comment -- it doesn't seem very relevant anymore

Done.

[xhwang, 2014-07-08 21:24:46]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-08 21:27:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/377003002/60001

[xhwang, 2014-07-08 23:11:57]

scherkus: I fixed a failing test. Please see updated CL description for details.
PTAL again!

[scherkus (not reviewing), 2014-07-08 23:56:54]

lgtm

[xhwang, 2014-07-09 00:00:14]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-09 00:05:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/377003002/80001

[commit-bot: I haz the power, 2014-07-09 04:53:58]

Change committed as 281938