On Sat, 2018-02-17 at 12:30 +0100, Georg Chini wrote:
> 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.
Dropping the argument completely seems reasonable in this case.
--
Tanu
https://liberapay.com/tanuk
https://www.patreon.com/tanuk
