PT: Replicate legacy answer negotiation for directionality Stricter directionality checks in the redesign path caused failures in legacy unit tests that expected codecs to be included in answers regardless of directionality. This CL reverts to the legacy behavior (union of capabilities) and adds a TODO to make it more restrictive in the future. Also corrected documentation examples for --force_fieldtrials flag. Bug: webrtc:360058654 Change-Id: Ic9df6e9f4b5570e711cbc434177af057bb115ec1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/477440 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47872}
diff --git a/GEMINI.md b/GEMINI.md index 2d7601f..1a69884 100644 --- a/GEMINI.md +++ b/GEMINI.md
@@ -1,19 +1,21 @@ # General workflow -@agents/prompts/common.md -@agents/README.md +@agents/prompts/common.md @agents/README.md + +**Important:** `depot_tools` must be in your `PATH` for working in this checkout +(e.g., for `git new-branch` to work). # Searching Downstream When auditing symbol usage or finding references in downstream projects, always prefer high-precision CodeSearch filters over raw text searches: -- **Semantic References:** Use `usage:webrtc::SymbolName` to find actual - call-sites. -- **Virtual Overrides:** Use `func:MethodName` to find overrides in downstream - implementations. -- **Exclude WebRTC Mirrors:** Always append `-file:stable/webrtc` to focus on - true downstream consumers. +- **Semantic References:** Use `usage:webrtc::SymbolName` to find actual + call-sites. +- **Virtual Overrides:** Use `func:MethodName` to find overrides in downstream + implementations. +- **Exclude WebRTC Mirrors:** Always append `-file:stable/webrtc` to focus on + true downstream consumers. # Codebase knowledge
diff --git a/g3doc/todo/payload_type_redesign.md b/g3doc/todo/payload_type_redesign.md index ffae917..2052ebb 100644 --- a/g3doc/todo/payload_type_redesign.md +++ b/g3doc/todo/payload_type_redesign.md
@@ -121,19 +121,20 @@ The redesign is now largely implemented for both audio and video codecs when the `WebRTC-PayloadTypesInTransport` field trial is enabled. Key milestones reached: -- **Bifurcated Negotiation Logic:** `CodecVendor` now has separate paths for - legacy and redesigned PT allocation. The redesigned path uses - `CodecConfiguration` and `MergeCodecsFromConfigurations` for all media types. -- **Unified Resiliency Expansion:** Late expansion of RTX, RED, ULPFEC, and - FlexFEC is handled uniformly in `pc/codec_vendor.cc`. -- **Audio/Video RED Collision:** Fixed by enforcing media type equality in - matching rules. -- **MID Recycling:** Correctly handled with media type validation, preventing - invalid codec merging when MIDs are reused. -- **Stable PT Assignment:** Verified to maintain payload type stability across - renegotiations and codec preference changes. -- **Conventional RTX Assignment:** RTX PTs now default to `Primary_PT + 1` to - maintain backwards compatibility with legacy expectations. +- **Bifurcated Negotiation Logic:** `CodecVendor` now has separate paths for + legacy and redesigned PT allocation. The redesigned path uses + `CodecConfiguration` and `MergeCodecsFromConfigurations` for all media + types. +- **Unified Resiliency Expansion:** Late expansion of RTX, RED, ULPFEC, and + FlexFEC is handled uniformly in `pc/codec_vendor.cc`. +- **Audio/Video RED Collision:** Fixed by enforcing media type equality in + matching rules. +- **MID Recycling:** Correctly handled with media type validation, preventing + invalid codec merging when MIDs are reused. +- **Stable PT Assignment:** Verified to maintain payload type stability across + renegotiations and codec preference changes. +- **Conventional RTX Assignment:** RTX PTs now default to `Primary_PT + 1` to + maintain backwards compatibility with legacy expectations. The implementation is verified by a dedicated suite of integration tests in `pc/codec_vendor_redesign_unittest.cc`. @@ -148,22 +149,22 @@ Introduced in `pc/codec_configuration.h`: -- **`ResiliencyInfo`**: Encapsulates the redundancy requirements (RTX, RED, - ULPFEC, FlexFEC). -- **`CodecConfiguration`**: Stores codec attributes and their associated - `ResiliencyInfo`. This allows the engine to express capabilities without - pre-assigning payload types. +- **`ResiliencyInfo`**: Encapsulates the redundancy requirements (RTX, RED, + ULPFEC, FlexFEC). +- **`CodecConfiguration`**: Stores codec attributes and their associated + `ResiliencyInfo`. This allows the engine to express capabilities without + pre-assigning payload types. ### 2. Unified Codec Collection `TypedCodecVendor` handles the bifurcated collection path: -- **Redesigned Path**: Collects `CodecConfiguration` objects from the media - engine factories. It also performs a "legacy expansion" to populate the - internal `codecs()` list for compatibility with existing code that expects - pre-assigned PTs. -- **Legacy Path**: Continues to use the engine's `LegacySendCodecs` / - `LegacyRecvCodecs` methods. +- **Redesigned Path**: Collects `CodecConfiguration` objects from the media + engine factories. It also performs a "legacy expansion" to populate the + internal `codecs()` list for compatibility with existing code that expects + pre-assigned PTs. +- **Legacy Path**: Continues to use the engine's `LegacySendCodecs` / + `LegacyRecvCodecs` methods. ### 3. Late Expansion and Parameter Linking @@ -177,7 +178,8 @@ 3. Links redundancy codecs to the primary PT (e.g., setting the `apt` parameter for RTX). -**Current Status:** RTX and RED linking are fully unified. RED linking for audio has been refactored into the unified expansion logic in `MergeRedCodec`. +**Current Status:** RTX and RED linking are fully unified. RED linking for audio +has been refactored into the unified expansion logic in `MergeRedCodec`. ## Testing Strategy @@ -195,11 +197,30 @@ the field trial flag to ensure there are no regressions, and then either ask to commit this set of changes or loop back to step 1. +### Canary Branch Strategy + +To ensure that no unit tests are missed, a "canary branch" approach is used. + +1. **Canary Branch (`pt-enable`)**: Maintain a branch where the field trial is + forced enabled by default. This branch is used to run the full WebRTC test + suite (especially `rtc_pc_unittests` and `peerconnection_unittests`) to + identify all edge cases and legacy behaviors that the redesign logic doesn't + yet handle. +2. **Reproduction and Isolation**: When a failure is identified on the canary + branch, the specific test case is cloned or ported into a specialized + integration test file (`pc/codec_vendor_redesign_unittest.cc`) on the + implementation branch. This allows for focused debugging and ensures the + failure is reproducible in a clean environment with the trial explicitly + enabled. +3. **Surgical Fixes**: Fixes are developed and verified on the implementation + branch using the isolated tests. +4. **Full Re-verification**: Once the implementation branch is stable, the + canary branch is rebased to include the fixes, and the full test suite is + run again to ensure no remaining failures and to catch new regressions. + ## Backwards Compatibility for Unit Testing -Test helpers like `CodecLookupHelperForTesting` are used in legacy unit tests -to "pre-seed" the `FakePayloadTypeSuggester` with hardcoded PT expectations. -This allows tests that depend on specific PT values to pass while the -underlying allocation logic transitions to a more generic, transport-aware -strategy. - +Test helpers like `CodecLookupHelperForTesting` are used in legacy unit tests to +"pre-seed" the `FakePayloadTypeSuggester` with hardcoded PT expectations. This +allows tests that depend on specific PT values to pass while the underlying +allocation logic transitions to a more generic, transport-aware strategy.
diff --git a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc index 32b5da4..a37483d 100644 --- a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc +++ b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc
@@ -39,8 +39,8 @@ force_fieldtrials, "", "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enable/" - " will assign the group Enable to field trial WebRTC-FooFeature."); + "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enabled/" + " will assign the group Enabled to field trial WebRTC-FooFeature."); ABSL_FLAG(int, pcmu, TestConfig::default_pcmu(), "RTP payload type for PCM-u"); ABSL_FLAG(int, pcma, TestConfig::default_pcma(), "RTP payload type for PCM-a"); ABSL_FLAG(int, isac, TestConfig::default_isac(), "RTP payload type for iSAC");
diff --git a/modules/audio_processing/test/audioproc_float_impl.cc b/modules/audio_processing/test/audioproc_float_impl.cc index 39ee176..63ceabb 100644 --- a/modules/audio_processing/test/audioproc_float_impl.cc +++ b/modules/audio_processing/test/audioproc_float_impl.cc
@@ -338,8 +338,8 @@ force_fieldtrials, "", "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enable/" - " will assign the group Enable to field trial WebRTC-FooFeature."); + "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enabled/" + " will assign the group Enabled to field trial WebRTC-FooFeature."); ABSL_FLAG(std::string, ree_model,
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index b6f17c1..4896234 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc
@@ -1222,10 +1222,25 @@ } } if (payload_types_in_transport_) { - // Redesign path: Use configurations to merge supported codecs. - MergeCodecsByDirection(media_description_options.type, answer_rtd, mid, - filtered_codecs, pt_suggester, - /*pick_from_top_of_range=*/false); + // TODO(webrtc:360058654): This is not according to the specification, + // which says we should only include codecs valid for the negotiated + // direction. We include all supported codecs to match legacy behavior. + // Make this more restrictive in the future. + const std::vector<CodecConfiguration>& send_configs = + (media_description_options.type == MediaType::AUDIO) + ? audio_send_codecs_.configurations() + : video_send_codecs_.configurations(); + const std::vector<CodecConfiguration>& recv_configs = + (media_description_options.type == MediaType::AUDIO) + ? audio_recv_codecs_.configurations() + : video_recv_codecs_.configurations(); + + MergeCodecsFromConfigurations(send_configs, mid, filtered_codecs, + pt_suggester, trials_, + /*pick_from_top_of_range=*/false); + MergeCodecsFromConfigurations(recv_configs, mid, filtered_codecs, + pt_suggester, trials_, + /*pick_from_top_of_range=*/false); } else { // Merge other_codecs into filtered_codecs, resolving PT conflicts. MergeCodecsLegacy(supported_codecs, mid, filtered_codecs, pt_suggester);
diff --git a/pc/codec_vendor_redesign_unittest.cc b/pc/codec_vendor_redesign_unittest.cc index e0eb284..7abca4b 100644 --- a/pc/codec_vendor_redesign_unittest.cc +++ b/pc/codec_vendor_redesign_unittest.cc
@@ -498,5 +498,58 @@ EXPECT_EQ(second_it->packetization, second_codec.packetization); } +TEST_F(CodecVendorRedesignTest, TestCodecsInAnswerStricterDirectionality) { + std::vector<Codec> offer_send_codecs({ + CreateAudioCodec(40, "codec0", 16000, 1), + CreateAudioCodec(41, "codec1", 8000, 1), + }); + std::vector<Codec> answer_recv_codecs({ + CreateAudioCodec(40, "codec0", 16000, 1), + CreateAudioCodec(41, "codec1", 8000, 1), + }); + std::vector<Codec> answer_send_codecs({ + CreateAudioCodec(42, "codec2", 8000, 1), + }); + + media_engine_.SetAudioSendCodecs(offer_send_codecs); + media_engine_.SetAudioRecvCodecs(offer_send_codecs); + + auto offer_vendor = std::make_unique<CodecVendor>( + &media_engine_, /*rtx_enabled=*/true, trials_); + + FakeMediaEngine answer_media_engine; + answer_media_engine.SetAudioSendCodecs(answer_send_codecs); + answer_media_engine.SetAudioRecvCodecs(answer_recv_codecs); + + auto answer_vendor = std::make_unique<CodecVendor>( + &answer_media_engine, /*rtx_enabled=*/true, trials_); + + MediaDescriptionOptions offer_opts(MediaType::AUDIO, "audio", + RtpTransceiverDirection::kSendOnly, + /*stopped=*/false); + auto offer_result = offer_vendor->GetNegotiatedCodecsForOffer( + offer_opts, MediaSessionOptions(), /*current_content=*/nullptr, + pt_suggester_); + ASSERT_TRUE(offer_result.ok()); + + MediaDescriptionOptions answer_opts(MediaType::AUDIO, "audio", + RtpTransceiverDirection::kSendOnly, + /*stopped=*/false); + + auto answer_result = answer_vendor->GetNegotiatedCodecsForAnswer( + answer_opts, MediaSessionOptions(), RtpTransceiverDirection::kSendOnly, + RtpTransceiverDirection::kInactive, /*current_content=*/nullptr, + offer_result.value(), pt_suggester_); + + ASSERT_TRUE(answer_result.ok()); + + const auto& answer_codecs = answer_result.value(); + + // We expect codec0 and codec1 to be present if we want to match legacy + // behavior. + EXPECT_THAT(answer_codecs, Contains(Field(&Codec::name, "codec0"))); + EXPECT_THAT(answer_codecs, Contains(Field(&Codec::name, "codec1"))); +} + } // namespace } // namespace webrtc
diff --git a/rtc_tools/data_channel_benchmark/data_channel_benchmark.cc b/rtc_tools/data_channel_benchmark/data_channel_benchmark.cc index e21fec2..c8baf2e 100644 --- a/rtc_tools/data_channel_benchmark/data_channel_benchmark.cc +++ b/rtc_tools/data_channel_benchmark/data_channel_benchmark.cc
@@ -61,8 +61,8 @@ force_fieldtrials, "", "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enable/" - " will assign the group Enable to field trial WebRTC-FooFeature."); + "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enabled/" + " will assign the group Enabled to field trial WebRTC-FooFeature."); struct SetupMessage { size_t packet_size;
diff --git a/test/create_test_field_trials.cc b/test/create_test_field_trials.cc index f95e9a1..c1644f4 100644 --- a/test/create_test_field_trials.cc +++ b/test/create_test_field_trials.cc
@@ -20,8 +20,8 @@ force_fieldtrials, "", "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enable/" - " will assign the group Enable to field trial WebRTC-FooFeature."); + "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enabled/" + " will assign the group Enabled to field trial WebRTC-FooFeature."); namespace webrtc {