In screenshare mode, suppress VP8 bitrate overshoot and increase quality

webrtc/modules/video_coding/main/source/generic_encoder.cc

Index: webrtc/modules/video_coding/main/source/generic_encoder.cc
diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.cc b/webrtc/modules/video_coding/main/source/generic_encoder.cc
index 109150101f1a965b7a7982693191e9cd89ac25d8..6884b8dc5dc292eae37b512a2d76738158737877 100644
--- a/webrtc/modules/video_coding/main/source/generic_encoder.cc
+++ b/webrtc/modules/video_coding/main/source/generic_encoder.cc
@@ -60,7 +60,8 @@ VCMGenericEncoder::VCMGenericEncoder(VideoEncoder* encoder,
       bit_rate_(0),
       frame_rate_(0),
       internal_source_(internalSource),
-      rotation_(kVideoRotation_0) {
+      rotation_(kVideoRotation_0),
+      is_screenshare_(false) {
 }
 
 VCMGenericEncoder::~VCMGenericEncoder()
@@ -90,6 +91,7 @@ VCMGenericEncoder::InitEncode(const VideoCodec* settings,
       frame_rate_ = settings->maxFramerate;
     }
 
+    is_screenshare_ = settings->mode == VideoCodecMode::kScreensharing;
     if (encoder_->InitEncode(settings, numberOfCores, maxPayloadSize) != 0) {
       LOG(LS_ERROR) << "Failed to initialize the encoder associated with "
                        "payload name: " << settings->plName;
@@ -114,7 +116,17 @@ int32_t VCMGenericEncoder::Encode(const VideoFrame& inputFrame,
     vcm_encoded_frame_callback_->SetRotation(rotation_);
   }
 
-  return encoder_->Encode(inputFrame, codecSpecificInfo, &video_frame_types);
+  int32_t result =
+      encoder_->Encode(inputFrame, codecSpecificInfo, &video_frame_types);
+  if (is_screenshare_ && result == WEBRTC_VIDEO_CODEC_OK &&

[stefan-webrtc, 2015/06/18 09:39:02]

This is really hard to understand. What Does it do, and why? Seems to rely on
the underlying encoder being synchronous and finish encoding/return a frame
before it the Encode() call returns?

[sprang_webrtc, 2015/06/18 13:05:02]

Yes, this was an ad-hoc way to discover that the encoder had dropped a frame a
we needed to re-encode it. I've changed it so the encoder returns a special code
for this case instead.

[stefan-webrtc, 2015/06/23 14:28:32]

I'm wondering if it would be better to have this logic in vp8_impl.cc instead?
Or is there a reason to have it generically?

[sprang_webrtc, 2015/06/23 15:07:30]

It's more difficult since we don't know until after GetEncodedPartitions if this
happpened or not. We would need to wrap Encode inside a method just for this or
risk using recursion.
While not needed for any other impl, I think it is not inconceivable that we
would like to use this elsewhere? 

+      vcm_encoded_frame_callback_ != nullptr &&
+      !vcm_encoded_frame_callback_->GetAndResetFrameProduced()) {
+    result =
+        encoder_->Encode(inputFrame, codecSpecificInfo, &video_frame_types);
+    vcm_encoded_frame_callback_->GetAndResetFrameProduced();
+  }
+
+  return result;
 }
 
 int32_t
@@ -214,7 +226,8 @@ VCMEncodedFrameCallback::VCMEncodedFrameCallback(
       _payloadType(0),
       _internalSource(false),
       _rotation(kVideoRotation_0),
-      post_encode_callback_(post_encode_callback)
+      post_encode_callback_(post_encode_callback),
+      frame_produced_(false)
 #ifdef DEBUG_ENCODER_BIT_STREAM
       ,
       _bitStreamAfterEncoder(NULL)
@@ -245,6 +258,8 @@ int32_t VCMEncodedFrameCallback::Encoded(
     const RTPFragmentationHeader* fragmentationHeader) {
   post_encode_callback_->Encoded(encodedImage, NULL, NULL);
 
+  frame_produced_ = true;
+
   if (_sendCallback == NULL) {
     return VCM_UNINITIALIZED;
   }
@@ -285,4 +300,10 @@ VCMEncodedFrameCallback::SetMediaOpt(
     _mediaOpt = mediaOpt;
 }
 
+bool VCMEncodedFrameCallback::GetAndResetFrameProduced() {
+  bool frame_produced = frame_produced_;
+  frame_produced_ = false;
+  return frame_produced;
+}
+
 }  // namespace webrtc