WIP: Add initial pulseaudio module support #102

Open
sochotnicky wants to merge 3 commits from sochotnicky/yambar:pulse-module-addition into master

Note - no need to review, I just figured this way it will be harder for me to forget about this :-)

TODO:

  • Not sure how to deal with PA volumes. Technically - there's a "max" volume (called base volume) for output. That's technically 100% but outputs can go over 100% with gain etc. Things work fine when used within ordinary number display, but ramp usage is weird because you don't really want to use "real" PA max volume.
Note - no need to review, I just figured this way it will be harder for me to forget about this :-) TODO: * Not sure how to deal with PA volumes. Technically - there's a "max" volume (called base volume) for output. That's technically 100% but outputs can go over 100% with gain etc. Things work fine when used within ordinary number display, but ramp usage is weird because you don't really want to use "real" PA max volume.
sochotnicky added 1 commit 3 months ago
ac5c754ac2 Add initial pulseaudio module support
Poster

One question when you get to this - any specific code style you'd like to follow (ideally in a .clang-format config form :-D).

One question when you get to this - any specific code style you'd like to follow (ideally in a `.clang-format` config form :-D).
Owner

You can use the .editorconfig from foot. Though that obviously only covers the basics.

I also prefer functions to be declared like this:

return_type
function_name(args)
{
}

This is mainly to make it easier to grep for function bodies without matching function calls, or function prototypes (you can simply grep for ^function_name). While I nowadays rely mostly on LSP for code navigation, I still find this useful.

I tend to skip curlies for single line if/for/while, but only if the statement is a single line (and not a single statement split over multiple lines).

Spaces, not tabs. Indent by 4. Aim for 80 columns, but feel free to use more if a line break makes the code unreadable. I'll probably start whining if you get close to 100 columns.

I think that covers the important things.

I've been telling myself to write a style guide for a long time now, but for some reason, I refuse to listen :D

You can use the `.editorconfig` from foot. Though that obviously only covers the basics. I also prefer functions to be declared like this: ``` return_type function_name(args) { } ``` This is mainly to make it easier to grep for function bodies without matching function calls, or function prototypes (you can simply grep for `^function_name`). While I nowadays rely mostly on LSP for code navigation, I still find this useful. I tend to skip curlies for single line `if`/`for`/`while`, but only if the statement is a single **line** (and not a single **statement** split over multiple lines). Spaces, not tabs. Indent by 4. Aim for 80 columns, but feel free to use more if a line break makes the code unreadable. I'll probably start whining if you get close to 100 columns. I think that covers the important things. I've been telling myself to write a style guide for a long time now, but for some reason, I refuse to listen :D
Poster

I've been telling myself to write a style guide for a long time now, but for some reason, I refuse to listen :D

My suggestion - don't bother with a style guide. Get a CI/tool set up so that you have consistency within your projects (perhaps as pre-commit hook etc) and just get on with your life. I don't think most people will care and having a clear "just run this and forget about formatting" tends to be a good baseline. gofmt does it for golang, black does it for python, and seems that clang-format can do a pretty good job with C/C++ - so my suggestion would be to just add .cang-format file to the repos and move on with your life :-)

I tried generating one that matches your style, can add it to the MR so you can play/tweak as you see fit?

> I've been telling myself to write a style guide for a long time now, but for some reason, I refuse to listen :D My suggestion - don't bother with a style guide. Get a CI/tool set up so that you have consistency within your projects (perhaps as pre-commit hook etc) and just get on with your life. I don't think most people will care and having a clear "just run this and forget about formatting" tends to be a good baseline. `gofmt` does it for golang, `black` does it for python, and seems that clang-format can do a pretty good job with C/C++ - so my suggestion would be to just add `.cang-format` file to the repos and move on with your life :-) I tried generating one that matches your style, can add it to the MR so you can play/tweak as you see fit?
sochotnicky force-pushed pulse-module-addition from ac5c754ac2 to 960bc6892e 3 months ago
sochotnicky force-pushed pulse-module-addition from 960bc6892e to d96058887e 3 months ago
sochotnicky force-pushed pulse-module-addition from d96058887e to c294f09dbc 3 months ago
Owner

Auto-formatting code is taking things a bit too far IMHO. Yes, it makes style a non-issue in PRs. But I think being a bit more flexible results in better (more readable, easier -to-maintain) code in the end. There will always be pieces of code where a slightly different style is better.

The number of PRs, and the sizes of them, that I need to review and merge are low and small enough that style issues aren't really a problem. I may change my mind if that changes.

Though, it might be useful to have a .clang-format file available, but without enforcing it in the CI. For contributors who prefer that.

