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
Review-Url: https://codereview.chromium.org/2420183002
Cr-Original-Commit-Position: refs/heads/master@{#425895}
Cr-Commit-Position: refs/heads/master@{#425998}

remoting/host/chromoting_host.cc

@@ -177,30 +177,24 @@ void ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
 
   login_backoff_.Reset();
 
-  // Disconnect all other clients. |it| should be advanced before Disconnect()
-  // is called to avoid it becoming invalid when the client is removed from
-  // the list.
-  ClientList::iterator it = clients_.begin();
+  // Disconnect all clients, except |client|.
   base::WeakPtr<ChromotingHost> self = weak_factory_.GetWeakPtr();
-  while (it != clients_.end()) {
-    ClientSession* other_client = *it++;
-    if (other_client != client) {
-      other_client->DisconnectSession(protocol::OK);
-
-      // Quit if the host was destroyed.
-      if (!self)
-        return;
-    }
+  while (clients_.size() > 1) {
+    clients_[(clients_.front().get() == client) ? 1 : 0]->DisconnectSession(
+        protocol::OK);
+
+    // Quit if the host was destroyed.
+    if (!self)
+      return;
   }
 
   // Disconnects above must have destroyed all other clients.
   DCHECK_EQ(clients_.size(), 1U);
+  DCHECK(clients_.front().get() == client);
 
   // Notify observers that there is at least one authenticated client.
-  const std::string& jid = client->client_jid();
-
   for (auto& observer : status_observers_)
-    observer.OnClientAuthenticated(jid);
+    observer.OnClientAuthenticated(client->client_jid());
 }
 
 void ChromotingHost::OnSessionChannelsConnected(ClientSession* client) {
@@ -222,13 +216,16 @@ void ChromotingHost::OnSessionAuthenticationFailed(ClientSession* client) {
 void ChromotingHost::OnSessionClosed(ClientSession* client) {
   DCHECK(CalledOnValidThread());
 
-  ClientList::iterator it = std::find(clients_.begin(), clients_.end(), client);
+  ClientSessions::iterator it =
+      std::find_if(clients_.begin(), clients_.end(),
+                   [client](const std::unique_ptr<ClientSession>& item) {
+                     return item.get() == client;
+                   });
   CHECK(it != clients_.end());
 
   bool was_authenticated = client->is_authenticated();
   std::string jid = client->client_jid();
   clients_.erase(it);
-  delete client;
 
   if (was_authenticated) {
     for (auto& observer : status_observers_)
@@ -277,11 +274,9 @@ void ChromotingHost::OnIncomingSession(
   }
 
   // Create a ClientSession object.
-  ClientSession* client = new ClientSession(
+  clients_.push_back(base::MakeUnique<ClientSession>(
       this, std::move(connection), desktop_environment_factory_,
-      max_session_duration_, pairing_registry_, extensions_.get());
-
-  clients_.push_back(client);
+      max_session_duration_, pairing_registry_, extensions_.get()));
 }
 
 void ChromotingHost::DisconnectAllClients() {

remoting/host/chromoting_host.h

@@ -5,9 +5,9 @@
 #ifndef REMOTING_HOST_CHROMOTING_HOST_H_
 #define REMOTING_HOST_CHROMOTING_HOST_H_
 
-#include <list>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -66,6 +66,8 @@ class ChromotingHost : public base::NonThreadSafe,
                        public ClientSession::EventHandler,
                        public HostStatusMonitor {
  public:
+  typedef std::vector<std::unique_ptr<ClientSession>> ClientSessions;
+
   // |desktop_environment_factory| must outlive this object.
   ChromotingHost(
       DesktopEnvironmentFactory* desktop_environment_factory,
@@ -133,14 +135,15 @@ class ChromotingHost : public base::NonThreadSafe,
     pairing_registry_ = pairing_registry;
   }
 
+  const ClientSessions& client_sessions_for_tests() { return clients_; }
+
   base::WeakPtr<ChromotingHost> AsWeakPtr() {
     return weak_factory_.GetWeakPtr();
   }
 
  private:
   friend class ChromotingHostTest;
 
-  typedef std::list<ClientSession*> ClientList;
   typedef ScopedVector<HostExtension> HostExtensionList;
 
   // Immediately disconnects all active clients. Host-internal components may
@@ -162,7 +165,7 @@ class ChromotingHost : public base::NonThreadSafe,
   base::ObserverList<HostStatusObserver> status_observers_;
 
   // The connections to remote clients.
-  ClientList clients_;
+  ClientSessions clients_;
 
   // True if the host has been started.
   bool started_;

remoting/host/chromoting_host_unittest.cc

@@ -136,7 +136,7 @@ class ChromotingHostTest : public testing::Test {
     get_client(connection_index) = client_ptr;
 
     // |host| is responsible for deleting |client| from now on.
-    host_->clients_.push_back(client.release());
+    host_->clients_.push_back(std::move(client));
 
     if (authenticate) {
       client_ptr->OnConnectionAuthenticated();
@@ -244,11 +244,6 @@ class ChromotingHostTest : public testing::Test {
     return (connection_index == 0) ? client1_ : client2_;
   }
 
-  // Returns the list of clients of the host_.
-  std::list<ClientSession*>& get_clients_from_host() {
-    return host_->clients_;
-  }
-
   const std::string& get_session_jid(int connection_index) {
     return (connection_index == 0) ? session_jid1_ : session_jid2_;
   }
@@ -338,7 +333,8 @@ TEST_F(ChromotingHostTest, LoginBackOffUponConnection) {
   host_->OnIncomingSession(session_unowned1_.release(), &response);
   EXPECT_EQ(protocol::SessionManager::ACCEPT, response);
 
-  host_->OnSessionAuthenticating(get_clients_from_host().front());
+  host_->OnSessionAuthenticating(
+      host_->client_sessions_for_tests().front().get());
   host_->OnIncomingSession(session_unowned2_.get(), &response);
   EXPECT_EQ(protocol::SessionManager::OVERLOAD, response);
 }
@@ -362,13 +358,15 @@ TEST_F(ChromotingHostTest, LoginBackOffUponAuthenticating) {
   EXPECT_EQ(protocol::SessionManager::ACCEPT, response);
 
   // This will set the backoff.
-  host_->OnSessionAuthenticating(get_clients_from_host().front());
+  host_->OnSessionAuthenticating(
+      host_->client_sessions_for_tests().front().get());
 
   // This should disconnect client2.
-  host_->OnSessionAuthenticating(get_clients_from_host().back());
+  host_->OnSessionAuthenticating(
+      host_->client_sessions_for_tests().back().get());
 
   // Verify that the host only has 1 client at this point.
-  EXPECT_EQ(get_clients_from_host().size(), 1U);
+  EXPECT_EQ(host_->client_sessions_for_tests().size(), 1U);
 }
 
 TEST_F(ChromotingHostTest, OnSessionRouteChange) {

remoting/host/client_session.cc

@@ -295,6 +295,9 @@ void ClientSession::CreateMediaStreams() {
 
   // Pause capturing if necessary.
   video_stream_->Pause(pause_video_);
+
+  if (event_timestamp_source_for_tests_)
+    video_stream_->SetEventTimestampsSource(event_timestamp_source_for_tests_);
 }
 
 void ClientSession::OnConnectionChannelsConnected() {
@@ -404,9 +407,17 @@ ClientSessionControl* ClientSession::session_control() {
   return this;
 }
 
-std::unique_ptr<protocol::ClipboardStub> ClientSession::CreateClipboardProxy() {
+void ClientSession::SetEventTimestampsSourceForTests(
+    scoped_refptr<protocol::InputEventTimestampsSource>
+        event_timestamp_source) {
   DCHECK(CalledOnValidThread());
+  event_timestamp_source_for_tests_ = event_timestamp_source;
+  if (video_stream_)
+    video_stream_->SetEventTimestampsSource(event_timestamp_source_for_tests_);
+}
 
+std::unique_ptr<protocol::ClipboardStub> ClientSession::CreateClipboardProxy() {
+  DCHECK(CalledOnValidThread());
   return base::MakeUnique<protocol::ClipboardThreadProxy>(
       client_clipboard_factory_.GetWeakPtr(),
       base::ThreadTaskRunnerHandle::Get());

remoting/host/client_session.h

@@ -140,6 +140,10 @@ class ClientSession : public base::NonThreadSafe,
     return client_capabilities_.get();
   }
 
+  void SetEventTimestampsSourceForTests(
+      scoped_refptr<protocol::InputEventTimestampsSource>
+          event_timestamp_source);
+
  private:
   // Creates a proxy for sending clipboard events to the client.
   std::unique_ptr<protocol::ClipboardStub> CreateClipboardProxy();
@@ -237,6 +241,9 @@ class ClientSession : public base::NonThreadSafe,
   // then it's stored in |pending_video_layout_message_|.
   std::unique_ptr<protocol::VideoLayout> pending_video_layout_message_;
 
+  scoped_refptr<protocol::InputEventTimestampsSource>
+      event_timestamp_source_for_tests_;
+
   // Used to disable callbacks to |this| once DisconnectSession() has been
   // called.
   base::WeakPtrFactory<ClientSessionControl> weak_factory_;

remoting/test/cyclic_frame_generator.cc

@@ -48,6 +48,7 @@ void CyclicFrameGenerator::SetTickClock(base::TickClock* tick_clock) {
 std::unique_ptr<webrtc::DesktopFrame> CyclicFrameGenerator::GenerateFrame(
     webrtc::SharedMemoryFactory* shared_memory_factory) {
   base::TimeTicks now = clock_->NowTicks();
+
   int frame_id = (now - started_time_) / cursor_blink_period_;
   int reference_frame =
       ((now - started_time_) / frame_cycle_period_) % reference_frames_.size();
@@ -80,20 +81,15 @@ std::unique_ptr<webrtc::DesktopFrame> CyclicFrameGenerator::GenerateFrame(
     last_frame_type_ = ChangeType::NO_CHANGES;
   }
 
-  if (draw_barcode_)
-    DrawBarcode(frame_id, frame_id != last_frame_id_, frame.get());
-
   last_reference_frame_ = reference_frame;
   last_cursor_state_ = cursor_state;
-  last_frame_id_ = frame_id;
 
   return frame;
 }
 
 CyclicFrameGenerator::ChangeInfoList CyclicFrameGenerator::GetChangeList(
-    webrtc::DesktopFrame* frame) {
-  CHECK(draw_barcode_);
-  int frame_id = ReadBarcode(*frame);
+    base::TimeTicks timestamp) {
+  int frame_id = (timestamp - started_time_) / cursor_blink_period_;
   CHECK_GE(frame_id, last_identifier_frame_);
 
   ChangeInfoList result;
@@ -110,5 +106,10 @@ CyclicFrameGenerator::ChangeInfoList CyclicFrameGenerator::GetChangeList(
   return result;
 }
 
+protocol::InputEventTimestamps CyclicFrameGenerator::TakeLastEventTimestamps() {
+  base::TimeTicks now = clock_->NowTicks();
+  return protocol::InputEventTimestamps{now, now};
+}
+
 }  // namespace test
 }  // namespace remoting

remoting/test/cyclic_frame_generator.h

@@ -9,8 +9,10 @@
 #include <memory>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/time/default_tick_clock.h"
+#include "remoting/protocol/input_event_timestamps.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
 
 namespace remoting {
@@ -21,8 +23,7 @@ namespace test {
 // loads a sequence of reference frames and switches between them with the
 // specified frequency (every 2 seconds by default). Between reference frames it
 // also generate frames with small changes which simulate a blinking cursor.
-class CyclicFrameGenerator
-    : public base::RefCountedThreadSafe<CyclicFrameGenerator> {
+class CyclicFrameGenerator : public protocol::InputEventTimestampsSource {
  public:
   enum class ChangeType {
     // No changes.
@@ -60,25 +61,22 @@ class CyclicFrameGenerator
 
   void SetTickClock(base::TickClock* tick_clock);
 
-  // When |draw_barcode| is set to true a barcode is drawn on each generated
-  // frame. This makes it possible to call GetChangeList() to identify the frame
-  // by its content.
-  void set_draw_barcode(bool draw_barcode) { draw_barcode_ = draw_barcode; }
-
   std::unique_ptr<webrtc::DesktopFrame> GenerateFrame(
       webrtc::SharedMemoryFactory* shared_memory_factory);
 
   ChangeType last_frame_type() { return last_frame_type_; }
 
-  // Identifies |frame| by its content and returns list of ChangeInfo for all
-  // changes between the frame passed to the previous GetChangeList() call and
-  // this one. GetChangeList() must be called for the frames in the order in
-  // which they were received, which is expected to match the order in which
-  // they are generated.
-  ChangeInfoList GetChangeList(webrtc::DesktopFrame* frame);
+  // Returns list of changes for the frame with the specified |timestamp|, which
+  // should be an event timestamp generated by InputEventTimestampsSource
+  // implemented in this class. Must be called in the same order in which frames
+  // were generated.
+  ChangeInfoList GetChangeList(base::TimeTicks timestamp);
+
+  // InputEventTimestampsSource interface.
+  protocol::InputEventTimestamps TakeLastEventTimestamps() override;
 
  private:
-  ~CyclicFrameGenerator();
+  ~CyclicFrameGenerator() override;
   friend class base::RefCountedThreadSafe<CyclicFrameGenerator>;
 
   std::vector<std::unique_ptr<webrtc::DesktopFrame>> reference_frames_;
@@ -92,9 +90,6 @@ class CyclicFrameGenerator
   // By default blink the cursor 4 times per seconds.
   base::TimeDelta cursor_blink_period_ = base::TimeDelta::FromMilliseconds(250);
 
-  // Id of the last frame encoded on the barcode.
-  int last_frame_id_ = -1;
-
   // Index of the reference frame used to render the last generated frame.
   int last_reference_frame_ = -1;
 
@@ -103,8 +98,6 @@ class CyclicFrameGenerator
 
   ChangeType last_frame_type_ = ChangeType::NO_CHANGES;
 
-  bool draw_barcode_ = false;
-
   base::TimeTicks started_time_;
 
   // frame_id of the frame passed to the last GetChangeList() call.

remoting/test/frame_generator_util.cc

@@ -15,14 +15,6 @@
 namespace remoting {
 namespace test {
 
-namespace {
-const int kBarcodeCellWidth = 8;
-const int kBarcodeCellHeight = 8;
-const int kBarcodeBits = 10;
-const int kBarcodeBlackThreshold = 85;
-const int kBarcodeWhiteThreshold = 170;
-}  // namespace
-
 std::unique_ptr<webrtc::DesktopFrame> LoadDesktopFrameFromPng(
     const char* name) {
   base::FilePath file_path;
@@ -59,47 +51,5 @@ void DrawRect(webrtc::DesktopFrame* frame,
   }
 }
 
-void DrawBarcode(int value, bool changed, webrtc::DesktopFrame* frame) {
-  CHECK(value < (1 << kBarcodeBits));
-  for (int i = 0; i < kBarcodeBits; ++i) {
-    DrawRect(frame, webrtc::DesktopRect::MakeXYWH(i * kBarcodeCellWidth, 0,
-                                                  kBarcodeCellWidth,
-                                                  kBarcodeCellHeight),
-             (value & 1) ? 0xffffffff : 0xff000000);
-    value >>= 1;
-  }
-  if (changed) {
-    frame->mutable_updated_region()->AddRect(webrtc::DesktopRect::MakeXYWH(
-        0, 0, kBarcodeCellWidth * kBarcodeBits, kBarcodeCellHeight));
-  }
-}
-
-int ReadBarcode(const webrtc::DesktopFrame& frame) {
-  int result = 0;
-  for (int i = kBarcodeBits - 1; i >= 0; --i) {
-    // Sample barcode in the center of the cell for each bit.
-    int x = i * kBarcodeCellWidth + kBarcodeCellWidth / 2;
-    int y = kBarcodeCellHeight / 2;
-    uint8_t* data = frame.GetFrameDataAtPos(webrtc::DesktopVector(x, y));
-    int b = data[0];
-    int g = data[1];
-    int r = data[2];
-    bool bit = 0;
-    if (b > kBarcodeWhiteThreshold && g > kBarcodeWhiteThreshold &&
-        r > kBarcodeWhiteThreshold) {
-      bit = 1;
-    } else if (b < kBarcodeBlackThreshold && g < kBarcodeBlackThreshold &&
-        r < kBarcodeBlackThreshold) {
-      bit = 0;
-    } else {
-      LOG(FATAL) << "Invalid barcode.";
-    }
-    result <<= 1;
-    if (bit)
-      result |= 1;
-  }
-  return result;
-}
-
 }  // namespace test
 }  // namespace remoting