[v10,2/3] pipe-source: respect frame boundaries

Submitted by Raman Shishniou on March 20, 2018, 12:27 p.m.

Details

Message ID 1521548857-11194-2-git-send-email-rommer@ibuffed.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Raman Shishniou March 20, 2018, 12:27 p.m.
Currently pipe-source doesn't track frame boundaries while posting
data. This can lead to assertion failure in source-output for any
sample specs except s8/u8 mono. The simplest way to reproduce is
to write s24le format to pipe:
$ pacat -r -d null --format=s24le > /tmp/music.input

E: [pipe-source] source-output.c: Assertion
'pa_frame_aligned(chunk->length, &o->source->sample_spec)' failed at
pulsecore/source-output.c:738, function pa_source_output_push().
---
 src/modules/module-pipe-source.c | 52 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/module-pipe-source.c b/src/modules/module-pipe-source.c
index c069dd4..4fd515b 100644
--- a/src/modules/module-pipe-source.c
+++ b/src/modules/module-pipe-source.c
@@ -202,6 +202,7 @@  static void thread_func(void *userdata) {
     struct userdata *u = userdata;
     struct pollfd *pollfd;
     int read_type = 0;
+    size_t fs;
 
     pa_assert(u);
 
@@ -209,6 +210,8 @@  static void thread_func(void *userdata) {
 
     pa_thread_mq_install(&u->thread_mq);
 
+    fs = pa_frame_size(&u->source->sample_spec);
+
     u->memchunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
     u->timestamp = pa_rtclock_now();
 
@@ -229,7 +232,7 @@  static void thread_func(void *userdata) {
             pa_assert(u->memchunk.length == 0);
 
             p = pa_memblock_acquire(u->memchunk.memblock);
-            l = pa_read(u->fd, p, pa_memblock_get_length(u->memchunk.memblock), &read_type);
+            l = pa_read(u->fd, (uint8_t*) p + u->memchunk.index, pa_memblock_get_length(u->memchunk.memblock) - u->memchunk.index, &read_type);
             pa_memblock_release(u->memchunk.memblock);
 
             if (l < 0) {
@@ -252,6 +255,7 @@  static void thread_func(void *userdata) {
                     }
 
                     u->writer_connected = false;
+                    u->memchunk.index = 0;
                 }
 
             } else {
@@ -269,21 +273,59 @@  static void thread_func(void *userdata) {
 
         /* Post data to source */
         if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
+            size_t total_len = u->memchunk.index + u->memchunk.length;
+            size_t frames = total_len / fs;
+            size_t tail = total_len % fs;
 
             if (u->writer_connected && u->corkfd >= 0) {
                 pa_assert_se(pa_close(u->corkfd) == 0);
                 u->corkfd = -1;
             }
 
-            if (u->memchunk.length > 0) {
+            if (frames > 0) {
+                pa_memblock *new_memblock;
+
+                new_memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
+
+                u->memchunk.index = 0;
+                u->memchunk.length = frames * fs;
+
+                if (tail > 0) {
+                    void *src, *dst;
+
+                    dst = pa_memblock_acquire(new_memblock);
+                    src = pa_memblock_acquire(u->memchunk.memblock);
+
+                    memcpy(dst, (uint8_t *) src + u->memchunk.length, tail);
+
+                    pa_memblock_release(u->memchunk.memblock);
+                    pa_memblock_release(new_memblock);
+                }
+
                 pa_source_post(u->source, &u->memchunk);
                 pa_memblock_unref(u->memchunk.memblock);
-                u->memchunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
-                u->memchunk.length = 0;
+                u->memchunk.memblock = new_memblock;
+            }
+
+            u->memchunk.index = tail;
+            u->memchunk.length = 0;
+
+        } else if (u->suspended_by_user) {
+            size_t total_len = u->memchunk.index + u->memchunk.length;
+            size_t frames = total_len / fs;
+            size_t tail = total_len % fs;
+
+            if (frames > 0 && tail > 0) {
+                void *p;
+
+                p = pa_memblock_acquire(u->memchunk.memblock);
+                memmove(p, (uint8_t *) p + frames * fs, tail);
+                pa_memblock_release(u->memchunk.memblock);
             }
 
-        } else if (u->suspended_by_user)
+            u->memchunk.index = tail;
             u->memchunk.length = 0;
+        }
 
         pollfd->events = u->memchunk.length ? 0 : POLLIN;
 

Comments

On 20.03.2018 13:27, Raman Shyshniou wrote:
> Currently pipe-source doesn't track frame boundaries while posting
> data. This can lead to assertion failure in source-output for any
> sample specs except s8/u8 mono. The simplest way to reproduce is
> to write s24le format to pipe:
> $ pacat -r -d null --format=s24le > /tmp/music.input
>
> E: [pipe-source] source-output.c: Assertion
> 'pa_frame_aligned(chunk->length, &o->source->sample_spec)' failed at
> pulsecore/source-output.c:738, function pa_source_output_push().
> ---
>   src/modules/module-pipe-source.c | 52 ++++++++++++++++++++++++++++++++++++----
>   1 file changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/src/modules/module-pipe-source.c b/src/modules/module-pipe-source.c
> index c069dd4..4fd515b 100644
> --- a/src/modules/module-pipe-source.c
> +++ b/src/modules/module-pipe-source.c
> @@ -202,6 +202,7 @@ static void thread_func(void *userdata) {
>       struct userdata *u = userdata;
>       struct pollfd *pollfd;
>       int read_type = 0;
> +    size_t fs;
>   
>       pa_assert(u);
>   
> @@ -209,6 +210,8 @@ static void thread_func(void *userdata) {
>   
>       pa_thread_mq_install(&u->thread_mq);
>   
> +    fs = pa_frame_size(&u->source->sample_spec);
> +
>       u->memchunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
>       u->timestamp = pa_rtclock_now();
>   
> @@ -229,7 +232,7 @@ static void thread_func(void *userdata) {
>               pa_assert(u->memchunk.length == 0);
>   
>               p = pa_memblock_acquire(u->memchunk.memblock);
> -            l = pa_read(u->fd, p, pa_memblock_get_length(u->memchunk.memblock), &read_type);
> +            l = pa_read(u->fd, (uint8_t*) p + u->memchunk.index, pa_memblock_get_length(u->memchunk.memblock) - u->memchunk.index, &read_type);
>               pa_memblock_release(u->memchunk.memblock);
>   
>               if (l < 0) {
> @@ -252,6 +255,7 @@ static void thread_func(void *userdata) {
>                       }
>   
>                       u->writer_connected = false;
> +                    u->memchunk.index = 0;
>                   }
>   
>               } else {
> @@ -269,21 +273,59 @@ static void thread_func(void *userdata) {
>   
>           /* Post data to source */
>           if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
> +            size_t total_len = u->memchunk.index + u->memchunk.length;
> +            size_t frames = total_len / fs;
> +            size_t tail = total_len % fs;
>   
>               if (u->writer_connected && u->corkfd >= 0) {
>                   pa_assert_se(pa_close(u->corkfd) == 0);
>                   u->corkfd = -1;
>               }
>   
> -            if (u->memchunk.length > 0) {
> +            if (frames > 0) {
> +                pa_memblock *new_memblock;
> +
> +                new_memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
> +
> +                u->memchunk.index = 0;
> +                u->memchunk.length = frames * fs;
> +
> +                if (tail > 0) {
> +                    void *src, *dst;
> +
> +                    dst = pa_memblock_acquire(new_memblock);
> +                    src = pa_memblock_acquire(u->memchunk.memblock);
> +
> +                    memcpy(dst, (uint8_t *) src + u->memchunk.length, tail);
> +
> +                    pa_memblock_release(u->memchunk.memblock);
> +                    pa_memblock_release(new_memblock);
> +                }
> +
>                   pa_source_post(u->source, &u->memchunk);
>                   pa_memblock_unref(u->memchunk.memblock);
> -                u->memchunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
> -                u->memchunk.length = 0;
> +                u->memchunk.memblock = new_memblock;
> +            }
> +
> +            u->memchunk.index = tail;
> +            u->memchunk.length = 0;
> +
> +        } else if (u->suspended_by_user) {
> +            size_t total_len = u->memchunk.index + u->memchunk.length;
> +            size_t frames = total_len / fs;
> +            size_t tail = total_len % fs;

You can move the three lines above outside the if-block because you
are doing the same thing in both branches.

> +
> +            if (frames > 0 && tail > 0) {
> +                void *p;
> +
> +                p = pa_memblock_acquire(u->memchunk.memblock);
> +                memmove(p, (uint8_t *) p + frames * fs, tail);
> +                pa_memblock_release(u->memchunk.memblock);
>               }
>   
> -        } else if (u->suspended_by_user)
> +            u->memchunk.index = tail;
>               u->memchunk.length = 0;
> +        }
>   
>           pollfd->events = u->memchunk.length ? 0 : POLLIN;
>