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

Support the Replit Audio messages #1525

Closed
wants to merge 2 commits into from

Conversation

lhchavez
Copy link
Contributor

@lhchavez lhchavez commented Feb 22, 2021

This change builds upon #1457 to support encoded audio support for the
RFB protocol. Notably, it:

a) Adds a button to toggle the audio stream. This is needed since most
browsers require the audio play action to be run in a user event
handler, so having the toggle audio be an explicit button achieves
that (and is also to avoid annoying users).
b) Drops support for the QEMU audio extension and instead uses the
Replit Audio messages, which allows for explicit negotiation for
audio compression.
c) Makes the audio library more robust and now works in Chrome and
Firefox.

The protocol extension and a first implementation can be found at https://github.com/replit/rfbproxy (tested with TigerVNC as the underlying server).

Tested in Chrome and Firefox (missing a volunteer to fix in Safari. maybe @tinyzimmer can help?)

core/rfb.js Outdated Show resolved Hide resolved
lhchavez added a commit to lhchavez/rfbproto that referenced this pull request 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@repl.it>
tinyzimmer and others added 2 commits March 26, 2021 19:31
This change:

a) Adds a button to toggle the audio stream. This is needed since most
   browsers require the audio play action to be run in a user event
   handler, so having the toggle audio be an explicit button achieves
   that (and is also to avoid annoying users).
b) Drops support for the QEMU audio extension and instead uses the
   Repl.it Audio messages, which allows for explicit negotiation for
   audio compression.
c) Makes the audio library more robust and now works in Chrome and
   Firefox.
lhchavez added a commit to replit/noVNC that referenced this pull request Apr 6, 2021
This change:

a) Adds a button to toggle the audio stream. This is needed since most
browsers require the audio play action to be run in a user event
handler, so having the toggle audio be an explicit button achieves
that (and is also to avoid annoying users).
b) Drops support for the QEMU audio extension and instead uses the
Repl.it Audio messages, which allows for explicit negotiation for
audio compression.
c) Makes the audio library more robust and now works in Chrome and
Firefox.

