TEL-886: Self review - #805
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.
| if err := c.joinRoom(ctx, disp.Room, status); err != nil { | ||
| return fmt.Errorf("failed joining room: %w", err) | ||
| } |
There was a problem hiding this comment.
Possibly fail on media first, then join room
| codecInfo := mc.Audio.Codec.Info() | ||
| sink = msdk.ResampleWriter(sink, codecInfo.SampleRate) | ||
|
|
||
| if p.conf.stats != nil { |
There was a problem hiding this comment.
line 160 already dereferences p.conf.stats
| type pcmCollector struct { | ||
| sampleRate int | ||
|
|
||
| mu sync.Mutex | ||
| buf msdk.PCM16Sample | ||
| } |
There was a problem hiding this comment.
Fix data race in test by synchronizing the harness
| func (p *mediaPort) reportPeerCodecs(d sdp.MediaDesc, reinvite bool) { | ||
| if p.mon == nil { | ||
| return | ||
| } | ||
| p.mon.PeerSDP(peerCodecNames(d)) | ||
| p.mon.PeerSDP(peerCodecNames(d), reinvite) | ||
| } |
There was a problem hiding this comment.
Added reinvite label to metrics.
| p.mu.Lock() // No concurrent rebuilding of the pipeline | ||
| defer p.mu.Unlock() | ||
|
|
||
| p.offer = nil |
There was a problem hiding this comment.
Nego done, can generate new offer, even if we don't apply last offer-answer exchange.
| audioToPort = nil | ||
| dtmfToPort = nil |
There was a problem hiding this comment.
Prevent restoring closed pipelines connectors at deferred funcs
dce2728 to
319a149
Compare
a805c4b to
5fcb481
Compare
| return nil, err | ||
| } | ||
| if activateTimeout { | ||
| p.SetTimeout(p.opts.MediaTimeoutInitial, p.opts.MediaTimeout) |
There was a problem hiding this comment.
Now that we're no longer arming the timeout in GenerateAnswer, do we need to invoke SetTimeout at some call sites in outbound.go or inbound.go?
There was a problem hiding this comment.
outbound: no. We are still arming the timeout as part of ProcessAnswer().
inbound: Yes, and we are. It's part of the full review, but we are doing it as part of acceptCall()
2 participants
To be added on top of #802
Remaining known issues: