PulseAudio support for volume input

[Watcom]

Not sure if this will be useful to anyone, it works for me. If compiled with make, nothing is changed (only ALSA is supported). By compiling with make USE_PULSEAUDIO=1, PulseAudio support for volume input is available by specifying device="pulse:N" line in i3status.conf, where N is the PulseAudio sink index. ALSA is still available if the device line does not match that format.

[stapelberg]

Before we dive into any of the details of the pull request itself, can we make it so that PulseAudio is automatically queried when it’s in use on the user’s system? The current ALSA implementation faills back to PulseAudio, but rather poorly, see #4

[Watcom]

You mean if the user does not specify a device? Now that you mention it, it makes sense to try the default sink in this case and, in case of failure, fall back to ALSA. I'll try that.

[Watcom]

Ok, now PulseAudio is tried first, then ALSA. Also, now the default sink is fetched and used if not explicitly specified. If the device specified is anything other than "pulse" or "default" (or missing), then PulseAudio is not used even if it's available.

[stapelberg]

Thanks for making that change.

I compiled and ran this, but it seems like even when using libpulse, there still is a fork() involved every second in order to get the volume:

$ strace -tt -f -o /tmp/st -s 2048 ./i3status
^C
$ grep clone /tmp/st
9699  09:42:20.121223 clone(child_stack=0x7fc3fc864f30, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0x7fc3fc8659d0, tls=0x7fc3fc865700, child_tidptr=0x7fc3fc8659d0) = 9700
9699  09:42:21.008604 clone(child_stack=0x7fc3fc864f30, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0x7fc3fc8659d0, tls=0x7fc3fc865700, child_tidptr=0x7fc3fc8659d0) = 9702
9699  09:42:22.007302 clone(child_stack=0x7fc3fc864f30, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0x7fc3fc8659d0, tls=0x7fc3fc865700, child_tidptr=0x7fc3fc8659d0) = 9704

Is that because of how this code uses the library or is it inherent to how the library works?

Also I’m wondering how libasound can avoid being linked against all these dependencies that libpulse brings in but still fall back to query pulseaudio for volume.

An alternative could be using dbus, but it seems like PulseAudio is shipped with the dbus module disabled on plenty of linux distributions :(.

I haven’t looked into it in detail, but would it be feasible to directly talk to PulseAudio without using libpulse?

[Watcom]

That's odd. I tested that here and I see no forking. Perhaps it's falling back to ALSA for some reason in your system?

How is your device= line in i3status.conf? Make sure it's either unset or set to "default" or "pulse". Also make sure that you compile with make USE_PULSEAUDIO=1

One quick way to see if it's using PulseAudio is by setting the sink volume to above 100% (e.g. with pavucontrol). It will only show values above 100% if using PulseAudio, but not with ALSA.

[Watcom]

Should I change the default to building with PulseAudio support and, subsequently, the make flag from USE_PULSEAUDIO=1 to NO_PULSEAUDIO=1?

[stapelberg]

The system in question is indeed using PulseAudio. After repeating and making sure I compile with make USE_PULSEAUDIO=1, I cannot reproduce the forking, so I think I might have screwed up in my initial test.

I’ve done some digging in the PulseAudio API, and have two high-level comments:

1. You can use the @DEFAULT_SINK@ sink name to avoid one level of callbacks. In fact, you don’t need the server_info callback either, I think, so you can get by with just one callback.
2. Instead of using the simple mainloop and having the volume lag behind one iteration (unclean), you could use the pa_threaded_mainloop much like the simple API is using it. I’ve written a little test program to illustrate what I mean: https://gist.github.com/stapelberg/507b400483e2f8efdebd

With regards to building: please just remove the conditionals entirely and make libpulse a regular dependency. These days PulseAudio is wide-spread, and having it optional will be more of a headache than it buys us.

Also, can you rebase your commits on top of master and squash your history? I.e., I’m not particularly interested in your “clang-format” commit :).

By the way, I think it’d be worth a shot to add a function to get the volume (or the sink info in general) to libpulse’s simple API. That way, we could eventually switch to just using their simple API, as we are basically replicating it (after going the route I suggested). Is that something you’d want to do?

[Watcom]

You can use the @DEFAULT_SINK@ sink name to avoid one level of callbacks. In fact, you don’t need the server_info callback either, I think, so you can get by with just one callback.

Nice catch, I completely missed that one. Done, and indeed, got rid of two callbacks.

With regards to building: please just remove the conditionals entirely and make libpulse a regular dependency.

Done. The added dependency seems to be failing the automated builds, though.

Also, can you rebase your commits on top of master and squash your history? I.e., I’m not particularly interested in your “clang-format” commit :).

Sorry about that, it's squashed now.

Instead of using the simple mainloop and having the volume lag behind one iteration (unclean), you could use the pa_threaded_mainloop much like the simple API is using it.

Originally I considered using pa_threaded_mainloop but I had doubts about the proper way to use it and decided to go the safe way. In particular, I thought of subscribing to volume changes from the sink and waking the main thread upon volume changes so that a refresh was issued when required. I don't know if this behaviour is always desirable, hence my doubt (maybe something configurable?). I can try this direction if you think it's ok.

By the way, I think it’d be worth a shot to add a function to get the volume (or the sink info in general) to libpulse’s simple API. That way, we could eventually switch to just using their simple API, as we are basically replicating it (after going the route I suggested). Is that something you’d want to do?

I think if we subscribe to volume changes like I've suggested then the simple API could not be used.

[stapelberg]

