Don't use barcodes in ProtocolPerfTests

Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Committed: https://crrev.com/b047307ab561878d4357bdf7e617785e4dbd4f44
Cr-Original-Commit-Position: refs/heads/master@{#425895}
Cr-Commit-Position: refs/heads/master@{#425998}

[Sergey Ulanov, 2016-10-14 19:52:43]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-10-14 21:52:35]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2016-10-14 23:26:07]

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/cyclic_fr...
File remoting/test/cyclic_frame_generator.h (right):

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/cyclic_fr...
remoting/test/cyclic_frame_generator.h:73: // they are generated.
Please update this comment.

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/protocol_...
File remoting/test/protocol_perftest.cc (right):

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/protocol_...
remoting/test/protocol_perftest.cc:467: }
Please add a comment explaining why warm-up frames are important.

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/protocol_...
remoting/test/protocol_perftest.cc:485: continue;
Under what circumstances do we expect this to be null?

[Sergey Ulanov, 2016-10-17 22:40:26]

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/cyclic_fr...
File remoting/test/cyclic_frame_generator.h (right):

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/cyclic_fr...
remoting/test/cyclic_frame_generator.h:73: // they are generated.
On 2016/10/14 23:26:07, Jamie wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/protocol_...
File remoting/test/protocol_perftest.cc (right):

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/protocol_...
remoting/test/protocol_perftest.cc:467: }
On 2016/10/14 23:26:07, Jamie wrote:
> Please add a comment explaining why warm-up frames are important.

Done. Also simplified this code to avoid duplicated while loop.

https://codereview.chromium.org/2420183002/diff/20001/remoting/test/protocol_...
remoting/test/protocol_perftest.cc:485: continue;
On 2016/10/14 23:26:07, Jamie wrote:
> Under what circumstances do we expect this to be null?

Actually it should never be null because
CyclicFrameGenerator::TakeLastEventTimestamps() never returns null. Replaced
with CHECK().
Also updated ClientSession to keep video stream paused until OnClientConnected()
is called. This ensures that the default EventsTimestampsSource is not used for
the first frame (previously the first frame was captured before
OnClientConnected()).

[Jamie, 2016-10-17 22:51:16]

lgtm

https://codereview.chromium.org/2420183002/diff/40001/remoting/test/cyclic_fr...
File remoting/test/cyclic_frame_generator.h (right):

https://codereview.chromium.org/2420183002/diff/40001/remoting/test/cyclic_fr...
remoting/test/cyclic_frame_generator.h:70: // must be the event timestamp
generated by InputEventTimestampsSource
s/the/an/?

Although looking at the impl, I don't think there are actually any constraints
on the timestamp.

[Sergey Ulanov, 2016-10-17 23:01:15]

Patchset #3 (id:60001) has been deleted

[Sergey Ulanov, 2016-10-17 23:03:31]

Found one more issue: the test wasn't working properly for ICE connections
because for them VideoStream is created after OnClientConnected(). Updated
ClientSession::SetEventTimestampsSourceForTests() to handle this cases
correctly.

https://codereview.chromium.org/2420183002/diff/40001/remoting/test/cyclic_fr...
File remoting/test/cyclic_frame_generator.h (right):

https://codereview.chromium.org/2420183002/diff/40001/remoting/test/cyclic_fr...
remoting/test/cyclic_frame_generator.h:70: // must be the event timestamp
generated by InputEventTimestampsSource
On 2016/10/17 22:51:16, Jamie wrote:
> s/the/an/?

Done.

> 
> Although looking at the impl, I don't think there are actually any constraints
> on the timestamp.

The point that the default timestamps source would not work here. Replaced
"must" with "should" in the comment.

[Sergey Ulanov, 2016-10-17 23:04:13]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-17 23:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2420183002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-10-17 23:04:59]

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-10-17 23:23:59]

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

[commit-bot: I haz the power, 2016-10-17 23:24:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sergey Ulanov, 2016-10-18 03:36:57]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-18 03:36:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2420183002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-10-18 03:37:14]

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-10-18 05:56:11]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-10-18 05:58:16]

Description was changed from

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.
==========