Auto-formatting code is taking things a bit too far IMHO. Yes, it makes style a non-issue in PRs. But I think being a bit more flexible results in better (more readable, easier -to-maintain) code in the end. There will always be pieces of code where a slightly different style is better. The number of PRs, and the sizes of them, that I need to review and merge are low and small enough that style issues aren't really a problem. I may change my mind if _that_ changes. Though, it might be useful to have a `.clang-format` file available, but without enforcing it in the CI. For contributors who prefer that.
sochotnicky force-pushed pulse-module-addition from c294f09dbc to 5d5dcf8086 3 months ago
sochotnicky force-pushed pulse-module-addition from 5d5dcf8086 to 3598d20ab8 3 months ago
Poster

One benefit of auto-formatting that I can see - if you have auto-formatting rules then if someone doesn't like it for their development they can easily reformat with their style while doing development and then reformat again into your style. But yes - that would be taking it a bit too far I guess :-)

Anyway - The .clang-format I used is here:
https://gist.github.com/sochotnicky/3bf56bcdb3bfe2165a51d6827e201833

If you want I can add it into this PR (or a separate one). Though maybe I'd wait for you to say how you like the style so maybe I can tweak things (i.e. I have line lenght set to 100 since that seemd your limit but perhaps we'd want to make it a little less etc..)

