From 394c2579a49fd089609bbd2856635d1f957a3efd Mon Sep 17 00:00:00 2001 From: smoghe-bw Date: Wed, 1 Jul 2026 09:51:21 -0400 Subject: [PATCH] fix: harden DTMF sending and always negotiate telephone-event codec Guard sendDtmf against unready or throwing RTCDTMFSender instances so one bad sender doesn't block others, and apply codec preferences unconditionally on audio tracks so telephone-event is always offered regardless of caller-provided codecPreferences. Also warn when an audio SDP offer lacks telephone-event, since DTMF can never negotiate for that session. --- src/v1/bandwidthRtc.test.ts | 60 +++++++++++++++++++++++++++++++++++-- src/v1/bandwidthRtc.ts | 54 +++++++++++++++++++++++---------- 2 files changed, 97 insertions(+), 17 deletions(-) diff --git a/src/v1/bandwidthRtc.test.ts b/src/v1/bandwidthRtc.test.ts index 81a4f61..0e6f5d7 100644 --- a/src/v1/bandwidthRtc.test.ts +++ b/src/v1/bandwidthRtc.test.ts @@ -61,8 +61,8 @@ describe("bandwidthRtcV1 sendDtmf", () => { setupMocks(); }); - function makeDtmfSender() { - return { insertDTMF: jest.fn() }; + function makeDtmfSender(canInsertDTMF: boolean = true) { + return { insertDTMF: jest.fn(), canInsertDTMF }; } test("calls insertDTMF on all registered senders when no streamId given", () => { @@ -117,9 +117,41 @@ describe("bandwidthRtcV1 sendDtmf", () => { expect(sender.insertDTMF).not.toHaveBeenCalled(); }); + + test("skips a sender that is not ready (canInsertDTMF false) without throwing", () => { + const brtc = new BandwidthRtc(); + const notReady = makeDtmfSender(false); + const ready = makeDtmfSender(true); + (brtc as any).localDtmfSenders.set("stream-1", notReady); + (brtc as any).localDtmfSenders.set("stream-2", ready); + + expect(() => brtc.sendDtmf("5")).not.toThrow(); + + expect(notReady.insertDTMF).not.toHaveBeenCalled(); + expect(ready.insertDTMF).toHaveBeenCalledWith("5", 100, 70); + }); + + test("catches an insertDTMF error on one sender and still calls the others", () => { + const brtc = new BandwidthRtc(); + const throwing = makeDtmfSender(); + throwing.insertDTMF.mockImplementation(() => { + throw new DOMException("not ready", "InvalidStateError"); + }); + const healthy = makeDtmfSender(); + (brtc as any).localDtmfSenders.set("stream-1", throwing); + (brtc as any).localDtmfSenders.set("stream-2", healthy); + + expect(() => brtc.sendDtmf("5")).not.toThrow(); + + expect(healthy.insertDTMF).toHaveBeenCalledWith("5", 100, 70); + }); }); describe("bandwidthRtcV1 addStreamToPublishingPeerConnection", () => { + afterEach(() => { + delete (global as any).RTCRtpSender; + }); + function makeTransceiver(dtmf: RTCDTMFSender | null = { insertDTMF: jest.fn() } as any) { return { sender: { dtmf }, setCodecPreferences: jest.fn() }; } @@ -191,6 +223,30 @@ describe("bandwidthRtcV1 addStreamToPublishingPeerConnection", () => { expect(transceiver.setCodecPreferences).toHaveBeenCalledWith([opusCodec]); }); + + test("forces telephone-event into codec preferences even without explicit codecPreferences", () => { + const brtc = new BandwidthRtc(); + const transceiver = makeTransceiver(); + withPublishingPeerConnection(brtc, transceiver); + + const opusCodec = { mimeType: "audio/opus", clockRate: 48000 }; + const telephoneEventCodec = { mimeType: "audio/telephone-event", clockRate: 8000 }; + (global as any).RTCRtpSender = { getCapabilities: jest.fn().mockReturnValue({ codecs: [opusCodec, telephoneEventCodec] }) }; + + (brtc as any).addStreamToPublishingPeerConnection(makeMockStream("stream-1", "audio")); + + expect(transceiver.setCodecPreferences).toHaveBeenCalledWith([opusCodec, telephoneEventCodec]); + }); + + test("skips setCodecPreferences when RTCRtpSender is unavailable (e.g. non-browser environment)", () => { + const brtc = new BandwidthRtc(); + const transceiver = makeTransceiver(); + withPublishingPeerConnection(brtc, transceiver); + + expect(() => (brtc as any).addStreamToPublishingPeerConnection(makeMockStream("stream-1", "audio"))).not.toThrow(); + + expect(transceiver.setCodecPreferences).not.toHaveBeenCalled(); + }); }); describe("bandwidthRtcV1 connect method", () => { diff --git a/src/v1/bandwidthRtc.ts b/src/v1/bandwidthRtc.ts index ef97fef..b0cae0c 100644 --- a/src/v1/bandwidthRtc.ts +++ b/src/v1/bandwidthRtc.ts @@ -292,10 +292,27 @@ export class BandwidthRtc { * @param interToneGap Gap between tones in milliseconds (default: 70). Minimum 30. */ sendDtmf(tone: string, streamId?: string, duration: number = 100, interToneGap: number = 70) { + const insert = (dtmfSender: RTCDTMFSender, id: string) => { + if (!dtmfSender.canInsertDTMF) { + logger.warn(`sendDtmf: DTMF sender for stream ${id} is not ready (canInsertDTMF is false); skipping`); + return; + } + try { + dtmfSender.insertDTMF(tone, duration, interToneGap); + } catch (err) { + logger.warn(`sendDtmf: insertDTMF failed for stream ${id}`, err); + } + }; + if (streamId) { - this.localDtmfSenders.get(streamId)?.insertDTMF(tone, duration, interToneGap); + const dtmfSender = this.localDtmfSenders.get(streamId); + if (dtmfSender) { + insert(dtmfSender, streamId); + } else { + logger.warn(`sendDtmf: no DTMF sender registered for stream ${streamId}`); + } } else { - this.localDtmfSenders.forEach((dtmfSender) => dtmfSender.insertDTMF(tone, duration, interToneGap)); + this.localDtmfSenders.forEach(insert); } } @@ -386,6 +403,12 @@ export class BandwidthRtc { iceRestart: restartIce, }); + // Diagnostic only: if an audio m-line is offered without telephone-event, DTMF + // can never negotiate for this session regardless of how long sendDtmf waits. + if (localSdpOffer.sdp?.includes("m=audio") && !localSdpOffer.sdp.includes(TELEPHONE_EVENT_MIME_TYPE.split("/")[1])) { + logger.warn("Publish SDP offer has an audio track but no telephone-event codec; DTMF will not be able to negotiate for this session"); + } + let publishMetadata = { mediaStreams: {}, dataChannels: {}, @@ -675,23 +698,24 @@ export class BandwidthRtc { this.localDtmfSenders.set(mediaStream.id, dtmfSender); } - if (codecPreferences) { - if (track.kind === TRACK_KIND_AUDIO && codecPreferences.audio) { - // setCodecPreferences is a strict allowlist: any codec omitted from the - // list is dropped from the SDP offer. telephone-event must always be - // present so that RTCDTMFSender can send RFC 4733 DTMF packets. - const hasTelephoneEvent = codecPreferences.audio.some((c) => c.mimeType.toLowerCase() === TELEPHONE_EVENT_MIME_TYPE); + if (track.kind === TRACK_KIND_AUDIO) { + // setCodecPreferences is a strict allowlist: any codec omitted from the list is + // dropped from the SDP offer. Apply it unconditionally (not just when the caller + // passes codecPreferences) so telephone-event is always present and RTCDTMFSender + // can send RFC 4733 DTMF packets, regardless of the browser's default codec offer. + const audioCapabilities = typeof RTCRtpSender !== "undefined" ? RTCRtpSender.getCapabilities(TRACK_KIND_AUDIO) : undefined; + const audioCodecs = codecPreferences?.audio ?? audioCapabilities?.codecs; + if (audioCodecs) { + const hasTelephoneEvent = audioCodecs.some((c) => c.mimeType.toLowerCase() === TELEPHONE_EVENT_MIME_TYPE); if (!hasTelephoneEvent) { - const telephoneEventCodec = RTCRtpSender.getCapabilities(TRACK_KIND_AUDIO)?.codecs.find( - (c) => c.mimeType.toLowerCase() === TELEPHONE_EVENT_MIME_TYPE, - ); - transceiver.setCodecPreferences(telephoneEventCodec ? [...codecPreferences.audio, telephoneEventCodec] : codecPreferences.audio); + const telephoneEventCodec = audioCapabilities?.codecs.find((c) => c.mimeType.toLowerCase() === TELEPHONE_EVENT_MIME_TYPE); + transceiver.setCodecPreferences(telephoneEventCodec ? [...audioCodecs, telephoneEventCodec] : audioCodecs); } else { - transceiver.setCodecPreferences(codecPreferences.audio); + transceiver.setCodecPreferences(audioCodecs); } - } else if (track.kind === TRACK_KIND_VIDEO && codecPreferences.video) { - transceiver.setCodecPreferences(codecPreferences.video); } + } else if (track.kind === TRACK_KIND_VIDEO && codecPreferences?.video) { + transceiver.setCodecPreferences(codecPreferences.video); } }); }