Refactor VideoTrackSource, without raw pointer injection.
Bug: None
Change-Id: If4aa8ba72eb3dbdd7dca8970cd6349f1679bc222
Reviewed-on: https://webrtc-review.googlesource.com/78403
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23370}diff --git a/pc/rtpreceiver.cc b/pc/rtpreceiver.cc
index ee45c4e..341261b 100644
--- a/pc/rtpreceiver.cc
+++ b/pc/rtpreceiver.cc
@@ -215,8 +215,7 @@
const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& streams)
: worker_thread_(worker_thread),
id_(receiver_id),
- source_(new RefCountedObject<VideoTrackSource>(&broadcaster_,
- true /* remote */)),
+ source_(new RefCountedObject<VideoRtpTrackSource>()),
track_(VideoTrackProxy::Create(
rtc::Thread::Current(),
worker_thread,
@@ -270,7 +269,6 @@
return;
}
source_->SetState(MediaSourceInterface::kEnded);
- source_->OnSourceDestroyed();
if (!media_channel_ || !ssrc_) {
RTC_LOG(LS_WARNING) << "VideoRtpReceiver::Stop: No video channel exists.";
} else {
@@ -293,7 +291,7 @@
SetSink(nullptr);
}
ssrc_ = ssrc;
- SetSink(&broadcaster_);
+ SetSink(source_->sink());
}
void VideoRtpReceiver::SetStreams(
diff --git a/pc/rtpreceiver.h b/pc/rtpreceiver.h
index 55ed29e..4d6457f 100644
--- a/pc/rtpreceiver.h
+++ b/pc/rtpreceiver.h
@@ -202,19 +202,31 @@
int AttachmentId() const override { return attachment_id_; }
private:
+ class VideoRtpTrackSource : public VideoTrackSource {
+ public:
+ VideoRtpTrackSource() : VideoTrackSource(true /* remote */) {}
+
+ rtc::VideoSourceInterface<VideoFrame>* source() override {
+ return &broadcaster_;
+ }
+ rtc::VideoSinkInterface<VideoFrame>* sink() { return &broadcaster_; }
+
+ private:
+ // |broadcaster_| is needed since the decoder can only handle one sink.
+ // It might be better if the decoder can handle multiple sinks and consider
+ // the VideoSinkWants.
+ rtc::VideoBroadcaster broadcaster_;
+ };
+
bool SetSink(rtc::VideoSinkInterface<VideoFrame>* sink);
rtc::Thread* const worker_thread_;
const std::string id_;
cricket::VideoMediaChannel* media_channel_ = nullptr;
rtc::Optional<uint32_t> ssrc_;
- // |broadcaster_| is needed since the decoder can only handle one sink.
- // It might be better if the decoder can handle multiple sinks and consider
- // the VideoSinkWants.
- rtc::VideoBroadcaster broadcaster_;
// |source_| is held here to be able to change the state of the source when
// the VideoRtpReceiver is stopped.
- rtc::scoped_refptr<VideoTrackSource> source_;
+ rtc::scoped_refptr<VideoRtpTrackSource> source_;
rtc::scoped_refptr<VideoTrackInterface> track_;
std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams_;
bool stopped_ = false;
diff --git a/pc/test/fakeperiodicvideotracksource.h b/pc/test/fakeperiodicvideotracksource.h
index 1be347c..8cdac0e 100644
--- a/pc/test/fakeperiodicvideotracksource.h
+++ b/pc/test/fakeperiodicvideotracksource.h
@@ -25,13 +25,13 @@
FakePeriodicVideoTrackSource(FakePeriodicVideoSource::Config config,
bool remote)
- // Note that VideoTrack constructor gets a pointer to an
- // uninitialized source object; that works because it only
- // stores the pointer for later use.
- : VideoTrackSource(&source_, remote), source_(config) {}
+ : VideoTrackSource(remote), source_(config) {}
~FakePeriodicVideoTrackSource() = default;
+ protected:
+ rtc::VideoSourceInterface<VideoFrame>* source() override { return &source_; }
+
private:
FakePeriodicVideoSource source_;
};
diff --git a/pc/test/fakevideotracksource.h b/pc/test/fakevideotracksource.h
index 9cc5ef1..1b5bd87 100644
--- a/pc/test/fakevideotracksource.h
+++ b/pc/test/fakevideotracksource.h
@@ -12,7 +12,6 @@
#define PC_TEST_FAKEVIDEOTRACKSOURCE_H_
#include "api/mediastreaminterface.h"
-#include "api/video/video_source_interface.h"
#include "pc/videotracksource.h"
namespace webrtc {
@@ -30,23 +29,19 @@
}
bool is_screencast() const override { return is_screencast_; }
+ void AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
+ const rtc::VideoSinkWants& wants) override {}
+ void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {}
protected:
explicit FakeVideoTrackSource(bool is_screencast)
- : VideoTrackSource(&source_, false /* remote */),
- is_screencast_(is_screencast) {}
- virtual ~FakeVideoTrackSource() {}
+ : VideoTrackSource(false /* remote */), is_screencast_(is_screencast) {}
+ ~FakeVideoTrackSource() override = default;
+
+ // Unused, since we override AddOrUpdateSink and RemoveSink above.
+ rtc::VideoSourceInterface<VideoFrame>* source() override { return nullptr; }
private:
- class Source : public rtc::VideoSourceInterface<VideoFrame> {
- public:
- void AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
- const rtc::VideoSinkWants& wants) override {}
-
- void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override {}
- };
-
- Source source_;
const bool is_screencast_;
};
diff --git a/pc/videocapturertracksource.cc b/pc/videocapturertracksource.cc
index bbfb334..d90ba51 100644
--- a/pc/videocapturertracksource.cc
+++ b/pc/videocapturertracksource.cc
@@ -288,7 +288,7 @@
rtc::Thread* worker_thread,
std::unique_ptr<cricket::VideoCapturer> capturer,
bool remote)
- : VideoTrackSource(capturer.get(), remote),
+ : VideoTrackSource(remote),
signaling_thread_(rtc::Thread::Current()),
worker_thread_(worker_thread),
video_capturer_(std::move(capturer)),
diff --git a/pc/videocapturertracksource.h b/pc/videocapturertracksource.h
index b80323f..5854944 100644
--- a/pc/videocapturertracksource.h
+++ b/pc/videocapturertracksource.h
@@ -57,6 +57,11 @@
std::unique_ptr<cricket::VideoCapturer> capturer,
bool remote);
virtual ~VideoCapturerTrackSource();
+
+ rtc::VideoSourceInterface<VideoFrame>* source() override {
+ return video_capturer_.get();
+ }
+
void Initialize(const webrtc::MediaConstraintsInterface* constraints);
private:
diff --git a/pc/videotrack_unittest.cc b/pc/videotrack_unittest.cc
index 77d16de..e833d6d 100644
--- a/pc/videotrack_unittest.cc
+++ b/pc/videotrack_unittest.cc
@@ -25,22 +25,31 @@
using webrtc::VideoTrack;
using webrtc::VideoTrackInterface;
+class TestVideoTrackSource : public VideoTrackSource {
+ public:
+ TestVideoTrackSource() : VideoTrackSource(true /* remote */) {}
+ rtc::VideoSourceInterface<webrtc::VideoFrame>* source() override {
+ return &capturer_;
+ }
+ cricket::FakeVideoCapturerWithTaskQueue* capturer() { return &capturer_; }
+
+ private:
+ cricket::FakeVideoCapturerWithTaskQueue capturer_;
+};
class VideoTrackTest : public testing::Test {
public:
VideoTrackTest() {
static const char kVideoTrackId[] = "track_id";
- video_track_source_ = new rtc::RefCountedObject<VideoTrackSource>(
- &capturer_, true /* remote */);
+ video_track_source_ = new rtc::RefCountedObject<TestVideoTrackSource>();
video_track_ = VideoTrack::Create(kVideoTrackId, video_track_source_,
rtc::Thread::Current());
- capturer_.Start(
+ video_track_source_->capturer()->Start(
cricket::VideoFormat(640, 480, cricket::VideoFormat::FpsToInterval(30),
cricket::FOURCC_I420));
}
protected:
- cricket::FakeVideoCapturerWithTaskQueue capturer_;
- rtc::scoped_refptr<VideoTrackSource> video_track_source_;
+ rtc::scoped_refptr<TestVideoTrackSource> video_track_source_;
rtc::scoped_refptr<VideoTrackInterface> video_track_;
};
@@ -58,18 +67,18 @@
std::unique_ptr<FakeVideoTrackRenderer> renderer_1(
new FakeVideoTrackRenderer(video_track_.get()));
- capturer_.CaptureFrame();
+ video_track_source_->capturer()->CaptureFrame();
EXPECT_EQ(1, renderer_1->num_rendered_frames());
// FakeVideoTrackRenderer register itself to |video_track_|
std::unique_ptr<FakeVideoTrackRenderer> renderer_2(
new FakeVideoTrackRenderer(video_track_.get()));
- capturer_.CaptureFrame();
+ video_track_source_->capturer()->CaptureFrame();
EXPECT_EQ(2, renderer_1->num_rendered_frames());
EXPECT_EQ(1, renderer_2->num_rendered_frames());
renderer_1.reset(nullptr);
- capturer_.CaptureFrame();
+ video_track_source_->capturer()->CaptureFrame();
EXPECT_EQ(2, renderer_2->num_rendered_frames());
}
@@ -78,17 +87,17 @@
std::unique_ptr<FakeVideoTrackRenderer> renderer(
new FakeVideoTrackRenderer(video_track_.get()));
- capturer_.CaptureFrame();
+ video_track_source_->capturer()->CaptureFrame();
EXPECT_EQ(1, renderer->num_rendered_frames());
EXPECT_FALSE(renderer->black_frame());
video_track_->set_enabled(false);
- capturer_.CaptureFrame();
+ video_track_source_->capturer()->CaptureFrame();
EXPECT_EQ(2, renderer->num_rendered_frames());
EXPECT_TRUE(renderer->black_frame());
video_track_->set_enabled(true);
- capturer_.CaptureFrame();
+ video_track_source_->capturer()->CaptureFrame();
EXPECT_EQ(3, renderer->num_rendered_frames());
EXPECT_FALSE(renderer->black_frame());
}
diff --git a/pc/videotracksource.cc b/pc/videotracksource.cc
index 56f48e7..de87acb 100644
--- a/pc/videotracksource.cc
+++ b/pc/videotracksource.cc
@@ -14,6 +14,9 @@
namespace webrtc {
+VideoTrackSource::VideoTrackSource(bool remote)
+ : VideoTrackSource(nullptr, remote) {}
+
VideoTrackSource::VideoTrackSource(
rtc::VideoSourceInterface<VideoFrame>* source,
bool remote)
@@ -28,26 +31,16 @@
}
}
-void VideoTrackSource::OnSourceDestroyed() {
- source_ = nullptr;
-}
-
void VideoTrackSource::AddOrUpdateSink(
rtc::VideoSinkInterface<VideoFrame>* sink,
const rtc::VideoSinkWants& wants) {
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
- if (!source_) {
- return;
- }
- source_->AddOrUpdateSink(sink, wants);
+ source()->AddOrUpdateSink(sink, wants);
}
void VideoTrackSource::RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
- if (!source_) {
- return;
- }
- source_->RemoveSink(sink);
+ source()->RemoveSink(sink);
}
} // namespace webrtc
diff --git a/pc/videotracksource.h b/pc/videotracksource.h
index 5ead399..f3eef3b 100644
--- a/pc/videotracksource.h
+++ b/pc/videotracksource.h
@@ -17,17 +17,16 @@
#include "media/base/mediachannel.h"
#include "rtc_base/thread_checker.h"
-// VideoTrackSource implements VideoTrackSourceInterface.
namespace webrtc {
+// VideoTrackSource is a convenience base class for implementations of
+// VideoTrackSourceInterface.
class VideoTrackSource : public Notifier<VideoTrackSourceInterface> {
public:
+ explicit VideoTrackSource(bool remote);
+ // TODO(nisse): Delete, kept only for temporary backwards compatibility.
VideoTrackSource(rtc::VideoSourceInterface<VideoFrame>* source, bool remote);
void SetState(SourceState new_state);
- // OnSourceDestroyed clears this instance pointer to |source_|. It is useful
- // when the underlying rtc::VideoSourceInterface is destroyed before the
- // reference counted VideoTrackSource.
- void OnSourceDestroyed();
SourceState state() const override { return state_; }
bool remote() const override { return remote_; }
@@ -41,8 +40,14 @@
const rtc::VideoSinkWants& wants) override;
void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override;
+ protected:
+ // TODO(nisse): Default implementations for temporary backwards
+ // compatibility.
+ virtual rtc::VideoSourceInterface<VideoFrame>* source() { return source_; }
+
private:
rtc::ThreadChecker worker_thread_checker_;
+ // TODO(nisse): Delete, kept only for temporary backwards compatibility.
rtc::VideoSourceInterface<VideoFrame>* source_;
SourceState state_;
const bool remote_;
