mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r531782386
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -145,6 +168,27 @@ typedef struct guac_rdp_audio_buffer {
*/
void* data;
+ /**
+ * The thread to send the audio input stream.
+ */
+ pthread_t send_thread;
+
+ /**
+ * The flag to stop the thread.
+ */
+ int stop_thread;
+
+ /**
+ * The list to take the audio buffers.
+ */
+ audio_stream_list* first_stream_list;
+ audio_stream_list* last_stream_list;
Review comment:
Two things:
* It is unclear from "The list to take the audio buffers" what these
structure members are actually for. Is there a better way to describe this?
* Both structure members need to be documented.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -233,6 +252,97 @@ static int guac_rdp_audio_buffer_read_sample(
void guac_rdp_audio_buffer_write(guac_rdp_audio_buffer* audio_buffer,
char* buffer, int length) {
+ pthread_mutex_lock(&(audio_buffer->lock));
+
+ /* Ignore packet if there is no buffer */
+ if (audio_buffer->packet_size == 0 || audio_buffer->packet == NULL) {
+ pthread_mutex_unlock(&(audio_buffer->lock));
+ return;
+ }
+
+ /* Allocate the new buffer. */
+ audio_stream_list *new_stream =
(audio_stream_list*)malloc(sizeof(audio_stream_list));
+ new_stream->stream_data = malloc(length);
+
+ /* Write the stream data to the new buffer. */
+ new_stream->length = length;
+ new_stream->next_stream = NULL;
+ memcpy(new_stream->stream_data, buffer, new_stream->length);
+
+ /* Add the new buffer into the list. */
+ if (audio_buffer->first_stream_list == NULL) {
+ audio_buffer->first_stream_list = new_stream;
+ audio_buffer->last_stream_list = new_stream;
+ }
+ else {
+ audio_buffer->last_stream_list->next_stream = new_stream;
+ audio_buffer->last_stream_list =
+ (audio_stream_list*)audio_buffer->last_stream_list->next_stream;
+ }
+
+ pthread_mutex_unlock(&(audio_buffer->lock));
+
+}
+
+void guac_rdp_audio_buffer_end(guac_rdp_audio_buffer* audio_buffer) {
+
+ /* Set the flag to stop the thread */
+ pthread_mutex_lock(&(audio_buffer->lock));
+ audio_buffer->stop_thread = 1;
+ pthread_mutex_unlock(&(audio_buffer->lock));
+
+ /* Stop the thread */
+ pthread_join(audio_buffer->send_thread, NULL);
Review comment:
This is not stopping the thread, but waiting for the thread to stop.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -276,49 +387,54 @@ void guac_rdp_audio_buffer_write(guac_rdp_audio_buffer*
audio_buffer,
/* Reset buffer in all cases */
audio_buffer->bytes_written = 0;
+ /* Wait in a specified interval. */
+ guac_timestamp_msleep(audio_buffer->packet_interval);
+
}
} /* end packet write loop */
/* Track current position in audio stream */
audio_buffer->total_bytes_received += length;
- pthread_mutex_unlock(&(audio_buffer->lock));
-
}
-void guac_rdp_audio_buffer_end(guac_rdp_audio_buffer* audio_buffer) {
+void* guac_rdp_audio_send_thread(void* data) {
- pthread_mutex_lock(&(audio_buffer->lock));
+ guac_rdp_audio_buffer* audio_buffer = (guac_rdp_audio_buffer*)data;
- /* The stream is now closed */
- guac_rdp_audio_buffer_ack(audio_buffer,
- "CLOSED", GUAC_PROTOCOL_STATUS_RESOURCE_CLOSED);
+ /* Handle the audio packets while client is running */
+ while (!audio_buffer->stop_thread) {
- /* Unset user and stream */
- audio_buffer->user = NULL;
- audio_buffer->stream = NULL;
-
- /* Reset buffer state */
- audio_buffer->bytes_written = 0;
- audio_buffer->packet_size = 0;
- audio_buffer->flush_handler = NULL;
-
- /* Reset I/O counters */
- audio_buffer->total_bytes_sent = 0;
- audio_buffer->total_bytes_received = 0;
-
- /* Free packet (if any) */
- free(audio_buffer->packet);
- audio_buffer->packet = NULL;
-
- pthread_mutex_unlock(&(audio_buffer->lock));
+ /* Get the audio buffer from the list */
+ pthread_mutex_lock(&(audio_buffer->lock));
+ audio_stream_list* current = audio_buffer->first_stream_list;
+ pthread_mutex_unlock(&(audio_buffer->lock));
-}
+ /* Check whether the audio buffer will be sent. */
+ if (current != NULL) {
+
+ /* Send the audio buffer to the remote server. */
+ char* stream = current->stream_data;
+ int length = current->length;
+ guac_rdp_audio_buffer_send(audio_buffer, stream, length);
+
+ /* Remove the audio buffer from the list. */
+ pthread_mutex_lock(&audio_buffer->lock);
+ audio_buffer->first_stream_list =
+
(audio_stream_list*)audio_buffer->first_stream_list->next_stream;
+ pthread_mutex_unlock(&audio_buffer->lock);
+
+ /* Free the audio buffer. */
+ free(current->stream_data);
+ free(current);
+ }
+ else {
+ /* Wait again if there is no any audio buffer */
+ guac_timestamp_msleep(audio_buffer->packet_interval);
+ }
Review comment:
I don't think we should use an approach which effectively polls
`audio_buffer` every ~5ms. The code can be aware of when data is actually
received and avoid busy-looping.
I would think the overall required algorithm here would be:
1. (Track the overall amount of audio data sent.)
2. Wait for receipt of audio data.
3. If the amount of data is insufficient for one packet (less than the
required frames per packet), continue waiting.
4. Once sufficient audio data has been received, ensure that at least one
packet's worth of time has elapsed since the last packet was sent (waiting the
remaining amount of time if needed), send the packet, repeat until insufficient
data remains.
That way:
* Audio data is sent (ideally) as soon as it is received.
* The rate of audio data packets never instantaneously exceeds the rate at
which it is recorded (the older code only ensured it wouldn't exceed this rate
_on average_, and thus could still exhaust remote buffer space).
It may be necessary to use a higher-resolution time source than that used by
`guac_timestamp_msleep()`.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -273,5 +317,22 @@ void guac_rdp_audio_buffer_end(guac_rdp_audio_buffer*
audio_buffer);
*/
void guac_rdp_audio_buffer_free(guac_rdp_audio_buffer* audio_buffer);
+/**
+ * This thread handler sends the audio input stream data to the remote server
+ * in the uniform interval.
+ * When sending it via the MME interface of the remote server as soon as
+ * receiving the audio input stream from the client, the missing some audio
stream
+ * occurs due to the lack of remote audio buffer.
+ * So, to prevent the remote audio buffer from running out of space seems,
+ * we need to use the thread to send the audio stream in the uniform interval.
+ *
+ * @param data
+ * The pointer to the guac_rdp_audio_buffer object maintained by RDP
session.
+ *
+ * @return
+ * NULL. This is a normal format of the general thread callback functions.
Review comment:
> This is a normal format of the general thread callback functions.
What do you mean by this?
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -276,49 +387,54 @@ void guac_rdp_audio_buffer_write(guac_rdp_audio_buffer*
audio_buffer,
/* Reset buffer in all cases */
audio_buffer->bytes_written = 0;
+ /* Wait in a specified interval. */
+ guac_timestamp_msleep(audio_buffer->packet_interval);
Review comment:
As mentioned in the previous review:
> Yes, that's what `guac_timestamp_msleep()` does, but there must be
something that you can capture within the documentation that adds information
and context beyond restating the code in English form.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -120,6 +121,11 @@ void guac_rdp_audio_buffer_begin(guac_rdp_audio_buffer*
audio_buffer,
void* data) {
pthread_mutex_lock(&(audio_buffer->lock));
+
+ /* Create a thread */
+ audio_buffer->stop_thread = 0;
+ pthread_create(&audio_buffer->send_thread, NULL,
+ guac_rdp_audio_send_thread, audio_buffer);
Review comment:
Please make sure that your comments provide more context than simply
restating the code itself in English.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -66,6 +66,29 @@ typedef struct guac_rdp_audio_format {
} guac_rdp_audio_format;
+/**
+ * A structure of linked list to store the audio stream periodically
+ * sent by the client.
+ */
+typedef struct audio_stream_list audio_stream_list;
+struct audio_stream_list {
+ /**
+ * The pointer of audio stream
+ */
+ char* stream_data;
Review comment:
Previously, this read:
```c
/**
* The data of audio buffer
*/
char* data;
```
From the previous review:
> The documentation of the structure and its members should clarify things
at a high level. You say "the data of the audio buffer", but what _is_ that?
What data?
I think the above still applies here. It is unclear what this structure
member actually is. The documentation should clarify things at a high level.
##########
File path: src/protocols/rdp/client.h
##########
@@ -87,6 +87,11 @@
*/
#define GUAC_RDP_AUDIO_BPS 16
+/**
+ * The default interval to send the audio input packet to a remote server, in
ms.
+ */
+#define GUAC_RDP_AUDIO_INPUT_SEND_DEFAULT_INTERVAL 5
Review comment:
I don't think this should be needed if the outbound audio data rate is
correctly throttled based on the server-requested frames per packet.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -233,6 +252,97 @@ static int guac_rdp_audio_buffer_read_sample(
void guac_rdp_audio_buffer_write(guac_rdp_audio_buffer* audio_buffer,
char* buffer, int length) {
+ pthread_mutex_lock(&(audio_buffer->lock));
+
+ /* Ignore packet if there is no buffer */
+ if (audio_buffer->packet_size == 0 || audio_buffer->packet == NULL) {
+ pthread_mutex_unlock(&(audio_buffer->lock));
+ return;
+ }
+
+ /* Allocate the new buffer. */
+ audio_stream_list *new_stream =
(audio_stream_list*)malloc(sizeof(audio_stream_list));
+ new_stream->stream_data = malloc(length);
+
+ /* Write the stream data to the new buffer. */
+ new_stream->length = length;
+ new_stream->next_stream = NULL;
+ memcpy(new_stream->stream_data, buffer, new_stream->length);
+
+ /* Add the new buffer into the list. */
+ if (audio_buffer->first_stream_list == NULL) {
+ audio_buffer->first_stream_list = new_stream;
+ audio_buffer->last_stream_list = new_stream;
+ }
+ else {
+ audio_buffer->last_stream_list->next_stream = new_stream;
+ audio_buffer->last_stream_list =
+ (audio_stream_list*)audio_buffer->last_stream_list->next_stream;
+ }
Review comment:
As mentioned in the previous review:
> If new memory will be allocated for every received `blob` of audio data,
there needs to be some reasonable upper bound on how much pending audio data
will be stored.
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -131,6 +137,19 @@ void guac_rdp_audio_buffer_begin(guac_rdp_audio_buffer*
audio_buffer,
* audio_buffer->out_format.channels
* audio_buffer->out_format.bps;
+ /* Calculate the interval to send each packet in ms.
+ * We need to set half of the time duration designated
+ * by packet frames according to Nyquist's theorem.
+ */
+ audio_buffer->packet_interval = packet_frames
+ * 1000
+ / audio_buffer->out_format.rate
+ / 2;
Review comment:
Nyquist's theorem deals with the sample rate required to adequately
reproduce a signal. It doesn't deal with how often audio packets must be sent,
which would simply be the length of time represented by those packets.
----------------------------------------------------------------
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:
[email protected]
