On 22.02.2018 17:15, Raman Shyshniou wrote:
> Currently the pipe-source will remain running even if no
> writer is connected and therefore no data is produced.
> This patch prevets this by auto-suspending source
> when all writers are disconnected.
> ---
> src/modules/module-pipe-source.c | 190 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 156 insertions(+), 34 deletions(-)
>
> diff --git a/src/modules/module-pipe-source.c b/src/modules/module-pipe-source.c
> index f8284c1..6b13e28 100644
> --- a/src/modules/module-pipe-source.c
> +++ b/src/modules/module-pipe-source.c
> @@ -33,6 +33,7 @@
> #include <sys/filio.h>
> #endif
>
> +#include <pulse/rtclock.h>
> #include <pulse/xmalloc.h>
>
> #include <pulsecore/core-error.h>
> @@ -62,6 +63,18 @@ PA_MODULE_USAGE(
> #define DEFAULT_FILE_NAME "/tmp/music.input"
> #define DEFAULT_SOURCE_NAME "fifo_input"
>
> +struct pipe_source_msg {
> + pa_msgobject parent;
> +};
> +
> +typedef struct pipe_source_msg pipe_source_msg;
> +PA_DEFINE_PRIVATE_CLASS(pipe_source_msg, pa_msgobject);
> +
> +enum {
> + PIPE_SOURCE_SUSPEND,
> + PIPE_SOURCE_RESUME
> +};
> +
> struct userdata {
> pa_core *core;
> pa_module *module;
> @@ -71,7 +84,15 @@ struct userdata {
> pa_thread_mq thread_mq;
> pa_rtpoll *rtpoll;
>
> + pipe_source_msg *msg;
> + pa_usec_t timestamp;
> + size_t pipe_size;
> +
> + bool writer_connected;
> + bool suspended_by_user;
> +
> char *filename;
> + int corkfd;
> int fd;
>
> pa_memchunk memchunk;
> @@ -90,6 +111,37 @@ static const char* const valid_modargs[] = {
> NULL
> };
>
> +/* Called from main context */
> +static int pipe_source_process_msg(
> + pa_msgobject *o,
> + int code,
> + void *data,
> + int64_t offset,
> + pa_memchunk *chunk) {
> +
> + struct userdata *u = (struct userdata *) data;
> +
> + pa_assert(u);
> +
> + switch (code) {
> + case PIPE_SOURCE_SUSPEND:
> + pa_log_debug("Suspending source %s because no writers left", u->source->name);
> + pa_source_suspend(u->source, true, PA_SUSPEND_UNAVAILABLE);
> + break;
> +
> + case PIPE_SOURCE_RESUME:
> + pa_log_debug("Resuming source %s", u->source->name);
> + pa_source_suspend(u->source, false, PA_SUSPEND_UNAVAILABLE);
> + break;
> +
> + default:
> + pa_assert_not_reached();
> + }
> +
> + return 0;
> +}
> +
> +/* Called from thread context */
> static int source_process_msg(
> pa_msgobject *o,
> int code,
> @@ -101,17 +153,28 @@ static int source_process_msg(
>
> switch (code) {
>
> - case PA_SOURCE_MESSAGE_GET_LATENCY: {
> - size_t n = 0;
> + case PA_SOURCE_MESSAGE_SET_STATE: {
> + pa_source_state_t new_state = ((pa_source_message_set_state *) data)->state;
> + pa_suspend_cause_t suspend_cause = ((pa_source_message_set_state *) data)->suspend_cause;
>
> -#ifdef FIONREAD
> - int l;
> + if (u->source->thread_info.state == PA_SOURCE_SUSPENDED || u->source->thread_info.state == PA_SOURCE_INIT)
> + if (PA_SOURCE_IS_OPENED(new_state)) {
> + u->timestamp = pa_rtclock_now();
> + u->suspended_by_user = false;
> + }
>
> - if (ioctl(u->fd, FIONREAD, &l) >= 0 && l > 0)
> - n = (size_t) l;
> -#endif
> + if (new_state == PA_SOURCE_SUSPENDED && suspend_cause & ~PA_SUSPEND_UNAVAILABLE)
> + u->suspended_by_user = true;
> +
> + break;
> + }
> +
> + case PA_SOURCE_MESSAGE_GET_LATENCY: {
> + pa_usec_t now;
> +
> + now = pa_rtclock_now();
> + *((int64_t*) data) = (int64_t)now - (int64_t)u->timestamp;
>
> - *((int64_t*) data) = pa_bytes_to_usec(n, &u->source->sample_spec);
> return 0;
> }
> }
> @@ -121,6 +184,7 @@ static int source_process_msg(
>
> static void thread_func(void *userdata) {
> struct userdata *u = userdata;
> + struct pollfd *pollfd;
> int read_type = 0;
>
> pa_assert(u);
> @@ -129,30 +193,30 @@ static void thread_func(void *userdata) {
>
> pa_thread_mq_install(&u->thread_mq);
>
> + u->memchunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
> +
> + u->timestamp = pa_rtclock_now();
> +
> + /* Close our writer here to suspend source if no writers left */
> + pa_assert_se(pa_close(u->corkfd) == 0);
> + u->corkfd = -1;
> +
> + pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
> +
> for (;;) {
> int ret;
> - struct pollfd *pollfd;
> -
> - pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>
> - /* Try to read some data and pass it on to the source driver */
> - if (u->source->thread_info.state == PA_SOURCE_RUNNING && pollfd->revents) {
> + /* Try to read some data if there are any events */
> + if (pollfd->revents) {
> ssize_t l;
> void *p;
>
> - if (!u->memchunk.memblock) {
> - u->memchunk.memblock = pa_memblock_new(u->core->mempool, pa_pipe_buf(u->fd));
> - u->memchunk.index = u->memchunk.length = 0;
> - }
> -
> - pa_assert(pa_memblock_get_length(u->memchunk.memblock) > u->memchunk.index);
> + pa_assert(u->memchunk.length == 0);
>
> p = pa_memblock_acquire(u->memchunk.memblock);
> - l = pa_read(u->fd, (uint8_t*) p + u->memchunk.index, pa_memblock_get_length(u->memchunk.memblock) - u->memchunk.index, &read_type);
> + l = pa_read(u->fd, p, pa_memblock_get_length(u->memchunk.memblock), &read_type);
> pa_memblock_release(u->memchunk.memblock);
>
> - pa_assert(l != 0); /* EOF cannot happen, since we opened the fifo for both reading and writing */
> -
> if (l < 0) {
>
> if (errno == EINTR)
> @@ -162,23 +226,52 @@ static void thread_func(void *userdata) {
> goto fail;
> }
>
> + } else if (l == 0) {
> +
> + if (u->writer_connected) {
> + pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(u->msg), PIPE_SOURCE_SUSPEND, u, 0, NULL, NULL);
> +
> + if ((u->corkfd = pa_open_cloexec(u->filename, O_WRONLY, 0)) < 0) {
> + pa_log("open('%s'): %s", u->filename, pa_cstrerror(errno));
> + goto fail;
> + }
> +
> + u->writer_connected = false;
> + }
> +
> } else {
>
> u->memchunk.length = (size_t) l;
> - pa_source_post(u->source, &u->memchunk);
> - u->memchunk.index += (size_t) l;
> + u->timestamp = pa_rtclock_now();
>
> - if (u->memchunk.index >= pa_memblock_get_length(u->memchunk.memblock)) {
> - pa_memblock_unref(u->memchunk.memblock);
> - pa_memchunk_reset(&u->memchunk);
> + if (!u->writer_connected) {
> + pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(u->msg), PIPE_SOURCE_RESUME, u, 0, NULL, NULL);
> + u->writer_connected = true;
> }
>
> - pollfd->revents = 0;
> }
> }
>
> - /* Hmm, nothing to do. Let's sleep */
> - pollfd->events = (short) (u->source->thread_info.state == PA_SOURCE_RUNNING ? POLLIN : 0);
> + /* Post data to source, discard data or wait for state transition to be complete */
> + if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
> +
> + if (u->writer_connected && u->corkfd >= 0) {
> + pa_assert_se(pa_close(u->corkfd) == 0);
> + u->corkfd = -1;
> + }
> +
> + if (u->memchunk.length > 0) {
> + 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;
> + }
> +
> + } else if (u->suspended_by_user)
You have to open the corkfd here as well to avoid POLLHUP spam if the writer
disconnects during user suspend.
> + u->memchunk.length = 0;
> +
> +
> + pollfd->events = u->memchunk.length ? 0 : POLLIN;
How do you wake up from autosuspend? When the writer disconnects,
memchunk.length will be 0 and so you do no longer listen for POLLIN.
I guess you need to know the current suspend cause here:
user suspended -> events = 0
(only auto suspended + no writer connected) or running -> events = POLLIN
only auto suspended + writer connected (=wake up transition) -> events = 0
>
> if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
> goto fail;
> @@ -188,8 +281,13 @@ static void thread_func(void *userdata) {
>
> pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>
> - if (pollfd->revents & ~POLLIN) {
> - pa_log("FIFO shutdown.");
> + if (pollfd->revents & ~(POLLIN|POLLHUP)) {
> + pa_log("FD error: %s%s%s%s",
> + pollfd->revents & POLLOUT ? "POLLOUT " : "",
> + pollfd->revents & POLLERR ? "POLLERR " : "",
> + pollfd->revents & POLLPRI ? "POLLPRI " : "",
> + pollfd->revents & POLLNVAL ? "POLLNVAL " : "");
> +
> goto fail;
> }
> }
> @@ -238,13 +336,26 @@ int pa__init(pa_module *m) {
> goto fail;
> }
>
> + if (!(u->msg = pa_msgobject_new(pipe_source_msg)))
> + goto fail;
> +
> + u->msg->parent.process_msg = pipe_source_process_msg;
> +
> u->filename = pa_runtime_path(pa_modargs_get_value(ma, "file", DEFAULT_FILE_NAME));
>
> if (mkfifo(u->filename, 0666) < 0) {
> pa_log("mkfifo('%s'): %s", u->filename, pa_cstrerror(errno));
> goto fail;
> }
> - if ((u->fd = pa_open_cloexec(u->filename, O_RDWR, 0)) < 0) {
> +
> + if ((u->corkfd = pa_open_cloexec(u->filename, O_RDWR, 0)) < 0) {
> + pa_log("open('%s'): %s", u->filename, pa_cstrerror(errno));
> + goto fail;
> + }
> +
> + u->writer_connected = true;
> +
> + if ((u->fd = pa_open_cloexec(u->filename, O_RDONLY, 0)) < 0) {
> pa_log("open('%s'): %s", u->filename, pa_cstrerror(errno));
> goto fail;
> }
> @@ -289,13 +400,18 @@ int pa__init(pa_module *m) {
>
> pa_source_set_asyncmsgq(u->source, u->thread_mq.inq);
> pa_source_set_rtpoll(u->source, u->rtpoll);
> - pa_source_set_fixed_latency(u->source, pa_bytes_to_usec(pa_pipe_buf(u->fd), &u->source->sample_spec));
> +
> + u->pipe_size = pa_pipe_buf(u->fd);
> + u->pipe_size = PA_MIN(pa_mempool_block_size_max(m->core->mempool), u->pipe_size);
> + pa_source_set_fixed_latency(u->source, pa_bytes_to_usec(u->pipe_size, &u->source->sample_spec));
>
> u->rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
> pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
> pollfd->fd = u->fd;
> pollfd->events = pollfd->revents = 0;
>
> + u->suspended_by_user = false;
> +
> if (!(u->thread = pa_thread_new("pipe-source", thread_func, u))) {
> pa_log("Failed to create thread.");
> goto fail;
> @@ -360,6 +476,12 @@ void pa__done(pa_module *m) {
> pa_xfree(u->filename);
> }
>
> + if (u->msg)
> + pa_xfree(u->msg);
> +
> + if (u->corkfd >= 0)
> + pa_assert_se(pa_close(u->corkfd) == 0);
> +
> if (u->fd >= 0)
> pa_assert_se(pa_close(u->fd) == 0);
>
I believe you need some user_unsuspended and/or auto_unsuspended
flag that is set in the SET_STATE handler to handle the first data read
after suspend correctly.
When you unsuspend from auto suspend, the first chunk you already read
from the pipe should be posted - as far as I can see this happens.
However, when you unsuspend from user suspend, the data in the pipe may
be old - you did not read anything during suspend. Therefore the first chunk
you read should be discarded. That does not happen as far as I can tell. Or
am I overlooking something (again)?
The general approach of the patch looks OK now.
