Skip to content

Commit 43989b4

Browse files
hensmi-amazonhensmi-amazon
authored
Avoid Chrome ERROR logs by ensuring all PT have same parameters on SDP. (#3120)
1 parent a155c25 commit 43989b4

File tree

5 files changed

+199
-37
lines changed

5 files changed

+199
-37
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1919
- Added new attributes signalingCloseCode, signalingCloseReason, and signalingCloseWasClean to signalingDropped events. These are not always guaranteed to be set.
2020
- Reset additional internal state on reconnect to fix issues with receiving content share in replica meetings when using the priority policy.
2121
- Avoid trying to send leave message on refresh, and just let browser close with expected 1001 Websocket status code.
22+
- Avoid Chrome ERROR log that starts with "A BUNDLE group contains a codec collision for payload_type=..." by ensuring all payload type have same parameters on SDP.
2223

2324
### Fixed
2425

‎docs/classes/sdp.html‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -629,16 +629,23 @@ <h3>with<wbr>Starting<wbr>Video<wbr>Send<wbr>Bitrate</h3>
629629
<li class="tsd-description">
630630
<aside class="tsd-sources">
631631
<ul>
632-
<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/sdp/SDP.ts#L914">src/sdp/SDP.ts:914</a></li>
632+
<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/sdp/SDP.ts#L923">src/sdp/SDP.ts:923</a></li>
633633
</ul>
634634
</aside>
635635
<div class="tsd-comment tsd-typography">
636636
<div class="lead">
637637
<p>Modifies the active camera section to include a specified starting bitrate
638638
for video sending by adding a &#39;x-google-start-bitrate&#39; fmtp line paramter for
639-
each payload type associated with video.</p>
639+
each non-RTX/FEC payload type associated with video.</p>
640640
</div>
641-
<p>If no active camera section is found in the SDP, returns the original SDP object.</p>
641+
<p>Note that this updates all video sections regardless of direction, as
642+
<a href="https://www.rfc-editor.org/rfc/rfc8843#name-payload-type-pt-value-reuse">https://www.rfc-editor.org/rfc/rfc8843#name-payload-type-pt-value-reuse</a> states that
643+
&quot;...all codecs associated with the payload type number MUST share an identical codec configuration.
644+
This means that the codecs MUST share the same media type, encoding name,
645+
clock rate, and any parameter that can affect the codec configuration and packetization.&quot;</p>
646+
<p>WebRTC maintainers may eventual enforce this (<a href="https://issues.webrtc.org/issues/42224689">https://issues.webrtc.org/issues/42224689</a>),
647+
though it just logs an error for now.</p>
648+
<p>If no active video sections are found in the SDP, returns the original SDP object.</p>
642649
</div>
643650
<h4 class="tsd-parameters-title">Parameters</h4>
644651
<ul class="tsd-parameters">

‎src/sdp/SDP.ts‎

Lines changed: 64 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -907,35 +907,79 @@ export default class SDP {
907907
/**
908908
* Modifies the active camera section to include a specified starting bitrate
909909
* for video sending by adding a 'x-google-start-bitrate' fmtp line paramter for
910-
* each payload type associated with video.
910+
* each non-RTX/FEC payload type associated with video.
911911
*
912-
* If no active camera section is found in the SDP, returns the original SDP object.
912+
* Note that this updates all video sections regardless of direction, as
913+
* https://www.rfc-editor.org/rfc/rfc8843#name-payload-type-pt-value-reuse states that
914+
* "...all codecs associated with the payload type number MUST share an identical codec configuration.
915+
* This means that the codecs MUST share the same media type, encoding name,
916+
* clock rate, and any parameter that can affect the codec configuration and packetization."
917+
*
918+
* WebRTC maintainers may eventual enforce this (https://issues.webrtc.org/issues/42224689),
919+
* though it just logs an error for now.
920+
*
921+
* If no active video sections are found in the SDP, returns the original SDP object.
913922
*/
914923
withStartingVideoSendBitrate(bitrateKbps: number): SDP {
915924
const sections = SDP.splitSections(this.sdp);
916925

917-
const cameraLineIndex: number = SDP.findActiveCameraSection(sections);
918-
if (cameraLineIndex === -1) {
919-
return this;
920-
}
926+
for (let i = 0; i < sections.length; i++) {
927+
if (!/^m=video/.test(sections[i])) {
928+
continue;
929+
}
921930

922-
const srcLines: string[] = SDP.splitLines(sections[cameraLineIndex]);
923-
const dstLines: string[] = [];
931+
const srcLines: string[] = SDP.splitLines(sections[i]);
932+
const dstLines: string[] = [];
924933

925-
for (const line of srcLines) {
926-
if (/^a=fmtp:\d*/.test(line)) {
927-
// `x-google-start-bitrate` is an unofficial flag that has existed in libwebrtc since its release and is unlikely
928-
// to be removed without notification.
929-
//
930-
// libwebrtc constant: https://webrtc.googlesource.com/src/+/b6ef1a736ee94d97cc28f3bd59b826c716a3278f/media/base/media_constants.cc#97
931-
// libwebrtc parsing: https://webrtc.googlesource.com/src/+/61c5e86dca59b0ac8240444e7df5c31da27ee40f/media/engine/webrtc_media_engine.cc#172
932-
const newLine = line + `;x-google-start-bitrate=${bitrateKbps}`;
933-
dstLines.push(newLine);
934-
} else {
935-
dstLines.push(line);
934+
let hasFmtp = false;
935+
let payloadType: string | undefined;
936+
937+
// First pass: check if fmtp exists and get payload type
938+
for (const line of srcLines) {
939+
if (/^a=fmtp:\d*/.test(line) && !/apt=/.test(line)) {
940+
hasFmtp = true;
941+
}
942+
// Extract payload type from rtpmap line (e.g., "a=rtpmap:96 VP8/90000")
943+
const rtpmapMatch = line.match(/^a=rtpmap:(\d+)/);
944+
if (rtpmapMatch) {
945+
payloadType = rtpmapMatch[1];
946+
}
936947
}
948+
949+
// Second pass: add bitrate parameter. We avoid adding this to retransmission payload types since it
950+
// is not relevant to those.
951+
for (const line of srcLines) {
952+
if (
953+
/^a=fmtp:\d*/.test(line) &&
954+
!/apt=/.test(line) &&
955+
!/x-google-start-bitrate=/.test(line)
956+
) {
957+
// `x-google-start-bitrate` is an unofficial flag that has existed in libwebrtc since its release and is unlikely
958+
// to be removed without notification.
959+
//
960+
// libwebrtc constant: https://webrtc.googlesource.com/src/+/b6ef1a736ee94d97cc28f3bd59b826c716a3278f/media/base/media_constants.cc#97
961+
// libwebrtc parsing: https://webrtc.googlesource.com/src/+/61c5e86dca59b0ac8240444e7df5c31da27ee40f/media/engine/webrtc_media_engine.cc#172
962+
const newLine = line + `;x-google-start-bitrate=${bitrateKbps}`;
963+
dstLines.push(newLine);
964+
} else {
965+
dstLines.push(line);
966+
}
967+
}
968+
969+
// If no fmtp line exists, add one after the rtpmap line
970+
if (!hasFmtp && payloadType) {
971+
const rtpmapIndex = dstLines.findIndex(line => line.match(/^a=rtpmap:\d+/));
972+
if (rtpmapIndex !== -1) {
973+
dstLines.splice(
974+
rtpmapIndex + 1,
975+
0,
976+
`a=fmtp:${payloadType} x-google-start-bitrate=${bitrateKbps}`
977+
);
978+
}
979+
}
980+
981+
sections[i] = dstLines.join(SDP.CRLF) + SDP.CRLF;
937982
}
938-
sections[cameraLineIndex] = dstLines.join(SDP.CRLF) + SDP.CRLF;
939983

940984
const newSdp = sections.join('');
941985
return new SDP(newSdp);

‎test/sdp/SDP.test.ts‎

Lines changed: 117 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -643,16 +643,125 @@ describe('SDP', () => {
643643
});
644644

645645
describe('withStartingVideoSendBitrate', () => {
646-
it('Returns original if no video', () => {
647-
const sdpObj = new SDP(SafariSDPMock.IOS_SAFARI_AUDIO_SENDRECV_VIDEO_INACTIVE);
648-
expect(sdpObj.withStartingVideoSendBitrate(1000)).to.equal(sdpObj);
649-
});
650-
651-
it('Adds starting video send bitrate', () => {
646+
it('Adds starting video send bitrate to non-apt lines', () => {
652647
const sdpObj = new SDP(SDPMock.LOCAL_OFFER_WITH_AUDIO_VIDEO);
653-
expect(sdpObj.withStartingVideoSendBitrate(1000).sdp).to.deep.equal(
654-
SDPMock.LOCAL_OFFER_WITH_AUDIO_VIDEO_WITH_STARTING_VIDEO_SEND_BITRATE
648+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
649+
// Should add to VP8 (no apt)
650+
expect(result).to.not.include('a=fmtp:97 apt=96;x-google-start-bitrate');
651+
// Should add to VP9 profile-id lines
652+
expect(result).to.include('a=fmtp:98 profile-id=0;x-google-start-bitrate=1000');
653+
expect(result).to.include('a=fmtp:100 profile-id=2;x-google-start-bitrate=1000');
654+
// Should add to H264 lines
655+
expect(result).to.include(
656+
'level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f;x-google-start-bitrate=1000'
655657
);
656658
});
659+
660+
it('Applies to all video sections', () => {
661+
const sdpWithMultipleVideo = `v=0\r
662+
o=- 123 0 IN IP4 127.0.0.1\r
663+
s=-\r
664+
t=0 0\r
665+
m=video 9 UDP/TLS/RTP/SAVPF 96\r
666+
a=rtpmap:96 VP8/90000\r
667+
a=fmtp:96 max-fr=30\r
668+
m=video 9 UDP/TLS/RTP/SAVPF 97\r
669+
a=rtpmap:97 VP9/90000\r
670+
a=fmtp:97 profile-id=0\r
671+
`;
672+
const sdpObj = new SDP(sdpWithMultipleVideo);
673+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
674+
expect(result).to.include('a=fmtp:96 max-fr=30;x-google-start-bitrate=1000');
675+
expect(result).to.include('a=fmtp:97 profile-id=0;x-google-start-bitrate=1000');
676+
});
677+
678+
it('Does not add to lines with apt=', () => {
679+
const sdpWithApt = `v=0\r
680+
o=- 123 0 IN IP4 127.0.0.1\r
681+
s=-\r
682+
t=0 0\r
683+
m=video 9 UDP/TLS/RTP/SAVPF 96 97\r
684+
a=rtpmap:96 VP8/90000\r
685+
a=fmtp:96 max-fr=30\r
686+
a=rtpmap:97 rtx/90000\r
687+
a=fmtp:97 apt=96\r
688+
`;
689+
const sdpObj = new SDP(sdpWithApt);
690+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
691+
expect(result).to.include('a=fmtp:96 max-fr=30;x-google-start-bitrate=1000');
692+
expect(result).to.include('a=fmtp:97 apt=96\r');
693+
expect(result).to.not.include('apt=96;x-google-start-bitrate');
694+
});
695+
696+
it('Does not add if already present', () => {
697+
const sdpWithExisting = `v=0\r
698+
o=- 123 0 IN IP4 127.0.0.1\r
699+
s=-\r
700+
t=0 0\r
701+
m=video 9 UDP/TLS/RTP/SAVPF 96\r
702+
a=rtpmap:96 VP8/90000\r
703+
a=fmtp:96 max-fr=30;x-google-start-bitrate=500\r
704+
`;
705+
const sdpObj = new SDP(sdpWithExisting);
706+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
707+
expect(result).to.include('x-google-start-bitrate=500');
708+
expect(result).to.not.include('x-google-start-bitrate=1000');
709+
// Ensure it's only present once
710+
const matches = result.match(/x-google-start-bitrate/g);
711+
expect(matches).to.have.lengthOf(1);
712+
});
713+
714+
it('Creates fmtp line when missing', () => {
715+
const sdpWithoutFmtp = `v=0\r
716+
o=- 123 0 IN IP4 127.0.0.1\r
717+
s=-\r
718+
t=0 0\r
719+
m=video 9 UDP/TLS/RTP/SAVPF 96\r
720+
a=rtpmap:96 VP8/90000\r
721+
`;
722+
const sdpObj = new SDP(sdpWithoutFmtp);
723+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
724+
expect(result).to.include('a=fmtp:96 x-google-start-bitrate=1000');
725+
// Verify fmtp line is added after rtpmap
726+
const lines = result.split('\r\n');
727+
const rtpmapIndex = lines.findIndex(line => line.includes('a=rtpmap:96'));
728+
const fmtpIndex = lines.findIndex(line => line.includes('a=fmtp:96'));
729+
expect(fmtpIndex).to.equal(rtpmapIndex + 1);
730+
});
731+
732+
it('Creates fmtp line for multiple video sections without fmtp', () => {
733+
const sdpWithoutFmtp = `v=0\r
734+
o=- 123 0 IN IP4 127.0.0.1\r
735+
s=-\r
736+
t=0 0\r
737+
m=video 9 UDP/TLS/RTP/SAVPF 96\r
738+
a=rtpmap:96 VP8/90000\r
739+
m=video 9 UDP/TLS/RTP/SAVPF 97\r
740+
a=rtpmap:97 VP9/90000\r
741+
`;
742+
const sdpObj = new SDP(sdpWithoutFmtp);
743+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
744+
expect(result).to.include('a=fmtp:96 x-google-start-bitrate=1000');
745+
expect(result).to.include('a=fmtp:97 x-google-start-bitrate=1000');
746+
});
747+
748+
it('Handles mixed sections with and without fmtp', () => {
749+
const sdpMixed = `v=0\r
750+
o=- 123 0 IN IP4 127.0.0.1\r
751+
s=-\r
752+
t=0 0\r
753+
m=video 9 UDP/TLS/RTP/SAVPF 96\r
754+
a=rtpmap:96 VP8/90000\r
755+
m=video 9 UDP/TLS/RTP/SAVPF 97\r
756+
a=rtpmap:97 VP9/90000\r
757+
a=fmtp:97 profile-id=0\r
758+
`;
759+
const sdpObj = new SDP(sdpMixed);
760+
const result = sdpObj.withStartingVideoSendBitrate(1000).sdp;
761+
// First section should get new fmtp line
762+
expect(result).to.include('a=fmtp:96 x-google-start-bitrate=1000');
763+
// Second section should append to existing fmtp
764+
expect(result).to.include('a=fmtp:97 profile-id=0;x-google-start-bitrate=1000');
765+
});
657766
});
658767
});

‎test/task/SetLocalDescriptionTask.test.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ chai.should();
2626

2727
describe('SetLocalDescriptionTask', () => {
2828
const expect: Chai.ExpectStatic = chai.expect;
29+
const CRLF = '\r\n';
2930
let context: AudioVideoControllerState;
3031
let domMockBuilder: DOMMockBuilder;
3132
let domMockBehavior: DOMMockBehavior;
@@ -59,21 +60,21 @@ describe('SetLocalDescriptionTask', () => {
5960
it('can be run and succeed', async () => {
6061
await task.run();
6162
const peerLocalSDP = context.peer.localDescription.sdp;
62-
expect(peerLocalSDP).to.be.equal(sdpOffer.sdp);
63+
expect(peerLocalSDP).to.be.equal(sdpOffer.sdp + CRLF);
6364
});
6465

6566
it('can be run and succeed in iOS 15.1', async () => {
6667
domMockBehavior.browserName = 'ios15.1';
6768
domMockBuilder = new DOMMockBuilder(domMockBehavior);
6869
await task.run();
6970
const peerLocalSDP = context.peer.localDescription.sdp;
70-
expect(peerLocalSDP).to.be.equal(sdpOffer.sdp);
71+
expect(peerLocalSDP).to.be.equal(sdpOffer.sdp + CRLF);
7172
});
7273

7374
it('can be run and received parameters are correct', async () => {
7475
class TestPeerConnectionMock extends RTCPeerConnection {
7576
setLocalDescription(description: RTCSessionDescriptionInit): Promise<void> {
76-
expect(description.sdp).to.be.equal(sdpOffer.sdp);
77+
expect(description.sdp).to.be.equal(sdpOffer.sdp + CRLF);
7778
return new Promise<void>((resolve, _reject) => {
7879
resolve();
7980
});
@@ -87,7 +88,7 @@ describe('SetLocalDescriptionTask', () => {
8788
it('can throw error during failure to set local description', async () => {
8889
class TestPeerConnectionMock extends RTCPeerConnection {
8990
setLocalDescription(description: RTCSessionDescriptionInit): Promise<void> {
90-
expect(description.sdp).to.be.equal(sdpOffer.sdp);
91+
expect(description.sdp).to.be.equal(sdpOffer.sdp + CRLF);
9192
return new Promise<void>((_resolve, reject) => {
9293
reject();
9394
});
@@ -107,7 +108,7 @@ describe('SetLocalDescriptionTask', () => {
107108
context.audioProfile = AudioProfile.fullbandMusicStereo();
108109
await task.run();
109110
const peerLocalSDP = context.peer.localDescription.sdp;
110-
expect(peerLocalSDP).to.be.equal(sdpOffer.sdp);
111+
expect(peerLocalSDP).to.be.equal(sdpOffer.sdp + CRLF);
111112
});
112113

113114
it('will update sdp with send codec preferences', async () => {
@@ -152,7 +153,7 @@ describe('SetLocalDescriptionTask', () => {
152153
it('sets start bitrate for SVC content', async () => {
153154
class TestPeerConnectionMock extends RTCPeerConnection {
154155
setLocalDescription(description: RTCSessionDescriptionInit): Promise<void> {
155-
expect(description.sdp).to.be.equal(sdpOffer.sdp);
156+
expect(description.sdp).to.be.equal(sdpOffer.sdp + CRLF);
156157
return new Promise<void>((resolve, _reject) => {
157158
resolve();
158159
});

0 commit comments

Comments
 (0)