[pulseaudio-discuss] [PATCH 23/25] raop: Refactor UDP packet send path

[Hajime Fujita]

Current code is written for a case where send(2) for UDP socket
succeeds partially. However unlike for TCP for other regular
file stream, UDP (SOCK_DGRAM) socket has a kind of atomic property
that single send(2) forms a single UDP datagram. Therefore looping
send for single packet data is useless. In fact, I believe that
"partial write" won't happen for UDP sockets (either succeeds or
fails if too big).

This patch also does some refactoring around UDP send path. RTP
packet header construction is moved to a newly introduced function
udp_build_audio_header and its endian dependency is removed.
---
 src/modules/raop/module-raop-sink.c | 13 ++++------
 src/modules/raop/raop_client.c      | 52 +++++++++++++++++++++----------------
 src/modules/raop/raop_client.h      |  2 +-
 3 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/src/modules/raop/module-raop-sink.c b/src/modules/raop/module-raop-sink.c
index bd9963b..b600cf8 100644
--- a/src/modules/raop/module-raop-sink.c
+++ b/src/modules/raop/module-raop-sink.c
@@ -766,7 +766,11 @@ static void udp_thread_func(struct userdata *u) {
 
         pa_assert(u->encoded_memchunk.length > 0);
 
-        pa_raop_client_udp_send_audio_packet(u->raop, &u->encoded_memchunk, &written);
+        written = pa_raop_client_udp_send_audio_packet(u->raop,&u->encoded_memchunk);
+        if (written < 0) {
+            pa_log("Failed to send UDP packet: %s", pa_cstrerror(errno));
+            goto fail;
+        }
 
         /* Determine when to wake up next:
            next_target = prev_target + interval, unless we passed
@@ -779,13 +783,6 @@ static void udp_thread_func(struct userdata *u) {
         /* Sleep until next packet transmission */
         pa_rtpoll_set_timer_absolute(u->rtpoll, wakeup_target);
 
-        pa_assert(written != 0);
-
-        if (written < 0) {
-            pa_log("Failed to write data to FIFO: %s", pa_cstrerror(errno));
-            goto fail;
-        }
-
         u->offset += written;
         u->encoding_overhead += overhead;
 
diff --git a/src/modules/raop/raop_client.c b/src/modules/raop/raop_client.c
index d8b1d17..f6dc76d 100644
--- a/src/modules/raop/raop_client.c
+++ b/src/modules/raop/raop_client.c
@@ -519,26 +519,24 @@ static int udp_send_sync_packet(pa_raop_client *c, uint32_t stamp) {
     return rv;
 }
 
-static int udp_send_audio_packet(pa_raop_client *c, uint32_t *buffer, size_t size, ssize_t *written) {
-    ssize_t length = 0;
-    int rv = 1;
+static void udp_build_audio_header(pa_raop_client *c, uint32_t *buffer, size_t size) {
+    pa_assert(size >= sizeof(udp_audio_header));
 
     memcpy(buffer, udp_audio_header, sizeof(udp_audio_header));
     if (c->udp_first_packet)
-        buffer[0] |= ((uint32_t) 0x80) << 8;
+        buffer[0] |= htonl((uint32_t) 0x80 << 16);
     buffer[0] |= htonl((uint32_t) c->seq);
     buffer[1] = htonl(c->rtptime);
     buffer[2] = htonl(c->udp_ssrc);
+}
 
-    length = pa_loop_write(c->udp_stream_fd, buffer, size, NULL);
-    if (length == ((ssize_t) size))
-        rv = 0;
+static ssize_t udp_send_audio_packet(pa_raop_client *c, uint8_t *buffer, size_t size) {
+    ssize_t length;
 
-    if (written != NULL)
-        *written = length;
+    length = pa_write(c->udp_stream_fd, buffer, size, NULL);
     c->seq++;
 
-    return rv;
+    return length;
 }
 
 static void do_rtsp_announce(pa_raop_client *c) {
@@ -1191,10 +1189,9 @@ int pa_raop_client_udp_get_blocks_size(pa_raop_client *c, size_t *size) {
     return rv;
 }
 
-int pa_raop_client_udp_send_audio_packet(pa_raop_client *c, pa_memchunk *block, ssize_t *written) {
-    uint32_t *buf = NULL;
-    ssize_t length = 0;
-    int rv = 0;
+ssize_t pa_raop_client_udp_send_audio_packet(pa_raop_client *c, pa_memchunk *block) {
+    uint8_t *buf = NULL;
+    ssize_t len;
 
     pa_assert(c);
     pa_assert(block);
@@ -1207,19 +1204,30 @@ int pa_raop_client_udp_send_audio_packet(pa_raop_client *c, pa_memchunk *block,
         c->udp_sync_count++;
     }
 
-    buf = (uint32_t *) pa_memblock_acquire(block->memblock);
-    if (buf != NULL && block->length > 0)
-        rv = udp_send_audio_packet(c, buf + block->index, block->length, &length);
+    buf = pa_memblock_acquire(block->memblock);
+    pa_assert(buf);
+    pa_assert(block->length > 0);
+    udp_build_audio_header(c, (uint32_t *) (buf + block->index), block->length);
+    len = udp_send_audio_packet(c, buf + block->index, block->length);
     pa_memblock_release(block->memblock);
-    block->index += length;
-    block->length -= length;
-    if (written != NULL)
-        *written = length;
+
+    if (len > 0) {
+        pa_assert((size_t) len <= block->length);
+        /* UDP packet has to be sent at once, so it is meaningless to
+           preseve the partial data
+           FIXME: This won't happen at least in *NIX systems?? */
+        if (block->length > (size_t) len) {
+            pa_log_warn("Tried to send %zu bytes but managed to send %zu bytes", block->length, len);
+            len = block->length;
+        }
+        block->index += block->length;
+        block->length = 0;
+    }
 
     if (c->udp_first_packet)
         c->udp_first_packet = false;
 
-    return rv;
+    return len;
 }
 
 int pa_raop_client_set_volume(pa_raop_client *c, pa_volume_t volume) {
diff --git a/src/modules/raop/raop_client.h b/src/modules/raop/raop_client.h
index 1b8048d..c319f34 100644
--- a/src/modules/raop/raop_client.h
+++ b/src/modules/raop/raop_client.h
@@ -51,7 +51,7 @@ int pa_raop_client_udp_handle_timing_packet(pa_raop_client *c, const uint8_t pac
 [], ssize_t size);
 int pa_raop_client_udp_handle_control_packet(pa_raop_client *c, const uint8_t packet[], ssize_t size);
 int pa_raop_client_udp_get_blocks_size(pa_raop_client *c, size_t *size);
-int pa_raop_client_udp_send_audio_packet(pa_raop_client *c, pa_memchunk *block, ssize_t *written);
+ssize_t pa_raop_client_udp_send_audio_packet(pa_raop_client *c, pa_memchunk *block);
 
 typedef void (*pa_raop_client_cb_t)(int fd, void *userdata);
 void pa_raop_client_tcp_set_callback(pa_raop_client *c, pa_raop_client_cb_t callback, void *userdata);
-- 
1.8.1.2