Skip to content
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

Topic/output limiter via gui #5106

Closed

Conversation

capital-G
Copy link
Contributor

@capital-G capital-G commented Jul 28, 2020

Purpose and Motivation

Implements general limiter as discussed in #4894 so could close #4894

First time I am doing something in SC and C++, so please be kind but do not hold back feedback!

Types of changes

  • Documentation
  • New feature
  • Breaking change (may introduce unexpected behaviour by users who drive past 1.0 on their output)

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

Would love to run tests but I could not figure out what

To run the tests you must first add the testsuite folder, from the main SuperCollider source tree, to your SuperCollider compile paths

as stated here means.

Sidenote: there are no mac binaries for clang-format version 8 - and building it yourself takes some time (~1 hour w/ pulling, setup etc) - maybe its possible to switch to another version or add a hint how to build this yourself on mac and else.

@muellmusik
Copy link
Contributor

Thanks for this.

Note that in the current configuration this will affect recording since the record node by default is added to the tail of the root node. It's a bit outside the scope of this PR, but if we're going to continue with the current approach of nodes after the default group for certain purposes I think we really need a clear and documented specification about how those nodes should be ordered and interact. At the moment it's vague and implicit.

For the current case, I'm not sure the best approach, but perhaps a flag to say whether volume (incl. clip) affects recording would be a good approach. I think this should default to no as the default record sample format is float, so there is no issue with dynamic range and we can always process later. This approach allows that we may want to monitor/limit separately from record levels, without supporting every unlikely possibility (eg. record limit but not volume, etc.).

My instinct as well is that Volume, Recorder and ServerMeter should not handle this ordering, as they should be fairly simple, agnostic where possible, and dumb. This could be done in Server, but as it's already a huge class, a separate manager might be better. ServerUtilities?

@capital-G
Copy link
Contributor Author

Thanks for your feedback!

The reason I considered to affect the recording as well is that I prefer to record exactly what I hear in SC. If I hear harmonic distortion generated by the clipping on the monitoring it should be reflected on the recording as well. I am really too unexperienced in SC for how people use it actually, this is just my opinion and regarding the issue it seems there is a lot of different opinions on this topic. But I am also OK by implementing your suggestion.

How should we proceed?

@joshpar
Copy link
Member

joshpar commented Jul 29, 2020

I also rely on recording to float and then mastering / scaling later. If what I'm hearing is wrong in a live performance, I want to be able to fix it later in a mix, etc. I would be very against anything that doesn't let me capture an unadulterated signal.

@muellmusik
Copy link
Contributor

If I hear harmonic distortion generated by the clipping on the monitoring it should be reflected on the recording as well.

As I understand this, the purpose of this feature is not as an effect, but as a last resort safety feature for ears and speakers. If it's an effect it should be in user code I think, not as a built-in. If it's a safety, it means that if it kicks in something's gone wrong, so I agree with @joshpar that my first preference would be that recordings don't include that accident and can be fixed by scaling.

@jamshark70
Copy link
Contributor

My instinct as well is that Volume, Recorder and ServerMeter should not handle this ordering, as they should be fairly simple, agnostic where possible, and dumb.

Perhaps makeDefaultGroups could create empty groups for volume, recording/scopes (order between multiple recorders and scopes doesn't matter, as these don't change the signal) and safety limiting in that order? Then the components just drop into the right group.

Agreed that safety limiting should not affect recording.

@muellmusik
Copy link
Contributor

Perhaps makeDefaultGroups could create empty groups for volume, recording/scopes (order between multiple recorders and scopes doesn't matter, as these don't change the signal) and safety limiting in that order? Then the components just drop into the right group.

Yeah that could work. Though somehow having possibly unused groups lying around seems a little messy. It might be a good use case for Server:reorder since that could just skip over any non-existing ones.

I wonder if record shouldn't have a separate level control as well. Hmm...

@jamshark70
Copy link
Contributor

jamshark70 commented Jul 31, 2020

Though somehow having possibly unused groups lying around seems a little messy.

There probably would be user questions about "why so many groups" but groups are CPU-cheap and it's completely unambiguous, and would require no complex logic to determine which server components are or aren't there.

reorder is not a bad idea, but that would make it mandatory to fix #3100 (supernova evaluates n_order differently, so any code using reorder is currently compatible with either scsynth or supernova, but not both).

@telephon
Copy link
Member

telephon commented Aug 20, 2020

Perhaps makeDefaultGroups could create empty groups for volume, recording/scopes (order between multiple recorders and scopes doesn't matter, as these don't change the signal) and safety limiting in that order? Then the components just drop into the right group.

