Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

feat: Sorted queue for voice chat frames #1363

Merged
merged 19 commits into from
Sep 28, 2020

Conversation

pablitar
Copy link
Contributor

@pablitar pablitar commented Sep 25, 2020

This PR adds a sorted queue for voice chat frames received from the network. This allows to potentially receive the voice packets in different order, and that should improve voice chat when there is a big P2P network.

We should probably replace ring buffer with something that orders frames
Also reverted frame size change because it would make the lasts frames to be incomplete.

We need to send various frames in the same packet, if we want to improve protocol efficiency.

Or we could try to implement a way to ensure that incomplete frames are completed with silences when recording is stopped so we can send them. Or round or recording side to frames.
@github-actions
Copy link

@pablitar pablitar changed the title feat: Ordered queue for voice chat frames feat: Sorted queue for voice chat frames Sep 25, 2020
Copy link
Contributor

@nchamo nchamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!


const maxCountToRead = this.writePointer - this.readPointer

const count = readCount ? Math.min(readCount, maxCountToRead) : maxCountToRead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const count = readCount ? Math.min(readCount, maxCountToRead) : maxCountToRead
const count = Math.min(readCount ?? Infinity, maxCountToRead)

import { expect } from 'chai'

describe('RingBuffer', () => {
let buffer: RingBuffer<Float32Array>
describe('OrderedRingBuffer', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you maybe change the name of the file as well?

@@ -112,7 +113,7 @@ function processEncodeMessage(e: MessageEvent) {
const sampleRate = getSampleRate(e)
const encoderWorklet = (encoderWorklets[e.data.streamId] = encoderWorklets[e.data.streamId] || {
working: false,
encoder: new libopus.Encoder(1, sampleRate, 24000, 20, true),
encoder: new libopus.Encoder(1, sampleRate, OPUS_BITS_PER_SECOND, OPUS_FRAME_SIZE_MS, true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍


private resolveBlockedDequeueWith(amount: number) {
const items = amount === 0 ? [] : this.dequeueItems(amount)
this.pendingDequeue?.futures.forEach((it) => it.resolve(items))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be:
this.pendingDequeue?.futures?.forEach((it) => it.resolve(items))
?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, I think that if pendingDequeue is not undefined then we know that futures is not undefined.

And I think the syntax here works in that sense: If pendingDequeue is undefined, all that follows won't execute. I think. I'll check just in case.

Copy link
Contributor Author

@pablitar pablitar Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code gets translated to (_a = this.pendingDequeue) === null || _a === void 0 ? void 0 : _a.futures.forEach((it) => it.resolve(items));

I think _a.futures.forEach((it) => it.resolve(items)); will only execute if (_a = this.pendingDequeue) === null || _a === void 0 is false. This is quite cryptic, but I think it works as we want :)

this.pendingDequeue.futures.push(newFuture)

setTimeout(() => {
if (this.pendingDequeue && this.pendingDequeue.futures.indexOf(newFuture) >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (this.pendingDequeue && this.pendingDequeue.futures.indexOf(newFuture) >= 0) {
if (this.pendingDequeue && this.pendingDequeue.futures.includes(newFuture)) {


if (this.queuedCount() >= count) {
const items = this.dequeueItems(count)
this.pendingDequeue?.futures.forEach((it) => it.resolve(items))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be:
this.pendingDequeue?.futures?.forEach((it) => it.resolve(items))
?

@pablitar pablitar merged commit 78d4f8a into master Sep 28, 2020
@pablitar pablitar deleted the feat/ordered-buffer-for-voice-chat branch September 28, 2020 21:40
This was referenced Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants