[Rollback] Don't end tracks when transceiver is still in use.

Prior to this CL, calling RtpTransceiver::SetChannel() with null
arguments would cause the receiver's track to end. This is wrong,
because the channel can be nulled for other reasons than the transceiver
being stopped/removed - such as when the transceiver is rolled back but
still in use. Also, stopping a transceiver will end the track, so we
should simply ensure to always stop the transceiver when that is needed.

This CL makes sure that the transceiver is stopped or stopping in all
appropriate places, allowing us to remove the ability to end the source
for any other reason. A side-effect of this is that:
- The track never ends prematurely, fixing https://crbug.com/1315611.
- Removed transceivers are always stopped, fixing
  https://crbug.com/webrtc/14005.

This CL fixes the issue of track being ended in the ontrack event when
running https://jsfiddle.net/henbos/nxebusjm/.
- We don't have WPT test coverage for this, so I'll add that separately.

With SetSourceEnded() removed, some stopping/stop in response to
rejecting locally SDP munged content had to be added in order not to
regress the existing test coverage for this:
*PeerConnectionInterfaceTest.RejectMediaContent/1

Bug: chromium:1315611, webrtc:14005.
Change-Id: I21f30a1259e51324066dc84f72a72485b9e0fadc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260180
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36669}
This commit is contained in:
Henrik Boström 2022-04-27 11:54:01 +02:00 committed by WebRTC LUCI CQ
parent a92d051e0f
commit a8ad11de82
9 changed files with 49 additions and 32 deletions

View File

