Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants.

webrtc/media/engine/webrtcvideoengine2.cc

Index: webrtc/media/engine/webrtcvideoengine2.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index 0dd8938dc3451758fb0441cb4df457baa6832abe..7e754a789fea5cd4aa3f0f3ea3e35a1ee4fc68e2 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -14,6 +14,7 @@
 #include <set>
 #include <string>
 
+#include "webrtc/base/asyncinvoker.h"
 #include "webrtc/base/buffer.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/stringutils.h"
@@ -314,6 +315,7 @@ static int GetMaxDefaultVideoBitrateKbps(int width, int height) {
     return 2500;
   }
 }
+
 }  // namespace
 
 // Constants defined in webrtc/media/engine/constants.h
@@ -332,6 +334,8 @@ static const int kDefaultQpMax = 56;
 
 static const int kDefaultRtcpReceiverReportSsrc = 1;
 
+static const int kMaxCpuDowngrades = 2;  // Downgrade at most 2 times for CPU.
+
 std::vector<VideoCodec> DefaultVideoCodecList() {
   std::vector<VideoCodec> codecs;
   codecs.push_back(MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp8PlType,
@@ -994,13 +998,10 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) {
     send_ssrcs_.insert(used_ssrc);
 
   webrtc::VideoSendStream::Config config(this);
-  config.overuse_callback = this;
-
   WebRtcVideoSendStream* stream = new WebRtcVideoSendStream(
       call_, sp, config, external_encoder_factory_, options_,
-      bitrate_config_.max_bitrate_bps, send_codec_, send_rtp_extensions_,
-      send_params_);
-
+      signal_cpu_adaptation_, bitrate_config_.max_bitrate_bps, send_codec_,
+      send_rtp_extensions_, send_params_);
   uint32_t ssrc = sp.first_ssrc();
   RTC_DCHECK(ssrc != 0);
   send_streams_[ssrc] = stream;
@@ -1398,26 +1399,6 @@ void WebRtcVideoChannel2::SetInterface(NetworkInterface* iface) {
                           kVideoRtpBufferSize);
 }
 
-void WebRtcVideoChannel2::OnLoadUpdate(Load load) {
-  // OnLoadUpdate can not take any locks that are held while creating streams
-  // etc. Doing so establishes lock-order inversions between the webrtc process
-  // thread on stream creation and locks such as stream_crit_ while calling out.
-  rtc::CritScope stream_lock(&capturer_crit_);
-  if (!signal_cpu_adaptation_)
-    return;
-  // Do not adapt resolution for screen content as this will likely result in
-  // blurry and unreadable text.
-  for (auto& kv : capturers_) {
-    if (kv.second != nullptr
-        && !kv.second->IsScreencast()
-        && kv.second->video_adapter() != nullptr) {
-      kv.second->video_adapter()->OnCpuResolutionRequest(
-          load == kOveruse ? CoordinatedVideoAdapter::DOWNGRADE
-                           : CoordinatedVideoAdapter::UPGRADE);
-    }
-  }
-}
-
 bool WebRtcVideoChannel2::SendRtp(const uint8_t* data,
                                   size_t len,
                                   const webrtc::PacketOptions& options) {
@@ -1476,12 +1457,56 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::AllocatedEncoder::AllocatedEncoder(
   }
 }
 
+class WebRtcVideoChannel2::WebRtcVideoSendStream::LoadObserverProxy {
+ public:
+  explicit LoadObserverProxy(webrtc::LoadObserver* observer) {
+    helper_ = new rtc::RefCountedObject<Helper>(observer);
+  }
+  ~LoadObserverProxy() { helper_->Detach(); }
+
+  webrtc::LoadObserver* proxy() { return helper_; }
+
+ private:
+  class Helper : public webrtc::LoadObserver, public rtc::RefCountInterface {
+   public:
+    explicit Helper(webrtc::LoadObserver* observer)
+        : thread_(rtc::Thread::Current()), observer_(observer) {}
+    void Detach() {
+      RTC_DCHECK(thread_checker_.CalledOnValidThread());
+      observer_ = nullptr;
+    }
+    void OnLoadUpdate(webrtc::LoadObserver::Load load) override {
+      if (rtc::Thread::Current() == thread_) {
+        observer_->OnLoadUpdate(load);
+        return;
+      }
+      invoker_.AsyncInvoke<void>(
+          thread_, rtc::Bind(&Helper::OnLoadUpdateOnCorrectThread, this, load));
+    }
+    void OnLoadUpdateOnCorrectThread(webrtc::LoadObserver::Load load) {
+      RTC_DCHECK(thread_checker_.CalledOnValidThread());
+      if (observer_) {
+        observer_->OnLoadUpdate(load);
+      }
+    }
+
+   private:
+    rtc::ThreadChecker thread_checker_;
+    rtc::AsyncInvoker invoker_;
+    rtc::Thread* thread_;
+    webrtc::LoadObserver* observer_;
+  };
+
+  rtc::scoped_refptr<Helper> helper_;
+};
+

