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

Allow empty or 'none' as audio.mixer value #1015

Merged
merged 1 commit into from Mar 13, 2015

Conversation

3 participants
@ZenithDK
Contributor

ZenithDK commented Feb 28, 2015

To disable mixing altogether, you can now let the configuration value
audio/mixer be empty or set it to 'none'.

Fixes #936

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch from c572eaf to 22ac2d8 Feb 28, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Looks good, only change I want to suggest is that we not make the config value optional. Better to have it explicitly be set to none IMO.

Also did you do any quick hands on testing with MPC or some other client with the mixer set to none or are we just assuming the unit tests cover not having a mixer set well enough?

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Mar 1, 2015

I tested with a web client and while I can see the requests going in via JSON, nothing changed in the model and the volume stays the same.
I will change to making it explicit and not optional.
I would like to implement unit tests for none but I'm unsure of how I would go about that.

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

For core and the WS case you can add a new test class to https://github.com/mopidy/mopidy/blob/develop/tests/core/test_mixer.py

For the MPD case the test cases you probably want to create a new no-mixer-testcase looking at the following tests for inspiration https://github.com/mopidy/mopidy/blob/develop/tests/mpd/protocol/test_playback.py#L81

And https://github.com/mopidy/mopidy/blob/develop/tests/mpd/protocol/__init__.py#L35 probably needs to be updated to have a way to opt out of having a mixer by setting some attribute or something.

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch from 22ac2d8 to aeca749 Mar 1, 2015

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Mar 1, 2015

Fixed

class CoreMixerNone(unittest.TestCase):
def setUp(self): # noqa: N802
self.mixer = None

This comment has been minimized.

@adamcik

adamcik Mar 1, 2015

Member

Might as well just inline the None in this case.

@@ -80,3 +80,19 @@ def assertNotInResponse(self, value): # noqa: N802
def assertEqualResponse(self, value): # noqa: N802
self.assertEqual(1, len(self.connection.response))
self.assertEqual(value, self.connection.response[0])
class BaseTestCaseMixerNone(BaseTestCase):

This comment has been minimized.

@adamcik

adamcik Mar 1, 2015

Member

Don't bother with using the BaseTest pattern here as we only have the one test case using this setup logic.

What I was thinking of was more something like the following snippet, mainly because it saves you having to re-implement the rest of the setup method.

class BaseTestCase(unittest.TestCase):
    enable_mixer = True

    def setUp(self):  # noqa: N802
        if self.enable_mixer:
            self.mixer = dummy_mixer.create_proxy()
        else:
            self.mixer = None
def test_setvol_min(self):
self.send_request('setvol "0"')
self.assertEqual(None, self.core.mixer.get_volume().get())
self.assertInResponse('OK')

This comment has been minimized.

@adamcik

adamcik Mar 1, 2015

Member
setvol 0
ACK [52@0] {setvol} problems setting volume
@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Did a quick test with an MPD without a mixer and go the following:

setvol 0
ACK [52@0] {setvol} problems setting volume
status
volume: -1
repeat: 0
random: 0
single: 0
consume: 0
playlist: 2
playlistlength: 14
mixrampdb: 0.000000
state: stop
song: 0
songid: 1
nextsong: 1
nextsongid: 2

@jodal jodal added this to the v0.20 - Audio cleanup 1 milestone Mar 1, 2015

@jodal jodal added the 2 - Working label Mar 1, 2015

selected_mixers = [
m for m in mixer_classes if m.name == config['audio']['mixer']]
if len(selected_mixers) != 1:
logger.error(
'Did not find unique mixer "%s". Alternatives are: %s',
config['audio']['mixer'],
', '.join([m.name for m in mixer_classes]))
', '.join([m.name for m in mixer_classes]) + ', none' or
'none')

This comment has been minimized.

@jodal

jodal Mar 1, 2015

Member

Maybe a bit clever, but I'll just add the trick to my book and it will become familiar ;-)

This comment has been minimized.

@ZenithDK

ZenithDK Mar 1, 2015

Contributor

It was that or a much larger rewrite :-) Hackish, but working ;)

@jodal

This comment has been minimized.

Member

jodal commented Mar 1, 2015

Looks good to me. Nothing to add to @adamcik's comments.

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Mar 1, 2015