Originally I considered using pa_threaded_mainloop but I had doubts about the proper way to use it and decided to go the safe way. In particular, I thought of subscribing to volume changes from the sink and waking the main thread upon volume changes so that a refresh was issued when required. I don't know if this behaviour is always desirable, hence my doubt (maybe something configurable?). I can try this direction if you think it's ok.

Ah, I didn’t see that one can subscribe to volume changes. That’d be the most elegant solution, I think (subscribing, modifying a variable when the volume changes, and just returning that variable in each loop iteration).

I don’t think we should make this configurable, because why would you not want it to work that way?

[Watcom]

I don’t think we should make this configurable, because why would you not want it to work that way?

I meant waking the main thread so that i3status would print the new volume immediately.

Currently I have something like this in my .i3/config for changing the PulseAudio volume from the keyboard:

bindsym XF86AudioRaiseVolume exec --no-startup-id pactl set-sink-volume 1 -- +2% && killall -SIGUSR1 i3status
bindsym XF86AudioLowerVolume exec --no-startup-id pactl set-sink-volume 1 -- -2% && killall -SIGUSR1 i3status

The killall -SIGUSR1 i3status would not be needed if i3status knew the volume changed and refreshed immediately.

[stapelberg]

Even tweaking the main thread to wake up is something I’d be willing to do without a configuration option, as long as the volume change event doesn’t trigger when it shouldn’t.

[Watcom]

Ok, converted it to a threaded mainloop plus waking of the main thread when the volume changes. Seems to be working quite well.

[stapelberg]

Use strncasecmp(device, "pulse", strlen("pulse")) instead.

[Watcom]

Done.

[Watcom]

The default sink is now kept updated by using a server change subscription. I tested here flipping the default sink between two devices and it updates immediately, alternating the display of each of the device's current volume.

The static array for volume storage was replaced by a dynamically allocated array of struct. There's no longer a hard limit on the number of sinks supported.

[stapelberg]

Instead of doing all this reallocation yourself, why not use the TAILQ macros I suggested?

[stapelberg]

Thanks for all the work you’re putting into this. I feel like we got the functionality mostly nailed and are dealing only with code questions now.

As a heads-up, I’ll also want to see whether we can use http://clang.llvm.org/docs/ThreadSafetyAnalysis.html in order to make sure our mutex-related code is correct.

[Watcom]

OK, I think all of the issues you pointed out were taken care of. The value to signify the default sink changed from -1 to UINT32_MAX and the type from int to uint32_t.

About TAILQ, I figured since it was ok to traverse the list of sinks to read a volume from the index, we might as well have one contiguous block which would be tighter than several tiny malloc'd blocks with the extra overhead of a doubly linked list. Of course we are talking about a few bytes here, the point being the realloc method doesn't seem more complicated to me than using TAILQ.

Very interesting and useful the Clang thread safety analysis feature. The page only shows C++ examples though, but I suppose there must be ways to make it work with C as well?

[stapelberg]

You’re not acquiring the pulse_mutex here, but in other places when accessing cached_volume.

[Watcom]

The code is correct, though. I've added a comment stating the mutex should be locked prior to calling find_sink_idx().

[stapelberg]

About TAILQ, I figured since it was ok to traverse the list of sinks to read a volume from the index, we might as well have one contiguous block which would be tighter than several tiny malloc'd blocks with the extra overhead of a doubly linked list. Of course we are talking about a few bytes here, the point being the realloc method doesn't seem more complicated to me than using TAILQ.

We’re talking about typically less than 10 mallocs. I don’t think it’s worth worrying about that, and I think using a TAILQ would make the code easier. Also note that modern libcs don’t translate every malloc() into an actual allocation, but have internal buffers. I really don’t think the memory layout is something that should concern us in such a case.

[stapelberg]

Very interesting and useful the Clang thread safety analysis feature. The page only shows C++ examples though, but I suppose there must be ways to make it work with C as well?

See https://llvm.org/bugs/show_bug.cgi?id=20403
With a bunch of type casts, you can actually compile i3status with -x c++: http://sprunge.us/aGUi

[Watcom]

TAILQ it is, then.

[stapelberg]

Why do you initialize the tailq again? This line is equivalent to what the TAILQ_HEAD_INITIALIZER does.

[Watcom]

Sorry, my mistake. Fixed.

[stapelberg]

Thanks for your continued patience on this. I think in one or two rounds we’ll end up with high-quality code that’s consistent with the rest of our code base. I’ll be happy to merge it then :).

[Watcom]

Thanks for your continued patience on this. I think in one or two rounds we’ll end up with high-quality code that’s consistent with the rest of our code base. I’ll be happy to merge it then :).

No problem at all. I also benefit from this as a user :).

[stapelberg]

Now it seems like you’re missing the tv_nsec part, i.e. you’ll not wake exactly on the full second, right? (see the comment above)

[Watcom]

Actually no, it behaves just like before. That includes the ":00 on every new minute" part. You can check that using a 5-second interval. The reason it looks simpler is that while nanosleep expects a relative time (i.e. a delay), pthread_cond_timedwait expects an absolute time, so it's actually easier to achieve the same result.

[stapelberg]

Ah, I see. Can you update the comment below, then? You’re currently saying pthread_cond_timedwait is equivalent to a nanosleep call, but based on this observation, it clearly isn’t :).

[Watcom]

Done, pushed commit.

[stapelberg]

Instead of calling get_sink_info, you could also just call store_volume_from_sink_cb to save another round-trip, right?

[Watcom]

Indeed!

[stapelberg]

Very cool. Thanks again!

[Watcom]

Thank you!