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 2020/11/06 15:44:08 UTC
[GitHub] [guacamole-server] myjimmy opened a new pull request #312: Fix audio input issue on mme
myjimmy opened a new pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312
I have fixed the audio input issues (noise & sped up) on MME.
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r518992278
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -233,6 +239,96 @@ 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 *tmp = (audio_stream_list*)malloc(sizeof(audio_stream_list));
+ tmp->data = malloc(length);
+
+ /* Write the stream data to the new buffer. */
+ tmp->length = length;
+ tmp->next = NULL;
+ memcpy(tmp->data, buffer, tmp->length);
+
+ /* Add the new buffer into the list. */
+ if (audio_buffer->first_stream_list == NULL) {
+ audio_buffer->first_stream_list = tmp;
+ audio_buffer->last_stream_list = tmp;
+ }
+ else {
+ audio_buffer->last_stream_list->next = (void*)tmp;
+ audio_buffer->last_stream_list =
+ (audio_stream_list*)audio_buffer->last_stream_list->next;
+ }
Review comment:
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.
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723257068
Regarding the above, I'd like to also point out the commit messages themselves:
Just like the comments, these should be aimed at capturing what someone reviewing the code later will need to understand about the context of your changes.
Something like "Fixed the audio issues on MME" doesn't capture what is being fixed and why. If someone goes through this in a `git bisect` later, finds that this is the change that broke something, they are going to have quite a bit of further research ahead of them before they find out what issue that is and why these changes were made.
A messages like "Added some comments" is only negligibly better that a message like "Changed some code". What were you doing and why? If the change doesn't feel significantly separate your earlier commits, it can just be squashed into the most relevant commit. If it does, then there should be some context that can be provided. Presumably those comments were to document something specific?
----------------------------------------------------------------
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] myjimmy commented on pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723833894
@mike-jumper, I have just modified the code, comments, and commits.
Could you check the pull request again?
----------------------------------------------------------------
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 closed pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper closed pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312
--
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] myjimmy commented on a change in pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r532774544
##########
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:
Because the time interval of sending audio packets isn't even and the time interval occasionally exceeds one packet length, the choppy issue occurred.
So, I think that we need to make sure that the intervals will at least be one half of one packet length.
----------------------------------------------------------------
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] myjimmy commented on pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-832234012
@mike-jumper Thanks for your reply.
And I checked your fix (#337).
This is wonderful. I learned many things from you.
--
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r532805321
##########
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:
Sure. That being the case:
* That would be the reasoning to cite here, not Nyquist's theorem.
* This sounds like a workaround for a problem that results from not properly calculating the send interval in the first place. Rather than use a constant interval, falling back to a hard-coded 5ms if the interval is sufficiently small, shouldn't the interval be determined dynamically based on (1) how much time has elapsed since the last time a packet was sent and (2) how much audio data is about to be sent?
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r518995032
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -273,5 +310,10 @@ 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.
+ */
+void* guac_rdp_audio_send_thread(void* data);
Review comment:
The parameter and return value need to be documented here.
Also, as mentioned in the earlier review regarding code comments, the documentation describing this function should also capture the high-level use. In this case, the fact that the thread will flush data at a regular, constant rate, and that this rate is intended to prevent the remote audio buffer from running out of space seems like it would be good to note here. Lacking that information, it is unclear why a timed and threaded approach is needed, and future changes may well revert this in an attempt to make things more efficient without realizing the consequences.
----------------------------------------------------------------
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 pull request #312: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723151267
@myjimmy: Please tag the pull request with the JIRA issue, as well.
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r519088659
##########
File path: src/protocols/rdp/client.h
##########
@@ -87,6 +87,11 @@
*/
#define GUAC_RDP_AUDIO_BPS 16
+/**
+ * Interval to send the audio input packet to a remote server, in ms.
+ */
+#define GUAC_RDP_AUDIO_INPUT_SEND_INTERVAL 5
Review comment:
Rather than specify an arbitrary static value, perhaps the interval should be derived from the number of audio frames that the RDP server dictates should be sent in each PDU?
----------------------------------------------------------------
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] myjimmy commented on pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723260155
@mike-jumper, Thanks for your reply.
In addition to the comments, the coding style, and the commit message that you pointed out, aren't there technically any other problems in my code?
----------------------------------------------------------------
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] myjimmy commented on a change in pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r532719764
##########
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:
I'll modify this.
----------------------------------------------------------------
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] myjimmy commented on pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723309496
@mike-jumper, Please let me know how to modify the code and the comment and the commit message in the pull request.
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r518991030
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.c
##########
@@ -242,6 +338,7 @@ void guac_rdp_audio_buffer_write(guac_rdp_audio_buffer* audio_buffer,
pthread_mutex_unlock(&(audio_buffer->lock));
return;
}
+ pthread_mutex_unlock(&(audio_buffer->lock));
Review comment:
This mutex is documented as guarding all access to `audio_buffer`. Unlocking things here and then operating on the `audio_buffer` without holding the mutex looks like a threadsafety issue.
If you are certain this is safe, can you describe how that has been verified?
----------------------------------------------------------------
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] myjimmy commented on pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723158362
Sorry, I understand. I have just modified the title of the pull request.
----------------------------------------------------------------
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] myjimmy commented on pull request #312: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723157117
> @myjimmy: Please tag the pull request with the JIRA issue, as well.
@necouchman, what does it mean? Sorry, I didn't understand.
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r518993125
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -66,6 +66,27 @@ typedef struct guac_rdp_audio_format {
} guac_rdp_audio_format;
+/**
+ * A structure of list to take the audio buffers.
+ */
+typedef struct audio_stream_list {
+ /**
+ * The data of audio buffer
+ */
+ char* data;
+
+ /**
+ * The length of audio buffer
+ */
+ int length;
+
+ /**
+ * The pointer of the next audio buffer
+ */
+ void* next;
Review comment:
This should be a pointer to a concrete type, presumably `audio_stream_list` (which would then need a forward declaration). We should only be using `void*` for pointers that truly are intended to point to arbitrary data.
----------------------------------------------------------------
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] myjimmy edited a comment on pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy edited a comment on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723309496
@mike-jumper, Thanks for your kind reply.
Please let me know how to modify the code and the comment and the commit message in the pull request.
----------------------------------------------------------------
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-830771770
Reattempted and addressed via #337. Thanks for finding this and the initial stab at the fix, @myjimmy.
--
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 #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#issuecomment-723286392
> In addition to the comments, the coding style, and the commit message that you pointed out, aren't there technically any other problems in my code?
I'll check, but ease of reviewing is part of the reason I'd like that addressed.
----------------------------------------------------------------
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] myjimmy commented on a change in pull request #312: GUACAMOLE-1201: Fix audio input issue on mme
Posted by GitBox <gi...@apache.org>.
myjimmy commented on a change in pull request #312:
URL: https://github.com/apache/guacamole-server/pull/312#discussion_r532719536
##########
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:
When defining the thread callback function and using it, the function generally returns a NULL pointer.
I documented this as the above comment.
----------------------------------------------------------------
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