-
Notifications
You must be signed in to change notification settings - Fork 248
Comment accuracy: nine measured corrections (draft / experiment) #3866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
86f08e8
buffer.cpp: name the real bound behind the block-count divide
2b63f9a
buffer.cpp: correct the wrap-detection horizon: 128 counts, not 256
3a4a715
buffer.cpp: document the second loss channel beside the window move
64af334
client.cpp: non-zero iActiveChannels does not imply a server restart
3166c59
server.cpp: drop the outdated QtConcurrent 5-parameter rationale
d6e6658
server.cpp: state the real reason the disconnect flag needs no mutex
78ed25f
channel.cpp: comment the true per-packet overhead (measured 46/66 B, …
cca203b
server.cpp: comment that the realtime path still allocates (send-path…
731b309
client.cpp: comment that FindClientChannel here is not guaranteed to …
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,8 +164,16 @@ bool CNetBuf::Put ( const CVector<uint8_t>& vecbyData, int iInSize ) | |
| return false; | ||
| } | ||
|
|
||
| // to get the number of input blocks we assume that the number of bytes for | ||
| // the sequence number is much smaller than the number of coded audio bytes | ||
| // This divide is exact if and only if iNumBlocks * iNumBytesSeqNum < iBlockSize, | ||
| // since the actual input is iNumBlocks * ( iBlockSize + iNumBytesSeqNum ) bytes. The | ||
| // sequence number merely being "much smaller" than the coded audio is not the | ||
| // condition: at iNumBlocks == iBlockSize the count comes out one too high whatever | ||
| // the ratio is. The bound is not enforced here but by the properties validator in | ||
| // protocol.cpp, EvaluateNetwTranspPropsMes, which rejects a base network packet size | ||
| // below CELT_MINIMUM_NUM_BYTES (10) and a block size factor outside | ||
| // { FRAME_SIZE_FACTOR_PREFERRED, _DEFAULT, _SAFE }. With iNumBytesSeqNum == 1 and | ||
| // iBlockSize == iBaseNetworkPacketSize - 1 (channel.cpp), the worst reachable case is | ||
| // a factor of 4 against a block size of 9. | ||
| const int iNumBlocks = /* floor */ ( iInSize / iBlockSize ); | ||
|
|
||
| // copy new data in internal buffer | ||
|
|
@@ -190,16 +198,23 @@ bool CNetBuf::Put ( const CVector<uint8_t>& vecbyData, int iInSize ) | |
| iSeqNumDiff -= 256; | ||
| } | ||
|
|
||
| // The 1-byte sequence number wraps around at a count of 256. So, if a packet is delayed | ||
| // further than this we cannot detect it. But it does not matter since such a packet is | ||
| // more than 100 ms delayed so we have a bad network situation anyway. Therefore we | ||
| // The 1-byte sequence number is folded into a signed difference above, so a | ||
| // delayed packet is mistaken for an early one once it is more than 128 counts | ||
| // late, not 256. At the fastest possible frame rate that is still 171 ms | ||
| // (64-sample frames, 750 counts/s) and at the default frame size 341 ms | ||
| // (128-sample frames, 375 counts/s), so such a packet is long useless either | ||
| // way and we have a bad network situation anyway. Therefore we | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't add anything at all. |
||
| // assume that the sequence number difference between the received and local counter is | ||
| // correct. The idea of the following code is that we always move our "buffer window" so | ||
| // that the received packet fits into the buffer. By doing this we are robust against | ||
| // sample rate offsets between client/server or buffer glitches in the audio driver since | ||
| // we adjust the window. The downside is that we never throw away single packets which arrive | ||
| // too late so we throw away valid packets when we move the "buffer window" to the delayed | ||
| // packet and then back to the correct place when the next normal packet is received. But | ||
| // packet and then back to the correct place when the next normal packet is received. | ||
| // Note that this is not the only way a valid block is lost: a block can also be | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This addition is not in context and doesn't belong here. |
||
| // overwritten in its slot before it is played out. That second channel is the only | ||
| // one that exists at a buffer length of 1, while the window move dominates from a | ||
| // buffer length of 3 upwards. But | ||
| // tests showed that the new buffer strategy does not perform worse than the old jitter | ||
| // buffer which did not use any sequence number at all. | ||
| if ( iSeqNumDiff < 0 ) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is far too verbose for anyone to read it clearly. Two lines looks like a good length. Ten lines isn't.
It needs to be short and to the point: