Skip to content

Commit

Permalink
Make CreateSendChannel and CreateReceiveChannel methods pure virtual
Browse files Browse the repository at this point in the history 
These methods previously had a default implementation that triggered
a crash. All implementations must now return a valid object, which
simplifies the code that calls them.

Bug: webrtc:13931
Change-Id: I877fbc929b58c6b83767c6ac5a81c8aa942e3fef
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/369021
Commit-Queue: Tomas Gunnarsson <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#43453}
  • Loading branch information
Tommi authored and WebRTC LUCI CQ committed Nov 26, 2024
1 parent d171832 commit 98b3588
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 83 deletions.
17 changes: 2 additions & 15 deletions media/base/fake_media_engine.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ bool FakeVideoMediaReceiveChannel::GetStats(VideoMediaReceiveInfo* /* info */) {
return false;
}

FakeVoiceEngine::FakeVoiceEngine() : fail_create_channel_(false) {
FakeVoiceEngine::FakeVoiceEngine() {
// Add a fake audio codec. Note that the name must not be "" as there are
// sanity checks against that.
SetCodecs({cricket::CreateAudioCodec(101, "fake_audio_codec", 8000, 1)});
Expand Down Expand Up @@ -606,8 +606,7 @@ void FakeVoiceEngine::SetRtpHeaderExtensions(
header_extensions_ = std::move(header_extensions);
}

FakeVideoEngine::FakeVideoEngine()
: capture_(false), fail_create_channel_(false) {
FakeVideoEngine::FakeVideoEngine() : capture_(false) {
// Add a fake video codec. Note that the name must not be "" as there are
// sanity checks against that.
send_codecs_.push_back(cricket::CreateVideoCodec(111, "fake_video_codec"));
Expand All @@ -625,10 +624,6 @@ FakeVideoEngine::CreateSendChannel(
const webrtc::CryptoOptions& /* crypto_options */,
webrtc::
VideoBitrateAllocatorFactory* /* video_bitrate_allocator_factory */) {
if (fail_create_channel_) {
return nullptr;
}

std::unique_ptr<FakeVideoMediaSendChannel> ch =
std::make_unique<FakeVideoMediaSendChannel>(options,
call->network_thread());
Expand All @@ -640,10 +635,6 @@ FakeVideoEngine::CreateReceiveChannel(
const MediaConfig& /* config */,
const VideoOptions& options,
const webrtc::CryptoOptions& /* crypto_options */) {
if (fail_create_channel_) {
return nullptr;
}

std::unique_ptr<FakeVideoMediaReceiveChannel> ch =
std::make_unique<FakeVideoMediaReceiveChannel>(options,
call->network_thread());
Expand Down Expand Up @@ -697,9 +688,5 @@ void FakeMediaEngine::SetVideoCodecs(const std::vector<Codec>& codecs) {
video_->SetSendCodecs(codecs);
video_->SetRecvCodecs(codecs);
}
void FakeMediaEngine::set_fail_create_channel(bool fail) {
voice_->fail_create_channel_ = fail;
video_->fail_create_channel_ = fail;
}

} // namespace cricket
4 changes: 0 additions & 4 deletions media/base/fake_media_engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,6 @@ class FakeVoiceEngine : public VoiceEngineInterface {
private:
std::vector<Codec> recv_codecs_;
std::vector<Codec> send_codecs_;
bool fail_create_channel_;
std::vector<webrtc::RtpHeaderExtensionCapability> header_extensions_;

friend class FakeMediaEngine;
Expand Down Expand Up @@ -847,7 +846,6 @@ class FakeVideoEngine : public VideoEngineInterface {
std::vector<Codec> recv_codecs_;
bool capture_;
VideoOptions options_;
bool fail_create_channel_;
std::vector<webrtc::RtpHeaderExtensionCapability> header_extensions_;

friend class FakeMediaEngine;
Expand All @@ -864,8 +862,6 @@ class FakeMediaEngine : public CompositeMediaEngine {
void SetAudioSendCodecs(const std::vector<Codec>& codecs);
void SetVideoCodecs(const std::vector<Codec>& codecs);

void set_fail_create_channel(bool fail);

FakeVoiceEngine* fake_voice_engine() { return voice_; }
FakeVideoEngine* fake_video_engine() { return video_; }

Expand Down
26 changes: 5 additions & 21 deletions media/base/media_engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,22 +109,14 @@ class VoiceEngineInterface : public RtpHeaderExtensionQueryInterface {
const MediaConfig& /* config */,
const AudioOptions& /* options */,
const webrtc::CryptoOptions& /* crypto_options */,
webrtc::AudioCodecPairId /* codec_pair_id */) {
// TODO(hta): Make pure virtual when all downstream has updated
RTC_CHECK_NOTREACHED();
return nullptr;
}
webrtc::AudioCodecPairId /* codec_pair_id */) = 0;

virtual std::unique_ptr<VoiceMediaReceiveChannelInterface>
CreateReceiveChannel(webrtc::Call* /* call */,
const MediaConfig& /* config */,
const AudioOptions& /* options */,
const webrtc::CryptoOptions& /* crypto_options */,
webrtc::AudioCodecPairId /* codec_pair_id */) {
// TODO(hta): Make pure virtual when all downstream has updated
RTC_CHECK_NOTREACHED();
return nullptr;
}
webrtc::AudioCodecPairId /* codec_pair_id */) = 0;

// Legacy: Retrieve list of supported codecs.
// + protection codecs, and assigns PT numbers that may have to be
Expand Down Expand Up @@ -159,22 +151,14 @@ class VideoEngineInterface : public RtpHeaderExtensionQueryInterface {
const MediaConfig& /* config */,
const VideoOptions& /* options */,
const webrtc::CryptoOptions& /* crypto_options */,
webrtc::
VideoBitrateAllocatorFactory* /* video_bitrate_allocator_factory */) {
// Default implementation, delete when all is updated
RTC_CHECK_NOTREACHED();
return nullptr;
}
webrtc::VideoBitrateAllocatorFactory*
/* video_bitrate_allocator_factory */) = 0;

virtual std::unique_ptr<VideoMediaReceiveChannelInterface>
CreateReceiveChannel(webrtc::Call* /* call */,
const MediaConfig& /* config */,
const VideoOptions& /* options */,
const webrtc::CryptoOptions& /* crypto_options */) {
// Default implementation, delete when all is updated
RTC_CHECK_NOTREACHED();
return nullptr;
}
const webrtc::CryptoOptions& /* crypto_options */) = 0;

// Legacy: Retrieve list of supported codecs.
// + protection codecs, and assigns PT numbers that may have to be
Expand Down
23 changes: 0 additions & 23 deletions pc/peer_connection_media_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,29 +278,6 @@ class PeerConnectionMediaTestPlanB : public PeerConnectionMediaBaseTest {
: PeerConnectionMediaBaseTest(SdpSemantics::kPlanB_DEPRECATED) {}
};

TEST_P(PeerConnectionMediaTest,
FailToSetRemoteDescriptionIfCreateMediaChannelFails) {
auto caller = CreatePeerConnectionWithAudioVideo();
auto callee = CreatePeerConnectionWithAudioVideo();
callee->media_engine()->set_fail_create_channel(true);

std::string error;
ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer(), &error));
EXPECT_THAT(error,
HasSubstr("Failed to set remote offer sdp: Failed to create"));
}

TEST_P(PeerConnectionMediaTest,
FailToSetLocalDescriptionIfCreateMediaChannelFails) {
auto caller = CreatePeerConnectionWithAudioVideo();
caller->media_engine()->set_fail_create_channel(true);

std::string error;
ASSERT_FALSE(caller->SetLocalDescription(caller->CreateOffer(), &error));
EXPECT_THAT(error,
HasSubstr("Failed to set local offer sdp: Failed to create"));
}

std::vector<std::string> GetIds(
const std::vector<cricket::StreamParams>& streams) {
std::vector<std::string> ids;
Expand Down
27 changes: 7 additions & 20 deletions pc/rtp_transceiver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ RtpTransceiver::RtpTransceiver(cricket::MediaType media_type,
context_(context) {
RTC_DCHECK(media_type == cricket::MEDIA_TYPE_AUDIO ||
media_type == cricket::MEDIA_TYPE_VIDEO);
RTC_DCHECK(context_);
}

RtpTransceiver::RtpTransceiver(
Expand All @@ -151,6 +152,7 @@ RtpTransceiver::RtpTransceiver(
header_extensions_to_negotiate_(
std::move(header_extensions_to_negotiate)),
on_negotiation_needed_(std::move(on_negotiation_needed)) {
RTC_DCHECK(context_);
RTC_DCHECK(media_type_ == cricket::MEDIA_TYPE_AUDIO ||
media_type_ == cricket::MEDIA_TYPE_VIDEO);
RTC_DCHECK_EQ(sender->media_type(), receiver->media_type());
Expand Down Expand Up @@ -214,18 +216,20 @@ RTCError RtpTransceiver::CreateChannel(
VideoBitrateAllocatorFactory* video_bitrate_allocator_factory,
std::function<RtpTransportInternal*(absl::string_view)> transport_lookup) {
RTC_DCHECK_RUN_ON(thread_);
RTC_DCHECK(!channel());

if (!media_engine()) {
// TODO(hta): Must be a better way
return RTCError(RTCErrorType::INTERNAL_ERROR,
"No media engine for mid=" + std::string(mid));
}

std::unique_ptr<cricket::ChannelInterface> new_channel;
if (media_type() == cricket::MEDIA_TYPE_AUDIO) {
// TODO(bugs.webrtc.org/11992): CreateVideoChannel internally switches to
// the worker thread. We shouldn't be using the `call_ptr_` hack here but
// simply be on the worker thread and use `call_` (update upstream code).
RTC_DCHECK(call_ptr);
RTC_DCHECK(media_engine());
// TODO(bugs.webrtc.org/11992): Remove this workaround after updates in
// PeerConnection and add the expectation that we're already on the right
// thread.
Expand All @@ -238,17 +242,10 @@ RTCError RtpTransceiver::CreateChannel(
media_send_channel = media_engine()->voice().CreateSendChannel(
call_ptr, media_config, audio_options, crypto_options,
codec_pair_id);
if (!media_send_channel) {
// TODO(bugs.webrtc.org/14912): Consider CHECK or reporting failure
return;
}
std::unique_ptr<cricket::VoiceMediaReceiveChannelInterface>
media_receive_channel = media_engine()->voice().CreateReceiveChannel(
call_ptr, media_config, audio_options, crypto_options,
codec_pair_id);
if (!media_receive_channel) {
return;
}
// Note that this is safe because both sending and
// receiving channels will be deleted at the same time.
media_send_channel->SetSsrcListChangedCallback(
Expand Down Expand Up @@ -276,16 +273,9 @@ RTCError RtpTransceiver::CreateChannel(
media_send_channel = media_engine()->video().CreateSendChannel(
call_ptr, media_config, video_options, crypto_options,
video_bitrate_allocator_factory);
if (!media_send_channel) {
return;
}

std::unique_ptr<cricket::VideoMediaReceiveChannelInterface>
media_receive_channel = media_engine()->video().CreateReceiveChannel(
call_ptr, media_config, video_options, crypto_options);
if (!media_receive_channel) {
return;
}
// Note that this is safe because both sending and
// receiving channels will be deleted at the same time.
media_send_channel->SetSsrcListChangedCallback(
Expand All @@ -301,11 +291,6 @@ RTCError RtpTransceiver::CreateChannel(
context()->ssrc_generator());
});
}
if (!new_channel) {
// TODO(hta): Must be a better way
return RTCError(RTCErrorType::INTERNAL_ERROR,
"Failed to create channel for mid=" + std::string(mid));
}
SetChannel(std::move(new_channel), transport_lookup);
return RTCError::OK();
}
Expand Down Expand Up @@ -339,6 +324,8 @@ void RtpTransceiver::SetChannel(
// helps with keeping the channel implementation requirements being met and
// avoids synchronization for accessing the pointer or network related state.
context()->network_thread()->BlockingCall([&]() {
// TODO(tommi): Isn't `channel_` guaranteed to be nullptr here? (i.e.
// remove this block).
if (channel_) {
channel_->SetFirstPacketReceivedCallback(nullptr);
channel_->SetFirstPacketSentCallback(nullptr);
Expand Down
1 change: 1 addition & 0 deletions pc/rtp_transceiver_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ TEST_F(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) {
// Clear the current channel - required to allow SetChannel()
EXPECT_CALL(*channel1_ptr, SetFirstPacketReceivedCallback(_));
transceiver->ClearChannel();
ASSERT_EQ(nullptr, transceiver->channel());
// Channel can no longer be set, so this call should be a no-op.
transceiver->SetChannel(std::move(channel2),
[](const std::string&) { return nullptr; });
Expand Down

0 comments on commit 98b3588

Please sign in to comment.