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

[Georg Chini]

On 17.02.2018 07:32, Tanu Kaskinen wrote:
> On Fri, 2018-02-16 at 13:04 +0100, Georg Chini wrote:
>> 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.
>>>>
>>>>            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.
> It's true that reading u->source->state is safe, but I think it's
> better to use thread_info.state anyway (for consistency if nothing
> else). Incidentally I have an unsubmitted patch that changes module-
> null-sink and module-pipe-sink to use thread_info.state. They are the
> only modules that use the main thread state variable in this context.
>
OK, one more question before I resend the patch: Is it OK to drop the 
latency_time
argument completely or should I accept it and warn that it is unused?

I guess nobody has been using the argument, it is not documented and if 
you use it,
the result is not what you would expect.