We miss a way to give groups an internal precedence, so I think this is a reasonably clear way of doing it in the current system. Then a quark like Safety (which has a bit more features than minimally needed) could use this, which would be an improvement as well.

This seems to be going toward a consensus, right? @joshpar ?
Or is it too messy, @muellmusik ? It would be good to use this to solve the principal problem of course.

Do you think the groups should be just an abstract order (0, 1, 2...) or should they be semantically bound to their functionality (volumeGroup, rawAudioGroup, auditionGroup / volumeGroup, scopeGroup, limiterGroup / or similar ?).

@telephon
Copy link
Member

I could imagine also that we could place the record before the volume, and integrate limiting in the volume group, for simplicity.

@telephon
Copy link
Member

actually I believe that volume shouldn't influence recording!

@joshpar
Copy link
Member

joshpar commented Aug 21, 2020 via email

@telephon
Copy link
Member

My suggestion would be to have a standard recordGroup or rawAudioGroup (better name?) that is inserted always first, directly after the default group. Volume and limiter etc. can add to tail of the root group. The clipping that @capital-G added to the volume can be there without influencing the recording.

If you want an effect on all audio that is part of the recording, it can be added to the tail of the default group or after the default group.

The whole thing is a breaking change, because recordings using int format may now clip, even though the volume is lowered. So recorder should indeed have its own level control.

Does this make sense?

@capital-G
Copy link
Contributor Author

capital-G commented Sep 1, 2020

Thanks for the suggestions!

From my understanding this would result in generating these default ServerGroups at Server.boot time

root node
|
| - - - DefaultGroup
|            | - Synth #1...
|
| - - - UnprocessedAudio
|           | - Recorder
|           | - Scope
|
| - - - ProcessedAudio
|           | - Volume
|
| -> to Output

This means that Recorder, Scope etc. needs not to be added to the Root-Node but to UnprocessedAudio Group, the same for Volume to ProcessedAudio (if this group should be created)

As this seems to me a major breaking change should this be done in this PR by applying some more commits on top?

I have an implementation of the solution described above by creating the 2 groups in Server.makeDefaultGroups but as I am not experienced enough in SC - I don't know how much breaking changes this would introduce and everything that needs to be touched for this (e.g. Scope, docs and so on and so on).

@capital-G
Copy link
Contributor Author

Another question would be how to indicate the function of the group as a "RawAudioGroup" as you can't give a group a name (yet).

The current Group 1 - default group name is created by a hardcoded GUI hack

(node.key == 1).if("- default group", ""),

btw, wouldn't (node.key == this.defaultGroupID).if("- default group", "") be a better solution here?

@jamshark70
Copy link
Contributor

The current Group 1 - default group name is created by a hardcoded GUI hack

Good point. If we have multiple groups created for system purposes, then we'd need a data structure to represent them, with names.

This means that Recorder, Scope etc. needs not to be added to the Root-Node but to UnprocessedAudio Group, the same for Volume to ProcessedAudio (if this group should be created)

Here's where things get tricky.

IMO the recorder should write a signal that is volume controlled but not limited. Probably the scope should behave similarly.

If a user generates a loud signal and relies on the volume control to bring it down to a reasonable range, and then they record it -- do you think they want the very loud (possibly out of range) signal in the file, or the one that is turned down?

If the recorder is in the unprocessed audio group, then they will get a very loud recording (and, if they used an integer recording format, it would suffer clipping even though the volume controlled hardware output was not clipped). I would estimate a near 100% probability of complaints about that.

I'm finding it hard to escape the conclusion that we would need to put the volume control immediately after the sources, then do recording and scoping, and then the safety limiter at the end where the threshold is equal to the set volume.

@telephon
Copy link
Member

telephon commented Sep 2, 2020

IMO the recorder should write a signal that is volume controlled but not limited. Probably the scope should behave similarly.

