Clean up writer interfaces. - #793
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| func (r *Room) WriteOutboundAudioTo(w msdk.PCM16Writer) msdk.PCM16Writer { | ||
| return r.outboundAudio.Swap(w) |
There was a problem hiding this comment.
We don't need r.mix.SampleRate() anymore?
| } | ||
|
|
||
| func (r *Room) GetInboundAudioWriter() (msdk.PCM16Writer, error) { | ||
| return r.NewParticipantTrack(RoomSampleRate) |
There was a problem hiding this comment.
Not sure I like this one. Would we not create a new track every call? (where we simply return the fixed writer in other places, including GetInboundDTMFWriter below)
There was a problem hiding this comment.
This is mirroring the existing behavior (where, previously, inbound and outbound would get new inbound writers by invoking NewParticipantTrack. We could have a fixed writer stored on Room, but we'd probably want to lazily initialize it the first time GetInboundAudioWriter is called. IMO, that doesn't really seem that much better.
84e160c to
a609e02
Compare
| p.audioIn.Close() // Propagate Close() to onwards to room | ||
| p.dtmfIn.Close() // Propagate Close() to onwards to room |
There was a problem hiding this comment.
🟡 Room audio writer for outgoing calls is shut down twice when a call ends
The room-facing audio writer is now shut down a second time (c.lkRoomIn.Close() at pkg/sip/outbound.go:393) after the media port already shut it down (p.audioIn.Close() at pkg/sip/media_port.go:703), so the same encoder and published track get torn down twice on every outbound hangup.
Impact: Every outgoing call end runs the same teardown twice, which can surface as spurious warnings in logs or, if the underlying writer is not tolerant of repeat shutdown, an error/panic during call cleanup.
Close propagation added to mediaPort.Close now overlaps the explicit lkRoomIn close in outboundCall.close
mediaPort.Close() newly propagates Close() through its inbound switches (pkg/sip/media_port.go:703-706). For outbound calls, audioIn's inner writer is c.lkRoomIn (set in connectMedia via c.media.WriteInboundAudioTo(c.lkRoomIn), pkg/sip/outbound.go:551), which is the opus encoder wrapping the published LiveKit track (Room.NewParticipantTrack, pkg/sip/room.go:741-763). outboundCall.close calls c.media.Close() first (pkg/sip/outbound.go:384) and then still calls c.lkRoomIn.Close() and logs a warning if it returns an error. opus.encoder.Close (pkg/media/opus/opus.go:198-207) flushes and then closes the wrapped sample writer, so the track-facing writer receives two Close() calls. Either drop the explicit c.lkRoomIn.Close() block now that the media port owns the propagation, or clear c.lkRoomIn before closing the media port.
Was this helpful? React with 👍 or 👎 to provide feedback.
b632e7f to
269b1a3
Compare
alexfish8
left a comment
alexfish8
left a comment
There was a problem hiding this comment.
Thanks for the reviews!
| } | ||
|
|
||
| func (r *Room) GetInboundAudioWriter() (msdk.PCM16Writer, error) { | ||
| return r.NewParticipantTrack(RoomSampleRate) |
There was a problem hiding this comment.
This is mirroring the existing behavior (where, previously, inbound and outbound would get new inbound writers by invoking NewParticipantTrack. We could have a fixed writer stored on Room, but we'd probably want to lazily initialize it the first time GetInboundAudioWriter is called. IMO, that doesn't really seem that much better.
3 participants
Rename
MediaSessionmethods to be more explicit about the direction of a particular writerRoomto satisfy theMediaSessioninterface, and so, for my own sanity, I renamed these things. Let me know what you think, I can always swap the names back or we can pick some thing else if you'd prefer]Update
Roommethod names to be similar to those inMediaPortMove some DTMF event conversion logic out of inbound/outbound and into room.
dtmfEventWriteradapter out of inbound.go, as, even though Room's methods have been updated to the new pattern, the adapter is still responsible for populating some inbound-internal DTFM channel that gets read when validating pin prompts.TODO (before landing this): Update tests to use the new method names.