@adamcik: I need to change the handling of volume with the none mixer to return an error in the ACK?
Also, not sure how set enable_mixer to False in your code before the setUp() function is called?

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Something like:

class FooTest(Base...):
    enable_mixer = False

    def test....(...):
        ....

The mpd volume handling needs to handle convert a None volume to -1 to cover the easy one first. For the other setting the volume we probably need to update the core APIs to return True/False so we can return an MPD error when appropriate.

I also just remembered that we have mute handling hackend into "outputs", we might want to test that as well.

Sorry for turning this into a yak shaving exercise :(

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch from aeca749 to 7e579aa Mar 8, 2015

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Mar 8, 2015

Updated the API to return True/False and implemented the MPD error and test cases.
Also implemented support for toggleoutput and added tests for that. I will probably look at increasing test coverage some more, py.test seems pretty good, I like how it displays the results.

@@ -306,13 +309,18 @@ def get_mixer_class(self, config, mixer_classes):
'Available Mopidy mixers: %s',
', '.join(m.__name__ for m in mixer_classes) or 'none')
if config['audio']['mixer'] == 'none':
logger.debug("Mixer disabled")

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

Prefer single quotes

@@ -21,7 +22,9 @@ def get_volume(self):
The volume scale is linear.
"""
if self._mixer is not None:
if self._mixer is None:
return -1

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

The behavior here no longer matches the docstring. Why not continue to return None if the volume is unknown?

@@ -31,8 +34,10 @@ def set_volume(self, volume):
The volume scale is linear.

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

This docstring must be updated with the return values.

This comment has been minimized.

@ZenithDK

ZenithDK Mar 9, 2015

Contributor

Fixed all of the docstring updates in latest update.

@@ -48,5 +55,10 @@ def set_mute(self, mute):
:class:`True` to mute, :class:`False` to unmute.
"""

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

This docstring must be updated with the return values.

@@ -23,7 +23,7 @@
LINE_TERMINATOR = '\n'
#: The MPD protocol version is 0.17.0.

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

Comment doesn't match next line.

This comment has been minimized.

@ZenithDK

ZenithDK Mar 8, 2015

Contributor

Reverted this, as I am not sure if increasing it has any unintended side effects with some clients, and it was for testing why "changed: outputs" suddenly stopped working with the none mixer.

@@ -397,7 +397,8 @@ def setvol(context, volume):
- issues ``setvol 50`` without quotes around the argument.
"""
# NOTE: we use INT as clients can pass in +N etc.
context.core.mixer.set_volume(min(max(0, volume), 100))
if context.core.mixer.set_volume(min(max(0, volume), 100)).get() is False:

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

What about this instead?

success = context.core.mixer.set_volume(min(max(0, volume), 100)).get()
if not success:
    # ...

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

Maybe even extract volume = min(max(0, volume), 100) as a variable of its own.

self.core = core.Core(None, backends=[])
def test_get_volume(self):
self.assertEqual(self.core.mixer.get_volume(), -1)

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

I think this should return None?

def test_set_mute(self):
self.core.mixer.set_mute(True)
self.assertEqual(self.core.mixer.set_mute(True), True)

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

The test name should tell more about what behavior you expect here. Currently, it is only defined in the implementation, not in tests or docstrings.

@@ -150,6 +150,14 @@ def test_replay_gain_status_default(self):
self.assertInResponse('OK')
self.assertInResponse('off')
def test_mixrampdb(self):
self.send_request('mixrampdb "10"')
self.assertInResponse('OK')

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

Shouldn't we return a not implemented error here?

def test_mixrampdelay(self):
self.send_request('mixrampdelay "10"')
self.assertInResponse('OK')

This comment has been minimized.

@jodal

jodal Mar 8, 2015

Member

And here?

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch from 7e579aa to 3f693b8 Mar 8, 2015

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch from 3f693b8 to 0ae95e8 Mar 8, 2015

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Mar 8, 2015

Updated with more tests and reworking here and there.
I updated the DummyMixer so that it also calls the MixerListener events now so we can test for this and made it return the proper return codes since I updated that per @adamcik's comments.

@@ -43,7 +42,11 @@ def toggleoutput(context, outputid):
Turns an output on or off, depending on the current state.
"""
pass
if outputid == 0:

This comment has been minimized.

@adamcik

adamcik Mar 8, 2015

Member