My intuition was different at the start (and I think @joshpar 's as well) – but you do have a point. Especially when we record float, a lower level also won't make a problem (just as a higher one wouldn't).

So let's assume we don't make this a breaking change, but just make sure the limiter always comes after anything else. Then it may be enough to add one specific group (let's call it "volume group", ID 2), and any recorder is added to the tail of it, while any effects are added to the head.

One important thing is: I suppose we want one such group, and not one for each client?

If yes, then there would be an instance variable in server:

var volumeGroup

this would be initialised in makeDefaultGroups, and added to the tail of the root node.

volume and recorder would add to tail of this group. The limiter (or clipping) would come after it.

@capital-G 's code adds a clipping to the volume itself, which needs to be changed to make this work.
Before we get into details about this, we should make sure we agree on the first thing (above).

@telephon
Copy link
Member

telephon commented Sep 8, 2020

@muellmusik @joshpar @jamshark70 what do you think: should we keep the volume on the recording? And is it reasonable to add a standard group 2?

@muellmusik
Copy link
Contributor

@telephon Well...

It's an interesting conversation. I can see the argument that you might intuitively expect the volume to affect recording.

I think it might help to adopt the principle of designing for the common cases, and then making more exotic ones easy if possible. I think we agree that recording by default should be float and not limited. So...

Common
Scale volume w/wo limiter
Record
Record with scale

Other configs seem less likely. Intuitively one might want to limit recording, but with float that's not needed. The main thing is that this is well documented.

So I think Volume then Record then Limiter is sensible. If Volume and Recorder could be made more flexible to support the exotic applications (e.g. if Volume supported a Group as target instead of just a Server) that would be great. Then individual Volumes per user etc are pretty simple to implement.

I don't mind much about the group structure. It might be good to have a design document specifying how the whole defaultGroups business works and interacts with this. That could help with thinking it through.

Devil's Advocate: Can I just ask, are we sure a clip at the end is really going to improve things or make them more 'safe'? Presumably the D/A convertor clips in any case? These sound more or less identical to me on headphones:

{ SinOsc.ar(mul: 100) }.play;

{ SinOsc.ar(mul: 100).clip }.play;

@muellmusik
Copy link
Contributor

Can I just ask, are we sure a clip at the end is really going to improve things or make them more 'safe'? Presumably the D/A convertor clips in any case? These sound more or less identical to me on headphones:

If I'm correct about this, then a proper limiter might be a better option.

@joshpar
Copy link
Member

joshpar commented Sep 8, 2020 via email

@jamshark70
Copy link
Contributor

Can I just ask, are we sure a clip at the end is really going to improve things or make them more 'safe'? Presumably the D/A convertor clips in any case?

Again, the issue is: the user has turned down the system volume and expects that the system will not allow out any sounds significant louder than this volume level. That is, the user expects the signal reaching the DAC to be well below full scale. But scsynth might generate a multiple-hundred-dB signal which, when multiplied by the system volume control's attenuation factor, is still far above full scale. So the DAC clips to full scale, but this still blows out the user's eardrums because the system was calibrated for signals below full scale.

The system volume control is later in the chain than scsynth, so if scsynth clips to full scale, the final output would be below full scale. This would be an improvement over the current situation in macOS.

@muellmusik
Copy link
Contributor

The system volume control is later in the chain than scsynth, so if scsynth clips to full scale, the final output would be below full scale. This would be an improvement over the current situation in macOS.

Ah, okay, that makes sense. I was checking at full volume.

A limiter would introduce latency, no?

Yes. But in this case, the clip is sensible.

@telephon
Copy link
Member

So I think Volume then Record then Limiter is sensible.
[…]
I don't mind much about the group structure. It might be good to have a design document specifying how the whole defaultGroups business works and interacts with this. That could help with thinking it through.

Yes, we should also try not to break too much, or nothing if possible. So I'll list what the different things currently do:

  • Volume: after default group (=> in the root node)
  • Recorder: tail of root (or, if target is passed, tail of target)
  • SafetyNet Quark: tail of root (or, if specified, tail of default group)
  • MasterEQ Quark, extension in wslib: after root node.

It seems that extensions that try to be after everything will usually refer to the root node (id: 0). One place to put the limiter would be after the root node. But I have the vague memory that this is unspecified and leads to weird things.

All the usual synths start in the default node (id: 1).

The main task is to have a place to safely put the limiter, so that it'll come after all other things that are put in some kind of end position.

@capital-G
Copy link
Contributor Author

Should this rawAudioGroup have a specific ID? I am still unsure b/c it is nice to have a hardcoded ID for this group as it would make the creation of groups more deterministic.
But on the other hand every number would be arbitrary so IMO it just should get an ID from the server and should not be a hardcoded value - also the ID does not need to correlate w/ the order so this will be counterintuitive for users.
Nonetheless - I was surprised that there is no way to give group names and that the default group just has this name b/c of a GUI hack (see above). Maybe it would be a nice feature to have the ability to give group names as this would lead to more self-explanation in this case but this is something for another time/feature.

As we seem to agree on the order

DefaultGroup -> Volume -> Recorder -> SafetyClip

the question remains on how such a SafetyClip should be implemented as I formerly extended the Volume class which is now not sufficient anymore (having two instances of Volume is ambiguous for user and server) so we probably need some new Class for this as we do not want to have too much logic in Server.sc.
In Java it is common to have only one class per file but this dogma seems to be followed a bit more loosely in SC (see e.g. Server.sc, Volume.sc)

So either

  • Create a new file with a new class (but this contradicts the pattern that each file in Supercollider actually is quite big and introduces something new rather than just plumbing existing stuff)
  • Add this to Volume.sc (but this violates the -already violated- Java approach)

Another question which I would be glad to get some recommendations or thoughts on:

  • SafetyClip could share the same but quiet verbose setup of Volume.sc - but having two complex parts that do the same is a code smell and should be therefore refactored to share a common setup
  • Make the setup of SafetyClip less verbose (init and add SynthDef and modify this synth instance) - but I am unsure if this covers all usecases of SuperCollider - there has probably gone some good thought into the setup of Volume.sc

On a sidenote: In Server.sc there is really raw OSC communication happening - shouldn't stuff like this be more abstracted into a class/method/function?

this.sendMsg("/g_new", group.nodeID, 0, 0);

In case you need to change the OSC protocol message you have to touch the code at multiple places and you will only find the occurrences by searching through the code.

@muellmusik
Copy link
Contributor

Should this rawAudioGroup have a specific ID? I am still unsure b/c it is nice to have a hardcoded ID for this group as it would make the creation of groups more deterministic.

As a principle, I am generally opposed to the idea of hardcoded or even permanent IDs, because it's poor design OOP-wise, breaking encapsulation, and encourages brittle, hybrid ways of working.

Ideally I think nothing should have a hardcoded ID that isn't intrinsic to scsynth as a standalone app and its OSC API. That would mean only the root node with 0. Anything else should probably be built at higher level in clients, and in a manner appropriate to the language/use.

Default group, volume nodes, etc. are all sclang things, not built into the server, and normally accessed using higher level constructs like server abstraction objects. There is a precedent for a hard coded ID with the default group, but not a clear one since multi-user scenarios complicate that. (To really be safe you have to go s.defaultGroup.nodeID not just assume 1.)

So, I'd say maybe the group could have a hardcoded ID, but I'd think carefully about that. Can it be done without? The actual processing nodes I'd say definitely not.

@muellmusik
Copy link
Contributor

Ideally I think nothing should have a hardcoded ID that isn't intrinsic to scsynth as a standalone app and its OSC API.

There is of course perhaps a case that clipping could be built into the server and controlled via an OSC command, but that's a separate issue I suppose.

@telephon
Copy link
Member

Yes, we are talking about a standard. If we have, in the sclang library, another default group like discussed here, I think this group should be useful not only for clipping. If it is only really needed because of the clipping, then I'd prefer to have it hardcoded into the server, without a running synth.

But if there are good uses of a standard group – it is very much worthwile establishing.

@telephon
Copy link
Member

In Java it is common to have only one class per file but this dogma seems to be followed a bit more loosely in SC (see e.g. Server.sc, Volume.sc)

This is definitely not a rule in sclang. There are some files that have long series of class definitions, which makes perfect sense.

* Create a new file with a new class

If we end up not hardcoding the clipping in the server, this would be completely OK. But see below.

Another question which I would be glad to get some recommendations or thoughts on […]

The implementation should include a refactoring of Volume (so that it can run relative to any group or target), which can have a common superclass, perhaps SystemSynth (¿?). (in analogy to SystemSynthDefs). Then SafetyClip can be run easily after any group, and it becomes a template for user defined special purpose synths.

@muellmusik
Copy link
Contributor

If it is only really needed because of the clipping, then I'd prefer to have it hardcoded into the server, without a running synth.

I guess one perspective is that if it's needed for safety, then maybe it should be there for all clients. Don't personally have a strong opinion though.

@telephon
Copy link
Member

I guess one perspective is that if it's needed for safety, then maybe it should be there for all clients. Don't personally have a strong opinion though.

yes. Also if it is for safety, it can be a flag at startup.

@capital-G capital-G mentioned this pull request Nov 18, 2020
@mossheim mossheim added this to the 3.12.0 milestone Nov 18, 2020
@telephon
Copy link
Member

Just to mention here that #5110 is merged, and nevertheless this PR is still a useful addition.

@capital-G
Copy link
Contributor Author

I will continuing working on it starting thursday, sorry for the stale development over the last few weeks.

@mossheim
Copy link
Contributor

@capital-G no worries, thanks for taking it on. this PR is very much appreciated. :)

@capital-G
Copy link
Contributor Author

Comming back to this issue it seems that #5110 indeed already achieved the feature of non-exploding outputs by applying a clipping on the server level of the macOS implementation (and this is the only OS where I had issues w/ exploding audio levels).

As applying a sclang limiter does not change the outcome of the signal anymore after #5110 I will close this issue as the implementation of would only introduce breaking changes but no benefits.

@capital-G capital-G closed this Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Output Limiter via GUI
6 participants