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.