@ -167,11 +167,6 @@ void AudioRtpReceiver::Stop() {
track_->internal()->set_ended();
}
void AudioRtpReceiver::SetSourceEnded() {
RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
source_->SetState(MediaSourceInterface::kEnded);
}
// RTC_RUN_ON(&signaling_thread_checker_)
void AudioRtpReceiver::RestartMediaChannel(absl::optional<uint32_t> ssrc) {
bool enabled = track_->internal()->enabled();

View File

@ -98,7 +98,6 @@ class AudioRtpReceiver : public ObserverInterface,
// RtpReceiverInternal implementation.
void Stop() override;
void SetSourceEnded() override;
void SetupMediaChannel(uint32_t ssrc) override;
void SetupUnsignaledMediaChannel() override;
uint32_t ssrc() const override;

View File

@ -1927,10 +1927,17 @@ TEST_F(PeerConnectionJsepTest, RollbackRemovesTransceiver) {
caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
auto callee = CreatePeerConnection();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
ASSERT_EQ(callee->pc()->GetTransceivers().size(), 1u);
auto transceiver = callee->pc()->GetTransceivers()[0];
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
EXPECT_EQ(callee->pc()->GetTransceivers().size(), 0u);
EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
// The removed transceiver should be stopped and its receiver track should be
// ended.
EXPECT_TRUE(transceiver->stopping());
EXPECT_TRUE(transceiver->stopping());
EXPECT_EQ(transceiver->receiver()->track()->state(),
MediaStreamTrackInterface::kEnded);
}
TEST_F(PeerConnectionJsepTest, RollbackKeepsTransceiverAndClearsMid) {
@ -1949,6 +1956,13 @@ TEST_F(PeerConnectionJsepTest, RollbackKeepsTransceiverAndClearsMid) {
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
// Because the transceiver is reusable, it must not be stopped and its
// receiver track must still be live.
auto transceiver = callee->pc()->GetTransceivers()[0];
EXPECT_FALSE(transceiver->stopping());
EXPECT_FALSE(transceiver->stopping());
EXPECT_EQ(transceiver->receiver()->track()->state(),
MediaStreamTrackInterface::kLive);
}
TEST_F(PeerConnectionJsepTest,

View File

@ -47,15 +47,6 @@ class RtpReceiverInternal : public RtpReceiverInterface {
// state set to `kEnded`, a final state that cannot be reversed.
virtual void Stop() = 0;
// Call on the signaling thread to set the source's state to `ended` before
// clearing the media channel (`SetMediaChannel(nullptr)`) on the worker
// thread.
// The difference between `Stop()` and `SetSourceEnded()` is that the latter
// does not change the state of the associated track.
// NOTE: Calling this function should be followed with a call to
// `SetMediaChannel(nullptr)` on the worker thread, to complete the operation.
virtual void SetSourceEnded() = 0;
// Sets the underlying MediaEngine channel associated with this RtpSender.
// A VoiceMediaChannel should be used for audio RtpSenders and
// a VideoMediaChannel should be used for video RtpSenders.

View File

@ -216,12 +216,6 @@ void RtpTransceiver::SetChannel(
RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1);
if (!channel_) {
for (const auto& receiver : receivers_)
receiver->internal()->SetSourceEnded();
RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); // There should not be an invoke.
}
if (channel_to_delete || !senders_.empty() || !receivers_.empty()) {
channel_manager_->worker_thread()->Invoke<void>(RTC_FROM_HERE, [&]() {
auto* media_channel = channel_ ? channel_->media_channel() : nullptr;

View File

@ -2933,6 +2933,7 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) {
if (transceiver->internal()->reused_for_addtrack()) {
transceiver->internal()->set_created_by_addtrack(true);
} else {
transceiver->internal()->StopTransceiverProcedure();
transceivers()->Remove(transceiver);
}
}
@ -3395,9 +3396,9 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiversAndDataChannels(
AssociateTransceiver(source, new_session.GetType(), i, new_content,
old_local_content, old_remote_content);
if (!transceiver_or_error.ok()) {
// In the case where a transceiver is rejected locally, we don't
// expect to find a transceiver, but might find it in the case
// where state is still "stopping", not "stopped".
// In the case where a transceiver is rejected locally prior to being
// associated, we don't expect to find a transceiver, but might find it
// in the case where state is still "stopping", not "stopped".
if (new_content.rejected) {
continue;
}
@ -3406,6 +3407,36 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiversAndDataChannels(
auto transceiver = transceiver_or_error.MoveValue();
RTCError error =
UpdateTransceiverChannel(transceiver, new_content, bundle_group);
// Handle locally rejected content. This code path is only needed for apps
// that SDP munge. Remote rejected content is handled in
// ApplyRemoteDescriptionUpdateTransceiverState().
if (source == cricket::ContentSource::CS_LOCAL && new_content.rejected) {
// Local offer.
if (new_session.GetType() == SdpType::kOffer) {
// If the RtpTransceiver API was used, it would already have made the
// transceiver stopping. But if the rejection was caused by SDP
// munging then we need to ensure the transceiver is stopping here.
if (!transceiver->internal()->stopping()) {
transceiver->internal()->StopStandard();
}
RTC_DCHECK(transceiver->internal()->stopping());
} else {
// Local answer.
RTC_DCHECK(new_session.GetType() == SdpType::kAnswer ||
new_session.GetType() == SdpType::kPrAnswer);
// When RtpTransceiver API is used, rejection happens in the offer and
// the transceiver will already be stopped at local answer time
// (calling stop between SRD(offer) and SLD(answer) would not reject
// the content in the answer - instead this would trigger a follow-up
// O/A exchange). So if the content was rejected but the transceiver
// is not already stopped, SDP munging has happened and we need to
// ensure the transceiver is stopped.
if (!transceiver->internal()->stopped()) {
transceiver->internal()->StopTransceiverProcedure();
}
RTC_DCHECK(transceiver->internal()->stopped());
}
}
if (!error.ok()) {
return error;
}

View File

@ -57,7 +57,6 @@ class MockRtpReceiverInternal : public RtpReceiverInternal {
// RtpReceiverInternal methods.
MOCK_METHOD(void, Stop, (), (override));
MOCK_METHOD(void, SetSourceEnded, (), (override));
MOCK_METHOD(void, SetMediaChannel, (cricket::MediaChannel*), (override));
MOCK_METHOD(void, SetupMediaChannel, (uint32_t), (override));
MOCK_METHOD(void, SetupUnsignaledMediaChannel, (), (override));

View File

@ -114,11 +114,6 @@ void VideoRtpReceiver::Stop() {
track_->internal()->set_ended();
}
void VideoRtpReceiver::SetSourceEnded() {
RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
source_->SetState(MediaSourceInterface::kEnded);
}
// RTC_RUN_ON(&signaling_thread_checker_)
void VideoRtpReceiver::RestartMediaChannel(absl::optional<uint32_t> ssrc) {
MediaSourceInterface::SourceState state = source_->state();

View File

@ -88,7 +88,6 @@ class VideoRtpReceiver : public RtpReceiverInternal {
// RtpReceiverInternal implementation.
void Stop() override;
void SetSourceEnded() override;
void SetupMediaChannel(uint32_t ssrc) override;
void SetupUnsignaledMediaChannel() override;
uint32_t ssrc() const override;