to

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Cr-Commit-Position: refs/heads/master@{#425895}
==========

[commit-bot: I haz the power, 2016-10-18 05:58:17]

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

[Guido Urdaneta, 2016-10-18 08:17:50]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.chromium.org/2431513003/ by guidou@chromium.org.

The reason for reverting is: This CL is suspect of breaking
ChromotingHostTest.ConnectWhenAnotherClientIsConnected on several bots.

See, for example,
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...

[ RUN      ] ChromotingHostTest.ConnectWhenAnotherClientIsConnected
[5433:5433:1018/003452:29645201974:INFO:chromoting_host.cc(107)] Starting host

GMOCK WARNING:
Uninteresting mock function call - returning directly.
    Function call: SetCapabilities(@0x7ffca86a1668 40-byte object <60-55 00-02
00-00 00-00 60-77 45-61 A6-3E 00-00 00-00 00-00 00-00 00-00 01-00 00-00 00-00
00-00 E0-75 45-61 A6-3E 00-00>)
NOTE: You can safely ignore the above warning unless this call should not
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean
to enforce the call.  See
http://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect for
details.
[5433:5433:1018/003452:29645202704:INFO:client_session.cc(336)] Client
disconnected: user@domain/rest-of-jid; error = 0
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/safe_iterator.518:
    error: attempt to compare a singular iterator to a past-the-end iterator    
    .

Objects involved in the operation:
iterator "lhs" @ 0x0x7ffca86a0d60 {
  state = singular;
  references sequence @ 0x0x7ffca86a0d60
}
iterator "rhs" @ 0x0x7ffca86a0d28 {
  state = past-the-end;
  references sequence @ 0x0x7ffca86a0d28
}
Received signal 6
#0 0x7f7299fb9ade base::debug::StackTrace::StackTrace()
#1 0x7f7299fb961f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f729a3f8cb0 <unknown>
#3 0x7f7294000035 gsignal
#4 0x7f729400379b abort
#5 0x7f72946005ad __gnu_debug::_Error_formatter::_M_error()
#6 0x00000123b828 __gnu_debug::operator!=<>()
#7 0x000001239ac5 remoting::ChromotingHost::OnSessionAuthenticated()
#8 0x0000012508fd remoting::ClientSession::OnConnectionAuthenticated()
#9 0x0000007534dd remoting::ChromotingHostTest::SimulateClientConnection()
#10 0x000000751eff
remoting::ChromotingHostTest_ConnectWhenAnotherClientIsConnected_Test::TestBody()
#11 0x0000006a00aa
_ZN7testing8internal12InvokeHelperIvSt5tupleIJEEE12InvokeMethodIN8remoting8protocol39MessageReaderTest_ReadFromCallback_TestEMNS7_17MessageReaderTestEFvvEEEvPT_T0_RKS3_
#12 0x000000a659fe testing::internal::HandleExceptionsInMethodIfSupported<>()
#13 0x000000a5a265 testing::Test::Run()
#14 0x000000a5aa08 testing::TestInfo::Run()
#15 0x000000a5afaa testing::TestCase::Run()
#16 0x000000a602ec testing::internal::UnitTestImpl::RunAllTests()
#17 0x000000a7b0ea testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#18 0x000000a6735e testing::internal::HandleExceptionsInMethodIfSupported<>()
#19 0x000000a5ff8f testing::UnitTest::Run()
#20 0x00000132a801 RUN_ALL_TESTS()
#21 0x000001329762 base::TestSuite::Run()
#22 0x0000004f23c9
_ZN4base8internal13FunctorTraitsIMN18OAuth2TokenService7FetcherEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_
#23 0x0000004f22f1
_ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN18OAuth2TokenService7FetcherEFvvEJPS5_EEEvOT_DpOT0_
#24 0x000000597457
_ZN4base8internal7InvokerINS0_9BindStateIMNS_9TestSuiteEFivEJNS0_17UnretainedWrapperIS3_EEEEEFivEE7RunImplIRKS5_RKSt5tupleIJS7_EEJLm0EEEEiOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#25 0x00000059739c
_ZN4base8internal7InvokerINS0_9BindStateIMNS_9TestSuiteEFivEJNS0_17UnretainedWrapperIS3_EEEEEFivEE3RunEPNS0_13BindStateBaseE
#26 0x00000051e6ab base::internal::RunMixin<>::Run()
#27 0x00000132c2a9 base::(anonymous namespace)::LaunchUnitTestsInternal()
#28 0x00000132c114 base::LaunchUnitTests()
#29 0x0000005971fe main
#30 0x7f7293feb7ed __libc_start_main
#31 0x0000004a0d85 <unknown>
  r8: 00007f728c0f2980  r9: 00007ffca86a0588 r10: 0000000000000008 r11:
0000000000000206
 r12: 00007ffca86a0a98 r13: 0000000000000038 r14: 0000000000000001 r15:
0000000000000000
  di: 0000000000001539  si: 0000000000001539  bp: 0000000000000002  bx:
00007ffca86a0a28
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp:
00007ffca86a05d8
  ip: 00007f7294000035 efl: 0000000000000206 cgf: 0000000000000033 erf:
0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
[772/772] ChromotingHostTest.ConnectWhenAnotherClientIsConnected (CRASHED)
1 test crashed:
    ChromotingHostTest.ConnectWhenAnotherClientIsConnected
(../../remoting/host/chromoting_host_unittest.cc:291)
Tests took 9 seconds..

[findit-for-me, 2016-10-18 08:22:02]

FYI: Findit identified this CL at revision 425895 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Sergey Ulanov, 2016-10-18 16:36:44]

Description was changed from

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Cr-Commit-Position: refs/heads/master@{#425895}
==========

to

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Cr-Commit-Position: refs/heads/master@{#425895}
==========

[Sergey Ulanov, 2016-10-18 16:52:06]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-18 16:52:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2420183002/#ps120001
(title: "Update ChromotingHost::OnSessionAuthenticated() to fix test crash")

[commit-bot: I haz the power, 2016-10-18 16:52:42]

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-10-18 17:19:49]

Description was changed from

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Cr-Commit-Position: refs/heads/master@{#425895}
==========

to

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Cr-Commit-Position: refs/heads/master@{#425895}
==========

[commit-bot: I haz the power, 2016-10-18 17:19:50]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-10-18 17:23:11]

Description was changed from

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Cr-Commit-Position: refs/heads/master@{#425895}
==========

to

==========
Don't use barcodes in ProtocolPerfTests

Previously the perf tests were using a unique barcode to identify video
frames when measuring latency. This is not longer necessary. Now the
tests will use fake input event timestamps and video stats messages to
measure latency.

Committed: https://crrev.com/69d9c4d4f5a66e0f7bcf02c40058d7d2e122300b
Committed: https://crrev.com/b047307ab561878d4357bdf7e617785e4dbd4f44
Cr-Original-Commit-Position: refs/heads/master@{#425895}
Cr-Commit-Position: refs/heads/master@{#425998}
==========

[commit-bot: I haz the power, 2016-10-18 17:23:13]

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