[PATCH] null-source: fix multiple bugs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux