Fix Server volume and mute after reboot #3125

patrickdupuis · 2017-08-11T20:51:02Z

This PR is an attempt to fix #3105 & #3110.

Description

The problem is that, on reboot, ServerTree calls its updateFunc (it was added the first time the server booted) before ServerBoot's initFunc is done sending the SynthDef. This causes the "SynthDef not found" error.

Changes proposed

The fix removes the updateFunc from ServerTree when the server quits. That way, on reboot, ServerTree has nothing to call. When initFunc is finished sending the SynthDef, updateFunc gets added to ServerTree.

Also, this.updateSynth needs to be called at the end of sendSynthDef so that the volume synth gets created, if necessary.

This change also makes Server.quit call Volume.freeSynth.

Result

The Server's volume and mute status persist across reboots. No more SynthDef not found errors posted after reboot.

Test

s.volume.volume = -36;
s.boot;
s.queryAllNodes;
/* should see this (if you have 2 outputs)
NODE TREE Group 0
   1 group
   1000 volumeAmpControl2
*/

s.quit;
s.boot;
s.queryAllNodes;
/* should see this (if you have 2 outputs)
NODE TREE Group 0
   1 group
   1000 volumeAmpControl2
*/
a = {SinOsc.ar}.play; // should play at -36db 
a.free;

s.mute;
s.quit;
s.boot;
a = {SinOsc.ar}.play; // no sound, server muted 
s.unmute; // should play at -36db
a.free;

s.quit;
s.options.numOutputBusChannels = 4;
s.boot;
s.queryAllNodes;
/* should see:
NODE TREE Group 0
   1 group
   1000 volumeAmpControl4
*/

This fixes supercollider#2551.

…server quit

patrickdupuis · 2017-08-11T20:51:53Z

I messed up my first attempt at this PR. This is a redo.

patrickdupuis · 2017-08-11T20:52:46Z

Oops... I should just stop now.

patrickdupuis · 2017-08-11T20:53:05Z

Please review :)

mossheim · 2017-08-14T17:37:08Z

would it be possible to add a regression test for this?

nhthn · 2017-08-14T18:29:35Z

SCClassLibrary/Common/Control/Volume.sc

 			}).send(server);

 			server.sync;

