Drop frames less than a millisecond apart in media time.

Drop frames less than a millisecond apart in media time.

Our rendering path has no reasonable expectation of being able to
render 1000fps content, so these are bad timestamps that end up
wrecking assumptions throughout the rendering pathway.

Instead of trying to guard against these frames everywhere, just
discard them immediately during enqueue.

BUG=488302
TEST=new unittest

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

[DaleCurtis, 2015-05-16 01:18:08]

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

[xhwang, 2015-05-18 17:45:24]

Looks pretty good. Just some comments trying to see whether we can make the code
simper.

https://codereview.chromium.org/1142723002/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1142723002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:381: << "recently received frame.";
We are replacing a frame if the timestamp is the same, but not replacing it if
the timestamp is earlier or later by 1 us. This sounds like a bug... I think
it's okay to just drop the frame which arrives later. This should be a rare case
and a simpler solution is a better solution.

https://codereview.chromium.org/1142723002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:401: }
The code from l.375-401 is a bit hard to read because we reset delta a couple of
times and have some intermediate returns. If we don't need to worry about the
special case of l.379-384, maybe we can make the code simper?

[DaleCurtis, 2015-05-18 23:17:14]

https://codereview.chromium.org/1142723002/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1142723002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:381: << "recently received frame.";
On 2015/05/18 17:45:24, xhwang wrote:
> We are replacing a frame if the timestamp is the same, but not replacing it if
> the timestamp is earlier or later by 1 us. This sounds like a bug... I think
> it's okay to just drop the frame which arrives later. This should be a rare
case
> and a simpler solution is a better solution.

Done.

https://codereview.chromium.org/1142723002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:401: }
On 2015/05/18 17:45:23, xhwang wrote:
> The code from l.375-401 is a bit hard to read because we reset delta a couple
of
> times and have some intermediate returns. If we don't need to worry about the
> special case of l.379-384, maybe we can make the code simper?

Done.

[xhwang, 2015-05-18 23:27:04]

LGTM with a tiny nit!

https://codereview.chromium.org/1142723002/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1142723002/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:389: << delta.InMicroseconds() << "
us";
nit: Can "<< delta" work? That'll print a double value in seconds...

https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.cc&...

[DaleCurtis, 2015-05-18 23:32:11]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-05-18 23:32:27]

https://codereview.chromium.org/1142723002/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1142723002/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:389: << delta.InMicroseconds() << "
us";
On 2015/05/18 23:27:04, xhwang wrote:
> nit: Can "<< delta" work? That'll print a double value in seconds...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.cc&...

It would, but these values are < 1ms, so displaying x / 1000000 isn't nearly as
readable as printing it in microseconds.

[commit-bot: I haz the power, 2015-05-18 23:33:50]

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

[commit-bot: I haz the power, 2015-05-19 00:30:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-19 00:31:07]

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