[pulseaudio-discuss] [PATCH] revive solaris module

[Lennart Poettering]

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.

> > > +                        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?

> > > +            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?

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

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

> > 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")

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.

Sorry for the long delay responding to this email!

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4