[pulseaudio-discuss] [PATCH 4/4] alsa-mixer: Add force-hw-volume flag to alsa profile sets

Tue Jun 28 15:13:33 PDT 2011

'Twas brillig, and Colin Guthrie at 28/06/11 22:22 did gyre and gimble:
> 'Twas brillig, and David Henningsson at 16/05/11 07:43 did gyre and gimble:
>> On 2011-05-15 16:02, Colin Guthrie wrote:
>>> 'Twas brillig, and Jyri Sarha at 11/04/11 10:19 did gyre and gimble:
>>>> On Sat, 09 Apr 2011 20:20:40 +0200, David Henningsson
>>>> <david.henningsson at canonical.com>  wrote:
>>>>> On 2011-04-08 17:18, Colin Guthrie wrote:
>>>>>> 'Twas brillig, and oku at iki.fi at 08/04/11 15:18 did gyre and gimble:
>>>>>>> From: Jyri Sarha<jyri.sarha at nokia.com>
>>>>>>>
>>>>>>> Before this patch, if any of the paths in a path set do not
>>>>>>> support HW volume then the HW volume is disabled for the whole
>>>>>>> set. In some cases this is a bit drastic measure. For instance,
>>>>>>> if all but one of the paths support HW volume and dB there no
>>>>>>> problem to pretend that we have HW volume for the whole set. The
>>>>>>> path without any mixers to control will just always return 0 dB
>>>>>>> and the rest is handled by SW volume. This patch adds a flag to
>>>>>>> the mapping section of profile set file to enables this behavior.
>>>>>>
>>>>>> David, this sounds similar to your USB Headset issue from a couple
>>>>>> days
>>>>>> ago... or am I just reading too much into the description?
>>>>>
>>>>> Sort of - it just feels like neither of us has tried to do the right
>>>>> thing so far - I added a workaround/quirk for a few devices, and this
>>>>> patch adds a setting to turn something on and off.
>>>>>
>>>>> I'd like it to "just work".
>>>>>
>>>>> Or put in another way - what's the recommended default setting of this
>>>>> new parameter, and why?
>>>>>
>>>>
>>>> Target for my patch was to be non intrusive, but if you agree I can
>>>> easily
>>>>
>>>> change my patch to always behave like force-hw-volume flag was set to
>>>> true
>>>> and remove the flag. It is even easier just to change the default for
>>>> the
>>>> flag.
>>>
>>> David, what's you're thinking on Jyri's suggestion here?
>>
>> Not 100% sure on this one, what would be the reason why we would want
>> the old behaviour?
> 
> 
> Just bringing up this patch again as I'd like to knock this problem on
> the head before v1.0.
> 
> 
> Looking at the code that this affects, the force on flag was only run in
> path_set_unify().
> 
> 
> If I understand it correctly, if it finds a path set that cannot fully
> support volume+dB, then it disables the volume+mute in all the paths in
> that path set.
> 
> The problem arises from the fact that the same path can be used in
> different path sets and thus if one path set contains an element that
> busts things up, then we cripple every path set that uses it.
> 
> This logic, in itself is flawed, as it depends on the order that the
> path sets are checked. If the paths and path sets are complex enough
> with weird combos, the order of the path sets could affect whether a
> given other path set works or not which is pretty messed up.
> 
> 
> What I wonder is if we could add has_volume, has_dB and has_mute flags
> to the path set itself. This allows us to shortcut straight to software
> volume when dealing with the path sets, but doesn't cripple any given
> path for use in other path sets.
> 
> This seems a cleaner approach than forcing the has_* flags on any given
> path that shouldn't have it.
> 
> 
> 
> Also, with regards to mute, we only really need one of the paths to
> support mute.... the fact that path_set_unify() disables mute support if
> one path in the set does not it... but this is pretty bizarre as we only
> really need to mute things once for it to be muted... it's not a
> cumulative thing like volume. Seems a little backwards to me!
> 
> 
> Anyway if this approach makes sense, let me know and I'll cook up a patch.

Ooops,

I've misread the code a bit here.

I mixed up (in my head) "Path Sets" + "Paths" with "Paths" + "Elements"...

I realise now how the collective result of the paths in a path set
affects the flags that go on a sink/source, which cannot change after
creating - thus the desire to make all the paths conform to a "lowest
common denominator" was is done in path_set_unify()

So, it seems wrong to me to pretend that we do have something when we
don't (i.e setting the HW_VOLUME_CTRL flag when we do not have a h/w
kcontrol for the currently active port), but by the same token, it's
just as wrong to say we do NOT have something when we do, so the
"force-hw-volume" approach kinda makes sense to enable functionality
rather than cripple it.

Personally, I think the flags used here (HW_VOLUME, HW_MUTE,
DECIBEL_VOLUME etc.) are maybe not actually that important. Perhaps
these can be moved over to sink/source properties (namely to inform the
user for debugging) and then internal code that currently uses the flags
can simply be refactored (I've not checked but I don't think this will
be a major refactor).

Obviously external code may also uses these flags, but I suspect usage
of this outside of PA will be minimal if not completely non-existent.

This way, we can switch ports, and allow a sink to change from h/w
volume control to software without any hassle and bypass the "issues"
mentioned here in the comment from path_set_unify()

    /* We have issues dealing with paths that vary too wildly. That
     * means for now we have to have all paths support volume/mute/dB
     * or none. */

Failing that, and assuming the flags stay, then I'd say the approach of
checking every path in the path set and if any of them support h/w
volume, then the sink gets that flag is a reasonably valid approach,
even if some ports will make the inclusion of that flag a downright lie!

I'm not sure I see the reason to specifically set the has_volume/has_dB
flags on a given path as with this patch - just leaving each path alone,
rather than forcing them all into some kind of misguided alignment seems
like the better solution.

If this approach is still misinterpreted (it's late and I've still not
really read the code as thoroughly as I should), please forgive me :D

Col

PS, while looking into this issue, I found a way to easily recreate the
"initial volume issue" I discussed on IRC.

 1. Comment out the path_set_unify() call.
 2. Use my USB Handset with the Speaker kcontrol (no Master or PCM)
 3. Switch between Analog Output and Analog Speakers profiles.

When witching from AS -> AO, the Speaker kcontrol is zeroed+muted. When
switching from AO -> AS, it stays in it's zeroed+muted state until the
volume control is touched. I think this is similar behaviour to the
initial startup problem.

Of course even with path_set_unify() commented out, the fact I have two
ports (one of which now at least works \o/) is bogus. Only Analog
Speakers should be present so this still needs to be addressed somehow,
but I'm beginning to actually "get" the problem in a bit more depth now :)

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]