TEL-886: Media Port rework - #802
Conversation
11f4b21 to
dd06f29
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if c.lkRoomIn != nil { | ||
| if err := c.lkRoomIn.Close(); err != nil { | ||
| log.Warnw("error closing livekit room audio input", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Outbound call closes the room audio track twice on teardown
When an outbound call ends, the room-facing audio track is closed once by the media port teardown (c.media.Close() at pkg/sip/outbound.go:382, which propagates through the media port's inbound-audio switch) and then closed a second time explicitly (c.lkRoomIn.Close() at pkg/sip/outbound.go:390), so the same audio writer is torn down twice.
Impact: The call-teardown path double-releases the outbound audio track, which can surface as spurious error logs or, if the underlying encoder is not close-idempotent, resource corruption during shutdown.
Why the same object is closed on two paths
In connectMedia the (possibly processor-wrapped) c.lkRoomIn is handed to the media port via c.media.WriteInboundAudioTo(c.lkRoomIn) (pkg/sip/outbound.go:559), which stores it as the inner writer of the port's audioIn switch (pkg/sip/media_port.go:734-735).
On mediaPort.Close(), p.audioIn.Close() is called with the explicit comment "Propagate Close() to onwards to room" (pkg/sip/media_port.go:693), closing c.lkRoomIn.
Then outboundCall.close closes it again at pkg/sip/outbound.go:389-393. The inbound path (inboundCall.closeMedia at pkg/sip/inbound.go:1578-1585) only closes it once via c.media.Close(), so the outbound explicit close is redundant and asymmetric.
Prompt for agents
In outboundCall.close (pkg/sip/outbound.go around lines 389-393), c.lkRoomIn.Close() is called explicitly, but c.media.Close() (called just above at line 382) already propagates Close() to the inbound-audio writer stored via connectMedia's c.media.WriteInboundAudioTo(c.lkRoomIn), which is c.lkRoomIn. This results in a double Close() of the room participant track / opus encoder on every outbound call teardown. The inbound path (inboundCall.closeMedia) relies solely on media.Close() and does not double-close. Decide on a single ownership model: either drop the explicit c.lkRoomIn.Close() here (relying on media.Close to close it), or ensure the explicit close only runs when connectMedia never wired c.lkRoomIn into the media port (e.g. early-failure paths). Make outbound consistent with inbound.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed that this is actually a thing: I'm seeing the log warning a lot in staging. Will address and fix.
|
Huh, the race in CI is real, and is apparently a problem in media-sdk (yay zerocopy) that previous tests simply did not expose. |
Will fix this in media-sdk. |
6a063e6 to
dce2728
Compare
| return nil, SDPError{Err: err} | ||
| } | ||
| } | ||
| p.reportPeerCodecs(offer.MediaDesc) |
There was a problem hiding this comment.
🟡 SDP re-INVITE metric label always false
isReinvite is derived from p.offer != nil, but p.offer is set only by outbound GenerateOffer and cleared once negotiated. GenerateAnswer runs in the answering role, so it is always nil here. Every parsed offer, including real inbound re-INVITEs, records reinvite=false on the new SDP metrics. The intended signal is p.negotiated != nil.
| p.reportPeerCodecs(offer.MediaDesc) | |
| isReinvite := p.negotiated != nil |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
This change is intended to: