You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2021/04/30 00:19:55 UTC

[GitHub] [guacamole-server] mike-jumper opened a new pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

mike-jumper opened a new pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337


   These changes are a fresh attempt at addressing the issue with audio input packet timing originally identified by @myjimmy. They take a similar approach to his proposed changes (#312), and are intended to achieve the same fix in principle (throttling audio data) while addressing the issues noted in code review of the original PR. Specifically:
   
   * Packet scheduling should be dynamic and based on the needs of the RDP server.
   * The flush thread should not poll the audio buffer in a loop to check for new data, but wait for that data to be received.
   * While received audio data does need to be buffered to compensate for difference in packet size and timing, that buffer needs to be finite. It should not grow without bound.
   
   I've tried to describe the nature of these changes in detail in the commit itself (72812ce73f594278acc70356f6f757ef02ae2242). From the commit message:
   
   >
   > The RDP specification for the AUDIO_INPUT channel requires that all audio be sent in packets of a specific size. Guacamole does correctly limit itself to sending packets of this size to the RDP server, but will send quite a few of these packets all at once if it has received more audio data than the RDP packet size. This is OK in principle (the Guacamole client should be able to send audio in packets of whatever size it chooses), but may overwhelm the software running within the RDP server if the amount of data received exceeds the available buffer space, resulting in dropped samples.
   >
   > As there is no way to know the size of the remote audio buffer, we need to instead ensure that audio is streamed as close to real time as possible, with each audio packet of N bytes not being sent until roughly the amount of time represented by those N bytes has elapsed since the last packet. This throttling ensures that software expecting to process audio in real time should never run out of buffer space.
   >
   > That said, if we never exceed the per-packet data rate and occasionally send a packet earlier than real time would dictate, unavoidable latency in sending/receiving audio data would accumulate over time. For example, if each audio packet represents 10ms of audio data, but we receive that audio packet 10.1ms after the previous packet, we need to adjust the timing of the next audio packet(s) to account for that additional 0.1ms. Simply waiting 10ms after sending each packet would cause that 0.1ms to accumulate each time it occurs, eventually resulting in noticable latency and finally running out of buffer space.
   >
   > Thus, these changes:
   >
   > 1. Leverage a flush thread and per-packet scheduling to ensure that each flushed audio packet does not exceed the equivalent real time rate.
   > 2. Calculate the amount of additional latency from the amount of data received beyond the required packet size, and amortize scheduling corrections to account for that latency over the next several audio packets.
   >
   > This ensures that audio is streamed exactly as it is received if the audio matches the packet size of the RDP server, and audio that is received in a different size or varying sizes is buffered and throttled to keep things within the expectations of software running within the RDP server.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] mike-jumper commented on pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

mike-jumper commented on pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#issuecomment-850243601


   > Tested this with Teams on Windows 10 via RDP guacd and still felt like there was stutters in the audio stream over a teams call.
   
   Are you sure that the latest guacamole-server code git master was built and installed, and that the FreeRDP plugin for Guacamole audio input installed by guacamole-server (`libguacai-client.so`) is the version from that build?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] davydotcom commented on pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

davydotcom commented on pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#issuecomment-850314704


   Positive. Built off master using the Dockerfile as baseline too. Works perfectly smooth btw in Chrome for Mac and Chrome for Linux. It's a windows issue. I'd wager it's due to using the scriptProcessor instead of audio worker api and the UI thread is bottlenecked. This was verified on 3 different windows PCs 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] mike-jumper commented on pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

mike-jumper commented on pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#issuecomment-850708878

> Works perfectly smooth btw in Chrome for Mac and Chrome for Linux. It's a windows issue. I'd wager it's due to using the scriptProcessor instead of audio worker api and the UI thread is bottlenecked. This was verified on 3 different windows PCs

I'm skeptical that usage of `ScriptProcessorNode` would be the cause of any issue. There is very little processing involved in streaming each packet of audio data. If the issue were that there is so much processing occurring in the UI thread that audio data is dropped, I would expect the issue to occur across all platforms and for the entire interface to feel unresponsive.

The nature of the changes in this PR is to allow the server side to adjust for variance in client-side buffer size, automatically spreading things out while throttling the data rate to ensure we don't deviate from the expectations of the remote audio buffer. Perhaps there is some platform-specific difference in the client-side buffer on Windows that results in these changes not behaving as expected?

What specific version of Windows and Chrome are you testing against when you see the issue? Do you see the issue when using something like Audacity?

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] davydotcom commented on pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

davydotcom commented on pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#issuecomment-842719676


   Tested this with Teams on Windows 10 via RDP guacd and still felt like there was stutters in the audio stream over a teams call. Receiving audio was perfect, microphone audio still seemed to stutter and have no difference from 1.2.0 guacamole. Connection was local LAN gigabit to remote target.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] necouchman commented on a change in pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

necouchman commented on a change in pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#discussion_r623808516



##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -136,13 +161,33 @@ struct guac_rdp_audio_buffer {
      */
     char* packet;
 
+    /**
+     * Thread which flushes the audio buffer at a rate that does not exceed the
+     * the audio sample rate (which might result dropped samples due to

Review comment:
       *might result _in_ dropped samples...




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

mike-jumper commented on a change in pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#discussion_r624580361



##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -136,13 +161,33 @@ struct guac_rdp_audio_buffer {
      */
     char* packet;
 
+    /**
+     * Thread which flushes the audio buffer at a rate that does not exceed the
+     * the audio sample rate (which might result dropped samples due to

Review comment:
       Oops. Fixed via rebase.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] necouchman merged pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

necouchman merged pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] davydotcom commented on pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

davydotcom commented on pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#issuecomment-843241758


   The plot thickens. When connected via chrome browser on Mac or Linux audio quality of microphone is pretty good. Its phenomenaly worse when using Chrome on a PC


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] mike-jumper edited a comment on pull request #337: GUACAMOLE-1201: Throttle outbound audio data to avoid overflowing RDP server audio input buffer.

Posted by GitBox <gi...@apache.org>.

mike-jumper edited a comment on pull request #337:
URL: https://github.com/apache/guacamole-server/pull/337#issuecomment-850708878

What specific version of Windows and Chrome are you testing against when you see the issue? Do you see the issue when using something like Audacity (rather than Microsoft Teams, which I do not currently have access to test against)?

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org