[pulseaudio-discuss] [PATCH] rtp: don't use memblocks for non-audio data

[Tanu Kaskinen]

pa_memblockq_push() expects all memchunks to be aligned to PCM frame
boundaries, and that means both the index and length fields of
pa_memchunk. pa_rtp_recv(), however, used a memblock for storing both
the RTP packet metadata and the actual audio data. The metadata was
"removed" from the audio data by setting the memchunk index
appropriately, so the metadata stayed in the memblock, but it was not
played back. The metadata length is not necessarily divisible by the PCM
frame size, which caused pa_memblock_push() to crash in an assertion
with some sample specs, because the memchunk index was not properly
aligned. In my tests the metadata length was 12, so it was compatible
with many configurations, but eight-channel audio didn't work.

This patch adds a separate buffer for receiving the RTP packets. As a
result, an extra memcpy is needed for moving the audio data from the
receive buffer to the memblock buffer.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96612
---
 src/modules/rtp/rtp.c | 66 ++++++++++++++++++++++++++++++++++-----------------
 src/modules/rtp/rtp.h |  2 ++
 2 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/src/modules/rtp/rtp.c b/src/modules/rtp/rtp.c
index 17c8d3c..5ab80e7 100644
--- a/src/modules/rtp/rtp.c
+++ b/src/modules/rtp/rtp.c
@@ -54,6 +54,8 @@ pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssr
     c->payload = (uint8_t) (payload & 127U);
     c->frame_size = frame_size;
 
+    c->recv_buf = NULL;
+    c->recv_buf_size = 0;
     pa_memchunk_reset(&c->memchunk);
 
     return c;
@@ -155,12 +157,16 @@ pa_rtp_context* pa_rtp_context_init_recv(pa_rtp_context *c, int fd, size_t frame
     c->fd = fd;
     c->frame_size = frame_size;
 
+    c->recv_buf_size = 2000;
+    c->recv_buf = pa_xmalloc(c->recv_buf_size);
     pa_memchunk_reset(&c->memchunk);
     return c;
 }
 
 int pa_rtp_recv(pa_rtp_context *c, pa_memchunk *chunk, pa_mempool *pool, struct timeval *tstamp) {
     int size;
+    size_t audio_length;
+    size_t metadata_length;
     struct msghdr m;
     struct cmsghdr *cm;
     struct iovec iov;
@@ -204,25 +210,17 @@ int pa_rtp_recv(pa_rtp_context *c, pa_memchunk *chunk, pa_mempool *pool, struct
         size = 1;
     }
 
-    if (c->memchunk.length < (unsigned) size) {
-        size_t l;
-
-        if (c->memchunk.memblock)
-            pa_memblock_unref(c->memchunk.memblock);
+    if (c->recv_buf_size < (size_t) size) {
+        do
+            c->recv_buf_size *= 2;
+        while (c->recv_buf_size < (size_t) size);
 
-        l = PA_MAX((size_t) size, pa_mempool_block_size_max(pool));
-
-        c->memchunk.memblock = pa_memblock_new(pool, l);
-        c->memchunk.index = 0;
-        c->memchunk.length = pa_memblock_get_length(c->memchunk.memblock);
+        c->recv_buf = pa_xrealloc(c->recv_buf, c->recv_buf_size);
     }
 
-    pa_assert(c->memchunk.length >= (size_t) size);
-
-    chunk->memblock = pa_memblock_ref(c->memchunk.memblock);
-    chunk->index = c->memchunk.index;
+    pa_assert(c->recv_buf_size >= (size_t) size);
 
-    iov.iov_base = pa_memblock_acquire_chunk(chunk);
+    iov.iov_base = c->recv_buf;
     iov.iov_len = (size_t) size;
 
     m.msg_name = NULL;
@@ -234,7 +232,6 @@ int pa_rtp_recv(pa_rtp_context *c, pa_memchunk *chunk, pa_mempool *pool, struct
     m.msg_flags = 0;
 
     r = recvmsg(c->fd, &m, 0);
-    pa_memblock_release(chunk->memblock);
 
     if (r != size) {
         if (r < 0 && errno != EAGAIN && errno != EINTR)
@@ -275,21 +272,42 @@ int pa_rtp_recv(pa_rtp_context *c, pa_memchunk *chunk, pa_mempool *pool, struct
     c->payload = (uint8_t) ((header >> 16) & 127U);
     c->sequence = (uint16_t) (header & 0xFFFFU);
 
-    if (12 + cc*4 > (unsigned) size) {
+    metadata_length = 12 + cc * 4;
+
+    if (metadata_length > (unsigned) size) {
         pa_log_warn("RTP packet too short. (CSRC)");
         goto fail;
     }
 
-    chunk->index += 12 + cc*4;
-    chunk->length = (size_t) size - 12 + cc*4;
+    audio_length = size - metadata_length;
 
-    if (chunk->length % c->frame_size != 0) {
+    if (audio_length % c->frame_size != 0) {
         pa_log_warn("Bad RTP packet size.");
         goto fail;
     }
 
-    c->memchunk.index = chunk->index + chunk->length;
-    c->memchunk.length = pa_memblock_get_length(c->memchunk.memblock) - c->memchunk.index;
+    if (c->memchunk.length < (unsigned) audio_length) {
+        size_t l;
+
+        if (c->memchunk.memblock)
+            pa_memblock_unref(c->memchunk.memblock);
+
+        l = PA_MAX((size_t) audio_length, pa_mempool_block_size_max(pool));
+
+        c->memchunk.memblock = pa_memblock_new(pool, l);
+        c->memchunk.index = 0;
+        c->memchunk.length = pa_memblock_get_length(c->memchunk.memblock);
+    }
+
+    memcpy(pa_memblock_acquire_chunk(&c->memchunk), c->recv_buf + metadata_length, audio_length);
+    pa_memblock_release(c->memchunk.memblock);
+
+    chunk->memblock = pa_memblock_ref(c->memchunk.memblock);
+    chunk->index = c->memchunk.index;
+    chunk->length = audio_length;
+
+    c->memchunk.index += audio_length;
+    c->memchunk.length -= audio_length;
 
     if (c->memchunk.length <= 0) {
         pa_memblock_unref(c->memchunk.memblock);
@@ -397,6 +415,10 @@ void pa_rtp_context_destroy(pa_rtp_context *c) {
 
     if (c->memchunk.memblock)
         pa_memblock_unref(c->memchunk.memblock);
+
+    pa_xfree(c->recv_buf);
+    c->recv_buf = NULL;
+    c->recv_buf_size = 0;
 }
 
 const char* pa_rtp_format_to_string(pa_sample_format_t f) {
diff --git a/src/modules/rtp/rtp.h b/src/modules/rtp/rtp.h
index bbd4278..0b877d5 100644
--- a/src/modules/rtp/rtp.h
+++ b/src/modules/rtp/rtp.h
@@ -34,6 +34,8 @@ typedef struct pa_rtp_context {
     uint8_t payload;
     size_t frame_size;
 
+    uint8_t *recv_buf;
+    size_t recv_buf_size;
     pa_memchunk memchunk;
 } pa_rtp_context;
 
-- 
2.9.3