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}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index ba05867..59459ec 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1242,6 +1242,7 @@
"video_track.h",
]
deps = [
+ ":rtc_pc_base",
"../api:media_stream_interface",
"../api:scoped_refptr",
"../api:sequence_checker",
diff --git a/pc/audio_track.h b/pc/audio_track.h
index 8a705cf..8e05f54 100644
--- a/pc/audio_track.h
+++ b/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:
diff --git a/pc/media_stream_track_proxy.h b/pc/media_stream_track_proxy.h
index 0278423..004bcdb 100644
--- a/pc/media_stream_track_proxy.h
+++ b/pc/media_stream_track_proxy.h
@@ -29,12 +29,12 @@
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 @@
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)
diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc
index c5ead9e..d1f8fb0 100644
--- a/pc/video_rtp_receiver.cc
+++ b/pc/video_rtp_receiver.cc
@@ -41,11 +41,7 @@
track_(VideoTrackProxyWithInternal<VideoTrack>::Create(
rtc::Thread::Current(),
worker_thread,
- VideoTrack::Create(receiver_id,
- CreateVideoTrackSourceProxy(rtc::Thread::Current(),
- worker_thread,
- source_),
- worker_thread))),
+ VideoTrack::Create(receiver_id, source_, worker_thread))),
attachment_id_(GenerateUniqueId()) {
RTC_DCHECK(worker_thread_);
SetStreams(streams);
diff --git a/pc/video_track.cc b/pc/video_track.cc
index ad552ea..f0da930 100644
--- a/pc/video_track.cc
+++ b/pc/video_track.cc
@@ -22,12 +22,14 @@
namespace webrtc {
-VideoTrack::VideoTrack(const std::string& label,
- VideoTrackSourceInterface* video_source,
- rtc::Thread* worker_thread)
+VideoTrack::VideoTrack(
+ const std::string& label,
+ 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 @@
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 @@
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 @@
}
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_FROM_HERE, [this, state = video_source_->state()]() {
- // TODO(tommi): Calling set_state() this way isn't ideal since we're
- // currently blocking the signaling thread and set_state() may
- // 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::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
+ auto* source = video_source_->internal();
+ auto state = source->state();
+ 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
diff --git a/pc/video_track.h b/pc/video_track.h
index 15bfc87..772f52a 100644
--- a/pc/video_track.h
+++ b/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 @@
std::string kind() const override;
protected:
- VideoTrack(const std::string& id,
- VideoTrackSourceInterface* video_source,
- rtc::Thread* worker_thread);
+ VideoTrack(
+ const std::string& id,
+ rtc::scoped_refptr<
+ VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>> source,
+ rtc::Thread* worker_thread);
~VideoTrack();
private:
@@ -60,7 +67,10 @@
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_);
};
