On Thu, 26.02.09 16:48, Finn Thain (fthain at telegraphics.com.au) wrote:
> Hi All,
Heya!
> This patch fixes the solaris audio device source and sink, and fixes some
> portability issues that break the build on solaris. Questions and comments
> welcomed.
I have now merged this patch of yours with minor modifications. For
comments see below. Thank you for your contributions.
> This is my first brush with pulseaudio and so I read the wiki docs and
> some of the source code but I'm still unsure of a few things. In
> particular I'm wondering about rewind processing, corking and what (if
> anything) the module needs for those. I'm also unclear on the implications
> of thread_info.buffer_size, .fragment_size and .max_request, and whether
> my code is correct or not.
You can ignore corking, that's a state of streams (i.e. sink
inputs/source outputs) -- it is not relevant for devices
(i.e. sinks/sources). What is relevant to the latter is
suspending/resuming.
Regarding rewind processing: the first thing you should do in each IO
thread loop iteration is check whether a rewind has been
requested. You can do that be checking
s->thread_info.rewind_requested. If that boolean is true you MUST call
pa_sink_process_rewind() before you go on to call pa_sink_render().
p_s_p_r() takes a size that encodes how much you actually managed to
rewind. In case your device doesn't support rewinding at all you can
simply call p_s_p_r(s, 0) -- but you MUST do this.
If you device doesn't support rewinding you can even drop the check
for s->t_i.r_r and directly call p_s_p_r(s,0) since it does the check
internally anyway.
If your device does support rewinding then you simply rewind as far as
you can but and as stored in s->thread_info.rewind_nbytes -- but not
more than that value. You then pass how much you actaully managed to
rewind to p_s_p_r(s, size);
If you allow rewinding then you should call pa_sink_set_max_rewind()
for your sink right after initialization to make sure that all streams
connected to your sink keep enough 'history' so that a complete buffer
refill can be done. Yo should pass how many bytes you could rewind at
max. Usually this equals the overall buffer size. If you don't allow
rewinding then it might still make sense to call this function
although not strictly necessary and passing 0 to make clear that you
understood what is going on and to make explicit that you don't allow
rewinding. That said it isn't strictly necessary to call this since
sinks are initialized by default to not allow rewinding.
You also should call pa_sink_set_max_request(). It tells the protocol
implementations the size they have to be able to fulfill at any time,
i.e. how much they need to buffer before things can get
rolling. Usually this also equals the hardware buffer size.
Regarding fragment sizes: if you have a traditional audio device that
works based on fragments you should should initialize it to the
parameter stored in pa_core->default_fragment_size_msec and
pa_core->default_n_fragments.
> 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.
> rm -f Makefile.am~ configure.ac~
> # Evil, evil, evil, evil hack
> - sed 's/read dummy/\#/' `which gettextize` | sh -s -- --copy --force
> + sed 's/read dummy/\#/' `which gettextize` | sh -s --copy --force
> test -f Makefile.am~ && mv Makefile.am~ Makefile.am
> test -f configure.ac~ && mv configure.ac~ configure.ac
This change breaks on my Linux system here (i.e. bash). I have now
changed the sh to bash since I guess '--' is a bashism.
> +# libtool
> +
> +AC_PROG_LIBTOOL
> +
This is an old macro use for libtool 1.5. We support 2.2 only an hence
use LT_INIT exclusively.
> +PA_MODULE_AUTHOR("Pierre Ossman");
You might want to add your name here, too.
> struct userdata {
> pa_core *core;
> @@ -87,15 +92,24 @@ struct userdata {
>
> pa_memchunk memchunk;
>
> - unsigned int page_size;
> -
> uint32_t frame_size;
> - uint32_t buffer_size;
> - unsigned int written_bytes, read_bytes;
> + int32_t buffer_size;
> + volatile uint64_t written_bytes, read_bytes;
> + pa_mutex *written_bytes_lock;
Hmm, we generally try do do things without locking in PA. This smells
as if it was solvable using atomic ints as well.
Actually, looking at this again I get the impression these mutex are
completely unnecessary here. All functions that lock these mutexes are
called from the IO thread so no locking should be nessary.
Please don't use volatile here. I am pretty sure it is a misuse. Also
see
http://kernel.org/doc/Documentation/volatile-considered-harmful.txt
which applies here too I think.
> +static void sink_set_volume(pa_sink *s) {
> + struct userdata *u;
> + audio_info_t info;
> +
> + pa_assert_se(u = s->userdata);
> +
> + if (u->fd >= 0) {
> + AUDIO_INITINFO(&info);
> +
> + info.play.gain = pa_cvolume_avg(&s->virtual_volume) * AUDIO_MAX_GAIN / PA_VOLUME_NORM;
> + assert(info.play.gain <= AUDIO_MAX_GAIN);
I'd prefer if you'd use pa_cvolume_max here instead of
pa_cvolume_avg() because this makes the volume independant of the balance.
> - info.play.error = 0;
> + info.play.gain = pa_cvolume_avg(&s->virtual_volume) * AUDIO_MAX_GAIN / PA_VOLUME_NORM;
> + assert(info.play.gain <= AUDIO_MAX_GAIN);
Same here. (i.e. for the source)
> + if (u->sink->thread_info.rewind_requested)
> + pa_sink_process_rewind(u->sink, 0);
This is correct.
>
> err = ioctl(u->fd, AUDIO_GETINFO, &info);
> pa_assert(err >= 0);
Hmm, if at all this should be pa_assert_se(), not pa_assert() (so that
it is not defined away by -DNDEBUG). However I'd prefer if the error
would be could correctly. (I see that this code is not yours, but still...)
> + case EINTR:
> + break;
I think you should simply try again in this case...
> + 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?
> +
> + 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? That really
only makes sense if you have to deal with large buffers and support
rewinding.
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.
> + u->frame_size = pa_frame_size(&ss);
>
> - if ((fd = open(p = pa_modargs_get_value(ma, "device", DEFAULT_DEVICE), mode | O_NONBLOCK)) < 0)
> + u->buffer_size = 16384;
It would appear more appropriate to me if the buffer size is adjusted
by the sample spec used.
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. 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!
I hope I answered all your questions,
Lennart
--
Lennart Poettering Red Hat, Inc.
lennart [at] poettering [dot] net ICQ# 11060553
http://0pointer.net/lennart/ GnuPG 0x1A015CC4
