media: In low delay mode, do not accumulate frames earlier than the start time.

media: In low delay mode, do not accumulate frames earlier than the start time.

In low delay mode, we'll start playback when we have any frame. If we accumulate
frames earlier than the start time, we'll start playing that frame prematurely.
Instead, drop those frames immediately. Playback will start when we start to
have frames on or later than the start time.

BUG=512145
TEST=Updated the unittest to cover this case.

Committed: https://crrev.com/e834ecea2e8293282f3fed7899f0b4bd56273818
Cr-Commit-Position: refs/heads/master@{#339774}

[xhwang, 2015-07-21 20:30:25]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[xhwang, 2015-07-21 20:30:26]

PTAL

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:512: // after prerolling has completed.
Is this mostly for the old rendering path? Can we just drop all early frames in
low-delay and non-low-delay modes? It seems to me early frames are just an
inevitable artifact of the demuxer, and it should be isolated from our rendering
logic.

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl_unittest.cc:501: .Times(AnyNumber());
When using the old rendering path, we declare HAVE_NOTHING whenever we run out
of frame, which could be pretty common in low delay mode in tests. In real life
this could also happen. Maybe we should have some grace period to declare
HAVE_NOTHING in low delay mode.

Or, when are we gonna drop old rendering path altogether? It's such a pain to
keep them both in our code...

[DaleCurtis, 2015-07-21 21:16:50]

lgtm

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:512: // after prerolling has completed.
On 2015/07/21 20:30:26, xhwang wrote:
> Is this mostly for the old rendering path? Can we just drop all early frames
in
> low-delay and non-low-delay modes? It seems to me early frames are just an
> inevitable artifact of the demuxer, and it should be isolated from our
rendering
> logic.

Well, consider what happens if T=1s and the frames are 0s, 10s. Would we discard
the frame at 0s and wait for the 10s frame?  At most I think you could always
discard [frame_start, frame_end] when T > frame_end.

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl_unittest.cc:501: .Times(AnyNumber());
On 2015/07/21 20:30:26, xhwang wrote:
> When using the old rendering path, we declare HAVE_NOTHING whenever we run out
> of frame, which could be pretty common in low delay mode in tests. In real
life
> this could also happen. Maybe we should have some grace period to declare
> HAVE_NOTHING in low delay mode.
> 
> Or, when are we gonna drop old rendering path altogether? It's such a pain to
> keep them both in our code...

I was planning to delete the old path this week as soon as I can find time :)

[xhwang, 2015-07-21 21:53:37]

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1246033008/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:512: // after prerolling has completed.
On 2015/07/21 21:16:50, DaleCurtis wrote:
> On 2015/07/21 20:30:26, xhwang wrote:
> > Is this mostly for the old rendering path? Can we just drop all early frames
> in
> > low-delay and non-low-delay modes? It seems to me early frames are just an
> > inevitable artifact of the demuxer, and it should be isolated from our
> rendering
> > logic.
> 
> Well, consider what happens if T=1s and the frames are 0s, 10s. Would we
discard
> the frame at 0s and wait for the 10s frame?  At most I think you could always
> discard [frame_start, frame_end] when T > frame_end.

I see your point. Things would be much simpler if we have a good estimation of
the duration... Not sure how hard to plumb that through.

[xhwang, 2015-07-21 21:53:40]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-07-21 21:54:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246033008/1

[commit-bot: I haz the power, 2015-07-21 23:11:17]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-07-21 23:12:14]

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