Switch back to using VideoRenderer::StartPlayingFrom().

Switch back to using VideoRenderer::StartPlayingFrom().

Turns out dropping the time argument in 0860287 doesn't work well in a
video frame scheduling based world as a VideoRenderer is no longer
able to poll for the current time. Instead, go back to passing in the
start timestamp so that VideoRenderer implementations can do frame
accurate seeking.

BUG=110814, 370634
Committed: https://crrev.com/b6f074ead239a14b8291610570a959daddc75a43
Cr-Commit-Position: refs/heads/master@{#295002}

[scherkus (not reviewing), 2014-09-12 22:30:03]

scherkus@chromium.org changed reviewers:
+ wolenetz@chromium.org

[scherkus (not reviewing), 2014-09-12 22:30:48]

wolenetz: want to take a look?

[wolenetz, 2014-09-13 01:11:45]

On 2014/09/12 22:30:48, scherkus wrote:
> wolenetz: want to take a look?

I just saw this incoming CR. I may not get to it fully today, but will Monday at
latest.
Description is confusing to me: what is "0860287" ?
Also in description, s/longer is/longer/

[scherkus (not reviewing), 2014-09-13 01:39:24]

No worries. Being in a git world I'm using abbreviated git hashes to refer
to revisions. 0860287 is an example of that.

crrev.com/0860287 *should* link to the proper revision, but currently
doesn't due to crbug.com/413933
On 2014/09/12 22:30:48, scherkus wrote:

> wolenetz: want to take a look?
>

I just saw this incoming CR. I may not get to it fully today, but will
Monday at
latest.
Description is confusing to me: what is "0860287" ?
Also in description, s/longer is/longer/

https://codereview.chromium.org/568113003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wolenetz, 2014-09-16 00:36:03]

lgtm % nits. This is first I've looked at the rendering-side code since CR'ing
Dale's gapless changes, so my apologies for the delay in CR.

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
File media/filters/video_renderer_impl.h (right):

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl.h:143: // rendering of video is controlled by
time advancing via |time_cb_|.
nit: what is |time_cb_|? It seems this comment may have gotten out of sync with
code already. Is |get_time_cb_| meant here?

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl.h:154: //         | StartPlaying()            
^
nit: /g()/gFrom()/

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
File media/filters/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl_unittest.cc:467: TEST_F(VideoRendererImplTest,
DestroyDuringOutstandingRead) {
drive by nit: this test case looks exactly like the previous. Drop it?

[scherkus (not reviewing), 2014-09-16 00:49:44]

thanks for the review!

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
File media/filters/video_renderer_impl.h (right):

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl.h:143: // rendering of video is controlled by
time advancing via |time_cb_|.
On 2014/09/16 00:36:03, wolenetz wrote:
> nit: what is |time_cb_|? It seems this comment may have gotten out of sync
with
> code already. Is |get_time_cb_| meant here?

fixed to use |get_time_cb_|

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl.h:154: //         | StartPlaying()            
^
On 2014/09/16 00:36:03, wolenetz wrote:
> nit: /g()/gFrom()/

Done.

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
File media/filters/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/568113003/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl_unittest.cc:467: TEST_F(VideoRendererImplTest,
DestroyDuringOutstandingRead) {
On 2014/09/16 00:36:03, wolenetz wrote:
> drive by nit: this test case looks exactly like the previous. Drop it?

indeed it is! I actually think the above test case is the one that needs to be
dropped as the Play() method that it mentions doesn't exist anymore

[scherkus (not reviewing), 2014-09-16 00:49:46]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-16 01:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568113003/20001

[commit-bot: I haz the power, 2014-09-16 02:29:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-16 02:29:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scherkus (not reviewing), 2014-09-16 03:11:35]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-16 03:12:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568113003/20001

[commit-bot: I haz the power, 2014-09-16 03:53:08]

Committed patchset #2 (id:20001) as f7580bfe2ae6db2af361824a071874bf99d7d28c

[commit-bot: I haz the power, 2014-09-16 03:55:17]

Patchset 2 (id:??) landed as
https://crrev.com/b6f074ead239a14b8291610570a959daddc75a43
Cr-Commit-Position: refs/heads/master@{#295002}