audio/pulseaudio: improve OSS support
AcceptedPublic
Actions

Authored by makc on Thu, Oct 1, 10:55 AM.

Details

Reviewers

Group Reviewers

desktop
multimedia

Summary

Set vol or pcm, but not both. Changing both channels makes volume feedback non-linear.
Set / read monitor channel.

Diff Detail

Repository

R11 FreeBSD ports repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

makc requested review of this revision.Thu, Oct 1, 10:55 AM

makc created this revision.

arrowd added a subscriber: arrowd.Thu, Oct 1, 11:04 AM

arrowd added inline comments.

audio/pulseaudio/files/patch-src_modules_oss_module-oss.c
104	That doesn't seem right. If we have both `SOUND_MASK_VOLUME` and `SOUND_MASK_PCM` requests, we should handle both. What's the root problem this patch tries to solve?

makc added inline comments.Fri, Oct 2, 9:42 AM

audio/pulseaudio/files/patch-src_modules_oss_module-oss.c
104	Pulseaudio does not allow to change `pcm` and `vol` for oss mixer independently. When changing volume in plasma-pa applet or using `pactl set-sink-volume` it set both `vol` and `pcm` channels to the same value. As a result real sound volume drops dramatically when you move volume slider away from 100%. Usually you want to keep `pcm` constant and change real volume via `vol` channel. KMix worked this way before we switched to Plasma-pa by default.

For the context: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/src/modules/oss/module-oss.c?ref_type=heads&plain=1#L840

static void sink_set_volume(pa_sink *s) {

    struct userdata *u;

    pa_assert_se(u = s->userdata);

    pa_assert(u->mixer_devmask & (SOUND_MASK_VOLUME|SOUND_MASK_PCM));

    if (u->mixer_devmask & SOUND_MASK_VOLUME)
        (void) pa_oss_set_volume(u->mixer_fd, SOUND_MIXER_WRITE_VOLUME, &s->sample_spec, &s->real_volume);

    if (u->mixer_devmask & SOUND_MASK_PCM)
        (void) pa_oss_set_volume(u->mixer_fd, SOUND_MIXER_WRITE_PCM, &s->sample_spec, &s->real_volume);
}

Please upstream this first, the last few commits to this file there were by us.

arrowd added inline comments.Fri, Oct 2, 1:06 PM

audio/pulseaudio/files/patch-src_modules_oss_module-oss.c
104	Are you sure it is really a PulseAudio API problem? I do see the problem - if I move the volume scrollbar in Plasma System settings and then run `mixer` from the console, both `vol` and `pcm` get changed indeed. But it might be Plasma problem, not a PulseAudio one. Do you have a reproducer that does not involve Plasma clients?

vishwin added inline comments.Fri, Oct 2, 1:09 PM

audio/pulseaudio/files/patch-src_modules_oss_module-oss.c
104	I just reproduced this on cinnamon, which uses the API directly. PulseAudio's own pavucontrol should exhibit the same.

@christos Maybe you have an idea what's the correct fix here?

makc added inline comments.Fri, Oct 2, 3:09 PM

audio/pulseaudio/files/patch-src_modules_oss_module-oss.c
104	Do you have a reproducer that does not involve Plasma clients? Sure, you can use `pactl` utility from pulseaudio package. At least after this patch `sink_set_volume` behaves consistently with `sink_get_volume`. `sink_get_volume` attempts to read `vol` channel first and returns its values, otherwise it tries `pcm` channel. I wonder whether we need to read/set `pcm` at all in these functions. Are there mixers which have `pcm` channel but not `vol`?

The mixer knobs are a bit confusing since they are part of the legacy OSS API, but typically vol controls the hardware master volume, and pcm the software volume, usually from applications and sound(4) virtual channels.

I think what the patch does is correct, even though I don't use pulseaudio at all.

This revision is now accepted and ready to land.Wed, Oct 7, 6:59 AM

All right, @makc please give me a couple of days, as I still want to research why PulseAudio ends up being passed both SOUND_MASK_VOLUME and SOUND_MASK_PCM.