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

[Tanu Kaskinen]

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

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.

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk