PT: Fix video and audio RED codec handling in TypedCodecVendor and CodecVendor For video, append RED codec (and its RTX) after all media codecs but before FEC codecs in TypedCodecVendor::codecs(). For audio, keep RED together with OPUS in CodecVendor negotiation. Bug: webrtc:360058654 Change-Id: Icc87c99aa30fb46d33ff2121f06d9b6c9c607e9a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/476242 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47860}
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index 47f4881..b6f17c1 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc
@@ -371,11 +371,20 @@ if (!error.ok()) { return error; } + + // Handle Audio RED immediately after the primary codec. + if (config.codec.type == Codec::Type::kAudio && config.resiliency.red) { + error = MergeRedCodec(config, primary_codec, mid, offered_codecs, + pt_suggester, pick_from_top_of_range); + if (!error.ok()) { + return error; + } + } } - // Pass 2: RED + // Pass 2: RED for Video for (const CodecConfiguration& config : configurations) { - if (!config.resiliency.red) { + if (!config.resiliency.red || config.codec.type == Codec::Type::kAudio) { continue; } // Find the primary codec in offered_codecs to pass to MergeRedCodec.
diff --git a/pc/peer_connection_stability_integrationtest.cc b/pc/peer_connection_stability_integrationtest.cc index 6c6db4e..dec846b 100644 --- a/pc/peer_connection_stability_integrationtest.cc +++ b/pc/peer_connection_stability_integrationtest.cc
@@ -460,13 +460,13 @@ {.factory_id = FactorySignature::Id::kWebRtcTipOfTreeWithPayloadTypeRedesign, .caller_local = {"1 [111:audio/opus/48000/2;minptime=10;useinbandfec=1]", + "1 [63:audio/red/48000/2;=111/111]", "1 [9:audio/G722/8000/1]", "1 [0:audio/PCMU/8000/1]", "1 [8:audio/PCMA/8000/1]", "1 [13:audio/CN/8000/1]", "1 [110:audio/telephone-event/48000/1]", "1 [126:audio/telephone-event/8000/1]", - "1 [63:audio/red/48000/2;=111/111]", "2 [96:video/VP8/90000/0]", "2 [97:video/rtx/90000/0;apt=96]", "2 " @@ -510,13 +510,13 @@ "2 [121:video/ulpfec/90000/0]"}, .caller_remote = {"1 [111:audio/opus/48000/2;minptime=10;useinbandfec=1]", + "1 [63:audio/red/48000/2;=111/111]", "1 [9:audio/G722/8000/1]", "1 [0:audio/PCMU/8000/1]", "1 [8:audio/PCMA/8000/1]", "1 [13:audio/CN/8000/1]", "1 [110:audio/telephone-event/48000/1]", "1 [126:audio/telephone-event/8000/1]", - "1 [63:audio/red/48000/2;=111/111]", "2 [96:video/VP8/90000/0]", "2 [97:video/rtx/90000/0;apt=96]", "2 " @@ -559,13 +559,13 @@ "2 [120:video/rtx/90000/0;apt=119]", "2 [121:video/ulpfec/90000/0]"}, .callee_local = {"1 [111:audio/opus/48000/2;minptime=10;useinbandfec=1]", + "1 [63:audio/red/48000/2;=111/111]", "1 [9:audio/G722/8000/1]", "1 [0:audio/PCMU/8000/1]", "1 [8:audio/PCMA/8000/1]", "1 [13:audio/CN/8000/1]", "1 [110:audio/telephone-event/48000/1]", "1 [126:audio/telephone-event/8000/1]", - "1 [63:audio/red/48000/2;=111/111]", "2 [96:video/VP8/90000/0]", "2 [97:video/rtx/90000/0;apt=96]", "2 " @@ -609,13 +609,13 @@ "2 [121:video/ulpfec/90000/0]"}, .callee_remote = {"1 [111:audio/opus/48000/2;minptime=10;useinbandfec=1]", + "1 [63:audio/red/48000/2;=111/111]", "1 [9:audio/G722/8000/1]", "1 [0:audio/PCMU/8000/1]", "1 [8:audio/PCMA/8000/1]", "1 [13:audio/CN/8000/1]", "1 [110:audio/telephone-event/48000/1]", "1 [126:audio/telephone-event/8000/1]", - "1 [63:audio/red/48000/2;=111/111]", "2 [96:video/VP8/90000/0]", "2 [97:video/rtx/90000/0;apt=96]", "2 "
diff --git a/pc/typed_codec_vendor.cc b/pc/typed_codec_vendor.cc index bf62693..6152e95 100644 --- a/pc/typed_codec_vendor.cc +++ b/pc/typed_codec_vendor.cc
@@ -188,6 +188,10 @@ bool rtx_enabled) { Codecs out; flat_set<std::string> shared_added; + bool video_red_needed = false; + bool video_ulpfec_needed = false; + bool video_flexfec_needed = false; + for (const auto& config : configurations) { out.push_back(config.codec); if (type == MediaType::AUDIO) { @@ -198,23 +202,33 @@ if (config.resiliency.rtx) { out.push_back(CreateVideoCodec(PayloadType::NotSet(), kRtxCodecName)); } - if (config.resiliency.red && shared_added.insert(kRedCodecName).second) { - out.push_back(CreateVideoCodec(kRedCodecName)); - // Video RED also gets an RTX codec. - if (rtx_enabled) { - out.push_back(CreateVideoCodec(PayloadType::NotSet(), kRtxCodecName)); - } + if (config.resiliency.red) { + video_red_needed = true; } - if (config.resiliency.ulpfec && - shared_added.insert(kUlpfecCodecName).second) { - out.push_back(CreateVideoCodec(kUlpfecCodecName)); + if (config.resiliency.ulpfec) { + video_ulpfec_needed = true; } - if (config.resiliency.flexfec && - shared_added.insert(kFlexfecCodecName).second) { - out.push_back(CreateVideoCodec(kFlexfecCodecName)); + if (config.resiliency.flexfec) { + video_flexfec_needed = true; } } } + + // Add video resiliency codecs at the end, in the order: RED, FEC + if (type == MediaType::VIDEO) { + if (video_red_needed) { + out.push_back(CreateVideoCodec(kRedCodecName)); + if (rtx_enabled) { + out.push_back(CreateVideoCodec(PayloadType::NotSet(), kRtxCodecName)); + } + } + if (video_ulpfec_needed) { + out.push_back(CreateVideoCodec(kUlpfecCodecName)); + } + if (video_flexfec_needed) { + out.push_back(CreateVideoCodec(kFlexfecCodecName)); + } + } return out; }