Signal logger #558
Signal logger #558
Conversation
alexlivekit
force-pushed
the
signal-logger
branch
8 times, most recently
from
2df31f9 to
0464494
Compare
alexlivekit
force-pushed
the
signal-logger
branch
from
0464494 to
e030c5f
Compare
| // Keeps an internal state of whether we're currently transmitting signal (voice or noise), or silence. | ||
| // This implements msdk.PCM16Writer to inspect decoded packet content. | ||
| // Used to log changes betweem those states. | ||
| type SignalLogger struct { |
There was a problem hiding this comment.
We should consider moving it to media-sdk, since it's not SIP-specific.
pkg/sip/signal_logger.go
Outdated
|
|
||
| s.framesProcessed++ | ||
| if s.framesProcessed > 10 { // Don't log any changes at first | ||
| if isSignal && isSignal != s.lastIsSignal { // silence -> signal: Immediate transition |
There was a problem hiding this comment.
This could potentially hit each frame, wouldn't it be too spammy?
There was a problem hiding this comment.
That's what hangoverDuration is about. It can flip to signal immediately, but to flip out of isSignal you'd need to wait at least a second of silence. Would that still be too spammy?
pkg/sip/signal_logger.go
Outdated
| } | ||
|
|
||
| func (s *SignalLogger) WriteSample(sample msdk.PCM16Sample) error { | ||
| currentEnergy := s.MeanAbsoluteDeviation(sample) |
There was a problem hiding this comment.
Probably doesn't matter here, but energy is usually computed as a square of amplitude. Maybe rename to ampMean?
pkg/sip/signal_logger.go
Outdated
| totalAbs += int64(v) | ||
| } | ||
| } | ||
| return float64(totalAbs) / float64(len(sample)) |
There was a problem hiding this comment.
We could also consider calculating a median, it will be less affected by sudden spikes.
There was a problem hiding this comment.
Would still want to use average to not filter out signal in high or low bands, which should count as signal still.
…on't need this at this time
…he config from some place
hechen-eng
force-pushed
the
signal-logger
branch
from
d1618ce to
fd042a9
Compare
| audioWriter = newMediaWriterCount(audioWriter, &p.stats.AudioInFrames, &p.stats.AudioInSamples) | ||
| } | ||
| if p.logSignalChanges { | ||
| signalLogger, err := NewSignalLogger(p.log, "input", audioWriter) |
There was a problem hiding this comment.
curious.. why don't we return error for the setupInput path?
There was a problem hiding this comment.
didn't want to change too much, and the function currently doesn't support erroring out. Using the original audioWriter in that case doesn't actually degrade anything.
hechen-eng
force-pushed
the
signal-logger
branch
from
2058df4 to
4c46cf9
Compare
alexlivekit
left a comment
alexlivekit
left a comment
There was a problem hiding this comment.
Overall, other than the two nits, this is looking good imo.
Since this is gated by a feature flag, imo this is okay to land, but please test the new changes in staging before enabling for anyone else.
3 participants
If we ever want a fancier version (used mostly for VAD, or Voice Activity Detection), where our noise floor is dynamically calculated (this helps indicate speech even in noisier environments), we can look at the implementation in signal-logger-adaptive.
For now, the purpose of this feature is to output whether there's any signal, voice or noise, for sanity and debugging.