fix!: correctly support wildcard payload types in feedback lines#27
fix!: correctly support wildcard payload types in feedback lines#27
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds proper support for wildcard payload type (*) in SDP rtcp-fb lines (e.g., a=rtcp-fb:* nack pli), which Chrome v147+ is starting to emit. Previously these lines would fail to parse and fall back to UnknownLine. The change introduces a PayloadTypeRef type wrapping number | '*' and a new CodecStore class to separate per-codec from wildcard feedback storage.
Changes:
- New
PayloadTypeRefclass andCodecStoreclass;RtcpFbLineregex updated to match*;RtpMapLine,FmtpLine,RtcpFbLineconstructors updated to acceptPayloadTypeRefinstead ofnumber. AvMediaDescription.codecschanged fromMap<number, CodecInfo>toCodecStore; codec-related line routing refactored to useCodecStore.addLine.disableRtcpFbValueinmunge.tssimplified to delegate toCodecStore.removeFeedback, correctly handling wildcard feedback.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lines/payload-type-ref.ts |
New class modeling a payload type reference as number or '*' |
src/lines/payload-type-ref.spec.ts |
Tests for the new PayloadTypeRef class |
src/lines/rtcpfb-line.ts |
Updated regex and parsing to support wildcard *; payloadType field changed to PayloadTypeRef |
src/lines/rtcpfb-line.spec.ts |
New tests for wildcard and numeric parsing/serialization of RtcpFbLine |
src/lines/rtpmap-line.ts |
payloadType field changed to PayloadTypeRef; constructor updated |
src/lines/fmtp-line.ts |
payloadType field changed to PayloadTypeRef; constructor updated |
src/lines/index.ts |
Exports PayloadTypeRef |
src/model/codec-store.ts |
New CodecStore class handling per-codec and wildcard feedback |
src/model/codec-store.spec.ts |
Tests for CodecStore |
src/model/codec-info.ts |
Uses PayloadTypeRef in toLines() |
src/model/av-media-description.ts |
Uses CodecStore instead of raw Map<number, CodecInfo> |
src/model/index.ts |
Exports CodecStore |
src/munge.ts |
Delegates feedback removal to CodecStore.removeFeedback |
src/utils.spec.ts |
Uses PayloadTypeRef when constructing RtpMapLine |
src/parser.spec.ts |
New test for parsing an SDP with wildcard rtcp-fb lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| toLines(): Line[] { | ||
| const lines: Line[] = []; | ||
| this.codecs.forEach((codec) => lines.push(...codec.toLines())); | ||
| this.wildcardFeedback.forEach((fb) => { | ||
| lines.push(new RtcpFbLine(new PayloadTypeRef('*'), fb)); | ||
| }); | ||
| return lines; | ||
| } |
There was a problem hiding this comment.
The toLines() method always emits all per-codec lines first, then wildcard feedback lines at the end. However, in the wildcard_rtcpfb.sdp corpus file (and in real-world Chrome SDPs), wildcard rtcp-fb lines appear before per-codec feedback lines. After a round-trip parse-and-serialize, the ordering changes: a=rtcp-fb:96 ccm fir will appear before a=rtcp-fb:* goog-remb, a=rtcp-fb:* nack, etc.
While the compareSdps test helper ignores ordering so the round-trip test passes, and RFC 4585 doesn't mandate a specific ordering of rtcp-fb lines, this reordering could be a concern for interoperability with SDP consumers that are sensitive to ordering. Consider whether preserving the original relative order of wildcard and per-codec lines is desirable. Note that this is consistent with the codebase's existing behavior of reordering lines during serialization (as documented in src/sdp-corpus/ordering.ts).
There was a problem hiding this comment.
I think this is probably fine as-is.
Description
This change adds support for wildcard payload types in feedback lines, e.g.:
Chrome is starting to use in v147. Currently, these would fail to parse but would fallback to
UnknownLinewhich is fine as long as no manipulation around them is needed.The design of this fix introduces a new
PayloadTypeReftype that models the payload type as anumberor*. There are still places, though, where we want a "resolved" payload type (number only), so the naming of this type was chosen to try and distinguish an SDP-syntax "reference" to a payload type, not an actual payload type (for whichnumberis still used).This change implements...
Is this a breaking change?
payloadtype member on some types as changed, as well as some constructor argument typesI certify that...
Generative AI (GAI) Usage Disclosure
Reference: Cisco GAI Coding Guidelines
If a GAI tool/IDE was used then select the category that best describes GAI usage in this PR: