New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New mixer API #760

Merged
merged 41 commits into from Jul 16, 2014

Conversation

4 participants
@jodal
Member

jodal commented Jun 22, 2014

For you interested. Still work in progress. Do not merge.

Addresses #665.

@jodal jodal added Audio labels Jun 22, 2014

@jodal jodal added this to the v0.20 - Audio and mixer cleanup milestone Jun 22, 2014

@jodal jodal removed the Core label Jun 22, 2014

@adamcik

This comment has been minimized.

Member

adamcik commented Jun 23, 2014

Should be consider adding a gst mixer and a auto gst mixer perhaps?

@jodal

This comment has been minimized.

Member

jodal commented Jun 23, 2014

If we pass not just config but also audio to the mixer's __init__(), we can have a bundled SoftwareMixer that use mopidy.audio to change the software volume in GStreamer's pipeline. That way, software mixing and hardware mixing will be the exact same thing from the perspective of the core layer.

Adding other GStreamer mixers feels short sighted as they're going way soon anyway.

@adamcik

This comment has been minimized.

Member

adamcik commented Jun 23, 2014

Since we have the code for the gst mixers it just seems trivial to wrap it. Wouldn't need to even be the auto one, just the pare from gst-launch and extract a mixer parts. Gives a simple transition plan as the mixer switch we can get in fast, a switch to 1.x will be more bumpy I'm sure.

@jodal

This comment has been minimized.

Member

jodal commented Jun 23, 2014

I'm hoping that the software mixer will be the safe and useful haven for users while transitioning.

@jodal

This comment has been minimized.

Member

jodal commented Jul 7, 2014

I think this is ready for review.

Not that Mopidy's develop branch is ready to get this merged, as we haven't released 0.19.0 yet.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/mixer.py in 9ca4dae Jul 8, 2014

No properties? Are we changing stance on this, and should be consider changing other APIs next time we break them anyway?

This comment has been minimized.

Owner

jodal replied Jul 8, 2014

I think it's easier to have people implement a couple of getters/setters than properties. The implementor of this API is various extension developers. The user of this API is just Mopidy's core layer, which doesn't need the nicety of properties.

Regarding properties in the core API, I'm open to removing them in preference of the getters/setters, just to align more with the HTTP API which only expose the getters/setters.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/mixer.py in 50d008a Jul 8, 2014

s/usually// IMO

This comment has been minimized.

Owner

jodal replied Jul 9, 2014

Fixed.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/commands.py in 4f53521 Jul 8, 2014

You know we only ever start the selected one, why not just explicitly stop it? Why depend on the registry more than we need to?

This comment has been minimized.

Owner

jodal replied Jul 9, 2014

Extracted the find-configured-mixer code, so we know the mixer class to start/stop before the try/except clause begins. Thus, I now only stop the single mixer that we may have started. I don't want the stop call to depend on the returned mixer object, as we don't know where in the try clause we're going to fail, and thus if mixer is defined yet or not.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/softwaremixer/mixer.py in 14d0433 Jul 8, 2014

I suspect we don't want to give out access to audio to all mixers if it can be avoided. But like I said on IRC I'm not sure how to get this class access to audio cleanly given such a constraint.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/softwaremixer/mixer.py in 14d0433 Jul 8, 2014

Why are we storing config when we don't need it?

This comment has been minimized.

Owner

jodal replied Jul 9, 2014

Fixed.

@adamcik

This comment has been minimized.

adamcik commented on mopidy/commands.py in 93ffde3 Jul 8, 2014

Should this perhaps be moved into the software mixer, or is it useful as a generic feature still?

This comment has been minimized.

Owner

jodal replied Jul 9, 2014

It's useful across all mixers. I want to be able to set the ALSA or external amp volume level to a predefined level when Mopidy is started.

@jodal

This comment has been minimized.

Member

jodal commented Jul 12, 2014

From alsamixer manpage:

In alsamixer, the volume is mapped to a value that is more natural for
a human ear. The mapping is designed so that the position in the
interval is proportional to the volume as a human ear would perceive
it, i.e. the position is the cubic root of the linear sample multipli‐
cation factor. For controls with a small range (24 dB or less), the
mapping is linear in the dB values so that each step has the same size
visually.

Only for controls without dB information, a linear mapping of the hard‐
ware volume register values is used (this is the same algorithm as used
in the old alsamixer).

amixer can expose both values, using -R and -M arguments.

alsamixer implementation of "raw" linear scale to "natural" logaritmic dB scale: http://git.alsa-project.org/?p=alsa-utils.git;a=blob;f=alsamixer/volume_mapping.c;h=1c0d7c45e6686239464e1b0bbc8983ea57f3914f;hb=HEAD

logger.debug('Mixer event: mute_changed(mute=%s)', mute)
MixerListener.send('mute_changed', mute=mute)
def trigger_events_for_any_changes(self):