+			this.updateSynth;
+
 			if(updateFunc.isNil) {
 				ServerTree.add(updateFunc = {


while we're at it... this use of an assignment as an expression makes me cringe, could you move it out to updateFunc = { }; ServerTree.add(updateFunc)? i'm warming up to brian's policy of cleaning up adjacent code when working on things.

nhthn

i can confirm that this fixes the issues. a unit test would be nice.

telephon · 2017-08-14T18:57:40Z

please do go ahead and change that. I don't think an assignment in an argument should make you cringe, though. It can be a bit obscure. But it is a clear consequence of the fact that every expression returns a value.

but I'm fine with a change of course if it helps readability

patrickdupuis · 2017-08-19T03:17:04Z

I can write a unit test for this in a few days.

patrickdupuis · 2017-08-19T05:06:57Z

Hmmm... with my last commit I get a warning while booting. Everthing seems to work fine though.

SuperCollider 3 server ready.
JackDriver: max output latency 21.3 ms
WARNING: Server internal not running, could not send SynthDef.
Shared memory server interface initialized
Receiving notification messages from server 'localhost'

Looks like SynthDef.send complains that the server isn't running. I don't think this is caused by a mistake on my part. Rather, it's an issue that has become apparent with my last commit.

patrickdupuis · 2017-08-19T05:19:52Z

I made the server sync before creating the SynthDef. Maybe this is a little too hacky? I'm open to improvements.

telephon · 2017-08-19T07:50:32Z

SCClassLibrary/Common/Control/Volume.sc


-			if(updateFunc.isNil) {


I can't test it here, but could you make sure that ServerTree doesn't accumulate these functions?

I don't see any accumulation happening.

telephon

does this mean that you start the server when you change the volume?

patrickdupuis · 2017-09-01T06:22:14Z

No, you can change the volume and mute the server as usual. The server volume mechanism still functions as before, except for the missing SynthDef errors. What gives you the impression that the server will boot when the volume changes?

mossheim · 2017-09-02T03:12:34Z

testsuite/classlibrary/TestVolume.sc

+	var server = Server.default;
+	var volume = Server.default.volume;
+	var level = Server.default.volume.volume;
+	var temp;


seems to me that a temp variable should not be declared at class scope, and probably deserves a better name

you're right. I'll fix that right away.

patrickdupuis · 2017-09-02T05:53:26Z

I've added a UnitTest to this PR. Please have a look.

In order to make the tests, I've had to modify Volume.sc slightly. Volume's ampSynth is now a getter. I've also made the local variable synthNumChans into a global variable called numOutputChannels. It's used in the SynthDef creation and can be used to get the number of channels of the ampSynth. I would have called it numChannels but that is already taken. To me, numOutputChannels is a better variable name. I can change it to synthNumChans if that's better..

LFSaw

looking good. UnitTest is nice to have.

LFSaw · 2017-09-02T22:20:37Z

testsuite/classlibrary/TestVolume.sc

@@ -0,0 +1,40 @@
+TestVolume : UnitTest {
+
+	var server = Server.default;


this creates a parse error. Did you test the UnitTest?

I started to look into this and got this far (see below). However, I got

PASS:a TestVolume:test_setVolume - initial volume is 0 db *** ERROR: SynthDef volumeAmpControl2 not found FAILURE IN SERVER /s_new SynthDef not found PASS:a TestVolume:test_setVolume - Server volume synth exists FAILURE IN SERVER /n_set Node 1000 not found FAILURE IN SERVER /s_get Node 1000 not found ERROR: Message '-' not understood.

now I need to sleep... so leave this to others...

TestVolume : UnitTest { test_setVolume { var server = Server.default; var serverVolume = server.volume.volume; var ampSynthVolume; this.bootServer; this.assert(serverVolume == 0, "initial volume is 0 db"); server.volume.volume = -36; server.sync; this.assert(server.volume.ampSynth.notNil, "Server volume synth exists"); server.volume.ampSynth.get(\volumeAmp, { |level| ampSynthVolume = level }); server.sync; this.assertFloatEquals(ampSynthVolume, serverVolume, "volume level correctly set"); server.volume.reset; server.sync; server.quit; } // [...] }

patrickdupuis · 2017-09-03T05:58:03Z

@LFSaw thanks for looking at this. The tests should work now 👍

LFSaw · 2017-09-03T08:57:36Z

I confirmed with running the UnitTests locally.

mossheim · 2017-09-03T15:40:53Z

thanks for that @LFSaw :)

telephon and others added 4 commits August 11, 2017 16:15

class library: Volume free

30657ba

This fixes supercollider#2551.

added call to updateSynth, remove updateFunc and release ampSynth on …

61473aa

…server quit

moved call to updateSynth

be254d6

renamed to 'freeSynth' & call this.updateSynth first

0b0b85f

patrickdupuis closed this Aug 11, 2017

patrickdupuis reopened this Aug 11, 2017

patrickdupuis mentioned this pull request Aug 11, 2017

Fix Server volume and mute after reboot #3120

Closed

nhthn added the comp: class library SC class library label Aug 12, 2017

nhthn added this to the 3.9 milestone Aug 12, 2017

nhthn changed the base branch from master to 3.9 August 12, 2017 04:46

nhthn self-assigned this Aug 12, 2017

nhthn reviewed Aug 14, 2017

View reviewed changes

nhthn approved these changes Aug 14, 2017

View reviewed changes

assign updateFunc & initFunc explicitly

1d1fa3f

make server sync before creating SynthDef

076b15c

telephon reviewed Aug 19, 2017

View reviewed changes

replace forkIfNeeded with doWhenBooted

af6ea14

telephon reviewed Sep 1, 2017

View reviewed changes

patrickdupuis added 3 commits September 1, 2017 19:49

add initial unit test

2776304

allow getting ampSynth

2ef2d55

improved unit test

fb26c94

mossheim reviewed Sep 2, 2017

View reviewed changes

patrickdupuis added 3 commits September 2, 2017 01:03

reworking of unit test

a0582aa

added numOutputChannels property

68be549

use numOutputChannels in test

44bd300

LFSaw approved these changes Sep 2, 2017

View reviewed changes

LFSaw requested changes Sep 2, 2017

View reviewed changes

fixed unit test

6149fd4

LFSaw approved these changes Sep 3, 2017

View reviewed changes

LFSaw merged commit 79aa79f into supercollider:3.9 Sep 3, 2017

LFSaw mentioned this pull request Sep 3, 2017

[CI] Travis: use repo testsuite instead of CommonTests quark #3132

Closed

patrickdupuis deleted the topic/volume-fix branch September 3, 2017 21:26

mossheim mentioned this pull request Sep 4, 2017

Travis: use repo's unit tests, improve scripts #3161

Closed

patrickdupuis mentioned this pull request Nov 21, 2017

Classlib: Lock client id while running #3275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Server volume and mute after reboot #3125

Fix Server volume and mute after reboot #3125

patrickdupuis commented Aug 11, 2017 •

edited

Loading

patrickdupuis commented Aug 11, 2017

patrickdupuis commented Aug 11, 2017

patrickdupuis commented Aug 11, 2017

mossheim commented Aug 14, 2017

nhthn Aug 14, 2017

patrickdupuis Aug 14, 2017

nhthn left a comment

telephon commented Aug 14, 2017

patrickdupuis commented Aug 19, 2017

patrickdupuis commented Aug 19, 2017

patrickdupuis commented Aug 19, 2017

telephon Aug 19, 2017

patrickdupuis Aug 31, 2017

telephon left a comment

patrickdupuis commented Sep 1, 2017

mossheim Sep 2, 2017

patrickdupuis Sep 2, 2017

patrickdupuis commented Sep 2, 2017 •

edited

Loading

LFSaw left a comment

LFSaw Sep 2, 2017

LFSaw Sep 2, 2017

patrickdupuis commented Sep 3, 2017

LFSaw commented Sep 3, 2017

mossheim commented Sep 3, 2017

		@@ -0,0 +1,40 @@
		TestVolume : UnitTest {

		var server = Server.default;

Fix Server volume and mute after reboot #3125

Fix Server volume and mute after reboot #3125

Conversation

patrickdupuis commented Aug 11, 2017 • edited Loading

Description

Changes proposed

Result

Test

patrickdupuis commented Aug 11, 2017

patrickdupuis commented Aug 11, 2017

patrickdupuis commented Aug 11, 2017

mossheim commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhthn left a comment

Choose a reason for hiding this comment

telephon commented Aug 14, 2017

patrickdupuis commented Aug 19, 2017

patrickdupuis commented Aug 19, 2017

patrickdupuis commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon left a comment

Choose a reason for hiding this comment

patrickdupuis commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdupuis commented Sep 2, 2017 • edited Loading

LFSaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdupuis commented Sep 3, 2017

LFSaw commented Sep 3, 2017

mossheim commented Sep 3, 2017

patrickdupuis commented Aug 11, 2017 •

edited

Loading

patrickdupuis commented Sep 2, 2017 •

edited

Loading