One benefit of auto-formatting that I can see - if you have auto-formatting rules then if someone doesn't like it for their development they can easily reformat with their style while doing development and then reformat again into your style. But yes - that would be taking it a bit too far I guess :-) Anyway - The `.clang-format` I used is here: https://gist.github.com/sochotnicky/3bf56bcdb3bfe2165a51d6827e201833 If you want I can add it into this PR (or a separate one). Though maybe I'd wait for you to say how you like the style so maybe I can tweak things (i.e. I have line lenght set to 100 since that seemd your limit but perhaps we'd want to make it a little less etc..)
sochotnicky force-pushed pulse-module-addition from 3598d20ab8 to f6a38be375 2 months ago
Poster

I've made a few cosmetic fixes/comment additions and cleanup when exiting. Even before I've been running this for the past few weeks and it's been working fine for my use cases (which include bluetooth headphones connect/disconnect and frequent volume/mute changes)

So from my POV - this is "good enough". YMMV of course - esp. since you are not using pulseaudio.

For each sink/source I only provide 2 values: muted/unmuted and a range of 0-100 of volume. But realistically in PA this can go over 100. It works fine as number display, on a ramp it does not easily show "over 100%". If someone cares enough it might make sense to add an additional bool that would say whether sink/source is over 100%.

One benefit this seems to have (for PA/PW users) over alsa module is that the volume percentage is consistent with PA/PW counting. Not a huge thing but a quality of life thing (i.e. with alsa it's not easy to say when you cross into clipping volume range)

I've made a few cosmetic fixes/comment additions and cleanup when exiting. Even before I've been running this for the past few weeks and it's been working fine for my use cases (which include bluetooth headphones connect/disconnect and frequent volume/mute changes) So from my POV - this is "good enough". YMMV of course - esp. since you are not using pulseaudio. For each sink/source I only provide 2 values: muted/unmuted and a range of 0-100 of volume. But realistically in PA this can go over 100. It works fine as number display, on a ramp it does not easily show "over 100%". If someone cares enough it might make sense to add an additional bool that would say whether sink/source is over 100%. One benefit this seems to have (for PA/PW users) over alsa module is that the volume percentage is consistent with PA/PW counting. Not a huge thing but a quality of life thing (i.e. with alsa it's not easy to say when you cross into clipping volume range)
sochotnicky changed title from WIP: Add initial pulseaudio module support to Add initial pulseaudio module support 2 months ago
dnkl requested changes 2 months ago
dnkl left a comment

First review iteration. Note that I haven't even tried to compile this yet. This is just me reading through the code for the first time.

modules/pulse.c Outdated
{
static char desc[32];
snprintf(desc, sizeof(desc), "pulse");
return desc;
dnkl commented 2 months ago
Poster
Owner

Would it be possible to include some source or sink information here? Or does having multiple pulseaudio (yambar) modules not make any sense?

(be aware - the number of available characters is limited!)

Would it be possible to include some source or sink information here? Or does having multiple pulseaudio (yambar) modules not make any sense? (be aware - the number of available characters is limited!)
Poster

Hmm, adding the configured sink/source would in theory be possible of course. The problem is that for example this is my current default sink name:

alsa_output.pci-0000_00_1f.3-platform-skl_hda_dsp_generic.HiFi__hw_sofhdadsp_5__sink

It's similarly long for the source. Now the actual configuration item I just leave default (i.e. @DEFAULT_SINK@ and @DEFAULT_SOURCE@) in my case. But other people might want to specify a specific device perhaps which would mean the above long string (times two).

I am not sure how much magic I'd want to (or should) do for the description?

Hmm, adding the configured sink/source would in theory be possible of course. The problem is that for example this is my current default sink name: `alsa_output.pci-0000_00_1f.3-platform-skl_hda_dsp_generic.HiFi__hw_sofhdadsp_5__sink` It's similarly long for the source. Now the actual *configuration* item I just leave default (i.e. `@DEFAULT_SINK@` and `@DEFAULT_SOURCE@`) in my case. But other people might want to specify a specific device perhaps which would mean the above long string (times two). I am not sure how much magic I'd want to (or should) do for the description?
Poster

But regarding whether it makes sense to have multiple instances - I would assume 99% users will not need multiple instances (since a single instance can do both one input and output).

But regarding whether it makes sense to have multiple instances - I would assume 99% users will not need multiple instances (since a single instance can do both one input and output).
dnkl commented 2 months ago
Poster
Owner

All of those alternatives are way too long. Typical limit is ~16 characters. Given that multiple instances are unlikely, let's go with the original "pulse".

All of those alternatives are way too long. Typical limit is ~16 characters. Given that multiple instances are unlikely, let's go with the original "pulse".
dnkl marked this conversation as resolved
modules/pulse.c Outdated
if (!p->current_sink_name || strcmp(i->default_sink_name, p->current_sink_name) != 0) {
LOG_DBG("Default sink changed (%s) - calling get_sink_info_by_name", i->default_sink_name);
free(p->current_sink_name);
p->current_sink_name = strdup(i->default_sink_name);
dnkl commented 2 months ago
Poster
Owner

Can i->current_sink_name be NULL? If so, strdup() will crash.

Can `i->current_sink_name` be `NULL`? If so, `strdup()` will crash.
Poster

Based on what I saw with my experiments - when there's no default sink/default source it gets set to @DEFAULT_SINK@ string (or @DEFAULT_SOURCE@). I have never seen it be set to NULL. But I could add an if guard for this here...

Based on what I saw with my experiments - when there's no default sink/default source it gets set to `@DEFAULT_SINK@` string (or `@DEFAULT_SOURCE@`). I have never seen it be set to NULL. But I could add an if guard for this here...
dnkl commented 2 months ago
Poster
Owner

Same as the other one, let's skip the check for now.

Same as the other one, let's skip the check for now.
dnkl marked this conversation as resolved
modules/pulse.c Outdated
strcmp(i->default_source_name, p->current_source_name) != 0) {
LOG_DBG("Default source changed (%s) - calling get_sink_info_by_name",
i->default_source_name);
free(p->current_source_name);
dnkl commented 2 months ago
Poster
Owner

Same here, can i->default_source_name be NULL?

Same here, can `i->default_source_name` be `NULL`?
Poster

Same situation as the default_source_name - I could guard here but I have never seen it be null even with no default output/input and no audio card enabled (though I see no guarantees it can't)

Same situation as the default_source_name - I could guard here but I have never seen it be null even with no default output/input and no audio card enabled (though I see no guarantees it can't)
dnkl commented 2 months ago
Poster
Owner

Let's be bold; assume they are never NULL, and skip the check. Easy enough to fix later, if necessary.

Let's be bold; assume they are never NULL, and skip the check. Easy enough to fix later, if necessary.
dnkl marked this conversation as resolved
modules/pulse.c Outdated
if ((t & PA_SUBSCRIPTION_EVENT_TYPE_MASK) == PA_SUBSCRIPTION_EVENT_REMOVE) {
if (idx == p->sink_index) {
p->sink_online = false;
p->sink_index = 0;
dnkl commented 2 months ago
Poster
Owner

Don't we need a bar refresh here?

Don't we need a bar refresh here?
dnkl marked this conversation as resolved
modules/pulse.c Outdated
if ((t & PA_SUBSCRIPTION_EVENT_TYPE_MASK) == PA_SUBSCRIPTION_EVENT_REMOVE) {
if (idx == p->source_index) {
p->source_online = false;
p->source_index = 0;
dnkl commented 2 months ago
Poster
Owner

Bar refresh?

Bar refresh?
dnkl marked this conversation as resolved
modules/pulse.c Outdated
}
pa_operation_unref(o);
}
case PA_SUBSCRIPTION_EVENT_SERVER:
dnkl commented 2 months ago
Poster
Owner

Intentional fall-through? Can you add a comment, /* FALLTHROUGH */, where the break would have been?

Intentional fall-through? Can you add a comment, `/* FALLTHROUGH */`, where the break would have been?
Poster

Nope, not intentional, though fortunately it only caused one more callback being done. Will fix

Nope, not intentional, though fortunately it only caused one more callback being done. Will fix
dnkl marked this conversation as resolved
modules/pulse.c Outdated
case PA_CONTEXT_SETTING_NAME:
mtx_lock(&mod->lock);
p->online = false;
mtx_unlock(&mod->lock);
dnkl commented 2 months ago
Poster
Owner

Don't you need to refresh the bar here?

Don't you need to refresh the bar here?
Poster

Yup, And I need to fix up a few lock/unlock places

Yup, And I need to fix up a few lock/unlock places
dnkl marked this conversation as resolved
modules/pulse.c Outdated
assert(c);
mtx_lock(&mod->lock);
p->online = true;
mtx_unlock(&mod->lock);
dnkl commented 2 months ago
Poster
Owner

Refresh bar?

Refresh bar?
dnkl marked this conversation as resolved
sochotnicky added 1 commit 2 months ago
Poster

I've pushed an additional commit fixing some of the locking/bar refresh parts. I can amend/squash - just wanted to make it easier to see what has changed.

I've pushed an additional commit fixing some of the locking/bar refresh parts. I can amend/squash - just wanted to make it easier to see what has changed.
dnkl requested changes 2 months ago
dnkl left a comment

I ran this PR through our CI, and the pulse module isn't built, due to missing dependencies: https://builds.sr.ht/~dnkl/job/605092#task-setup-904 (not sure if this is visible to you), and/or https://gitlab.com/dnkl/yambar/-/jobs/1665873063#L330.

Can you add the necessary dependencies to .gitlab-ci.yml, and .builds/alpine-x64.yml. If you cannot (easily) test it, let me know what the dependencies are, and I can test the CI scripts manually.

: string
: no
: Sink name to monitor. Defaults to _@DEFAULT\_SINK@_
| source_name
dnkl commented 2 months ago
Poster
Owner

Did you consider naming them "sink" and "source"? The "_name" suffix does add additional information, but I still feel they aren't exactly necessary...

Did you consider naming them "sink" and "source"? The "\_name" suffix does add additional information, but I still feel they aren't exactly necessary...
Poster

WRT builds - I yeah, I guess a missing libpulse-dev in https://codeberg.org/dnkl/yambar/src/branch/master/.gitlab-ci.yml#L10

Will add

WRT naming - I don't have a strong preference, I guess it was just a first thing that came to me. More than happy to tweak

WRT builds - I yeah, I guess a missing `libpulse-dev` in https://codeberg.org/dnkl/yambar/src/branch/master/.gitlab-ci.yml#L10 Will add WRT naming - I don't have a strong preference, I guess it was just a first thing that came to me. More than happy to tweak
dnkl commented 2 months ago
Poster
Owner

I took a quick look at the other modules, and most seem to favor short names. So, I suggest we change them to sink and source.

I took a quick look at the other modules, and most seem to favor short names. So, I suggest we change them to `sink` and `source`.
Poster

ACK, will do - while testing I noticed some unwanted behaviour with non-default source/sink so I'll need to fix those up. Putting this back into WIP until I do... sorry

ACK, will do - while testing I noticed some unwanted behaviour with non-default source/sink so I'll need to fix those up. Putting this back into WIP until I do... sorry
dnkl commented 2 months ago
Poster
Owner

No worries, take your time :)

No worries, take your time :)
dnkl commented 2 months ago
Poster
Owner

Btw, if you rebase against master, you'll get proper CI (https://ci.codeberg.org/dnkl/yambar)

Btw, if you rebase against master, you'll get proper CI (https://ci.codeberg.org/dnkl/yambar)
sochotnicky changed title from Add initial pulseaudio module support to WIP: Add initial pulseaudio module support 2 months ago
sochotnicky added 1 commit 2 months ago
sochotnicky force-pushed pulse-module-addition from d7a7b22c26 to d08bf87691 2 months ago
sochotnicky force-pushed pulse-module-addition from d08bf87691 to abf979818d 2 months ago
sochotnicky force-pushed pulse-module-addition from abf979818d to 7633158f6e 2 months ago
sochotnicky force-pushed pulse-module-addition from 7633158f6e to 318f8ea4d3 2 months ago
sochotnicky force-pushed pulse-module-addition from 318f8ea4d3 to 78d144429d 2 months ago

Reviewers

dnkl requested changes 2 months ago
All checks were successful
continuous-integration/woodpecker the build was successful
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.