fix(sampleWriter): handle dropped packets & opus dtx #13
Conversation
anunaym14
requested review from
boks1971 and
dennwc
and removed request for
a team
anunaym14
changed the title
pcm.go
Outdated
| var droppedPackets uint16 | ||
| if !w.lastWrite.IsZero() { | ||
| timeSinceLastWrite := time.Since(w.lastWrite) | ||
| if timeSinceLastWrite > w.sampleDur { |
There was a problem hiding this comment.
Maybe need to make this a bit more fuzzy? As it is written here, a write coming through after 19.99ms will not insert a silence packet in between. Guess, it is okay, but just something to thinking about a bit and see if some fuzz factor should be applied (for example anything over 90% of duration can be considered a full duration).
There was a problem hiding this comment.
Makes sense, added a 10% tolerance
There was a problem hiding this comment.
Left a comment in the specific commit. I may be reading it wrong, but it looked like an empty packet would not be inserted if the gap is 19.5 ms for a sample duration of 10ms.
There was a problem hiding this comment.
You're right, missed that it's an integer division, not float. Converting it to float and rounding it off to the nearest integer instead.
There was a problem hiding this comment.
That could add a dropped packet at 15.1 ms. I am thinking it should add a packet at > 19ms only. So, you would have to include that tolerance in the number of packets calculation too.
There was a problem hiding this comment.
Hmm okay, let me adjust that
There was a problem hiding this comment.
fpushed, it should be correct now!
| // If the return value is 1 byte, then the packet does not need to be transmitted (DTX). | ||
| if n == 1 && e.useDtx { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
:TIL: I thought 1 byte should be transmitted to signal background noise level. I think I have seen 1 byte packets comes in to the server. But, libwebrtc mentions this also (https://source.chromium.org/chromium/chromium/src/+/main:media/audio/audio_opus_encoder.cc;drc=dfb49a410ff213451f72db8d792973550025033e;l=288). Where are the 1 byte packets coming from into the server? Have to check again if I am not remembering something properly.
There was a problem hiding this comment.
Maybe some clients are not negotiating dtx? meet.livekit.io does not seem to be negotiating it.
There was a problem hiding this comment.
JS SDK sample app does DTX. I think I checked with that. Will test out and check packet sizes with DTX enabled.
There was a problem hiding this comment.
DTX packets (or the packet before DTX kicks in) is 1 byte when trying with JS SDK sample app (setting red: false in the demo app) and logging on SFU side. Puzzled what opus_encode returns and how that is getting sent. Definitely does not jive with opus_encode() doc or libwebrtc code.
anunaym14
force-pushed
the
feat/skip-samples
branch
from
18d37fb to
1a044d5
Compare
No description provided.