Re: [pulseaudio-discuss] [PATCH] null-source: fix multiple bugs

[Georg Chini]

On 16.02.2018 11:46, Raman Shishniou wrote:
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();
+            }


It is safe to access the main thread variables because the main thread 
is waiting for us.
The same code is also used in module-null-sink. That's why I just copied 
it over.


_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss