Cache effective frame count. Make expired frame count useful.

Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=569015, 599908
TEST=updated unittests.

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

[DaleCurtis, 2016-04-11 20:44:05]

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

[DaleCurtis, 2016-04-11 20:51:16]

Description was changed from

==========
Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=527990, 569015
TEST=updated unittests.
==========

to

==========
Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=527990, 569015, 599908
TEST=updated unittests.
==========

[DaleCurtis, 2016-04-11 20:51:42]

The preliminary version of the followup CL can be found here
https://codereview.chromium.org/1863373002

[DaleCurtis, 2016-04-12 00:55:26]

Description was changed from

==========
Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=527990, 569015, 599908
TEST=updated unittests.
==========

to

==========
Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=569015, 599908
TEST=updated unittests.
==========

[xhwang, 2016-04-14 00:24:15]

LGTM with nits. Sorry for the delay.

https://chromiumcodereview.appspot.com/1878783002/diff/1/media/filters/video_...
File media/filters/video_renderer_algorithm.cc (right):

https://chromiumcodereview.appspot.com/1878783002/diff/1/media/filters/video_...
media/filters/video_renderer_algorithm.cc:261: // Always leave at least one
frame in the queue.
This comment is confusing coz the following is a check.

https://chromiumcodereview.appspot.com/1878783002/diff/1/media/filters/video_...
media/filters/video_renderer_algorithm.cc:395: ready_frame.start_time =
wall_clock_times[0];
Can you use ConvertMediaTimestamp()?

https://chromiumcodereview.appspot.com/1878783002/diff/1/media/filters/video_...
File media/filters/video_renderer_algorithm_unittest.cc (right):

https://chromiumcodereview.appspot.com/1878783002/diff/1/media/filters/video_...
media/filters/video_renderer_algorithm_unittest.cc:388: tg.current() +
algorithm_.average_frame_duration()));
Is this what "git cl format" did?

[DaleCurtis, 2016-04-14 00:54:01]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-04-14 00:54:02]

Thanks for review!

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

https://codereview.chromium.org/1878783002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:261: // Always leave at least one
frame in the queue.
On 2016/04/14 at 00:24:15, xhwang wrote:
> This comment is confusing coz the following is a check.

Clarified.

https://codereview.chromium.org/1878783002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:395: ready_frame.start_time =
wall_clock_times[0];
On 2016/04/14 at 00:24:15, xhwang wrote:
> Can you use ConvertMediaTimestamp()?

That's a function in VideoRendererImpl, not VideoRendererAlgorithm.

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

https://codereview.chromium.org/1878783002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm_unittest.cc:388: tg.current() +
algorithm_.average_frame_duration()));
On 2016/04/14 at 00:24:15, xhwang wrote:
> Is this what "git cl format" did?

Yup.

[DaleCurtis, 2016-04-14 00:54:03]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1878783002/#ps20001
(title: "Clarify comment.")

[commit-bot: I haz the power, 2016-04-14 00:54:25]

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

[commit-bot: I haz the power, 2016-04-14 02:31:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-14 02:32:01]

Description was changed from

==========
Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=569015, 599908
TEST=updated unittests.
==========

to

==========
Cache effective frame count. Make expired frame count useful.

This change affects a few cleanups and API changes in preparation for
some underflow in low delay / stalling cases to VideoRendererImpl:
- RemoveExpiredFrames() now returns the number of undisplayed frames
that have been removed instead of the count of all frames removed.
This is consistent with how we count dropped frames in Render().
- EffectiveFramesQueued() is replaced with a hacker_style() accessor
to a cached value. This method was being called ~4x as much as the
value needs to be computed and an upcoming change will expand this
to nearly ~6x w/o clunky manual caching.
- EnqueueFrame() now computes an estimate for added frame times,
allowing the effective frame count more accuracy immediately after
a frame has been added.
- Reset() now allows the entire frame queue to be dropped while
preserving next frame estimates. This allows decoders which have
a limited number of frames to recover from underflow effectively.
Otherwise the last frame can be stuck in the queue indefinitely,
preventing new frame generation while the decoder is stalled for
output space. This will be used in a followup CL.
- Re-enables a disabled cadence test on non-debug builds.
- s/== base::TimeDelta()/.is_zero()/ for zero tests.
- s/.get()// for boolean testing.

BUG=569015, 599908
TEST=updated unittests.

Committed: https://crrev.com/b1f342180355f130e968574e7271e5d17fe9c783
Cr-Commit-Position: refs/heads/master@{#387213}
==========

[commit-bot: I haz the power, 2016-04-14 02:32:03]

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