Skip to content
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

Add documentation of the Replit Audio messages #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lhchavez
Copy link

@lhchavez lhchavez commented Feb 22, 2021

These messages are intended to allow clients to request the server to
send compressed audio (with the Opus and MP3 codecs) in push and pull
models.

The discussion is in https://groups.google.com/g/novnc/c/pQ6h2xLf3Kc

Reference implementation (client-side): novnc/noVNC#1525
Reference implementation (server-side): https://github.com/replit/rfbproxy

Signed-off-by: Luis Héctor Chávez luis@replit.com

Copy link
Contributor

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

Great, thanks. I've added some notes on things that I think could need clarification.

rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
rfbproto.rst Outdated Show resolved Hide resolved
@samhed
Copy link
Contributor

samhed commented Mar 30, 2021

What's the status on this one, has all the comments been addressed?

@lhchavez
Copy link
Author

What's the status on this one, has all the comments been addressed?

they have!

@CendioOssman
Copy link
Contributor

Setting up a test environment is still on my todo so I can get a better picture of the latency feedback situation.

@lhchavez
Copy link
Author

lhchavez commented Mar 30, 2021

Setting up a test environment is still on my todo so I can get a better picture of the latency feedback situation.

you can use https://pulseaudio.luisreplit.repl.co/ as the most realistic test environment possible: a remote shared VM running halfway across the globe, running a JS game in Chrome. The source for that can be found at https://replit.com/@luisreplit/PulseAudio

note that if you clone that and run a copy, you might get fewer resources on the VM than what I have since I'm a paid subscriber. when that's the case, the audio will be all choppy, since PA will be starved by the scheduler.

Copy link
Contributor

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

Some cleanup from all the changes and we should be good to go.

rfbproto.rst Outdated Show resolved Hide resolved
information, the server can decide to drop audio frames if a client lags
behind too much, by consuming the audio frames from the audio source and
not advancing the encoder state. This way, the stream can still be
decoded by the client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you see a need for keeping this? I think it's enough that we've stated that the client should handle the synchronisation.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, it's a good suggestion to give server implementers: since the client is responsible for synchronization, it's something that servers can do without issues.

the reference implementation also uses it, so i would prefer for that behavior to be present in the spec.

These messages are intended to allow clients to request the server to
send compressed audio (with the Opus and MP3 codecs) in push model.

The discussion is in https://groups.google.com/g/novnc/c/pQ6h2xLf3Kc

Reference implementation (client-side): novnc/noVNC#1525
Reference implementation (server-side): https://github.com/replit/rfbproxy

Signed-off-by: Luis Héctor Chávez <luis@repl.it>
@lhchavez
Copy link
Author

rebased and squashed the commits

@lhchavez
Copy link
Author

gentle ping. is there any chance this could be merged?

@CendioOssman
Copy link
Contributor

I'm afraid I haven't had time to continue my testing. Since there was an issue there I'd like to hold off on merging this in case there is a protocol problem. It's still on my todo though and I hope to get back to it and get all of this resolved.

@lhchavez
Copy link
Author

I'm afraid I haven't had time to continue my testing. Since there was an issue there I'd like to hold off on merging this in case there is a protocol problem. It's still on my todo though and I hope to get back to it and get all of this resolved.

is there anything i can help with? the protocol as is written has been running in production for a while with no issues.

@CendioOssman
Copy link
Contributor

I think you wrote some suggestions on the noVNC PR. I just need to find time to try them out.

@mdevaev
Copy link
Contributor

mdevaev commented Jul 19, 2022

Hello! I'm also interested in this, but I have a couple of questions.

  • I watched the discussion, but I still didn't understand why it was necessary to put OPUS frames in a WebM container. There is a similar RTP specification that allows you to transmit OPUS audio, and RAW frames are used there. It seems to me that the need for WebM is due to the browser requirements for noVNC, because without WebM it will not be able to play OPUS, am I right? Even so, it's a good argument anyway.
  • You have provided a message that allows the server to tell which codecs it supports. But I would like to clarify the following behavior: what happens if the client has chosen a supported codec, but the encoding parameters do not match those supported by the server, or the server is not able to change them at all? I use a single OPUS encoder process and don't want to let users change the settings. That is, the server is like, "You asked for OPUS, okay, here's OPUS for you, but the parameters will be different, it will be stereo and 48k, not mono and 44.1k". I suggest either expanding the server response so that it contains the settings, and the client can understand whether the server has accepted the requested parameters and can give the audio, but with others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants