[remoting host] Guard against encoding multiple frames in parallel.

With the standard encoding pipeline, WebRTC will occasionally try to
encode another frame while a frame is being encoded (before the
registered encode-complete callback is run).

The remoting/codec encoders have not been developed or tested for
multiple parallel encodes. The frame-scheduler currently waits for
encode-complete before scheduling a new capture, but this will
change with standard-encoding-pipeline. So this CL protects this by
dropping any Encode() requests while a frame is still pending.

This will also ensure that the stored RTP timestamp, and per-frame
stats (when this is implemented) are valid for the frame, as there
can now be only 1 frame encoded at a time.

Bug: 1192865
Change-Id: I31cff4aea119d32e23aa7a130f43e759cf5510a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2911514
Auto-Submit: Lambros Lambrou <lambroslambrou@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#885796}

remoting/protocol/webrtc_video_encoder_wrapper.cc

@@ -141,6 +141,15 @@ int32_t WebrtcVideoEncoderWrapper::Encode(
     const std::vector<webrtc::VideoFrameType>* frame_types) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  if (encode_pending_) {
+    LOG(WARNING) << "Encoder busy, dropping frame.";
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&WebrtcVideoEncoderWrapper::NotifyFrameDropped,
+                       weak_factory_.GetWeakPtr()));
+    return WEBRTC_VIDEO_CODEC_OK;
+  }
+
   // Frames of type kNative are expected to have the adapter that was used to
   // wrap the DesktopFrame, so the downcast should be safe.
   if (frame.video_frame_buffer()->type() !=
@@ -179,6 +188,8 @@ int32_t WebrtcVideoEncoderWrapper::Encode(
       (frame_types && !frame_types->empty() &&
        ((*frame_types)[0] == webrtc::VideoFrameType::kVideoFrameKey));
 
+  encode_pending_ = true;
+
   // Just in case the encoder runs the callback on an arbitrary thread,
   // BindPostTask() is used here to trampoline onto the correct thread.
   // This object is bound via a WeakPtr which must only be dereferenced
@@ -299,6 +310,9 @@ void WebrtcVideoEncoderWrapper::OnFrameEncoded(
       frame(encoded_frame.release(),
             base::OnTaskRunnerDeleter(main_task_runner_));
 
+  DCHECK(encode_pending_);
+  encode_pending_ = false;
+
   main_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&VideoChannelStateObserver::OnFrameEncoded,
                                 video_channel_state_observer_, encode_result,
@@ -310,11 +324,13 @@ void WebrtcVideoEncoderWrapper::OnFrameEncoded(
     // return any error, but hardware-decoders such as H264 may fail.
     LOG(ERROR) << "Video encoder returned error "
                << EncodeResultToString(encode_result);
+    NotifyFrameDropped();
     return;
   }
 
   if (!frame || frame->data.empty()) {
     SetTopOffActive(false);
+    NotifyFrameDropped();
     return;
   }
 
@@ -331,6 +347,13 @@ void WebrtcVideoEncoderWrapper::OnFrameEncoded(
                      video_channel_state_observer_, send_result, *frame));
 }
 
+void WebrtcVideoEncoderWrapper::NotifyFrameDropped() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(encoded_callback_);
+  encoded_callback_->OnDroppedFrame(
+      webrtc::EncodedImageCallback::DropReason::kDroppedByEncoder);
+}
+
 void WebrtcVideoEncoderWrapper::SetTopOffActive(bool active) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 

remoting/protocol/webrtc_video_encoder_wrapper.h

@@ -57,6 +57,9 @@ class WebrtcVideoEncoderWrapper : public webrtc::VideoEncoder {
   void OnFrameEncoded(WebrtcVideoEncoder::EncodeResult encode_result,
                       std::unique_ptr<WebrtcVideoEncoder::EncodedFrame> frame);
 
+  // Notifies WebRTC that this encoder has dropped a frame.
+  void NotifyFrameDropped();
+
   // Sets whether top-off is active, and fires a notification if the setting
   // changes.
   void SetTopOffActive(bool active);
@@ -82,6 +85,10 @@ class WebrtcVideoEncoderWrapper : public webrtc::VideoEncoder {
 
   webrtc::VideoCodecType codec_type_ GUARDED_BY_CONTEXT(sequence_checker_);
 
+  // True when a frame is being encoded. This guards against encoding multiple
+  // frames in parallel, which the encoders are not prepared to handle.
+  bool encode_pending_ GUARDED_BY_CONTEXT(sequence_checker_) = false;
+
   // TaskRunner used for notifying |video_channel_state_observer_|.
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 

remoting/protocol/webrtc_video_encoder_wrapper_unittest.cc

@@ -213,5 +213,21 @@ TEST_F(WebrtcVideoEncoderWrapperTest, NotifiesFrameEncodedAndReturned) {
   PostQuitAndRun();
 }
 
+TEST_F(WebrtcVideoEncoderWrapperTest, FrameDroppedIfEncoderBusy) {
+  EXPECT_CALL(callback_, OnEncodedImage(_, Field(&CodecSpecificInfo::codecType,
+                                                 kVideoCodecVP9)))
+      .WillOnce(Return(kResultOk));
+
+  auto frame1 = MakeVideoFrame();
+  auto frame2 = MakeVideoFrame();
+  auto encoder = InitEncoder(GetVp9Format(), GetVp9Codec());
+  std::vector<VideoFrameType> frame_types;
+  frame_types.push_back(VideoFrameType::kVideoFrameKey);
+  encoder->Encode(frame1, &frame_types);
+  encoder->Encode(frame1, &frame_types);
+
+  PostQuitAndRun();
+}
+
 }  // namespace protocol
 }  // namespace remoting