diff --git a/media/base/fake_media_engine.cc b/media/base/fake_media_engine.cc index 8c084815f6..602ef80777 100644 --- a/media/base/fake_media_engine.cc +++ b/media/base/fake_media_engine.cc @@ -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)}); @@ -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")); @@ -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 ch = std::make_unique(options, call->network_thread()); @@ -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 ch = std::make_unique(options, call->network_thread()); @@ -697,9 +688,5 @@ void FakeMediaEngine::SetVideoCodecs(const std::vector& 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 diff --git a/media/base/fake_media_engine.h b/media/base/fake_media_engine.h index 44c046ba38..e741b555b3 100644 --- a/media/base/fake_media_engine.h +++ b/media/base/fake_media_engine.h @@ -805,7 +805,6 @@ class FakeVoiceEngine : public VoiceEngineInterface { private: std::vector recv_codecs_; std::vector send_codecs_; - bool fail_create_channel_; std::vector header_extensions_; friend class FakeMediaEngine; @@ -847,7 +846,6 @@ class FakeVideoEngine : public VideoEngineInterface { std::vector recv_codecs_; bool capture_; VideoOptions options_; - bool fail_create_channel_; std::vector header_extensions_; friend class FakeMediaEngine; @@ -864,8 +862,6 @@ class FakeMediaEngine : public CompositeMediaEngine { void SetAudioSendCodecs(const std::vector& codecs); void SetVideoCodecs(const std::vector& codecs); - void set_fail_create_channel(bool fail); - FakeVoiceEngine* fake_voice_engine() { return voice_; } FakeVideoEngine* fake_video_engine() { return video_; } diff --git a/media/base/media_engine.h b/media/base/media_engine.h index ea89116cf6..85692f8421 100644 --- a/media/base/media_engine.h +++ b/media/base/media_engine.h @@ -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 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 @@ -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 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 diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc index 13280158d3..c34e98cb7b 100644 --- a/pc/peer_connection_media_unittest.cc +++ b/pc/peer_connection_media_unittest.cc @@ -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 GetIds( const std::vector& streams) { std::vector ids; diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index c7aaf91968..807cf26880 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -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( @@ -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()); @@ -214,18 +216,20 @@ RTCError RtpTransceiver::CreateChannel( VideoBitrateAllocatorFactory* video_bitrate_allocator_factory, std::function 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 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. @@ -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 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( @@ -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 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( @@ -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(); } @@ -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); diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 9979154388..2b13d37291 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -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; });