PulseObject: Add channel_map attribute with raw position integers

[mhthies]

This commit adds an attribute channel_map to all PulseObject objects that are build from a C struct with a channel_map field, such as PulseSinkInfo or PulseSourceInfo. In contrast to channel_list, which is already included with these objects, channel_map is a list of the raw integer position codes from libpulse's pa_channel_position enum instead of a string representation of the channel positions.

This is useful to do automated volume calculations, dynamically based on the channel map of a device. In my specific use case, I created additional C bindings for the pavolume* functions to separate a multi-channel volume into balance, fade, etc. These methods require a pa_channel_map struct, which can easily be built from the proposed channel_map attribute.

[mk-fg]

Thanks.

I'd rather avoid exposing raw ints from C though, so will probably store IntEnum values there which transparently compare/hash to same int values, and should probably work exactly same in the code, but are self-descriptive (i.e. printing them should say something like <0: mono>).

This should be easy to do via python's enum module, but that'll break python2 compatibility, which I wonder if safe to do by now too... gotta check if pypi has download numbers for 2.7 vs 3.x and maybe just discontinue 2.7 support here as well.

[mk-fg]

Running pypi bigdata queries like

SELECT
  REGEXP_EXTRACT(details.python, r"[0-9]+\.[0-9]+") AS python_version,
  COUNT(*) AS num_downloads
FROM `bigquery-public-data.pypi.file_downloads`
WHERE file.project = 'pulsectl'
  AND DATE(timestamp)
    BETWEEN DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY)
    AND CURRENT_DATE()
GROUP BY python_version
ORDER BY num_downloads DESC

Seem to be getting the results with around 2.5-5% of 2.7 downloads among those that send python version info, and these fluctuate in 300-500+ range per month, so guess not entirely bots and automated systems (and those might indicate some usage too).

So probably should keep 2.7 compatibility around and just merge integers for now, but only use enum values in modern python, which hopefully won't be too broken or confusing, as adding another weird custom enum class just for these values in 2.7 sounds like a bad idea.

[mk-fg]

Ah, I was thinking that this map was different than stuff in channel_list, but apparently not, list just uses strings and this one C constants, so guess there isn't much point decoding these constants, and better to just have enum value for strings.

Don't like redundancy and ambiguity that this introduces - presumably high-level code should use descriptive stuff from channel_list and having a separate enum for C-interfacing code seems a bit too niche. Wonder if there's a good way to make init from transient C structs extendable, so that you can use any raw values from there, without needing to always expose them...

[mk-fg]

Did remove channel_map attr in 48aa367 and added channel_pos string-enum (similar to all other enums used in this module) to Pulse object instead, which is generated by libpulse, and each value there should have _c_val attr that exposes original value:

>>> pulse.channel_pos
<Enum channel_pos [aux0 aux1 aux10 aux11 aux12 aux13 aux14 aux15 aux16 aux17 aux18 aux19 aux2 aux20 aux21 aux22 aux23 aux24 aux25 aux26 aux27 aux28 aux29 aux3 aux30 aux31 aux4 aux5 aux6 aux7 aux8 aux9 front-center front-left front-left-of-center front-right front-right-of-center lfe mono rear-center rear-left rear-right side-left side-right top-center top-front-center top-front-left top-front-right top-rear-center top-rear-left top-rear-right]>

>>> pulse.channel_pos.mono
<EnumValue channel_pos=mono>
>>> pulse.channel_pos.mono._c_val
0
>>> pulse.channel_pos.top_rear_center._c_val
50

>>> pulse.channel_pos._c_val(0)
<EnumValue channel_pos=mono>
>>> pulse.channel_pos._c_val(50)
<EnumValue channel_pos=top-rear-center>

channel_list still has string values for simplicity and best backwards compatibility, but they should compare just fine with these enum values. Will translation through these work for your use-case?

Did also briefly consider how to make PulseObject extensible, but outside of obvious monkey-patching, couldn't think of a way to do it in a more idiomatic manner, as it's just baked as a superclass into everything.

