Cast: Make castv2 mirroring performance test run again

Cast: Make castv2 mirroring performance test run again

Note that this CL doesn't actually make the tests *pass*, but now they report actual
problems in the cast library rather than just fail immediately. Making the tests pass
will be a problem for the next CL.

Also, add tests for what happens when the sender/receiver clocks are off.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262860

[hubbe, 2014-04-08 22:32:39]

Piman for render_widgt_host_impl.cc

[piman, 2014-04-08 22:46:03]

https://codereview.chromium.org/229503002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/229503002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:638: #endif
This hardcodes the knowledge that "on mac we don't use delegated rendering" in
the test, which is unfortunate (it won't be true forever).
Maybe we can have both methods generate the same event "got a frame from the
renderer", so that we don't need to hardcode this here?

https://codereview.chromium.org/229503002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/229503002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:1548: //
chrome/browser/extensions/api/cast_streaming/performance_test.cc
mmh, I think I would prefer if we added separate UNSHIPPED_TRACE_EVENTs
specifically for the tests, in the "test" or "testing" category, that way it
wouldn't pollute the regular traces (bonus points if they're
TRACE_DISABLED_BY_DEFAULT and explicitly enabled in the test).

[hubbe, 2014-04-08 22:58:29]

https://codereview.chromium.org/229503002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/229503002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:638: #endif
On 2014/04/08 22:46:03, piman wrote:
> This hardcodes the knowledge that "on mac we don't use delegated rendering" in
> the test, which is unfortunate (it won't be true forever).
> Maybe we can have both methods generate the same event "got a frame from the
> renderer", so that we don't need to hardcode this here?

Now uses a special trace event which is normally OFF.

https://codereview.chromium.org/229503002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/229503002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:1548: //
chrome/browser/extensions/api/cast_streaming/performance_test.cc
On 2014/04/08 22:46:03, piman wrote:
> mmh, I think I would prefer if we added separate UNSHIPPED_TRACE_EVENTs
> specifically for the tests, in the "test" or "testing" category, that way it
> wouldn't pollute the regular traces (bonus points if they're
> TRACE_DISABLED_BY_DEFAULT and explicitly enabled in the test).

Done.

[piman, 2014-04-08 22:59:18]

LGTM, thanks.

[miu, 2014-04-09 19:44:34]

lgtm, if you only change comments.  If not, I should take another look at the
next PS.

https://codereview.chromium.org/229503002/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/229503002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:628: // Only
one of these PrintResults should actually print something.
Does this comment need updating?

https://codereview.chromium.org/229503002/diff/30001/media/cast/test/utility/...
File media/cast/test/utility/standalone_cast_environment.h (right):

https://codereview.chromium.org/229503002/diff/30001/media/cast/test/utility/...
media/cast/test/utility/standalone_cast_environment.h:21: explicit
StandaloneCastEnvironment(scoped_ptr<base::TickClock> clock);
This concerns me, since the task runner for the threads won't respect the
passage of time of |clock|.  Suggest you either re-think this, or add a ctor
comment here to warn that the use of PostDelayedTask() will be timed w.r.t. the
default clock, not |clock|.

https://codereview.chromium.org/229503002/diff/30001/media/cast/video_receive...
File media/cast/video_receiver/video_receiver.cc (left):

https://codereview.chromium.org/229503002/diff/30001/media/cast/video_receive...
media/cast/video_receiver/video_receiver.cc:74: // If any callbacks for encoded
video frames are queued, flush them out now.
The comment in video_receiver.h (starting at line 51) needs to be adjusted too. 
...and also in audio_receiver.h.

[hubbe, 2014-04-09 20:39:42]

PTAL

https://codereview.chromium.org/229503002/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/229503002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:628: // Only
one of these PrintResults should actually print something.
On 2014/04/09 19:44:35, miu wrote:
> Does this comment need updating?

Comment removed.

https://codereview.chromium.org/229503002/diff/30001/media/cast/test/utility/...
File media/cast/test/utility/standalone_cast_environment.h (right):

https://codereview.chromium.org/229503002/diff/30001/media/cast/test/utility/...
media/cast/test/utility/standalone_cast_environment.h:21: explicit
StandaloneCastEnvironment(scoped_ptr<base::TickClock> clock);
On 2014/04/09 19:44:35, miu wrote:
> This concerns me, since the task runner for the threads won't respect the
> passage of time of |clock|.  Suggest you either re-think this, or add a ctor
> comment here to warn that the use of PostDelayedTask() will be timed w.r.t.
the
> default clock, not |clock|.

The task runners do all their work in relative time (the input to PostDelayTask
is a TimeDelta) so this should just work. However, I ended up changing the
implementation while I was testing, let me know which implementation you prefer.

https://codereview.chromium.org/229503002/diff/30001/media/cast/video_receive...
File media/cast/video_receiver/video_receiver.cc (left):

https://codereview.chromium.org/229503002/diff/30001/media/cast/video_receive...
media/cast/video_receiver/video_receiver.cc:74: // If any callbacks for encoded
video frames are queued, flush them out now.
On 2014/04/09 19:44:35, miu wrote:
> The comment in video_receiver.h (starting at line 51) needs to be adjusted
too. 
> ...and also in audio_receiver.h.

Done.

[miu, 2014-04-09 21:06:25]

lgtm

[commit-bot: I haz the power, 2014-04-09 21:08:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/229503002/70001

[commit-bot: I haz the power, 2014-04-09 23:45:40]

Change committed as 262860