This comment has been minimized.

@adamcik

adamcik Jul 14, 2014

Member

To me this feels like it doesn't belong in a base class with a minimal API. This could either be moved to core, though that would be a bit more back and forth. Or we could have a state tracker helper which implementations could use to get this behavior.

This comment has been minimized.

@jodal

jodal Jul 14, 2014

Member

Moving it to core doesn't help mixers with emitting mixer events, as they don't know anything about core.

A state tracker helper sounds like something to consider if/when we revamp the event system, like we've been talking about.

This comment has been minimized.

@adamcik

adamcik Jul 14, 2014

Member

Sorry, forgot to add that I'm assuming that the event gets changed to something changed for that case.

changed, and events needs to be sent.
"""
if not hasattr(self, '_last_volume'):

This comment has been minimized.

@adamcik

adamcik Jul 14, 2014

Member

Magically relying on non declared attributes and that nobody else is using them for something else feels fishy. If you really want this it would be a valid case for using the __last_volume type magic which will protect the attr from other classes through mangling.

This comment has been minimized.

@jodal

jodal Jul 14, 2014

Member

Fixed.

This comment has been minimized.

@jodal

jodal Jul 14, 2014

Member

Reason for doing it this way is that I want it to work even if subclass implementors forget to call the superclass' init method.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 14, 2014

That should be it for this time round.

end result should be the same without any changes to this config value.
- Deprecated the :confval:`audio/mixer_track` config value. This config value
is no longer in use. Mixer extensions that needs additional configuration

This comment has been minimized.

@kingosticks

kingosticks Jul 14, 2014

Member

s/needs/need/

jodal added some commits Jul 14, 2014

def set_mute(self, mute):
"""
Mute or unmute of the installed mixer.
Mute or unmute of the software mixer.
:param mute: Wether to mute the mixer or not.

This comment has been minimized.

@kingosticks

kingosticks Jul 14, 2014

Member

/wether/whether/ (while we're here). Fun fact: a 'wether' is a castrated ram!

This comment has been minimized.

@jodal

jodal Jul 14, 2014

Member

I looked that word up in dictionary just yesterday!

This comment has been minimized.

@jodal

jodal Jul 14, 2014

Member

That is, "whether", not "castrated ram" :P

@@ -24,7 +24,7 @@
_audio_schema = ConfigSchema('audio')
_audio_schema['mixer'] = String()
_audio_schema['mixer_track'] = String(optional=True)
_audio_schema['mixer_track'] = Deprecated()

This comment has been minimized.

@kingosticks

kingosticks Jul 14, 2014

Member

Maybe this is another PR... but s/Deprecated/Depreciated/

This comment has been minimized.

@ZenithDK

ZenithDK Jul 14, 2014

Contributor

You must be mistaken on this one - "diminish in value over a period of time."
Whereas "deprecated" is something that is no longer used or will soon be out of use.

Good catches on the other ones, though.

This comment has been minimized.

@kingosticks

kingosticks Jul 14, 2014

Member

And I always thought it was the other one, how embarrassing!

This comment has been minimized.

@ZenithDK

ZenithDK Jul 14, 2014

Contributor

It doesn't matter wether or not you knew, together we can fix anything ;-)

Max volume for given system.
:class:`None`:
No mixer present, so the volume is unknown.
Max volume.

This comment has been minimized.

@kingosticks

kingosticks Jul 14, 2014

Member

/Max/Maximum/ (for consistency with 'Minimum')

0:
Minimum volume, usually silent.
100:
Max volume.

This comment has been minimized.

@kingosticks

kingosticks Jul 14, 2014

Member

/Max/Maximum/ (for consistency with 'Minimum')

Send ``volume_changed`` event to all mixer listeners.
This method should be called by subclasses when the volume is changed,
either because of a call to :meth:`set_volume` or because or any

This comment has been minimized.

@kingosticks
Send ``mute_changed`` event to all mixer listeners.
This method should be called by subclasses when the mute state is
changed, either because of a call to :meth:`set_mute` or because or

This comment has been minimized.

@kingosticks

jodal added some commits Jul 14, 2014

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 14, 2014

I'm still not sold on adding the any value changed helper as part of the API. Other than that this looks good to go.

@jodal jodal modified the milestones: v0.19 - MPD engine rewrite and new web server, v0.20 - Audio cleanup Jul 14, 2014

@jodal

This comment has been minimized.

Member

jodal commented Jul 14, 2014

I think my only problem with that method is the name. What about something like trigger_events_for_changed_values?

@jodal

This comment has been minimized.

Member

jodal commented Jul 15, 2014

I renamed it and moved it into the software helper, so it's no longer part of the public mixer API.

adamcik added a commit that referenced this pull request Jul 16, 2014

@adamcik adamcik merged commit c6d810a into mopidy:develop Jul 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jodal jodal deleted the jodal:feature/mixers branch Jul 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment