[pulseaudio-discuss] [PATCH] revive solaris module

[Finn Thain]

On Fri, 27 Mar 2009, Lennart Poettering wrote:

> On Sat, 07.03.09 16:48, Finn Thain (fthain at telegraphics.com.au) wrote:
> 
> > 
> > 
> > 
> > On Wed, 4 Mar 2009, Lennart Poettering wrote:
> > 
> > [snip]
> > > > This patch disables link map/library versioning unless ld is GNU 
> > > > ld. Another approach for solaris would be to use that linker's -M 
> > > > option, but I couldn't make that work (due to undefined mainloop, 
> > > > browse and simple symbols when linking pacat. I can post the 
> > > > errors if anyone is intested.)
> > > 
> > > The linking in PA is a bit weird since we have a cyclic dependency 
> > > between libpulse and libpulsecommon which however is not explicit.
> > 
> > Could that affect the pacat link somehow?
> 
> No. It shouldn't. pacat only accesses symbols from libpulse, not from 
> libpulsecommon.
>
> > What are the implications for client apps that link with the 
> > non-versioned libraries I've been building on solaris?
> 
> Not sure. It will certainly built though. As long as no app accesses 
> symbols it shouldn't be accessing things should be fine when building a 
> version that doesn't hide anything.

OK.
 
> > > > +                        case EAGAIN:
> > > > +                            u->buffer_size = u->buffer_size * 18 / 25;
> > > > +                            u->buffer_size -= u->buffer_size % u->frame_size;
> > > > +                            u->buffer_size = PA_MAX(u->buffer_size, (int32_t)MIN_BUFFER_SIZE);
> > > > +                            pa_sink_set_max_request(u->sink, u->buffer_size);
> > > > +                            pa_log("EAGAIN. Buffer size is now %u bytes (%llu buffered)", u->buffer_size, buffered_bytes);
> > > > +                            break;
> > > 
> > > Hmm, care to explain this?
> > 
> > EAGAIN happens when the user requests a buffer size that is too large for 
> > the STREAMS layer to accept. We end up looping with EAGAIN every time we 
> > try to write out the rest of the buffer, which burns enough CPU time to 
> > trip the CPU limit.
> > 
> > So, I reduce the buffer size with each EAGAIN. This gets us reasonably 
> > close to the largest usable buffer size. (Perhaps there's a better way to 
> > determine what that limit is, but I don't know how.)
> 
> And you are sure that EAGAIN may only be thrown in this case? This
> interpretation of EAGAIn is completely different from how things are
> understood on Unix otherwise.
> 
> Normally on Unix write() would do a partial write if the destination
> buffer is shorter than the source buffer. It would then return how
> much it wrote so that the caller can handle that. Only if nothing at
> all can be written (and O_NDELAY is enabled) EAGAIN would be
> returned. 
> 
> Are you really sure this interpretation of EAGAIN of yours is correct?

The only interpretation here was my suggestion that STREAMS was the cause. 
That is guesswork on my part.

If you are asking "am I sure that my observation of this behaviour is 
correct?" then yes, I am.

Perhaps a better solution is to use blocking writes. Would that be OK with 
you?

> > > > +            pa_rtpoll_set_timer_absolute(u->rtpoll, xtime0 + pa_bytes_to_usec(buffered_bytes / 2, &u->sink->sample_spec));
> > > > +        } else {
> > > > +            pa_rtpoll_set_timer_disabled(u->rtpoll);
> > > >          }
> > > 
> > > Hmm, you schedule audio via timers? Is that a good idea?
> > 
> > Perhaps not. I won't know until I test on more hardware.
> > 
> > But, given that we have rt priority and high resolution timers on solaris, 
> > I think it is OK in theory...
> > 
> > The reason I used a timer was to minimise CPU usage and avoid the CPU 
> > limit. Recall that getting woken up by poll is not an option for playback 
> > unfortunately. 
> 
> Why?

Because we artificially limit the amount of queued writes (so called 
"buffer size"). That was the algorithm in 0.9.14 too -- before I started 
hacking on it. I think the intention is to limit latency.

