[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.