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.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