Cool that you are fixing this, but next time I would make it a separate commit within the PR as this is a distinct change from the main thing you are changing in the PR.

@@ -46,11 +45,10 @@ def mixrampdb(context, decibels):
volume so use negative values, I prefer -17dB. In the absence of mixramp
tags crossfading will be used. See http://sourceforge.net/projects/mixramp
"""
pass
raise exceptions.MpdNotImplemented # TODO

This comment has been minimized.

@adamcik

adamcik Mar 8, 2015

Member

Same goes for these, that is I would have preferred that these be a separate commit with just "Make mixramp commands raise not implemented" instead of squashing everything into a single commit. And just to be clear I don't expect you to fix this for this PR/commit, just keep it in mind next time around :-)

@@ -57,3 +57,6 @@ def test_listener_has_default_impl_for_mute_changed(self):
def test_listener_has_default_impl_for_seeked(self):
self.listener.seeked(0)
def test_listener_has_default_impl_for_current_metadata_changed(self):

This comment has been minimized.

@adamcik

adamcik Mar 8, 2015

Member

Heh, this will likely break with my other changes I have underway, guess we have to see who of us can get the change in first :P

@@ -113,6 +116,9 @@ v0.20.0 (UNRELEASED)
- Switch the ``list`` command over to using
:meth:`mopidy.core.LibraryController.get_distinct`. (Fixes: :issue:`913`)
- Add support for ``toggleoutput`` command. ``mixrampdb`` and ``mixrampdelay``
have also been added but have not been implemented yet.

This comment has been minimized.

@adamcik

adamcik Mar 9, 2015

Member

This is a bit of a nitpick, but I wouldn't write "not been implemented yet" as it implies we might add support for it. At this time I have no intention of adding Gnonlin as a dependency just so people can get crossfaded songs.

This comment has been minimized.

@ZenithDK

ZenithDK Mar 9, 2015

Contributor

"The mixrampdb and mixrampdelay commands are now supported but throw a NotImplemented exception"?

if self._mixer is not None:
self._mixer.set_mute(bool(mute))
if self._mixer is None:
self._mute = mute

This comment has been minimized.

@adamcik

adamcik Mar 9, 2015

Member

Why do we track the mute state when there is no mixer?

This comment has been minimized.

@ZenithDK

ZenithDK Mar 9, 2015

Contributor

Because you can mute the output - and the state is needed when get_mute is called. The mute_changed event could get the value directly from the parameters, though.

This comment has been minimized.

@ZenithDK

ZenithDK Mar 9, 2015

Contributor

I got "output" (from MPD) mixed up with "mute" in mopidy - so I'm removing this and just returning False always. The mpd frontend will translate this into an exception.

self.assertNoEvents()
self.assertOnceInResponse('changed: player')
self.assertOnceInResponse('OK')

This comment has been minimized.

@ZenithDK

ZenithDK Mar 9, 2015

Contributor

A copy-paste error here, should be "output" of course. Fixed that in latest commit.

self.assertNoEvents()
self.assertNoSubscriptions()
self.assertOnceInResponse('changed: output')
self.assertOnceInResponse('OK')

This comment has been minimized.

@ZenithDK

ZenithDK Mar 9, 2015

Contributor

As we don't want to be able to mute/unmute (or enable/disable in MPD speak) - I have removed this as there should not be any changed: output events sent any longer.

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch 2 times, most recently from 11e2de6 to 5208489 Mar 9, 2015

Allow 'none' as audio.mixer value
To disable mixing altogether, you can now set the configuration value
 audio/mixer to 'none'.

@ZenithDK ZenithDK force-pushed the ZenithDK:feature/mixer_none branch from 5208489 to cb19b2c Mar 9, 2015

@ZenithDK

This comment has been minimized.

Contributor

ZenithDK commented Mar 9, 2015

Updated changelog and removed tracking of the mute status and cleaned up the testcases some more, and fixed a few bugs in the testcases.

adamcik added a commit that referenced this pull request Mar 13, 2015

Merge pull request #1015 from ZenithDK/feature/mixer_none
core/mpd: Allow empty or 'none' as audio.mixer value

@adamcik adamcik merged commit 083ec13 into mopidy:develop Mar 13, 2015

2 checks passed

Scrutinizer 33 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jodal jodal added 3 - Done and removed 2 - Working labels Mar 13, 2015

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