Recorder options handling

[HotwheelsSisyphus]

Sorry for this wall of text. All of this is more complicated to explain that to implement.

Looking over the docs for the Recorder object, I have some other suggestions for changes to the class library code. They have to do with consistency in the naming of methods and arguments and the role of the defaults. (Very marginal gains there I think but oh well)

They have to do with four instance variables that I'll collectively refer to as "recorder options":recHeaderFormat, recSampleFormat, recBufSize, recChannels/numChannels. These currently exists in two (three) distinct places: ServerOptions and Recorder (and Server, where those variables forward to the eponymous variables in the servers .options, i.e., its ServerOptions instance). This can be confusing, as it's not always clear which set of options takes precedence and when. If I were to just document the current state of affairs, this would probably end up being kind of convoluted.

In Recorder.sc this looks like this:

	recHeaderFormat { ^recHeaderFormat ? server.recHeaderFormat }
	recSampleFormat { ^recSampleFormat ? server.recSampleFormat }
	recBufSize { ^recBufSize ?? { server.recBufSize } ?? { server.sampleRate.nextPowerOfTwo } }

numChannels is a special case. It doesn't have an explicit setter/getter, but it does get set in

 prepareForRecord { | path, numChannels |
		numChannels = numChannels ? server.recChannels;
        ... do some stuff ...
        this.numChannels = numChannels;
		}

Finally, the options in the Server class simply forward to its own options instance variable, a ServerOptions:

	/* recording formats */

	recHeaderFormat { ^options.recHeaderFormat }
	recHeaderFormat_ { |string| options.recHeaderFormat_(string) }
	recSampleFormat { ^options.recSampleFormat }
	recSampleFormat_ { |string| options.recSampleFormat_(string) }
	recChannels { ^options.recChannels }
	recChannels_ { |n| options.recChannels_(n) }
	recBufSize { ^options.recBufSize }
	recBufSize_ { |n| options.recBufSize_(n) }

---

As for the proposed changes:

1. is straightforward: because recChannels (in Server and ServerOptions) provides the default for numChannels in Recorder, it would be good to name this consistently: i.e., rename Recorder.numChannels to Recorder.recChannels. But Recorder.numChannels can stay around as an alias (maybe deprecated if people agree on that). I.e. this need not be a breaking change.

2. Change the way Recorder initializes these options. Currently, the Recorder's recHeaderFormat, recSampleFormat, recBufSize variables are set from the defaults in server (which in turn simply forward to server.options) the first time prepareForRecord is called, unless they have been explicitly set before that time, or called from within some other instance method (e.g. makePath). numChannels is even more confusing: because prepareForRecord takes an optional numChannels argument as well, the order is as follows: argument ? server.options.numChannels, which means that if numChannels is already set in the recorder, this is actually ignored at this stage, and simply overwritten either with the argument or the server/server.options variable.

   I think this confusion is because of the shortcut s.record (and similar), which internally forwards to s.recorder.record, but makes it not terribly obvious whether the recording and the related options are the Server object's or the Recorder object's business. I propose to simplify as follows (but see exception re recBufSize at the bottom): When a new Recorder object is created (and only then), take the defaults for the recorder options from server.options (including numChannles/recChannels) When prepareForRecord is called, use whatever options are in the Recorder object, not those that are in the server or server.options Change the forwarding of these variables in Server such that they forward to server.recorder rather than server.options; that way, all the recorder related methods in Server forward to its Recorder.

   The idea is to reduce complexity: treat all four variables the same; and treat all the Server methods that involve the recorder the same, i.e., forward them all to Recorder.

What would this break? If I've thought this through correctly, the only situation where this would change behavior is if one were to

1. Have recorder call prepareForRecord, or otherwise have already set the Recorder instance variables (say, recBufSize)
2. Explicitly set server.options.recBufSize (which is currently undocumented) instead of server.recBufSize
3. Rely on that change to propagate to the existing recorder when prepareForRecord is next called. Currently, this would propagate; with the proposed change, it wouldn't.

---

