Reland "Fix race between enabled() and set_enabled() in VideoTrack."
This reverts commit 096ad02c02b4bc6c046282b8793ef84d041dd0d8.
Reason for revert: Including a fix for the test issue.
Original change's description:
> Revert "Fix race between enabled() and set_enabled() in VideoTrack."
>
> This reverts commit 5ffefe9d2d743c66f8a8bcbc5ad9662a3138840a.
>
> Reason for revert: Breaks Chromium Android browser tests on fyi bots.
>
> Original change's description:
> > Fix race between enabled() and set_enabled() in VideoTrack.
> >
> > Along the way I introduced VideoSourceBaseGuarded, which is equivalent
> > to VideoSourceBase except that it applies thread checks. I found that
> > it's easy to use VideoSourceBase incorrectly and in fact there appear
> > to be tests that do this.
> >
> > I made the source object const in VideoTrack, as it already was in
> > AudioTrack, and that allowed for making the GetSource() accessors
> > bypass the proxy thread hop and give the caller direct access.
> >
> > Bug: webrtc:12773, b/188139639, webrtc:12780
> > Change-Id: I022175c4239a1306ef54059c131d81411d5124fe
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219160
> > Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> > Reviewed-by: Andrey Logvin <landrey@webrtc.org>
> > Commit-Queue: Tommi <tommi@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#34096}
>
> TBR=mbonadei@webrtc.org,tommi@webrtc.org,landrey@webrtc.org,webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com
>
> Change-Id: I16323d459c76eb6a87cc602a0048f6ee01c81626
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:12773
> Bug: b/188139639
> Bug: webrtc:12780
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219637
> Reviewed-by: Evan Shrubsole <eshr@google.com>
> Commit-Queue: Evan Shrubsole <eshr@google.com>
> Cr-Commit-Position: refs/heads/master@{#34101}
# Not skipping CQ checks because this is a reland.
Bug: webrtc:12773
Bug: b/188139639
Bug: webrtc:12780
Change-Id: Ib35fe15a6c43de8f286d60aff02b19df1ab76925
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219639
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Andrey Logvin <landrey@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34104}
diff --git a/pc/track_media_info_map_unittest.cc b/pc/track_media_info_map_unittest.cc
index 2a4889a..1d5caac 100644
--- a/pc/track_media_info_map_unittest.cc
+++ b/pc/track_media_info_map_unittest.cc
@@ -31,6 +31,45 @@
namespace {
+class MockVideoTrack : public VideoTrackInterface {
+ public:
+ // NotifierInterface
+ MOCK_METHOD(void,
+ RegisterObserver,
+ (ObserverInterface * observer),
+ (override));
+ MOCK_METHOD(void,
+ UnregisterObserver,
+ (ObserverInterface * observer),
+ (override));
+
+ // MediaStreamTrackInterface
+ MOCK_METHOD(std::string, kind, (), (const, override));
+ MOCK_METHOD(std::string, id, (), (const, override));
+ MOCK_METHOD(bool, enabled, (), (const, override));
+ MOCK_METHOD(bool, set_enabled, (bool enable), (override));
+ MOCK_METHOD(TrackState, state, (), (const, override));
+
+ // VideoSourceInterface
+ MOCK_METHOD(void,
+ AddOrUpdateSink,
+ (rtc::VideoSinkInterface<VideoFrame> * sink,
+ const rtc::VideoSinkWants& wants),
+ (override));
+ // RemoveSink must guarantee that at the time the method returns,
+ // there is no current and no future calls to VideoSinkInterface::OnFrame.
+ MOCK_METHOD(void,
+ RemoveSink,
+ (rtc::VideoSinkInterface<VideoFrame> * sink),
+ (override));
+
+ // VideoTrackInterface
+ MOCK_METHOD(VideoTrackSourceInterface*, GetSource, (), (const, override));
+
+ MOCK_METHOD(ContentHint, content_hint, (), (const, override));
+ MOCK_METHOD(void, set_content_hint, (ContentHint hint), (override));
+};
+
RtpParameters CreateRtpParametersWithSsrcs(
std::initializer_list<uint32_t> ssrcs) {
RtpParameters params;
@@ -79,23 +118,35 @@
return receiver;
}
+rtc::scoped_refptr<VideoTrackInterface> CreateVideoTrack(
+ const std::string& id) {
+ return VideoTrack::Create(id, FakeVideoTrackSource::Create(false),
+ rtc::Thread::Current());
+}
+
+rtc::scoped_refptr<VideoTrackInterface> CreateMockVideoTrack(
+ const std::string& id) {
+ auto track = rtc::make_ref_counted<MockVideoTrack>();
+ EXPECT_CALL(*track, kind())
+ .WillRepeatedly(::testing::Return(VideoTrack::kVideoKind));
+ return track;
+}
+
class TrackMediaInfoMapTest : public ::testing::Test {
public:
TrackMediaInfoMapTest() : TrackMediaInfoMapTest(true) {}
- explicit TrackMediaInfoMapTest(bool use_current_thread)
+ explicit TrackMediaInfoMapTest(bool use_real_video_track)
: voice_media_info_(new cricket::VoiceMediaInfo()),
video_media_info_(new cricket::VideoMediaInfo()),
local_audio_track_(AudioTrack::Create("LocalAudioTrack", nullptr)),
remote_audio_track_(AudioTrack::Create("RemoteAudioTrack", nullptr)),
- local_video_track_(VideoTrack::Create(
- "LocalVideoTrack",
- FakeVideoTrackSource::Create(false),
- use_current_thread ? rtc::Thread::Current() : nullptr)),
- remote_video_track_(VideoTrack::Create(
- "RemoteVideoTrack",
- FakeVideoTrackSource::Create(false),
- use_current_thread ? rtc::Thread::Current() : nullptr)) {}
+ local_video_track_(use_real_video_track
+ ? CreateVideoTrack("LocalVideoTrack")
+ : CreateMockVideoTrack("LocalVideoTrack")),
+ remote_video_track_(use_real_video_track
+ ? CreateVideoTrack("RemoteVideoTrack")
+ : CreateMockVideoTrack("LocalVideoTrack")) {}
~TrackMediaInfoMapTest() {
// If we have a map the ownership has been passed to the map, only delete if
@@ -179,8 +230,8 @@
std::unique_ptr<TrackMediaInfoMap> map_;
rtc::scoped_refptr<AudioTrack> local_audio_track_;
rtc::scoped_refptr<AudioTrack> remote_audio_track_;
- rtc::scoped_refptr<VideoTrack> local_video_track_;
- rtc::scoped_refptr<VideoTrack> remote_video_track_;
+ rtc::scoped_refptr<VideoTrackInterface> local_video_track_;
+ rtc::scoped_refptr<VideoTrackInterface> remote_video_track_;
};
} // namespace