> > We can arrange for a signal when the FD becomes writable, but that 
> > throws out the whole buffer size concept, which acts to reduce 
> > latency.
> 
> > > That really only makes sense if you have to deal with large buffers 
> > > and support rewinding.
> > 
> > I've implemented rewind support, but I'm still not sure that I have 
> > understood the concept; I take it that we "rewind" (from the 
> > point-of-view of the renderer, not the sink) so that some rendered but 
> > as yet unplayed portion of the memblock/buffers can then be rendered 
> > again?
> 
> Yes, that is correct. The idea is to have very large buffers and hence 
> very few wakeups and hence save power and reduce drop outs -- but still 
> have the ability to react quickly on user input, i.e. seeking, pausing, ...
> 
> > > Please keep in mind that the system clock and the sound card clock 
> > > deviate. If you use the system timers to do PCM scheduling ou might 
> > > need a pa_smoother object that is able to estimate the deviation for 
> > > you.
> > 
> > Actually, in an earlier version I did use a smoother (after reading 
> > about that in the wiki). But because of the non-monotonic sample 
> > counter (bug?) I decided that it probably wasn't worth the added 
> > complexity so I removed it. I'll put the smoother back if I can figure 
> > out the problem with the sample counter.
> 
> Non-monotonic sample counter? That sounds wrong. How did that happen?

It appears to be a bug in the driver. This problem is mentioned in the 
comments in the original module-solaris.c even before I touched it.

So, all I'm saying here is that the problem is still there in solaris nv 
103, that I'll try and figure out what the cause is, and that probably a 
smoother is not a good idea (yet).

> 
> > > One last thing: it would probably be a good idea to allocate a 
> > > pa_card object and attach the sink and the source to it.
> > 
> > It is possible to open /dev/audio twice by loading the solaris module 
> > twice -- once for the sink (passing record=0) and once for source 
> > (passing playback=0), thus giving seperate threads/LWPs for source and 
> > sink. It might be misleading to allocate two cards in that situation?
> 
> Why would you want to do this? We should make the common case work.

Are you saying that the common case cannot work without a pa_card 
abstraction?

> If people like to configure weird things, then they are welcome to, but 
> we don't need to add special support for that. And hence I think you 
> should ignore that case.

I added no special support for it. I suppose I could have explicitly 
disallowed it when I added the missing PA_MODULE_LOAD_ONCE macro in my 
first patch, but why do that?

Perhaps a backend that uses the future Boomer/OSS API could model the 
hardware devices better. I expect that the OSS module will replace this 
one eventually.

> > > Right now pa_cards are mostly useful for switching profiles but even 
> > > if you do not allow switching profiles on-the-fly it is of some 
> > > value to find out via the cards object which source belongs to which 
> > > sink.
> > > 
> > > Otherwise I am happy!
> > > 
> > > Thanks for your patch! I'd be thankful if you could fix the issues 
> > > pointed out and prepare another patch on top of current git!
> > 
> > No problem. Patch follows. It also includes a portability fix for 
> > pa_realpath and a fix for a bug in the pa_signal_new() error path that 
> > causes signal data be freed if you attempt to register the same signal 
> > twice.
> 
> Thanks. I'll have a closer look tonight and merge it then.
> 
> >  char *pa_realpath(const char *path) {
> > -    char *r, *t;
> > +    char *r, *t, *path_buf;
> >      pa_assert(path);
> >  
> >      /* We want only abolsute paths */
> > @@ -2617,17 +2617,16 @@ char *pa_realpath(const char *path) {
> >          return NULL;
> >      }
> >  
> > -#ifndef __GLIBC__
> > -#error "It's not clear whether this system supports realpath(..., NULL) like GNU libc does. If it doesn't we need a private version of realpath() here."
> > -#endif
> > -
> > -    if (!(r = realpath(path, NULL)))
> > +    path_buf = pa_xmalloc(PATH_MAX);
> > +    if (!(r = realpath(path, path_buf))) {
> > +        pa_xfree(path_buf);
> >          return NULL;
> > +    }
> 
> 
> Uh, this is not really an option. Please see the discussion at:
> 
> http://linux.die.net/man/3/realpath
>
> (Ignore the weird formatting, just read the part starting with "Bugs")

Are you referring to the claim in the Darwin and Linux man pages that 
Solaris may return a relative path? FWIW, that claim contradicts the 
present Solaris man page.

> 
> I'd rather stick with realpath(NULL) (especially since both Linux and 
> MacOS handle this properly, and POSIX is planning to make this official) 
> and only fallback to PATH_MAX on the few OSes that need it.

OK. I'll search out or write an m4 macro for the realpath(NULL) feature 
test and send another patch. Please disregard this one.

Finn

> 
> Sorry for the long delay responding to this email!
> 
> Lennart
> 
>