Exception re: recBufSize. = the number of frames for the Buffer in the DiskOut in the recording synth. Note that this memory is only actually allocated once prepareForRecord is called, and deallocated when the recording is done. Because this is currently set to default so server.sampleRate.nextPowerOfTwo, and sampleRate is nil until the server is booted, this cannot strictly speaking be set on initialization of the Server object in the language... so I inserted a doWhenBooted. But I wonder if this could not also simply default to some integer, e.g. 2^16... this could then be overwritten as needed both in the server.options and the Recorder. DiskOut.schelp suggests 2^18 ( 262144, which is 192000.nextPowerOfTwo, and roughly 6s in 44.1k, 1.3s in 192k; I don't know how much that is in bytes (262144 * 8 = 3MB presumably?), and I also don't know whether the DiskOut buffer has maybe become less important with SSDs. A lot of the recorder related docs seem pretty ancient.

[HotwheelsSisyphus]

Thanks for the review! I left the converstation re recBufSize open, just in case someone else wants to chime in.

[telephon]

Just a note: when I wrote Recorder, I consciously called numChannels so, because it would be redundant to call them recNumChannels: we are already in a recorder.

Also, the idea was that you could start a recorder with different setting than the server – Recorder should be decoupled from Server from the perspective of Recorder. The server makes a default Recorder so the server is responsible for supplying the correct values.

This was my reasoning, just so it is clear the current design is not made out of carelessness.

[HotwheelsSisyphus]

Also, the idea was that you could start a recorder with different setting than the server – Recorder should be decoupled from Server from the perspective of Recorder. The server makes a default Recorder so the server is responsible for supplying the correct values.

I see! In that light, it would be good to hear your thoughts on the proposed changes (good? bad? pointless?). What I was trying to do was just to make that encapsulation clearer, also with a view to maybe potentially having multiple Recorder instances at once on the same server. E.g., record in flac and wav simultaneously, who knows.

As I see it, what my changes effectively do is move the level from which defaults are taken: If the defaults for a new recorder were previously taken from, say, server.recHeaderFormat, they are now taken form server.options.recHeaderFormat. Both levels of default still exist, but where previously server.recHeaderFormat and server.options.recHeaderFormat were aliases, now server.recHeaderFormat and server.recorder.recHeaderFormat are aliases of each other. Then one can think of all the recorder-related methods in server as simply relating to the default recorder object that lives in server.recorder, and only to that one; which makes it easier (in theory) to manage multiple recorder instances without them accidentally changing settings on each other.

[telephon]

What I was trying to do was just to make that encapsulation clearer, also with a view to maybe potentially having multiple Recorder instances at once on the same server. E.g., record in flac and wav simultaneously, who knows.

Oh, is this not possible? I thought that you could make any number of Recorder instances and just set their properties. Any property set explicitly should not be taken from the server options?

[HotwheelsSisyphus]

record flac and wav simultaneously, who knows. Oh, is this not possible?

Apologies, you are right, this is indeed possible... I used the wrong example there. The main example would be numChannels/recChannels, which always took the default from the server in prepareForRecord, so you wouldn't be able to record, say, a mono dry and a multichannel reverb/wet simultaneously without always providing a numChannels argument (which raises the question as to why there would be a numChannels instance variable in the first place). This could technically be changed independently of the other proposed changes concerning the recorder options, in case you disagree with the latter.

---

More re the general motivation... which probably had more to do with thinking about documentation than with the actual use case. I thought of the intended behavior (after the PR) like that:

ServerOptions (the class)	provides the defaults for	options.recOption in new servers;
s.options.recOption	provides the defaults for	recOption in new recorders on s (including the default recorder in s.recorder)
s.recorder.recOption or Recorder(someOtherServer).recOption	provide the defaults for	new recordings (!) via that recorder (i.e., what file/buffer gets created in prepareForRecord)
arguments to s.recorder.prepareForRecord	can override (some of ) those defaults

Two things to note in this proposed layout:

• First, there isn't any distinct class that would receive defaults froms.recOption as opposed to s.options.recOption - that (and backward compatibility) is why I opted to simply redirect those to s.recorder.recOption.
• Second, this rests on the assumption that one might want to do both: 1. use mutliple Recorders simultaneously, on the same servers, 2. change settings in one Recorder successively, as the present documentation implies. (Another possible approach would be to say that instead, a new Recorder object gets generated for each change in settings.)

[telephon]

I am happy if you can fix it if something is wrong. What I don't understand yet is what is wrong with the way it currently runs?

s.boot;

a = Recorder(s);
b = Recorder(s);

b.recHeaderFormat // default "wav", from s.options
b.recHeaderFormat = "aiff";

[a,b].do(_.prepareForRecord);
[a,b].do(_.record(duration: 1));

[HotwheelsSisyphus]

Again, the one thing that is genuinely "wrong" in my opinion is this:

a = s.recorder;
b = Recorder(s);

a.numChannels = 1;
b.numChannels = 5;
a.prepareForRecord();
a.numChannels // -> 2 (or whatever is in s.recChannels)
b.prepareForRecord();
b.numChannels // -> 2 

The other stuff isn't "wrong" but just not ideal in terms of documentation and probably, maintenance (as I am noticing now). Will expand on that after the meeting!

[HotwheelsSisyphus]

@telephon, just to follow up (btw I don't consider this to be urgent at all):

1. For the "wrong" behavior of the numChannels variable, see the last post;

2. The other thing that seemed strange or inconvenient (not necessarily "wrong") to me was this:

s.recOption_ and s.recorder.recOption_ currently both do nearly the same thing, namely set the recOption for the next time .prepareForRecord is called; except that s.recOption_ will only have an effect if s.recorder.recOption has not already been set. It seems like a slightly unintuitive way to handle defaults, because the effect of s.recOption_ unnecessarily depends on something outside that method, and will only become apparent at a later point in time (i.e., once prepareForRecord is called). The problem as I see it, from a useability standpoint, is that the recorder-related methods of Server objects and the methods of their Recorder object are neither fully distinct nor fully identical. I think this is in fact unnecessary, and it would be better to fix the implementation here than document all the exceptions.

What I proposed (see the table above) is a straightforward hierarchy of defaults, with an eye to maximum backward compatibility (and that backward compatibility is the reason I suggested simply forwarding all the s.recOptions to s.recorder.recOption). As mentioned already, it changes behavior only in one very specific case:

What would this break? If I've thought this through correctly, the only situation where this would change behavior is if one were to

Have recorder call prepareForRecord, or otherwise have already set the Recorder instance variables (say, recBufSize) Explicitly set server.options.recBufSize (which is currently undocumented) instead of server.recBufSize Rely on that change to propagate to the existing recorder when prepareForRecord is next called. Currently, this would propagate; with the proposed change, it wouldn't.