(cherry-pick of novnc#1525)
@CendioOssman
Copy link
Member

I'm trying to test this but I'm getting very choppy audio. Any suggestions on debugging and getting it working?

@lhchavez
Copy link
Contributor Author

lhchavez commented Apr 8, 2021

I'm trying to test this but I'm getting very choppy audio. Any suggestions on debugging and getting it working?

oh no! does the rfbproxy log mention a lot of reconnects? if not, you can add this diff to diagnose it client-side:

diff --git a/core/util/audio.js b/core/util/audio.js
index dbfc400..40b37c6 100644
--- a/core/util/audio.js
+++ b/core/util/audio.js
@@ -125,6 +125,12 @@ export default class AudioStream {
     // seeked to the latest possible position that doesn't trigger buffering
     // to avoid an arbitrarily large desync between video and audio.
     _appendChunk(timestamp, chunk) {
+        console.debug("appending chunk", {
+            timestamp,
+            currentTime: this._audio.currentTime,
+            buffered: this._audio.buffered.length && this._audio.buffered.end(this._audio.buffered.length - 1),
+        });
+
         this._audioBuffer.appendBuffer(chunk);
         if (
             timestamp - this._audio.currentTime > MAX_ALLOWABLE_DESYNC &&

are both connections local, or are you using the PoC that i shared before?

lhchavez added a commit to lhchavez/rfbproto that referenced this pull request Apr 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 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>
@CendioOssman
Copy link
Member

Sorry about the delay. I could spend a little time on this today, but unfortunately not a lot. The proxy doesn't log anything so I'm afraid there is no help there.

Without any modifications I'm getting this in the browser log:

maximum allowable desync reached 
Object { readyState: 2, buffered: "0.08", seekable: "0.08", time: "0.08", delta: "1.97" }
audio.js:133:21
maximum allowable desync reached 
Object { readyState: 2, buffered: "6.10", seekable: "6.10", time: "4.13", delta: "2.01" }
audio.js:133:21
maximum allowable desync reached 
Object { readyState: 2, buffered: "10.19", seekable: "12.30", time: "8.22", delta: "2.04" }
audio.js:133:21
maximum allowable desync reached 
Object { readyState: 2, buffered: "14.30", seekable: "14.30", time: "12.34", delta: "2.00" }
audio.js:133:21
...

With that patch I'm getting a bunch of lines, but nothing obviously wrong:

...
appending chunk 
Object { timestamp: 25.521, currentTime: 25.15025, buffered: 25.521 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.561, currentTime: 25.15025, buffered: 25.561 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.601, currentTime: 25.231, buffered: 25.601 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.641, currentTime: 25.231, buffered: 25.641 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.721, currentTime: 25.351937, buffered: 25.721 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.761, currentTime: 25.351937, buffered: 25.761 }
audio.js:128:17
...

Whole log: console-export-2021-7-23_15-0-12.txt

@lhchavez
Copy link
Contributor Author

Sorry about the delay. I could spend a little time on this today, but unfortunately not a lot. The proxy doesn't log anything so I'm afraid there is no help there.

Without any modifications I'm getting this in the browser log:

maximum allowable desync reached 
Object { readyState: 2, buffered: "0.08", seekable: "0.08", time: "0.08", delta: "1.97" }
audio.js:133:21
maximum allowable desync reached 
Object { readyState: 2, buffered: "6.10", seekable: "6.10", time: "4.13", delta: "2.01" }
audio.js:133:21
maximum allowable desync reached 
Object { readyState: 2, buffered: "10.19", seekable: "12.30", time: "8.22", delta: "2.04" }
audio.js:133:21
maximum allowable desync reached 
Object { readyState: 2, buffered: "14.30", seekable: "14.30", time: "12.34", delta: "2.00" }
audio.js:133:21
...

With that patch I'm getting a bunch of lines, but nothing obviously wrong:

...
appending chunk 
Object { timestamp: 25.521, currentTime: 25.15025, buffered: 25.521 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.561, currentTime: 25.15025, buffered: 25.561 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.601, currentTime: 25.231, buffered: 25.601 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.641, currentTime: 25.231, buffered: 25.641 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.721, currentTime: 25.351937, buffered: 25.721 }
audio.js:128:17
appending chunk 
Object { timestamp: 25.761, currentTime: 25.351937, buffered: 25.761 }
audio.js:128:17
...

Whole log: console-export-2021-7-23_15-0-12.txt

gotcha. let me build a Docker container so that we're working with the same environment, and have all the logging.

@CendioOssman
Copy link
Member

@lhchavez, were you able to get a docker image up for testing?

@samhed samhed added this to the Future Features milestone Aug 26, 2021
@lhchavez
Copy link
Contributor Author

not yet, i've been on vacation for a couple of weeks. i'll give it a try tomorrow!

@lhchavez
Copy link
Contributor Author

lhchavez commented Sep 3, 2021

not yet, i've been on vacation for a couple of weeks. i'll give it a try tomorrow!

got it:

FROM ubuntu:groovy

RUN DEBIAN_FRONTEND=noninteractive apt-get update -y && \
    DEBIAN_FRONTEND=noninteractive TZ=UTC apt-get install \
      --no-install-recommends -y \
      curl \
      fluxbox \
      gpg \
      gpg-agent \
      libmp3lame0 \
      libopus0 \
      libpulse0 \
      mplayer \
      pulseaudio \
      software-properties-common \
      tigervnc-standalone-server \
      vlc \
      xterm \
      xz-utils && \
    rm -rf /var/lib/apt/lists/*

RUN curl -sL https://github.com/replit/rfbproxy/releases/download/v0.1.1-b81eb2f/rfbproxy.tar.xz | tar xJ -C /

RUN useradd --create-home runner
WORKDIR /home/runner
USER runner

ENV DISPLAY=:0
# Verbosity can be upped by changing `RUST_LOG=debug` in the script below.
RUN echo $'\n\
#!/bin/bash\n\
Xtigervnc --SecurityTypes=None --rfbport=5901 --localhost "${DISPLAY}" 2>&1 | sed "s/^/[Xtigervnc ] /" & \n\
sleep 1 \n\
RUST_LOG=info rfbproxy --enable-audio 2>&1 | sed "s/^/[rfbproxy  ] /" & \n\
sleep 1 \n\
fluxbox 2>&1 | sed "s/^/[fluxbox   ] /" & \n\
pulseaudio --load="module-null-sink sink_name=null" 2>&1 | sed "s/^/[pulseaudio] /" & \n\
sleep 1 \n\
/usr/bin/vlc --no-qt-privacy-ask --alsa-audio-device=pulse --aout=pulse http://ice2.somafm.com/groovesalad-128-aac 2>&1 | sed "s/^/[vlc       ] /"'\
> /home/runner/start.sh

EXPOSE 5900
ENTRYPOINT ["/bin/bash", "/home/runner/start.sh"]

run with:

docker build -f Dockerfile -t rfbproxy . && docker run -it --rm -p 5900:5900 rfbproxy

to get some nice tunes courtesy of https://somafm.com/groovesalad/ on ws://localhost:5900/

@CendioOssman
Copy link
Member

Thanks. That image seems to work fine as constructed. I get a noticeable latency in the audio though, but that is somewhat expected given that this protocol has no synchronisation.

However, only VLC seems to work properly. If I use mplayer then the stuttering is back. Can you see if it's the same when you test it? Are things perhaps reliant on the application providing a certain amount of buffered data?

@Neustradamus
Copy link

@lhchavez: Any news about it?

@masad-frost
Copy link

Hey! I'm from Replit and would love to land this instead of maintaining our own fork.

What would you like to see for this PR to land?

For what it's worth, we have been using this in production for a few months now and people seem to be happy with the audio.

@masad-frost masad-frost mentioned this pull request Oct 27, 2022
@CendioOssman
Copy link
Member

Please see my previous comment. I could not get this to work reliably, which means I don't feel confident it will work reliably for our users.

Have you applied any more fixes in your fork that aren't included here?

@CendioOssman
Copy link
Member

No response for two years. Closing.

@samhed samhed removed this from the Future Features milestone Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants