SoundFile: fix cue method #3728

patrickdupuis · 2018-05-16T01:50:37Z

This PR introduces a few fixes and documentation improvements to SoundFile's cue method.

This PR fixes a mistake introduces in a previous PR (SoundFile: fix SynthDef and Helpfile to match #3713) by correctly renaming the event key to ~sustainTime
The main fix here is related to the closeWhenDone argument. Previously, this arg had no effect. Now, the OSCFunc correctly fires which closes the buffer ~~and SoundFile~~.
The documentation was improved to indicate the closing of the buffer by closeWhenDone.
bufferSize used to be hardcoded to a value, even though the documentation suggested that it was settable. This control is now settable and the documentation has been improved.
The sample rate is now obtained by server.sampleRate instead of looking it up in server.options.
An unnecessary Condition was removed.
A test for the cue method was added to ~~TestSoundFile~~ TestSoundFile_Server

telephon · 2018-05-16T19:54:57Z

HelpSource/Classes/SoundFile.schelp

@@ -118,7 +118,7 @@ e = f[1].cue( (addAction: 2, group: 1) );	// synth will play ahead of the defaul

 argument:: closeWhenDone

-A flag to indicate wether the code::SoundFile:: will be closed after it's done playing. Default is False.
+A flag to indicate wether the code::SoundFile:: and buffer will be closed after playback is finished. Default is False.


wether -> whether

telephon · 2018-05-16T20:03:36Z

SCClassLibrary/Common/Files/SoundFile.sc

-						server.sendBundle(server.latency, ["/b_close", ev[\bufnum]],
-							["/b_read", ev[\bufnum], path, ev[\firstFrame], ev[\bufferSize], 0, 1]);
+						ev.close;
+						this.close;


if someone adds a method extension:

+ Object { close { ... do something ... } }

the ev.close will break.

better write:

ev[\close].value(ev)

telephon · 2018-05-16T20:05:03Z

SCClassLibrary/Common/Files/SoundFile.sc

 				~firstFrame = ~firstFrame ? 0;
 				~lastFrame = ~lastFrame ? numFrames;
-				~sustain = (~lastFrame - ~firstFrame)/(sampleRate ?? {server.options.sampleRate ? 44100});
+				~sustainTime = (~lastFrame - ~firstFrame)/(sampleRate ?? {server.sampleRate ? 44100});


why sustainTime ? sustain is the standard value for the length of a sound event.

See @jamshark70's comment in #3713 (comment)

ok, makes sense.

mossheim

See notes on tests below. Otherwise LGTM :)

mossheim · 2018-05-26T20:28:14Z

testsuite/classlibrary/TestSoundFile.sc

+
+		// playNow argument
+		cueEvent = soundFile.cue((), true);
+		0.5.wait;


is there any way to wait on something specific here (other than 0.5)? if not, we should add a callback argument to cue.

I'm sure there's a way to avoid waiting. I can look into this test again next week to see if I can improve it.

Formally, the right thing to wait for would be /n_go coming back from the server. But, if you're waiting for that, then the test needs to fail if /n_go doesn't come back. The only way I can think of is to include a timer.

If you don't handle the case of a broken server not sending /n_go back (or any other reply), then the entire test suite hangs at this point. You can use the incoming reply to move forward with the test sooner than the wait time, but you can't get rid of a wait time altogether.

Formally, the right thing to wait for would be /n_go coming back from the server. But, if you're waiting for that, then the test needs to fail if /n_go doesn't come back. The only way I can think of is to include a timer.

Then let's do that please. In the last few months we've established a pattern in tests for dealing with callback timeout - fork a timeout thread that signals a condition variable. Since there is so so much async callback code in SC I think we should probably find a way to simplify it.

mossheim · 2018-05-26T20:33:01Z

testsuite/classlibrary/TestSoundFile.sc

+		Buffer.freeAll;
+		server.quit.remove;
+
+	}


this should be split into several tests.; one for playNow; one for setting controls; and one for closeWhenDone. there should be additional tests for when playNow is set to false, and when closeWhenDone is set to false.

I don't mind separating this into multiple tests. The reason I lumped them together is because they all need the server to be booted. We've already discussed why we don't want to make use of setUp and tearDown to boot the server in this particular UnitTest in a previous PR (#3712 (comment)). Maybe we need to create a separate UnitTest for cue?

Yeah, you could move those tests into a separate TestSoundFile_Cue or TestSoundFile_Server.

patrickdupuis · 2018-05-27T02:18:35Z

See @brianlheim 's comment here #3737 (comment).

It came to my attention when working on this that a SoundFile is not required to be open in order to use cue. @brianlheim is making me think that closeWhenDone is simply bad design. Maybe it should be left unused or removed?

mossheim · 2018-05-29T04:17:22Z

I am definitely in favor of marking it unused. Since it's the final parameter in the parameter list, we could deprecate and remove it later!

patrickdupuis · 2018-05-31T18:08:15Z

It look like the real intention of closeWhenDone is to free the buffer, not the soundfile, when finished playing. I had this idea early on and I wish I had gone with my instinct. Would it be ok to rename this argument freeWhenDone (or similar), document it properly, and remove the this.close from within the OSCFunc?

update: The argument's name can stay as is since the Buffer needs to be closed (not just freed) because it is being used by DiskIn.

jamshark70

In general, OK, apart from a couple of notes.

jamshark70 · 2018-05-26T09:44:14Z

SCClassLibrary/Common/Files/SoundFile.sc

 				~firstFrame = ~firstFrame ? 0;
 				~lastFrame = ~lastFrame ? numFrames;
-				~sustain = (~lastFrame - ~firstFrame)/(sampleRate ?? {server.options.sampleRate ? 44100});
+				~sustainTime = (~lastFrame - ~firstFrame)/(sampleRate ?? {server.sampleRate ? 44100});


Only a minor bit of style-guide fussiness here: generally we recommend spaces around binary ops (here I see spaces around - in the numerator but no spaces around the higher-level / operator), and spaces inside of curly braces.

~sustainTime = (~lastFrame - ~firstFrame) / (sampleRate ?? { server.sampleRate ? 44100 });

jamshark70 · 2018-06-09T02:36:17Z

testsuite/classlibrary/TestSoundFile.sc

+
+		// playNow argument
+		cueEvent = soundFile.cue((), true);
+		0.5.wait;


Formally, the right thing to wait for would be /n_go coming back from the server. But, if you're waiting for that, then the test needs to fail if /n_go doesn't come back. The only way I can think of is to include a timer.

If you don't handle the case of a broken server not sending /n_go back (or any other reply), then the entire test suite hangs at this point. You can use the incoming reply to move forward with the test sooner than the wait time, but you can't get rid of a wait time altogether.

telephon · 2018-06-09T09:16:59Z

You could use a condition with a hang value for time out:

(
c = Condition.new;
fork {
	"started".postln;
	c.hang(2);
	if(c.test) { "was activated" } { "timed out" }.postln;
	
	
}
)


(
c = Condition.new;
fork {
	"started".postln;
	fork { 1.wait; c.test = true; c.signal };
	c.hang(2);
	if(c.test) { "was activated" } { "timed out" }.postln;
	
	
}
)

jamshark70 · 2018-06-09T11:14:09Z

You could use a condition with a hang value for time out.

No, that was found to be unreliable. It doesn't cancel the scheduled wake-up if you unhang or signal the condition, so the thread is scheduled twice: once by unhang, and again by the wait time.

It's a very clever idea. Like a lot of clever programming ideas, it seems to solve a problem without architecting a solution, but it causes other problems along the way.

telephon · 2018-06-09T12:34:42Z

Ah I see. The method hang was never intended to do this, so it is just a "clever idea" in that (harmless) sense.

I am not completely sure it is true though, and why (I remember we had an exchange about this, but I can't find it).

This is a bit off topic, sorry, here is a test:

(
f = { |dt, maxTime, callback|
	var c = Condition.new;
	fork {
		var t0 = thisThread.seconds;
		fork { dt.wait; c.test = true; c.signal };
		c.hang(maxTime);
		callback.(thisThread.seconds - t0);
	}
};
)

(
var c = 0;
(0..20).normalize(0, 1).do { |x, i| f.(x, 0.5, { |val| if(val <= 0.5) { c = c + 1 }; if(i == 19) { c.postln } }) };
)

telephon · 2018-06-09T12:49:50Z

Ok, I see, if you do anything after that callback, you are not guaranteed that hanging another time will succeed. This is because the line fork { dt.wait; c.test = true; c.signal }; schedules past the hang time.

mossheim · 2018-06-09T13:12:04Z

Could this discussion go in the mailing list thread about hang/wait please? That's why I started it.

telephon · 2018-06-09T16:38:19Z

what is the subject of the email? I can't find the thread.

alln4tural · 2018-06-09T18:52:57Z

Testing async code

patrickdupuis · 2018-07-05T23:35:18Z

Please have another look at the tests. I've decided that closeWhenDone only needs to close the buffer. This makes the most sense. I believe the documentation has been update correctly. Thanks 🍰

patrickdupuis · 2018-09-01T04:32:31Z

I've applied these changes to 3.10 locally and all tests pass. @brianlheim please review when you get the chance.

Please squash when merging.

mossheim

2 changes in the tests and then should be all good :) thanks for your patience

mossheim · 2018-09-16T00:43:30Z

testsuite/classlibrary/TestSoundFile_Server.sc

+	test_cue_playNow {
+		var cueEvent, condition = Condition.new;
+
+		OSCFunc({ condition.unhang }, '/n_go', server.addr);


please clean this up at the end of the test in case it doesn't fire.

mossheim · 2018-09-16T00:43:54Z

testsuite/classlibrary/TestSoundFile_Server.sc

+
+		OSCFunc({ condition.unhang }, '/n_go', server.addr);
+		cueEvent = soundFile.cue((), true);
+		fork { 3.wait; condition.unhang };


stop this route at the end of the test so it doesn't fire and unhang a future hang

testsuite/classlibrary/TestSoundFile_Server.sc

mossheim · 2018-09-18T18:50:31Z

Should be picked to 3.10 right @snappizz ?

patrickdupuis · 2018-09-18T18:55:10Z

Oh dear, that's a messy commit history. I should have force pushed a rebased version of this... my bad

Merge #3728 into 3.10

patrickdupuis added 10 commits May 14, 2018 16:58

arg name should be sustainTime

4bd0dd5

fix non-functioning closeWhenDone arg

fc5b018

use sampleRate method to obtain sample rate

5375e0d

closeWhenDone arg should close SoundFile

c51f683

bufferSize should be settable as per help

89fe6f3

remove Condition

0443eed

closeWhenDone should close file, refactoring

51286e7

document CloseWhenDone and bufferSize

50b392b

add test for cue method

9674d1f

test improvements

5a93d9b

patrickdupuis added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. enhancement comp: class library SC class library labels May 16, 2018

patrickdupuis added this to the 3.10 milestone May 16, 2018

patrickdupuis requested review from telephon, mossheim and jamshark70 May 16, 2018 01:51

telephon reviewed May 16, 2018

View reviewed changes

fix wording, improve Event close

5f99254

mossheim suggested changes May 26, 2018

View reviewed changes

jamshark70 reviewed Jun 9, 2018

View reviewed changes

patrickdupuis added 2 commits June 8, 2018 22:52

fix whitespace

009d1dc

remove unused variable

ffb090a

patrickdupuis added 5 commits June 10, 2018 22:15

separate tests based on server usage

c3161c0

refactor test

681c342

cue method should not close soundfile

ad2132b

document cue event close

6098044

remove unused variable

db0dd2e

antonhornquist mentioned this pull request Sep 1, 2018

fix tape play monome/norns#533

Merged

mossheim self-requested a review September 16, 2018 00:39

mossheim suggested changes Sep 16, 2018

View reviewed changes

cleanup OSC and routine

fde8821

mossheim reviewed Sep 18, 2018

View reviewed changes

testsuite/classlibrary/TestSoundFile_Server.sc Show resolved Hide resolved

mossheim approved these changes Sep 18, 2018

View reviewed changes

mossheim merged commit f377b09 into supercollider:develop Sep 18, 2018

patrickdupuis deleted the topic/sf-cue1 branch September 18, 2018 18:55

patrickdupuis mentioned this pull request Sep 19, 2018

Merge #3728 into 3.10 #4069

Merged

mossheim added a commit that referenced this pull request Sep 23, 2018

Merge pull request #4069 from patrickdupuis/topic/cp3728

0307b6f

Merge #3728 into 3.10

joshpar mentioned this pull request Jul 16, 2019

Fix build with boost-1.70 #4491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SoundFile: fix cue method #3728

SoundFile: fix cue method #3728

patrickdupuis commented May 16, 2018 •

edited

Loading

telephon May 16, 2018

telephon May 16, 2018

telephon May 16, 2018

patrickdupuis May 16, 2018

telephon May 16, 2018

mossheim left a comment

mossheim May 26, 2018

patrickdupuis May 27, 2018

jamshark70 Jun 9, 2018

mossheim Jun 9, 2018

mossheim May 26, 2018

patrickdupuis May 27, 2018

mossheim May 29, 2018

patrickdupuis commented May 27, 2018

mossheim commented May 29, 2018

patrickdupuis commented May 31, 2018 •

edited

Loading

jamshark70 left a comment

jamshark70 May 26, 2018

jamshark70 Jun 9, 2018

telephon commented Jun 9, 2018

jamshark70 commented Jun 9, 2018

telephon commented Jun 9, 2018

telephon commented Jun 9, 2018 •

edited

Loading

mossheim commented Jun 9, 2018

telephon commented Jun 9, 2018

alln4tural commented Jun 9, 2018

patrickdupuis commented Jul 5, 2018

patrickdupuis commented Sep 1, 2018

mossheim left a comment

mossheim Sep 16, 2018

mossheim Sep 16, 2018

mossheim commented Sep 18, 2018

patrickdupuis commented Sep 18, 2018

SoundFile: fix cue method #3728

SoundFile: fix cue method #3728

Conversation

patrickdupuis commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdupuis commented May 27, 2018

mossheim commented May 29, 2018

patrickdupuis commented May 31, 2018 • edited Loading

jamshark70 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon commented Jun 9, 2018

jamshark70 commented Jun 9, 2018

telephon commented Jun 9, 2018

telephon commented Jun 9, 2018 • edited Loading

mossheim commented Jun 9, 2018

telephon commented Jun 9, 2018

alln4tural commented Jun 9, 2018

patrickdupuis commented Jul 5, 2018

patrickdupuis commented Sep 1, 2018

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim commented Sep 18, 2018

patrickdupuis commented Sep 18, 2018

patrickdupuis commented May 16, 2018 •

edited

Loading

patrickdupuis commented May 31, 2018 •

edited

Loading

telephon commented Jun 9, 2018 •

edited

Loading