Reland "Use non-proxied source object in VideoTrack."

This is a reland of 3eb29c12358930a60134f185cd849e0d12aa9166

This reland doesn't contain the AudioTrack changes (see original
description) that got triggered in some cases and needs to be
addressed separately.

Another change in this re-land is that instead of the `state` property
of the VideoTrack be marshalled to the signaling thread, it's readable
from the calling thread. Previously this was marshalled to the worker
and the original changed that to the signaling thread (same as for
AudioTrack) - but in case that's causing downstream problems this reland
uses BYPASS_PROXY_CONSTMETHOD0 for the `state()` accessor of the
VideoTrack proxy.

Original change's description:
> Use non-proxied source object in VideoTrack.
>
> Use the internal representation of the video source object from the
> track. Before there were implicit thread hops due to use of the proxy.
>
> Also, override AudioTrack's enabled methods to enforce thread
> expectations.
>
> Bug: webrtc:13540
> Change-Id: I4bc7aca96d6fc24f31ade45e47f52599f1cc2f97
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250180
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#35911}

Bug: webrtc:13540
Change-Id: Icb3e165f07240ae10730a316d3a8a3b2b9167d82
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251387
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35979}

pc/BUILD.gn

@@ -1242,6 +1242,7 @@ rtc_library("video_track") {
     "video_track.h",
   ]
   deps = [
+    ":rtc_pc_base",
     "../api:media_stream_interface",
     "../api:scoped_refptr",
     "../api:sequence_checker",

pc/audio_track.h

@@ -20,6 +20,10 @@
 
 namespace webrtc {
 
+// TODO(tommi): Instead of inheriting from `MediaStreamTrack<>`, implement the
+// properties directly in this class. `MediaStreamTrack` doesn't guard against
+// conflicting access, so we'd need to override those methods anyway in this
+// class in order to make sure things are correctly checked.
 class AudioTrack : public MediaStreamTrack<AudioTrackInterface>,
                    public ObserverInterface {
  protected:

pc/media_stream_track_proxy.h

@@ -29,12 +29,12 @@ BYPASS_PROXY_CONSTMETHOD0(std::string, kind)
 BYPASS_PROXY_CONSTMETHOD0(std::string, id)
 PROXY_CONSTMETHOD0(TrackState, state)
 PROXY_CONSTMETHOD0(bool, enabled)
+PROXY_METHOD1(bool, set_enabled, bool)
 BYPASS_PROXY_CONSTMETHOD0(AudioSourceInterface*, GetSource)
 PROXY_METHOD1(void, AddSink, AudioTrackSinkInterface*)
 PROXY_METHOD1(void, RemoveSink, AudioTrackSinkInterface*)
 PROXY_METHOD1(bool, GetSignalLevel, int*)
 PROXY_METHOD0(rtc::scoped_refptr<AudioProcessorInterface>, GetAudioProcessor)
-PROXY_METHOD1(bool, set_enabled, bool)
 PROXY_METHOD1(void, RegisterObserver, ObserverInterface*)
 PROXY_METHOD1(void, UnregisterObserver, ObserverInterface*)
 END_PROXY_MAP(AudioTrack)
@@ -43,7 +43,9 @@ BEGIN_PROXY_MAP(VideoTrack)
 PROXY_PRIMARY_THREAD_DESTRUCTOR()
 BYPASS_PROXY_CONSTMETHOD0(std::string, kind)
 BYPASS_PROXY_CONSTMETHOD0(std::string, id)
-PROXY_SECONDARY_CONSTMETHOD0(TrackState, state)
+// TODO(bugs.webrtc.org/13540): This should probably be PROXY_CONSTMETHOD0, but
+// leaving as is due to downstream incompatibility.
+BYPASS_PROXY_CONSTMETHOD0(TrackState, state)
 PROXY_SECONDARY_CONSTMETHOD0(bool, enabled)
 PROXY_SECONDARY_METHOD1(bool, set_enabled, bool)
 PROXY_SECONDARY_CONSTMETHOD0(ContentHint, content_hint)

pc/video_rtp_receiver.cc

@@ -41,11 +41,7 @@ VideoRtpReceiver::VideoRtpReceiver(
       track_(VideoTrackProxyWithInternal<VideoTrack>::Create(
           rtc::Thread::Current(),
           worker_thread,
-          VideoTrack::Create(receiver_id,
+          VideoTrack::Create(receiver_id, source_, worker_thread))),
-                             CreateVideoTrackSourceProxy(rtc::Thread::Current(),
-                                                         worker_thread,
-                                                         source_),
-                             worker_thread))),
       attachment_id_(GenerateUniqueId()) {
   RTC_DCHECK(worker_thread_);
   SetStreams(streams);

pc/video_track.cc

@@ -22,12 +22,14 @@
 
 namespace webrtc {
 
-VideoTrack::VideoTrack(const std::string& label,
+VideoTrack::VideoTrack(
-                       VideoTrackSourceInterface* video_source,
+    const std::string& label,
-                       rtc::Thread* worker_thread)
+    rtc::scoped_refptr<
+        VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>> source,
+    rtc::Thread* worker_thread)
     : MediaStreamTrack<VideoTrackInterface>(label),
       worker_thread_(worker_thread),
-      video_source_(video_source),
+      video_source_(std::move(source)),
       content_hint_(ContentHint::kNone) {
   RTC_DCHECK_RUN_ON(&signaling_thread_);
   // Detach the thread checker for VideoSourceBaseGuarded since we'll make calls
@@ -54,18 +56,18 @@ void VideoTrack::AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
   VideoSourceBaseGuarded::AddOrUpdateSink(sink, wants);
   rtc::VideoSinkWants modified_wants = wants;
   modified_wants.black_frames = !enabled();
-  video_source_->AddOrUpdateSink(sink, modified_wants);
+  video_source_->internal()->AddOrUpdateSink(sink, modified_wants);
 }
 
 void VideoTrack::RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {
   RTC_DCHECK_RUN_ON(worker_thread_);
   VideoSourceBaseGuarded::RemoveSink(sink);
-  video_source_->RemoveSink(sink);
+  video_source_->internal()->RemoveSink(sink);
 }
 
 void VideoTrack::RequestRefreshFrame() {
   RTC_DCHECK_RUN_ON(worker_thread_);
-  video_source_->RequestRefreshFrame();
+  video_source_->internal()->RequestRefreshFrame();
 }
 
 VideoTrackSourceInterface* VideoTrack::GetSource() const {
@@ -88,10 +90,11 @@ void VideoTrack::set_content_hint(ContentHint hint) {
 
 bool VideoTrack::set_enabled(bool enable) {
   RTC_DCHECK_RUN_ON(worker_thread_);
+  auto* source = video_source_->internal();
   for (auto& sink_pair : sink_pairs()) {
     rtc::VideoSinkWants modified_wants = sink_pair.wants;
     modified_wants.black_frames = !enable;
-    video_source_->AddOrUpdateSink(sink_pair.sink, modified_wants);
+    source->AddOrUpdateSink(sink_pair.sink, modified_wants);
   }
   return MediaStreamTrack<VideoTrackInterface>::set_enabled(enable);
 }
@@ -102,28 +105,30 @@ bool VideoTrack::enabled() const {
 }
 
 MediaStreamTrackInterface::TrackState VideoTrack::state() const {
-  RTC_DCHECK_RUN_ON(worker_thread_);
+  // TODO(bugs.webrtc.org/13540): Re-enable this DCHECK and update the proxy.
+  // RTC_DCHECK_RUN_ON(&signaling_thread_);
   return MediaStreamTrack<VideoTrackInterface>::state();
 }
 
 void VideoTrack::OnChanged() {
   RTC_DCHECK_RUN_ON(&signaling_thread_);
-  worker_thread_->Invoke<void>(
+  rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
-      RTC_FROM_HERE, [this, state = video_source_->state()]() {
+  auto* source = video_source_->internal();
-        // TODO(tommi): Calling set_state() this way isn't ideal since we're
+  auto state = source->state();
-        // currently blocking the signaling thread and set_state() may
+  set_state(state == MediaSourceInterface::kEnded ? kEnded : kLive);
-        // internally fire notifications via `FireOnChanged()` which may further
-        // amplify the blocking effect on the signaling thread.
-        rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
-        set_state(state == MediaSourceInterface::kEnded ? kEnded : kLive);
-      });
 }
 
 rtc::scoped_refptr<VideoTrack> VideoTrack::Create(
     const std::string& id,
     VideoTrackSourceInterface* source,
     rtc::Thread* worker_thread) {
-  return rtc::make_ref_counted<VideoTrack>(id, source, worker_thread);
+  rtc::scoped_refptr<
+      VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>>
+      source_proxy = VideoTrackSourceProxy::Create(rtc::Thread::Current(),
+                                                   worker_thread, source);
+
+  return rtc::make_ref_counted<VideoTrack>(id, std::move(source_proxy),
+                                           worker_thread);
 }
 
 }  // namespace webrtc

pc/video_track.h

@@ -21,11 +21,16 @@
 #include "api/video/video_sink_interface.h"
 #include "api/video/video_source_interface.h"
 #include "media/base/video_source_base.h"
+#include "pc/video_track_source_proxy.h"
 #include "rtc_base/thread.h"
 #include "rtc_base/thread_annotations.h"
 
 namespace webrtc {
 
+// TODO(tommi): Instead of inheriting from `MediaStreamTrack<>`, implement the
+// properties directly in this class. `MediaStreamTrack` doesn't guard against
+// conflicting access, so we'd need to override those methods anyway in this
+// class in order to make sure things are correctly checked.
 class VideoTrack : public MediaStreamTrack<VideoTrackInterface>,
                    public rtc::VideoSourceBaseGuarded,
                    public ObserverInterface {
@@ -49,9 +54,11 @@ class VideoTrack : public MediaStreamTrack<VideoTrackInterface>,
   std::string kind() const override;
 
  protected:
-  VideoTrack(const std::string& id,
+  VideoTrack(
-             VideoTrackSourceInterface* video_source,
+      const std::string& id,
-             rtc::Thread* worker_thread);
+      rtc::scoped_refptr<
+          VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>> source,
+      rtc::Thread* worker_thread);
   ~VideoTrack();
 
  private:
@@ -60,7 +67,10 @@ class VideoTrack : public MediaStreamTrack<VideoTrackInterface>,
 
   RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker signaling_thread_;
   rtc::Thread* const worker_thread_;
-  const rtc::scoped_refptr<VideoTrackSourceInterface> video_source_;
+  const rtc::scoped_refptr<
+      VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>>
+      video_source_;
+
   ContentHint content_hint_ RTC_GUARDED_BY(worker_thread_);
 };