Revert "Reland "Use non-proxied source object in VideoTrack.""
This reverts commit 1158bff15f33c467543928dd6a49cb6ad04da1ba.
Reason for revert: Downstream issues unresolved (2nd of two reverts)
Original change's description:
> 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}
# Not skipping CQ checks because original CL landed > 1 day ago.
Using "No-Try" to not have to wait for the win chromium bot to unblock
(currently takes hours).
No-Try: true
Bug: webrtc:13540
Change-Id: I8f34536bf472a6d069344e84d889864f195c93f6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251686
Reviewed-by: Christoffer Jansson <jansson@google.com>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35993}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 59459ec..ba05867 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1242,7 +1242,6 @@
"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 8e05f54..8a705cf 100644
--- a/pc/audio_track.h
+++ b/pc/audio_track.h
@@ -20,10 +20,6 @@
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 004bcdb..0278423 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,9 +43,7 @@
PROXY_PRIMARY_THREAD_DESTRUCTOR()
BYPASS_PROXY_CONSTMETHOD0(std::string, kind)
BYPASS_PROXY_CONSTMETHOD0(std::string, id)
-// 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(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 d1f8fb0..c5ead9e 100644
--- a/pc/video_rtp_receiver.cc
+++ b/pc/video_rtp_receiver.cc
@@ -41,7 +41,11 @@
track_(VideoTrackProxyWithInternal<VideoTrack>::Create(
rtc::Thread::Current(),
worker_thread,
- VideoTrack::Create(receiver_id, source_, worker_thread))),
+ VideoTrack::Create(receiver_id,
+ CreateVideoTrackSourceProxy(rtc::Thread::Current(),
+ worker_thread,
+ 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 f0da930..ad552ea 100644
--- a/pc/video_track.cc
+++ b/pc/video_track.cc
@@ -22,14 +22,12 @@
namespace webrtc {
-VideoTrack::VideoTrack(
- const std::string& label,
- rtc::scoped_refptr<
- VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>> source,
- rtc::Thread* worker_thread)
+VideoTrack::VideoTrack(const std::string& label,
+ VideoTrackSourceInterface* video_source,
+ rtc::Thread* worker_thread)
: MediaStreamTrack<VideoTrackInterface>(label),
worker_thread_(worker_thread),
- video_source_(std::move(source)),
+ video_source_(video_source),
content_hint_(ContentHint::kNone) {
RTC_DCHECK_RUN_ON(&signaling_thread_);
// Detach the thread checker for VideoSourceBaseGuarded since we'll make calls
@@ -56,18 +54,18 @@
VideoSourceBaseGuarded::AddOrUpdateSink(sink, wants);
rtc::VideoSinkWants modified_wants = wants;
modified_wants.black_frames = !enabled();
- video_source_->internal()->AddOrUpdateSink(sink, modified_wants);
+ video_source_->AddOrUpdateSink(sink, modified_wants);
}
void VideoTrack::RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {
RTC_DCHECK_RUN_ON(worker_thread_);
VideoSourceBaseGuarded::RemoveSink(sink);
- video_source_->internal()->RemoveSink(sink);
+ video_source_->RemoveSink(sink);
}
void VideoTrack::RequestRefreshFrame() {
RTC_DCHECK_RUN_ON(worker_thread_);
- video_source_->internal()->RequestRefreshFrame();
+ video_source_->RequestRefreshFrame();
}
VideoTrackSourceInterface* VideoTrack::GetSource() const {
@@ -90,11 +88,10 @@
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;
- source->AddOrUpdateSink(sink_pair.sink, modified_wants);
+ video_source_->AddOrUpdateSink(sink_pair.sink, modified_wants);
}
return MediaStreamTrack<VideoTrackInterface>::set_enabled(enable);
}
@@ -105,30 +102,28 @@
}
MediaStreamTrackInterface::TrackState VideoTrack::state() const {
- // TODO(bugs.webrtc.org/13540): Re-enable this DCHECK and update the proxy.
- // RTC_DCHECK_RUN_ON(&signaling_thread_);
+ RTC_DCHECK_RUN_ON(worker_thread_);
return MediaStreamTrack<VideoTrackInterface>::state();
}
void VideoTrack::OnChanged() {
RTC_DCHECK_RUN_ON(&signaling_thread_);
- rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
- auto* source = video_source_->internal();
- auto state = source->state();
- set_state(state == MediaSourceInterface::kEnded ? kEnded : kLive);
+ 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::scoped_refptr<VideoTrack> VideoTrack::Create(
const std::string& id,
VideoTrackSourceInterface* source,
rtc::Thread* 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);
+ return rtc::make_ref_counted<VideoTrack>(id, source, worker_thread);
}
} // namespace webrtc
diff --git a/pc/video_track.h b/pc/video_track.h
index 772f52a..15bfc87 100644
--- a/pc/video_track.h
+++ b/pc/video_track.h
@@ -21,16 +21,11 @@
#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 {
@@ -54,11 +49,9 @@
std::string kind() const override;
protected:
- VideoTrack(
- const std::string& id,
- rtc::scoped_refptr<
- VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>> source,
- rtc::Thread* worker_thread);
+ VideoTrack(const std::string& id,
+ VideoTrackSourceInterface* video_source,
+ rtc::Thread* worker_thread);
~VideoTrack();
private:
@@ -67,10 +60,7 @@
RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker signaling_thread_;
rtc::Thread* const worker_thread_;
- const rtc::scoped_refptr<
- VideoTrackSourceProxyWithInternal<VideoTrackSourceInterface>>
- video_source_;
-
+ const rtc::scoped_refptr<VideoTrackSourceInterface> video_source_;
ContentHint content_hint_ RTC_GUARDED_BY(worker_thread_);
};
