admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove ability to do SetChannel() without ClearChannel()

Browse Source 
This calls out the fact that SetChannel() is only used on M-section activation; ClearChannel is called on deactivation, and we never change the channel while a transceiver is active.

Bug: webrtc:13931
Change-Id: I3a3bfeec7c1d27d98c3f94a9401bee2130754ed7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260461
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36709}
This commit is contained in:
Harald Alvestrand 2022-04-29 11:42:55 +00:00 committed by WebRTC LUCI CQ
parent d4fce5a361
commit daee870a35
3 changed files with 26 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
pc/rtp_transceiver.cc
View File

@ -164,25 +164,19 @@ void RtpTransceiver::SetChannel(
cricket::ChannelInterface* channel,
std::function<RtpTransportInternal*(const std::string&)> transport_lookup) {
RTC_DCHECK_RUN_ON(thread_);
RTC_DCHECK(channel); // Use ClearChannel() if clearing.
RTC_DCHECK(channel);
RTC_DCHECK(transport_lookup);
RTC_DCHECK(!channel_);
// Cannot set a channel on a stopped transceiver.
if (stopped_ || channel == channel_) {
if (stopped_) {
return;
}
RTC_LOG_THREAD_BLOCK_COUNT();
if (channel_) {
signaling_thread_safety_->SetNotAlive();
signaling_thread_safety_ = nullptr;
}
RTC_DCHECK_EQ(media_type(), channel->media_type());
signaling_thread_safety_ = PendingTaskSafetyFlag::Create();
cricket::ChannelInterface* channel_to_delete = nullptr;
// An alternative to this, could be to require SetChannel to be called
// on the network thread. The channel object operates for the most part
// on the network thread, as part of its initialization being on the network
@ -193,12 +187,6 @@ void RtpTransceiver::SetChannel(
// helps with keeping the channel implementation requirements being met and
// avoids synchronization for accessing the pointer or network related state.
channel_manager_->network_thread()->Invoke<void>(RTC_FROM_HERE, [&]() {
if (channel_) {
channel_->SetFirstPacketReceivedCallback(nullptr);
channel_->SetRtpTransport(nullptr);
channel_to_delete = channel_;
}
channel_ = channel;
channel_->SetRtpTransport(transport_lookup(channel_->mid()));
@ -208,12 +196,7 @@ void RtpTransceiver::SetChannel(
[this]() { OnFirstPacketReceived(); }));
});
});
RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1);
if (channel_to_delete || !senders_.empty() || !receivers_.empty()) {
DeleteChannel(channel_to_delete);
}
PushNewMediaChannelAndDeleteChannel(nullptr);
RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2);
}
@ -244,17 +227,20 @@ void RtpTransceiver::ClearChannel() {
});
RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1);
if (channel_to_delete || !senders_.empty() || !receivers_.empty()) {
DeleteChannel(channel_to_delete);
}
PushNewMediaChannelAndDeleteChannel(channel_to_delete);
RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2);
}
void RtpTransceiver::DeleteChannel(
void RtpTransceiver::PushNewMediaChannelAndDeleteChannel(
cricket::ChannelInterface* channel_to_delete) {
// The clumsy combination of pushing down media channel and deleting
// the channel is due to the desire to do both things in one Invoke().
if (!channel_to_delete && senders_.empty() && receivers_.empty()) {
return;
}
channel_manager_->worker_thread()->Invoke<void>(RTC_FROM_HERE, [&]() {
// Push down the new media_channel, if any, otherwise clear it.
auto* media_channel = channel_ ? channel_->media_channel() : nullptr;
for (const auto& sender : senders_) {
sender->internal()->SetMediaChannel(media_channel);

14
pc/rtp_transceiver.h
View File

@ -107,18 +107,19 @@ class RtpTransceiver : public RtpTransceiverInterface,
// furthermore be made on the signaling thread.
//
// `channel`: The channel instance to be associated with the transceiver.
// When a valid pointer is passed for `channel`, the state of the object
// This must be a valid pointer.
// The state of the object
// is expected to be newly constructed and not initalized for network
// activity (see next parameter for more).
//
// NOTE: For all practical purposes, the ownership of the channel
// object should be considered to lie with the transceiver until
// `SetChannel()` is called again with nullptr set as the new channel.
// `ClearChannel()` is called.
// Moving forward, this parameter will change to either be a
// std::unique_ptr<> or the full construction of the channel object will
// be moved to happen within the context of the transceiver class.
//
// `transport_lookup`: When `channel` points to a valid channel object, this
// `transport_lookup`: This
// callback function will be used to look up the `RtpTransport` object
// to associate with the channel via `BaseChannel::SetRtpTransport`.
// The lookup function will be called on the network thread, synchronously
@ -130,6 +131,7 @@ class RtpTransceiver : public RtpTransceiverInterface,
// synchronously to the network thread from the signaling thread.
// The callback allows us to combine the transport lookup with network
// state initialization of the channel object.
// ClearChannel() must be used before calling SetChannel() again.
void SetChannel(cricket::ChannelInterface* channel,
std::function<RtpTransportInternal*(const std::string&)>
transport_lookup);
@ -280,8 +282,10 @@ class RtpTransceiver : public RtpTransceiverInterface,
private:
void OnFirstPacketReceived();
void StopSendingAndReceiving();
// Delete a channel. Used by SetChannel and ClearChannel.
void DeleteChannel(cricket::ChannelInterface* channel_to_delete);
// Delete a channel, and ensure that references to its media channel
// are updated before deleting it.
void PushNewMediaChannelAndDeleteChannel(
cricket::ChannelInterface* channel_to_delete);
// Enforce that this object is created, used and destroyed on one thread.
TaskQueueBase* const thread_;

9
pc/rtp_transceiver_unittest.cc
View File

@ -75,14 +75,15 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) {
EXPECT_CALL(channel2, media_type())
.WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO));
// Clear the current channel - required to allow SetChannel()
EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_));
EXPECT_CALL(cm, DestroyChannel(&channel1)).WillRepeatedly(testing::Return());
transceiver->ClearChannel();
// Channel can no longer be set, so this call should be a no-op.
transceiver->SetChannel(&channel2,
[](const std::string&) { return nullptr; });
EXPECT_EQ(&channel1, transceiver->channel());
EXPECT_EQ(nullptr, transceiver->channel());
// Clear the current channel before `transceiver` goes out of scope.
EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_));
EXPECT_CALL(cm, DestroyChannel(&channel1)).WillRepeatedly(testing::Return());
transceiver->ClearChannel();
}