[mhthies]

Thanks for investigating in this issue.

Will translation through these work for your use-case?

Hm, I could use that to translate from the channel_list back to a list of integers, which then can be used with libpulse's volume methods. However, my original intention in the PR was to avoid that double int-to-string-to-int conversion for performance and simplicity reasons, by just exposing the int values as they are.

I agree with you that enum values are the much more idiomatic approach than raw integer values without semantics. IMHO, the best solution would be exposing a list of enum members of the channel_pos enum in the PulseObject objects. The channel_list attribute with string values would still be required for backwards compatibility (however, it could be converted into a property, generating the string list from the list of enum values on-the-fly).

[mk-fg]

Hm, I could use that to translate from the channel_list back to a list of integers

Yeah, that's kinda what I was thinking there, basically make a custom attr on each object that'd be created once when you first get that object, which seem to be simple enough for a rather niche use-case.

Performance-wise, imo it shouldn't be a concern with python, as pretty sure I've never actually profiled this code, and if you do so, there'd likely be something that overshadows all these petty conversions by 1000-to-1 at least, so that thinking about optimizing those is kinda pointless.

IMHO, the best solution would be exposing a list of enum members of the channel_pos enum in the PulseObject objects

I think it's done through a _values or _c_vals attributes on it, which are either str: EnumValue or int: EnumValue mappings respectively. Nevermind the underscore, which is used there not to mark them as "private" but to avoid clashing with actual EnumValue attrs, same as in e.g. collections.namedtuple objects.

The channel_list attribute with string values would still be required for backwards compatibility however, it could be converted into a property, generating the string list from the list of enum values on-the-fly

But wouldn't it be strange to have an obvious "use this thing idiomatic here" and then behind the scenes make that a second-class attribute, instead of having it a first-class like it currently is, and leaving any second-class conversions for a way more niche use-cases (e.g. custom C interfaces)?

Tbf, channel_pos already makes channel_list into a special case (and to be even more fair, all attrs converted individually in that init are special already), so guess might as well have some "channel_list_raw" attr that'd keep the integers. Feel like all attrs relating to that one thing should be named similarly though to group them together, and "channel_pos" was probably a bad pick - something like "channel_list_enum" might be better from that perspective. Guess might as well tweak that before someone started using it...

[mk-fg]

something like "channel_list_enum" might be better from that perspective.

Oh wait, there's also channelcount, so guess channel is the common prefix for these, so nevermind. I think I'll still call "channel_map" a "channel_list_raw", to try and avoid someone using it in code accidentally, instead of "channel_list" (e.g. as if s.channel_map[0] == 1: and such non-obvious stuff, when there's a better alternative).

[mk-fg]

Ended up renaming "channel_pos" to "channel_list_enum" in f081c29 anyway, to hopefully make it more obvious how the two relate to each other, and restored "channel_list_raw" (aka "channel_map" from original merge). Bit verbose for my liking, but hopefully more obvious what the three are for and how to use those.

[mhthies]

Performance-wise, imo it shouldn't be a concern with python, as pretty sure I've never actually profiled this code, and if you do so, there'd likely be something that overshadows all these petty conversions by 1000-to-1 at least, so that thinking about optimizing those is kinda pointless.

Yes, you're probably right. But … it just feels wrong to convert the strings vom channel_list back to integers. :D

But wouldn't it be strange to have an obvious "use this thing idiomatic here" and then behind the scenes make that a second-class attribute, instead of having it a first-class like it currently is, and leaving any second-class conversions for a way more niche use-cases (e.g. custom C interfaces)?

My thought was making the list of position enum members the first-class attribute, as it's the most idiomatic representation to my mind. All other representations (such as the channel_list as a list of strings) would only be generated as second-class attributes for backwards compatibility.

But that may also be way too over-engineered for the niche use cases of this attribute. :D So, I'm fine with the current solution. Thanks again for all the time you invested on this rather unimportant feature.