Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass, new unit test VideoRendererAlgorithmTest.EffectiveFramesQueuedWithoutFrameDropping

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

[wdzierzanowski, 2016-11-15 21:57:45]

wdzierzanowski@opera.com changed reviewers:
+ dalecurtis@chromium.org

[wdzierzanowski, 2016-11-15 21:57:46]

Dale, PTAL if this is what you had in mind.

[DaleCurtis, 2016-11-15 22:42:21]

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

https://codereview.chromium.org/2502093002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:682: // If frame dropping is disabled,
only count as queued those frames that were
This can go below the l.691 block. Also while this works, I think we want this
to be a minimum value, and not necessarily the returned value. I.e. it's
possible for a frame with a non-zero render count to still be effective. So i'd
do something like

size_t min_effective_frames = frame_dropping_disabled_ ? <count them> : 0;

...

l.706: effective_frames_queued_ = std::max(min_effective_frames,
frame_queue_.size() - expired_frames);
l.713: effective_frames_queued_ = min_effective_frames;
l.728: effective_frames_queued_ = std::max(min_effective_frames,
renderable_frame_count)

[wdzierzanowski, 2016-11-16 11:42:57]

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

https://codereview.chromium.org/2502093002/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:682: // If frame dropping is disabled,
only count as queued those frames that were
On 2016/11/15 22:42:21, DaleCurtis wrote:
> This can go below the l.691 block. Also while this works, I think we want this
> to be a minimum value, and not necessarily the returned value. I.e. it's
> possible for a frame with a non-zero render count to still be effective. So
i'd
> do something like
> 
> size_t min_effective_frames = frame_dropping_disabled_ ? <count them> : 0;
> 
> ...
> 
> l.706: effective_frames_queued_ = std::max(min_effective_frames,
> frame_queue_.size() - expired_frames);
> l.713: effective_frames_queued_ = min_effective_frames;
> l.728: effective_frames_queued_ = std::max(min_effective_frames,
> renderable_frame_count)

Done.

[DaleCurtis, 2016-11-16 22:03:44]

Want to add a small unittest for this too? lgtm

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

https://codereview.chromium.org/2502093002/diff/40001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:691: // If frame dropping is disabled,
the lower bound is the number of frames
Add line between these.

https://codereview.chromium.org/2502093002/diff/40001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:691: // If frame dropping is disabled,
the lower bound is the number of frames
Add line between these.

https://codereview.chromium.org/2502093002/diff/40001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:700: const size_t frames_queued =
CountEffectiveFramesQueued();
Just inline this into the std::max.

[wdzierzanowski, 2016-11-23 09:53:52]

On 2016/11/16 22:03:44, DaleCurtis wrote:
> Want to add a small unittest for this too?

Done.

> it's possible for a frame with a non-zero render count to still be effective

I'm having trouble exercising this case in the unit test I've added. It seems
the non-zero-render-count frames are getting chopped off in
https://cs.chromium.org/chromium/src/media/filters/video_renderer_algorithm.c....
Any advice on how to test it?

[wdzierzanowski, 2016-11-23 09:54:06]

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

https://codereview.chromium.org/2502093002/diff/40001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:691: // If frame dropping is disabled,
the lower bound is the number of frames
On 2016/11/16 22:03:44, DaleCurtis wrote:
> Add line between these.

Done.

https://codereview.chromium.org/2502093002/diff/40001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:700: const size_t frames_queued =
CountEffectiveFramesQueued();
On 2016/11/16 22:03:44, DaleCurtis wrote:
> Just inline this into the std::max.

Done.

[DaleCurtis, 2016-11-23 20:14:28]

On 2016/11/23 at 09:53:52, wdzierzanowski wrote:
> On 2016/11/16 22:03:44, DaleCurtis wrote:
> > Want to add a small unittest for this too?
> 
> Done.
> 
> > it's possible for a frame with a non-zero render count to still be effective
> 
> I'm having trouble exercising this case in the unit test I've added. It seems
the non-zero-render-count frames are getting chopped off in
https://cs.chromium.org/chromium/src/media/filters/video_renderer_algorithm.c....
Any advice on how to test it?

You need to test something that has a cadence of 2 or higher, so that each frame
needs > 1 render count for optimal rendering. This lgtm though.

[wdzierzanowski, 2016-11-23 20:28:48]

Description was changed from

==========
Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass
==========

to

==========
Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass, new unit test
VideoRendererAlgorithmTest.EffectiveFramesQueuedWithoutFrameDropping
==========

[wdzierzanowski, 2016-11-23 20:30:02]

Alright, I'll leave it as it is then. Thanks for the hints and for reviewing!

[wdzierzanowski, 2016-11-23 20:30:19]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2016-11-23 20:30:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 00:44:48]

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

[commit-bot: I haz the power, 2016-11-24 00:44:49]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wdzierzanowski, 2016-11-24 08:01:23]

The failure in "webgl_conformance_tests on NVIDIA GPU on Linux" seems unrelated.
Retrying.

[wdzierzanowski, 2016-11-24 08:01:31]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2016-11-24 08:02:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 10:03:37]

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

[commit-bot: I haz the power, 2016-11-24 10:03:38]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[wdzierzanowski, 2016-11-24 10:06:45]

JOB_TIMED_OUT? Trying once more.

[wdzierzanowski, 2016-11-24 10:06:55]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2016-11-24 10:07:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 12:08:21]

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

[commit-bot: I haz the power, 2016-11-24 12:08:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[wdzierzanowski, 2016-11-24 12:10:15]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2016-11-24 12:10:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wdzierzanowski, 2016-11-24 14:02:21]

Turns out some CQ builders are having issues,
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/LjiTwXkVrUU/dis...

[commit-bot: I haz the power, 2016-11-24 14:21:19]

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

[commit-bot: I haz the power, 2016-11-24 14:21:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[wdzierzanowski, 2016-11-24 15:27:52]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2016-11-24 15:28:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 16:47:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480001272922320,
"parent_rev": "652dc90d0fa7b7b6f6b99613b47ed8fd64636041", "commit_rev":
"dc0575e6a0ddae03408a3f2266bbe11a61ccd702"}

[commit-bot: I haz the power, 2016-11-24 16:47:34]

Description was changed from

==========
Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass, new unit test
VideoRendererAlgorithmTest.EffectiveFramesQueuedWithoutFrameDropping
==========

to

==========
Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass, new unit test
VideoRendererAlgorithmTest.EffectiveFramesQueuedWithoutFrameDropping
==========

[commit-bot: I haz the power, 2016-11-24 16:47:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-24 16:50:08]

Description was changed from

==========
Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass, new unit test
VideoRendererAlgorithmTest.EffectiveFramesQueuedWithoutFrameDropping
==========

to

==========
Adjust VideoRendererAlgorithm for |frame_dropping_disabled_|

This makes video frame hashing in tests immune to timing variations that
are inherent in the rendering algorithm.

BUG=663709
TEST=media_unittests pass, new unit test
VideoRendererAlgorithmTest.EffectiveFramesQueuedWithoutFrameDropping

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

[commit-bot: I haz the power, 2016-11-24 16:50:09]

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