[pthatcher1, 2016/02/17 23:05:56]

Can you explain why this proxy is necessary?

[perkj_webrtc, 2016/02/18 13:22:46]

OnLoad is called on a media engine thread. Not the worker thread.
So it is to not have to make the videobroadcaster have to take care of
AddOrUpdateSink calls from multiple threads + probably in the near future -
accept frames to be delivered on a third thread.

[pthatcher1, 2016/02/19 06:08:39]

It seems like a lot of complexity to add.

And I think we need to allow AddOrUpdateSink to be callable from any thread
anyway.   Otherwise, the caller needs to know what thread is has to call it on,
which I think would be a mess.  Shouldn't that be pretty simple with a
CriticalSection?

Similarly,  I think we should allow OnFrame to be called from any thread, and
have a sink do a thread-hop or lock if it needs to.  Several places we already
do that, because frames can come from random threads on different mobile OSes.  

So, can we just make both OnFrame and AddOrUpdateSink thread-safe, and not have
to deal with this proxy complexity?

[perkj_webrtc, 2016/02/19 14:57:52]

I agreee with OnFrame as said before. 


I don't so much like having AddOrUpdateSink and RemoveSink be called on
arbitrary threads..

If AddOrUpdateSink is called on two threads for the same sink - it feels like
you can end up in a weird state. Ex- CPU is low so the CPU monitor allows the
sink to increase the bitrate. But the encoder thread say that the produced
quality is awesome - so it thinks it can produce frames with a higher resolution
and calls AddOrUpdateSink on another tread... Somewhere these two requests
should be coordinated. I agree that is does not necessarily has to be here. 

AddOrUpdateSink for two separate sinks on two separate threads might make more
sence. But it means that the source implementation must be thread safe too so
that it can handle the changed sinkwants.

Also- pbos pointed out a possible problem of holding |lock_| in
webrtcvideosendstream::onload from a tread the webrtc engine. That might cause a
possible deadlock when the webrtcvideosendstream is reconstructed. 

 WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
     webrtc::Call* call,
     const StreamParams& sp,
     const webrtc::VideoSendStream::Config& config,
     WebRtcVideoEncoderFactory* external_encoder_factory,
     const VideoOptions& options,
+    bool enable_cpu_overuse_detection,
     int max_bitrate_bps,
     const rtc::Optional<VideoCodecSettings>& codec_settings,
     const std::vector<webrtc::RtpExtension>& rtp_extensions,
@@ -1491,6 +1516,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
     : ssrcs_(sp.ssrcs),
       ssrc_groups_(sp.ssrc_groups),
       call_(call),
+      load_proxy_(new LoadObserverProxy(this)),
       external_encoder_factory_(external_encoder_factory),
       stream_(NULL),
       parameters_(config, options, max_bitrate_bps, codec_settings),
@@ -1512,6 +1538,8 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
   parameters_.config.rtp.rtcp_mode = send_params.rtcp.reduced_size
                                          ? webrtc::RtcpMode::kReducedSize
                                          : webrtc::RtcpMode::kCompound;
