On 02/15/2018 11:51 PM, Georg Chini wrote:
> The current null-source implementation has several bugs:
>
> 1) The latency reported is the negative of the correct latency.
> 2) The memchunk passed to pa_source_post() is not initialized
> with silence.
> 3) In PA_SOURCE_MESSAGE_SET_STATE the timestamp is always set
> when the source transitions to RUNNING state. This should only
> happen when the source transitions from SUSPENDED to RUNNING
> but also if it changes from SUSPENDED to IDLE.
> 4) The timing of the thread function is incorrect. It always
> uses u->latency_time, regardless of the specified source
> latency.
> 5) The latency_time argument seems pointless because the source
> is defined with dynamic latency.
>
> This patch fixes the issues by
> 1) inverting the sign of the reported latency,
> 2) initializing the memchunk with silence,
> 3) changing the logic in PA_SOURCE_MESSAGE_SET_STATE so that
> the timestamp is set when needed,
> 4) using u->block_usec instead of u->latency_time for setting
> the rtpoll timer and checking if the timer has elapsed,
> 5) removing the latency_time option.
> ---
> src/modules/module-null-source.c | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/src/modules/module-null-source.c b/src/modules/module-null-source.c
> index 41f17bd9..7bbe7f47 100644
> --- a/src/modules/module-null-source.c
> +++ b/src/modules/module-null-source.c
> @@ -51,12 +51,11 @@ PA_MODULE_USAGE(
> "rate=<sample rate> "
> "source_name=<name of source> "
> "channel_map=<channel map> "
> - "description=<description for the source> "
> - "latency_time=<latency time in ms>");
> + "description=<description for the source> ");
>
> #define DEFAULT_SOURCE_NAME "source.null"
> -#define DEFAULT_LATENCY_TIME 20
> #define MAX_LATENCY_USEC (PA_USEC_PER_SEC * 2)
> +#define MIN_LATENCY_USEC (500)
>
> struct userdata {
> pa_core *core;
> @@ -71,7 +70,6 @@ struct userdata {
>
> pa_usec_t block_usec;
> pa_usec_t timestamp;
> - pa_usec_t latency_time;
> };
>
> static const char* const valid_modargs[] = {
> @@ -81,7 +79,6 @@ static const char* const valid_modargs[] = {
> "source_name",
> "channel_map",
> "description",
> - "latency_time",
> NULL
> };
>
> @@ -91,8 +88,10 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off
> switch (code) {
> case PA_SOURCE_MESSAGE_SET_STATE:
>
> - if (PA_PTR_TO_UINT(data) == PA_SOURCE_RUNNING)
> - u->timestamp = pa_rtclock_now();
> + if (pa_source_get_state(u->source) == PA_SOURCE_SUSPENDED || pa_source_get_state(u->source) == PA_SOURCE_INIT) {
> + if (PA_PTR_TO_UINT(data) == PA_SOURCE_RUNNING || PA_PTR_TO_UINT(data) == PA_SOURCE_IDLE)
> + u->timestamp = pa_rtclock_now();
> + }
pa_source_get_state() is the macro:
#define pa_source_get_state(s) ((pa_source_state_t) (s)->state)
I think it's unsafe to access u->source->state in source_process_msg() since it called from i/o thread context.
Also there is the macro PA_SOURCE_IS_OPENED(state) which check the source is running or idle.
I think it should look like this:
- if (PA_PTR_TO_UINT(data) == PA_SOURCE_RUNNING)
- u->timestamp = pa_rtclock_now();
+ if (u->source->thread_info.state == PA_SOURCE_SUSPENDED || u->source->thread_info.state == PA_SOURCE_INIT) {
+ if (PA_SOURCE_IS_OPENED(PA_PTR_TO_UINT(data)))
+ u->timestamp = pa_rtclock_now();
+ }
>
> break;
>
> @@ -100,7 +99,7 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off
> pa_usec_t now;
>
> now = pa_rtclock_now();
> - *((int64_t*) data) = (int64_t)u->timestamp - (int64_t)now;
> + *((int64_t*) data) = (int64_t)now - (int64_t)u->timestamp;
>
> return 0;
> }
> @@ -117,10 +116,13 @@ static void source_update_requested_latency_cb(pa_source *s) {
> pa_assert(u);
>
> u->block_usec = pa_source_get_requested_latency_within_thread(s);
> + if (u->block_usec == (pa_usec_t)-1)
> + u->block_usec = u->source->thread_info.max_latency;
> }
>
> static void thread_func(void *userdata) {
> struct userdata *u = userdata;
> + bool timer_elapsed = false;
>
> pa_assert(u);
>
> @@ -140,17 +142,19 @@ static void thread_func(void *userdata) {
>
> now = pa_rtclock_now();
>
> - if ((chunk.length = pa_usec_to_bytes(now - u->timestamp, &u->source->sample_spec)) > 0) {
> + if (timer_elapsed && (chunk.length = pa_usec_to_bytes(now - u->timestamp, &u->source->sample_spec)) > 0) {
>
> - chunk.memblock = pa_memblock_new(u->core->mempool, (size_t) -1); /* or chunk.length? */
> + chunk.length = PA_MIN(pa_mempool_block_size_max(u->core->mempool), chunk.length);
> + chunk.memblock = pa_memblock_new(u->core->mempool, chunk.length);
> chunk.index = 0;
> + pa_silence_memchunk(&chunk, &u->source->sample_spec);
> pa_source_post(u->source, &chunk);
> pa_memblock_unref(chunk.memblock);
>
> - u->timestamp = now;
> + u->timestamp += pa_bytes_to_usec(chunk.length, &u->source->sample_spec);
> }
>
> - pa_rtpoll_set_timer_absolute(u->rtpoll, u->timestamp + u->latency_time * PA_USEC_PER_MSEC);
> + pa_rtpoll_set_timer_absolute(u->rtpoll, u->timestamp + u->block_usec);
> } else
> pa_rtpoll_set_timer_disabled(u->rtpoll);
>
> @@ -158,6 +162,8 @@ static void thread_func(void *userdata) {
> if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
> goto fail;
>
> + timer_elapsed = pa_rtpoll_timer_elapsed(u->rtpoll);
> +
> if (ret == 0)
> goto finish;
> }
> @@ -178,7 +184,6 @@ int pa__init(pa_module*m) {
> pa_channel_map map;
> pa_modargs *ma = NULL;
> pa_source_new_data data;
> - uint32_t latency_time = DEFAULT_LATENCY_TIME;
>
> pa_assert(m);
>
> @@ -221,13 +226,6 @@ int pa__init(pa_module*m) {
> goto fail;
> }
>
> - u->latency_time = DEFAULT_LATENCY_TIME;
> - if (pa_modargs_get_value_u32(ma, "latency_time", &latency_time) < 0) {
> - pa_log("Failed to parse latency_time value.");
> - goto fail;
> - }
> - u->latency_time = latency_time;
> -
> u->source->parent.process_msg = source_process_msg;
> u->source->update_requested_latency = source_update_requested_latency_cb;
> u->source->userdata = u;
> @@ -235,7 +233,7 @@ 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_latency_range(u->source, 0, MAX_LATENCY_USEC);
> + pa_source_set_latency_range(u->source, MIN_LATENCY_USEC, MAX_LATENCY_USEC);
> u->block_usec = u->source->thread_info.max_latency;
>
> u->source->thread_info.max_rewind =
>
--
Raman
