Fix WebrtcVideoStream to handle failed capture requests.

remoting/protocol/connection_unittest.cc

Index: remoting/protocol/connection_unittest.cc
diff --git a/remoting/protocol/connection_unittest.cc b/remoting/protocol/connection_unittest.cc
index 1217aea7b3673ad11c6a521940ccbc90c54372b8..56606bd2c4705fc50745cdd724cc11ce99bcb6a8 100644
--- a/remoting/protocol/connection_unittest.cc
+++ b/remoting/protocol/connection_unittest.cc
@@ -76,19 +76,25 @@ class TestScreenCapturer : public webrtc::DesktopCapturer {
   void Start(Callback* callback) override {
     callback_ = callback;
   }
+
   void CaptureFrame() override {
+    if (capture_request_index_to_fail_ >= 0) {
+      capture_request_index_to_fail_--;
+      if (capture_request_index_to_fail_ < 0) {
+        callback_->OnCaptureResult(
+            webrtc::DesktopCapturer::Result::ERROR_TEMPORARY, nullptr);
+        return;
+      }
+    }
+
     // Return black 100x100 frame.
     std::unique_ptr<webrtc::DesktopFrame> frame(
         new webrtc::BasicDesktopFrame(webrtc::DesktopSize(100, 100)));
-    memset(frame->data(), 0, frame->stride() * frame->size().height());
-
-    // Set updated_region only for the first frame, as the frame content
-    // doesn't change.
-    if (!first_frame_sent_) {
-      first_frame_sent_ = true;
-      frame->mutable_updated_region()->SetRect(
-          webrtc::DesktopRect::MakeSize(frame->size()));
-    }
+    memset(frame->data(), frame_index_,
+           frame->stride() * frame->size().height());
+    frame_index_++;
+    frame->mutable_updated_region()->SetRect(
+        webrtc::DesktopRect::MakeSize(frame->size()));
 
     callback_->OnCaptureResult(webrtc::DesktopCapturer::Result::SUCCESS,
                                std::move(frame));
@@ -102,9 +108,13 @@ class TestScreenCapturer : public webrtc::DesktopCapturer {
     return true;
   }
 
+  void FailNthFrame(int n) { capture_request_index_to_fail_ = n; }
+
  private:
   Callback* callback_ = nullptr;
-  bool first_frame_sent_ = false;
+  int frame_index_ = 0;
+
+  int capture_request_index_to_fail_ = -1;
 };
 
 static const int kAudioSampleRate = AudioPacket::SAMPLING_RATE_48000;
@@ -346,7 +356,14 @@ class ConnectionTest : public testing::Test,
       run_loop_->Quit();
   }
 
-  void WaitFirstVideoFrame() {
+  void WaitNextVideoFrame() {
+    size_t received_frames =
+        is_using_webrtc()
+            ? client_video_renderer_.GetFrameConsumer()
+                  ->received_frames()
+                  .size()
+            : client_video_renderer_.GetVideoStub()->received_packets().size();
+
     base::RunLoop run_loop;
 
     // Expect frames to be passed to FrameConsumer when WebRTC is used, or to
@@ -364,7 +381,7 @@ class ConnectionTest : public testing::Test,
     if (is_using_webrtc()) {
       EXPECT_EQ(
           client_video_renderer_.GetFrameConsumer()->received_frames().size(),
-          1U);
+          received_frames + 1);
       EXPECT_EQ(
           client_video_renderer_.GetVideoStub()->received_packets().size(), 0U);
       client_video_renderer_.GetFrameConsumer()->set_on_frame_callback(
@@ -374,7 +391,8 @@ class ConnectionTest : public testing::Test,
           client_video_renderer_.GetFrameConsumer()->received_frames().size(),
           0U);
       EXPECT_EQ(
-          client_video_renderer_.GetVideoStub()->received_packets().size(), 1U);
+          client_video_renderer_.GetVideoStub()->received_packets().size(),
+          received_frames + 1);
       client_video_renderer_.GetVideoStub()->set_on_frame_callback(
           base::Closure());
     }
@@ -494,7 +512,7 @@ TEST_P(ConnectionTest, Video) {
       host_connection_->StartVideoStream(
           base::MakeUnique<TestScreenCapturer>());
 
-  WaitFirstVideoFrame();
+  WaitNextVideoFrame();
 }
 
 // Verifies that the VideoStream doesn't loose any video frames while the
@@ -510,7 +528,7 @@ TEST_P(ConnectionTest, VideoWithSlowSignaling) {
       host_connection_->StartVideoStream(
           base::WrapUnique(new TestScreenCapturer()));
 
-  WaitFirstVideoFrame();
+  WaitNextVideoFrame();
 }
 
 TEST_P(ConnectionTest, DestroyOnIncomingMessage) {
@@ -554,7 +572,7 @@ TEST_P(ConnectionTest, VideoStats) {
           base::MakeUnique<TestScreenCapturer>());
   video_stream->SetEventTimestampsSource(input_event_timestamps_source);
 
-  WaitFirstVideoFrame();
+  WaitNextVideoFrame();
 
   base::TimeTicks finish_time = base::TimeTicks::Now();
 
@@ -597,5 +615,49 @@ TEST_P(ConnectionTest, Audio) {
   client_audio_player_.Verify();
 }
 
+TEST_P(ConnectionTest, VideoFirstFrameFailed) {
+  Connect();
+
+  base::TimeTicks event_timestamp = base::TimeTicks::FromInternalValue(42);
+
+  scoped_refptr<InputEventTimestampsSourceImpl> input_event_timestamps_source =
+      new InputEventTimestampsSourceImpl();
+  input_event_timestamps_source->OnEventReceived(
+      InputEventTimestamps{event_timestamp, base::TimeTicks::Now()});
+
+  auto capturer = base::MakeUnique<TestScreenCapturer>();
+  capturer->FailNthFrame(0);
+  auto video_stream = host_connection_->StartVideoStream(std::move(capturer));
+  video_stream->SetEventTimestampsSource(input_event_timestamps_source);
+
+  WaitNextVideoFrame();
+
+  // Currently stats work in this test only for WebRTC because for ICE
+  // connections stats are reported by SoftwareVideoRenderer which is not used
+  // in this test.
+  // TODO(sergeyu): Fix this.
+  if (is_using_webrtc())  {
+    WaitFirstFrameStats();
+
+    // Verify that the event timestamp received before the first frame gets used
+    // for the second frame.
+    const FrameStats& stats = client_video_renderer_.GetFrameStatsConsumer()
+                                  ->received_stats()
+                                  .front();
+    EXPECT_EQ(event_timestamp, stats.host_stats.latest_event_timestamp);
+  }
+}
+
+TEST_P(ConnectionTest, VideoSecondFrameFailed) {
+  Connect();
+
+  auto capturer = base::MakeUnique<TestScreenCapturer>();
+  capturer->FailNthFrame(1);
+  auto video_stream = host_connection_->StartVideoStream(std::move(capturer));
+
+  WaitNextVideoFrame();
+  WaitNextVideoFrame();

[joedow, 2017/01/09 20:00:28]

Would it make sense to wait one more time to make sure the third frame is
received correctly (after receiving a failure on the second)?

[Sergey Ulanov, 2017/01/09 22:24:06]

WaitNextVideoFrame() waits for a frame on the receiving side. Second Capture()
request fails on the sender, but the next one should succeed, and that's the
frame we get here. Renamed these tests to make it clearer

+}
+
 }  // namespace protocol
 }  // namespace remoting