Improve test outcomes for WebRTC-PayloadTypesInTransport
Previous CL left tests that did not succeed when field trial
"WebRTC-PayloadTypesInTransport" was enabled.
This fixes most of them.
The changes involve rewriting the fake media engine and letting a peer connection factory have its own CodecVendor member.
Bug: webrtc:360058654
Change-Id: Idb722fb6da52e8b2af2786a9c5331c19ecd52b3d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/380841
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44103}
diff --git a/media/base/codec_list.cc b/media/base/codec_list.cc
index da1d1d0..9fa1b3c 100644
--- a/media/base/codec_list.cc
+++ b/media/base/codec_list.cc
@@ -63,6 +63,15 @@
// TODO: https://issues.webrtc.org/384756622 - reject codec earlier and
// enable check. RTC_DCHECK(apt_it != codec.params.end()); Until that is
// fixed:
+ if (codec.id == Codec::kIdNotSet) {
+ // Should not have an apt parameter.
+ if (apt_it != codec.params.end()) {
+ RTC_LOG(LS_WARNING) << "Surprising condition: RTX codec without "
+ << "PT has an apt parameter";
+ }
+ // Stop checking the associated PT.
+ break;
+ }
if (apt_it == codec.params.end()) {
RTC_LOG(LS_WARNING) << "Surprising condition: RTX codec without"
<< " apt parameter: " << codec;
@@ -70,6 +79,8 @@
}
int associated_pt;
if (!(rtc::FromString(apt_it->second, &associated_pt))) {
+ RTC_LOG(LS_ERROR) << "Non-numeric argument to rtx apt: " << codec
+ << " apt=" << apt_it->second;
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
"Non-numeric argument to rtx apt parameter");
}
diff --git a/media/base/codec_list_unittest.cc b/media/base/codec_list_unittest.cc
index e7303f0..6dea0cd 100644
--- a/media/base/codec_list_unittest.cc
+++ b/media/base/codec_list_unittest.cc
@@ -33,6 +33,7 @@
std::vector<Codec> apt_without_number{
CreateVideoCodec({webrtc::SdpVideoFormat{
"rtx", webrtc::CodecParameterMap{{"apt", "not-a-number"}}}})};
+ apt_without_number[0].id = 96;
RTCErrorOr<CodecList> checked_codec_list =
CodecList::Create(apt_without_number);
EXPECT_FALSE(checked_codec_list.ok());
@@ -51,6 +52,7 @@
std::vector<Codec> apt_without_number{
CreateVideoCodec({webrtc::SdpVideoFormat{
"rtx", webrtc::CodecParameterMap{{"apt", "not-a-number"}}}})};
+ apt_without_number[0].id = 96;
#if RTC_DCHECK_IS_ON
EXPECT_DEATH(
CodecList bad = CodecList::CreateFromTrustedData(apt_without_number),
diff --git a/media/base/fake_media_engine.cc b/media/base/fake_media_engine.cc
index 298efd6..a2d56d0 100644
--- a/media/base/fake_media_engine.cc
+++ b/media/base/fake_media_engine.cc
@@ -559,7 +559,10 @@
return false;
}
-FakeVoiceEngine::FakeVoiceEngine() {
+FakeVoiceEngine::FakeVoiceEngine()
+ : encoder_factory_(webrtc::make_ref_counted<FakeVoiceEncoderFactory>(this)),
+ decoder_factory_(
+ webrtc::make_ref_counted<FakeVoiceDecoderFactory>(this)) {
// 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)});
diff --git a/media/base/fake_media_engine.h b/media/base/fake_media_engine.h
index 36c0d81..1f92789 100644
--- a/media/base/fake_media_engine.h
+++ b/media/base/fake_media_engine.h
@@ -818,10 +818,10 @@
const std::vector<Codec>& LegacySendCodecs() const override;
const std::vector<Codec>& LegacyRecvCodecs() const override;
webrtc::AudioEncoderFactory* encoder_factory() const override {
- return nullptr;
+ return encoder_factory_.get();
}
webrtc::AudioDecoderFactory* decoder_factory() const override {
- return nullptr;
+ return decoder_factory_.get();
}
void SetCodecs(const std::vector<Codec>& codecs);
void SetRecvCodecs(const std::vector<Codec>& codecs);
@@ -837,8 +837,66 @@
std::vector<webrtc::RtpHeaderExtensionCapability> header_extensions);
private:
+ class FakeVoiceEncoderFactory : public webrtc::AudioEncoderFactory {
+ public:
+ explicit FakeVoiceEncoderFactory(FakeVoiceEngine* owner) : owner_(owner) {}
+ std::vector<webrtc::AudioCodecSpec> GetSupportedEncoders() override {
+ // The reason for this convoluted mapping is because there are
+ // too many tests that expect to push codecs into the fake voice
+ // engine's "send_codecs/recv_codecs" and have them show up later.
+ std::vector<webrtc::AudioCodecSpec> specs;
+ for (const auto& codec : owner_->send_codecs_) {
+ specs.push_back(webrtc::AudioCodecSpec{
+ {codec.name, codec.clockrate, codec.channels},
+ {codec.clockrate, codec.channels, codec.bitrate}});
+ }
+ return specs;
+ }
+ std::optional<webrtc::AudioCodecInfo> QueryAudioEncoder(
+ const webrtc::SdpAudioFormat& format) override {
+ return std::nullopt;
+ }
+ absl::Nullable<std::unique_ptr<webrtc::AudioEncoder>> Create(
+ const webrtc::Environment& env,
+ const webrtc::SdpAudioFormat& format,
+ Options options) override {
+ return nullptr;
+ }
+ FakeVoiceEngine* owner_;
+ };
+ class FakeVoiceDecoderFactory : public webrtc::AudioDecoderFactory {
+ public:
+ explicit FakeVoiceDecoderFactory(FakeVoiceEngine* owner) : owner_(owner) {}
+ std::vector<webrtc::AudioCodecSpec> GetSupportedDecoders() override {
+ // The reason for this convoluted mapping is because there are
+ // too many tests that expect to push codecs into the fake voice
+ // engine's "send_codecs/recv_codecs" and have them show up later.
+ std::vector<webrtc::AudioCodecSpec> specs;
+ for (const auto& codec : owner_->recv_codecs_) {
+ specs.push_back(webrtc::AudioCodecSpec{
+ {codec.name, codec.clockrate, codec.channels},
+ {codec.clockrate, codec.channels, codec.bitrate}});
+ }
+ return specs;
+ }
+ bool IsSupportedDecoder(const webrtc::SdpAudioFormat& format) override {
+ return false;
+ }
+ absl::Nullable<std::unique_ptr<webrtc::AudioDecoder>> Create(
+ const webrtc::Environment& env,
+ const webrtc::SdpAudioFormat& format,
+ std::optional<webrtc::AudioCodecPairId> codec_pair_id) override {
+ return nullptr;
+ }
+
+ private:
+ FakeVoiceEngine* owner_;
+ };
+
std::vector<Codec> recv_codecs_;
std::vector<Codec> send_codecs_;
+ webrtc::scoped_refptr<FakeVoiceEncoderFactory> encoder_factory_;
+ webrtc::scoped_refptr<FakeVoiceDecoderFactory> decoder_factory_;
std::vector<webrtc::RtpHeaderExtensionCapability> header_extensions_;
friend class FakeMediaEngine;
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index a07b5d6..f147c49 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -2420,6 +2420,7 @@
":pc_test_utils",
":peer_connection",
":peer_connection_factory",
+ ":peer_connection_factory_proxy",
":peer_connection_internal",
":peer_connection_proxy",
":peerconnection_wrapper",
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index cb6a581..bc07ab3 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -107,6 +107,10 @@
rtc::scoped_refptr<ConnectionContext> context,
PeerConnectionFactoryDependencies* dependencies)
: context_(context),
+ codec_vendor_(context_->media_engine(),
+ context_->use_rtx(),
+ context_->env().field_trials()),
+
event_log_factory_(std::move(dependencies->event_log_factory)),
fec_controller_factory_(std::move(dependencies->fec_controller_factory)),
network_state_predictor_factory_(
@@ -147,19 +151,17 @@
RtpCapabilities PeerConnectionFactory::GetRtpSenderCapabilities(
cricket::MediaType kind) const {
RTC_DCHECK_RUN_ON(signaling_thread());
- cricket::CodecVendor codec_vendor(media_engine(), context_->use_rtx(),
- context_->env().field_trials());
switch (kind) {
case cricket::MEDIA_TYPE_AUDIO: {
cricket::Codecs cricket_codecs;
- cricket_codecs = codec_vendor.audio_send_codecs().codecs();
+ cricket_codecs = codec_vendor_.audio_send_codecs().codecs();
auto extensions =
GetDefaultEnabledRtpHeaderExtensions(media_engine()->voice());
return ToRtpCapabilities(cricket_codecs, extensions);
}
case cricket::MEDIA_TYPE_VIDEO: {
cricket::Codecs cricket_codecs;
- cricket_codecs = codec_vendor.video_send_codecs().codecs();
+ cricket_codecs = codec_vendor_.video_send_codecs().codecs();
auto extensions =
GetDefaultEnabledRtpHeaderExtensions(media_engine()->video());
return ToRtpCapabilities(cricket_codecs, extensions);
@@ -176,19 +178,17 @@
RtpCapabilities PeerConnectionFactory::GetRtpReceiverCapabilities(
cricket::MediaType kind) const {
RTC_DCHECK_RUN_ON(signaling_thread());
- cricket::CodecVendor codec_vendor(media_engine(), context_->use_rtx(),
- context_->env().field_trials());
switch (kind) {
case cricket::MEDIA_TYPE_AUDIO: {
cricket::Codecs cricket_codecs;
- cricket_codecs = codec_vendor.audio_recv_codecs().codecs();
+ cricket_codecs = codec_vendor_.audio_recv_codecs().codecs();
auto extensions =
GetDefaultEnabledRtpHeaderExtensions(media_engine()->voice());
return ToRtpCapabilities(cricket_codecs, extensions);
}
case cricket::MEDIA_TYPE_VIDEO: {
cricket::Codecs cricket_codecs =
- codec_vendor.video_recv_codecs().codecs();
+ codec_vendor_.video_recv_codecs().codecs();
auto extensions =
GetDefaultEnabledRtpHeaderExtensions(media_engine()->video());
return ToRtpCapabilities(cricket_codecs, extensions);
diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h
index 3d99c13..0b1e167 100644
--- a/pc/peer_connection_factory.h
+++ b/pc/peer_connection_factory.h
@@ -39,6 +39,8 @@
#include "call/call.h"
#include "call/rtp_transport_controller_send_factory_interface.h"
#include "media/base/media_engine.h"
+#include "p2p/base/port_allocator.h"
+#include "pc/codec_vendor.h"
#include "pc/connection_context.h"
#include "rtc_base/thread.h"
#include "rtc_base/thread_annotations.h"
@@ -107,6 +109,7 @@
}
cricket::MediaEngineInterface* media_engine() const;
+ cricket::CodecVendor& CodecVendorForTesting() { return codec_vendor_; }
protected:
// Constructor used by the static Create() method. Modifies the dependencies.
@@ -134,6 +137,7 @@
rtc::scoped_refptr<ConnectionContext> context_;
PeerConnectionFactoryInterface::Options options_
RTC_GUARDED_BY(signaling_thread());
+ cricket::CodecVendor codec_vendor_;
std::unique_ptr<RtcEventLogFactoryInterface> event_log_factory_;
std::unique_ptr<FecControllerFactoryInterface> fec_controller_factory_;
std::unique_ptr<NetworkStatePredictorFactoryInterface>
diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc
index 3f301cd..f656494c 100644
--- a/pc/peer_connection_media_unittest.cc
+++ b/pc/peer_connection_media_unittest.cc
@@ -72,27 +72,30 @@
using ::testing::Bool;
using ::testing::Combine;
using ::testing::ElementsAre;
+using ::testing::Gt;
using ::testing::HasSubstr;
using ::testing::NotNull;
using ::testing::Values;
-cricket::MediaSendChannelInterface* SendChannelInternal(
+RtpTransceiver* RtpTransceiverInternal(
rtc::scoped_refptr<RtpTransceiverInterface> transceiver) {
auto transceiver_with_internal = static_cast<
RefCountedObject<RtpTransceiverProxyWithInternal<RtpTransceiver>>*>(
transceiver.get());
auto transceiver_internal =
static_cast<RtpTransceiver*>(transceiver_with_internal->internal());
+ return transceiver_internal;
+}
+
+cricket::MediaSendChannelInterface* SendChannelInternal(
+ rtc::scoped_refptr<RtpTransceiverInterface> transceiver) {
+ auto transceiver_internal = RtpTransceiverInternal(transceiver);
return transceiver_internal->channel()->media_send_channel();
}
cricket::MediaReceiveChannelInterface* ReceiveChannelInternal(
rtc::scoped_refptr<RtpTransceiverInterface> transceiver) {
- auto transceiver_with_internal = static_cast<
- RefCountedObject<RtpTransceiverProxyWithInternal<RtpTransceiver>>*>(
- transceiver.get());
- auto transceiver_internal =
- static_cast<RtpTransceiver*>(transceiver_with_internal->internal());
+ auto transceiver_internal = RtpTransceiverInternal(transceiver);
return transceiver_internal->channel()->media_receive_channel();
}
@@ -1550,7 +1553,8 @@
auto caller = CreatePeerConnectionWithAudio(std::move(fake_engine));
- auto transceiver = caller->pc()->GetTransceivers().front();
+ auto transceiver =
+ RtpTransceiverInternal(caller->pc()->GetTransceivers().front());
auto codecs =
caller->pc_factory()
->GetRtpSenderCapabilities(cricket::MediaType::MEDIA_TYPE_AUDIO)
@@ -1564,7 +1568,7 @@
codec.name == cricket::kUlpfecCodecName);
});
codecs_only_rtx_red_fec.erase(it, codecs_only_rtx_red_fec.end());
-
+ ASSERT_THAT(codecs_only_rtx_red_fec.size(), Gt(0));
auto result = transceiver->SetCodecPreferences(codecs_only_rtx_red_fec);
EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type());
}
diff --git a/pc/typed_codec_vendor.cc b/pc/typed_codec_vendor.cc
index fd320cd..74741ae 100644
--- a/pc/typed_codec_vendor.cc
+++ b/pc/typed_codec_vendor.cc
@@ -100,20 +100,29 @@
bool is_sender,
bool rtx_enabled,
const webrtc::FieldTrialsView& trials) {
- // TODO: https://issues.webrtc.org/360058654 - move codec selection here
- // when field trial WebRTC-PayloadTypesInTransport is enabled.
if (trials.IsEnabled("WebRTC-PayloadTypesInTransport")) {
// Get the capabilities from the factory and compute the codecs.
- // Use legacy mechanisms for getting codecs from media engine.
if (type == MEDIA_TYPE_AUDIO) {
if (is_sender) {
- codecs_ = CodecList::CreateFromTrustedData(CollectAudioCodecs(
- media_engine->voice().encoder_factory()->GetSupportedEncoders()));
+ if (media_engine->voice().encoder_factory()) {
+ codecs_ = CodecList::CreateFromTrustedData(CollectAudioCodecs(
+ media_engine->voice().encoder_factory()->GetSupportedEncoders()));
+ } else {
+ RTC_LOG(LS_WARNING)
+ << "No voice encoder factory. Should only happen in test.";
+ }
} else {
- codecs_ = CodecList::CreateFromTrustedData(CollectAudioCodecs(
- media_engine->voice().decoder_factory()->GetSupportedDecoders()));
+ if (media_engine->voice().decoder_factory()) {
+ codecs_ = CodecList::CreateFromTrustedData(CollectAudioCodecs(
+ media_engine->voice().decoder_factory()->GetSupportedDecoders()));
+ } else {
+ RTC_LOG(LS_WARNING)
+ << "No voice decoder factory. Should only happen in test.";
+ }
}
} else {
+ // Use legacy mechanisms for getting codecs from video engine.
+ // TODO: https://issues.webrtc.org/360058654 - apply late assign to video.
if (is_sender) {
codecs_ = CodecList::CreateFromTrustedData(
media_engine->video().LegacySendCodecs(rtx_enabled));