+  parameters_.config.overuse_callback =
+      enable_cpu_overuse_detection ? load_proxy_->proxy() : nullptr;
 
   if (codec_settings) {
     SetCodecAndOptions(*codec_settings, parameters_.options);
@@ -1635,9 +1663,6 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::DisconnectCapturer() {
     if (capturer_ == NULL)
       return false;
 
-    if (capturer_->video_adapter() != nullptr)
-      old_adapt_changes_ += capturer_->video_adapter()->adaptation_changes();
-
     capturer = capturer_;
     capturer_ = NULL;
   }
@@ -1929,6 +1954,30 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::Stop() {
   sending_ = false;
 }
 
+void WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate(Load load) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  LOG(LS_INFO) << "OnLoadUpdate " << load;
+  rtc::CritScope cs(&lock_);
+  if (!capturer_) {
+    return;
+  }
+
+  if (load == kOveruse) {
+    if (cpu_downgrade_count_ < kMaxCpuDowngrades) {
+      cpu_downgrade_count_++;
+      sink_wants_.max_number_of_pixels =
+          (last_dimensions_.height * last_dimensions_.width) / 2;
+    }
+  } else {
+    if (cpu_downgrade_count_ > 0) {
+      cpu_downgrade_count_--;
+      sink_wants_.max_number_of_pixels =
+          (last_dimensions_.height * last_dimensions_.width) * 2;
+    }

[pthatcher1, 2016/02/17 23:05:56]

This doesn't seem to work right for situations where the frame size changes on
its own, like with a screencast being resized.

For example:

start at 1080p
handle overuse, set max to 1080/2 = 720p
screencast changes on its own to 360p
handle non-overuse, set max to 360p*2 = 720p.

Wouldn't it make more sense to do:

sink_wants_.max_number_of_pixels *= 2;
or
sink_wants_.max_number_of_pixels /= 2;

?


Also, instead of recording the count of the number of times we've gone down, I
think it would make sense to record the amount we've scaled it down (1, 2, 4),
and have a max for that (4).

In fact, what I think we need to do is:

1.  Have the adapter here, near the encoder.
2.  Have the adapter act like a source and a sink, and the encoder attaches to
it as a source, and it attaches to the main source as a sink.  Then, adaptation
happens even if local preview is attached to the original video track at the
beginning of the pipeline.

[perkj_webrtc, 2016/02/18 13:22:46]

Currently - CPU adaptation is not done for screen cast - I added the check in
VideoCapturer instead of as it was (See old
WebRtcVideoChannel2::OnLoadUpdate(Load load). I can move the logic back here if
you want. 

But from your example- isn't that exactly what we want to express here: The load
for a certain resolution is either too high or too low. We would like to get the
max_number_of_pixels as an argument to OnLoad so that we know what resolution
the decision was based on but this is as good guess as we can currently do.
Alternatively we can have a scale factor or just forwarw Up /Down in the wants
(See patchset 1). But that gets klunky since that express a request for a change
not a want. Nisse did not like it and I agree.

An adapter does not necessarily know how to scale a native handle currently.
That can be solved by adding a scale factor to the video frames as we discussed
and actually let the encoder do the scaling. 
But that is a bit larger change that I think we should do down the line. This
currently work since the adapter causes the frame factory in the capture
implementation to create the correct sized frames from the beginning.

Regardless - I think it make sense to notify the source of what resolution the
sinks wants. In the future we can decide to reconfigure the camera to the
resolution that the sinks are actually using.

Currently adaptation happens even if local preview is attached, that is no
change from how it is now. The source produce the minimum resolution requested
by all sinks. I also think that make sence for CPU adaptation - doing scaling
early saves CPU for all sinks instead of them doing scaling individually.

[perkj_webrtc, 2016/02/18 14:30:54]

So back to your example- pbos convinced me of your visdom in not expressing up /
down in terms of max_number_of_pixels like this.
How about changing the wants again to express Up / Down from a resolution?  

+  }
+  capturer_->AddOrUpdateSink(this, sink_wants_);
+}
+
 VideoSenderInfo
 WebRtcVideoChannel2::WebRtcVideoSendStream::GetVideoSenderInfo() {
   VideoSenderInfo info;
@@ -1967,10 +2016,6 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::GetVideoSenderInfo() {
         info.input_frame_width = last_captured_frame_format.width;
         info.input_frame_height = last_captured_frame_format.height;
       }
-      if (capturer_->video_adapter() != nullptr) {
-        info.adapt_changes += capturer_->video_adapter()->adaptation_changes();
-        info.adapt_reason = capturer_->video_adapter()->adapt_reason();
-      }
     }
   }