Review sound implementation for Fourier series (6/1/2021)

[pixelzoom]

Sound implementation for the Discrete screen is complete, and there is no sim-specific use of sound in the other 2 screens. So it would be good to review how sound has been implemented, and how it's behaving, so that we can evaluate whether changes need to be made.

I'll need some time from @arouinfar and @jbphet for this -- @arouinfar to evaluate behavior, @jbphet to evaluate behavior & implementation. "Feature complete" milestone is 6/30/21, but I'd like to have this reviewed well before then, to avoid surprises. So... Medium priority, done by 6/1/21 at the latest, eariler preferred.

Some details on the type of feedback that I'm looking for:

Behavior
Anything goes here, but the kinds of things that I'm interested in are:

• Is the output level range OK?
• Is the default output level OK?
• Does output level change as desired/expected as you move the slider?
• Is the balance with other sounds OK? (Ask me to publish a dev version with UI sounds enabled.)
• Does the sound behave as desired with various numbers of harmonics & amplitude values?
• etc.

Implementation
The full implementation is in FourierSoundGenerator.js. Any suggestions for improvements? Any potential conflicts with other aspects of sound?

There are a couple of open questions that came up when I consulted with @jbphet on the implementation, and those can be found as TODO comments in FourierSoundGenerator.js:

//TODO https://github.com/phetsims/fourier-making-waves/issues/45 do we need a limiter to prevent overdrive/clipping?
//TODO https://github.com/phetsims/fourier-making-waves/issues/45 should output level be converted to log scale?

Sound for UI components is currently disabled in fourier-making-waves-main.js:

  soundManager.setOutputLevelForCategory( 'user-interface', 0 );

... and (based on design discussions) is likely to remain disabled for the 1.0 release. Any concerns about that?

[jbphet]

Sound for UI components is currently disabled...and (based on design discussions) is likely to remain disabled for the 1.0 release. Any concerns about that?

@arouinfar - I think more of a "complete" sound design should be done. Otherwise, it will be a bit inconsistent with other sims that have sound enabled. I think the main thing would be to turn on sounds for UI components for the 1st screen, but I haven't looked at the 2nd and 3rd screens. I may be able to get some of @Ashton-Morris's time to assist if needed.

I'll review FourierSoundGenerator.js once this question has been resolved.

[pixelzoom]

I think the main thing would be to turn on sounds for UI components for the 1st screen, but I haven't looked at the 2nd and 3rd screens.

The 2nd and 3rd screens have no sim-specific sound.

[pixelzoom]

UI sounds are currently disabled in main.js, and having to build a version with UI sounds enabled is a pain. Since there is no common-code query parameter support for enabling/disabling categories of sounds (and unlikely to be any support anytime soon), I've added this sim-specific query parameter in FMWQueryParameters.js:

  // Enables sounds for the SoundManager 'user-interface' category.
  // For internal use only.
  uiSoundsEnabled: {
    type: 'boolean',
    defaultValue: false //TODO disabled for 1.0 release?
  }

So to evaluate the behavior of sound for the Fourier series, please run the sim with ?uiSoundsEnabled=true.

[arouinfar]

@arouinfar - I think more of a "complete" sound design should be done. Otherwise, it will be a bit inconsistent with other sims that have sound enabled.

I think this is a bit of a special case, similar to CCK: DC. There is a single pedagogical sound in both of these sims (sine waves in Fourier and a barking dog in CCK). There are also UI elements in these sims that don't have a sound design, so turning on common UI sounds could leave a lot of gaps. Users may also decide that common UI sounds aren't particularly helpful and mute the sim before getting to the important things.

@jbphet happy to discuss further over zoom or in a general sound-related design meeting (where we could also take care of phetsims/build-a-molecule#223).

[jbphet]

I've gone through @pixelzoom's questions and reviewed the behavior and the implementation. Below are my comments.

Behavior

• Is the output level range OK?

I think so. I listened to it and played with the range, and noticed some other potential issues that I'll list below, but I did not hear any distortion on my system, and it didn't get overwhelmingly loud, so I think it's reasonable. This is, of course, a mostly subjective comment. I plan to create a web audio "audio worklet node" to do peak detection and potentially RMS calculations. If and when I get this working, I'll try it out on this and make sure that the peak and RMS levels are reasonable, and this will be a more quantitative analysis.

• Is the default output level OK?

This is pretty subjective, but I think it's a little bit loud, and I would advocate for turning it down just a little. In the sound design testing that we've done so far, people seem to get annoyed by pure sine waves very quickly, so starting off pretty soft and letting them turn it up if desired seems prudent.

• Does output level change as desired/expected as you move the slider?

Kind of, but something seems a little odd. I tried the case where all harmonics were completely cranked up and, as far as I could tell, the output level didn't seem to go up after the slider got to around 20% of the the way to the right. My best theory as to why it behaved this way is that there is a limiter built in to the audio path on my system, and the output levels started to get too high, and the limiter kicked in. I would suggest trying this on a few systems and seeing if the behavior is similar or if distortion ensues. If so, you may need a more dynamic volume scaling algorithm that turns down the output from individual harmonics as the number of harmonics increases.

• Is the balance with other sounds OK? (Ask me to publish a dev version with UI sounds enabled.)

Yes, though again, a little softer for the default would be preferable to me.

PS: I used the uiSoundsEnabled=true query param, so no dev version was needed.

• Does the sound behave as desired with various numbers of harmonics & amplitude values?
  etc.

Yes, except for the note above about appearing to hit some sort of limit.

Implementation

I read through the implementation of FourierSoundGenerator, and it looks quite good to me.

Any potential conflicts with other aspects of sound?

Not that I'm aware of. The sound generators are meant to be pretty independent of one another, and I tried using the sim with the UI control sounds turned on, and it all seemed to work just fine.

Follow Up

We used to have a peak detector in tambo that could analyze the numerical output of audio nodes, but Web Audio deprecated ScriptProcessorNode (for good reason - it ran on the main thread rather than the audio rendering thread), so I had to get rid of it. The discussion above is motivating me to make a new peak detector using AudioWorkletNode. I'll try to carve out the time do so this and, if it's done in time, I'll circle back to this sim and we can more precisely quantify the output levels. This may also help in determining whether a dynamics compressor is needed and, if so, how to configure it.

[jbphet]

@arouinfar - My part on this is done, at least for now. Please assign it back to @pixelzoom once you've done your bit.

[pixelzoom]

@jbphet Thanks for the thorough review! My takeaway is:

• Consider reducing the default output level, which I will defer until @arouinfar has reviewed.
• Consider adding a dynamics compressor, which I will defer until @jbphet has investigated a peak detector using AudioWorkletNode. (Do we need a GitHub issue for this?)

[pixelzoom]

In the above commit, I added logging of the output level. When running with ?log, the value of FourierSoundGenerator will be logged to the browser console, e.g.:

FourierSoundGenerator outputLevel=0.25

This can be used to identify the desired initial level. Start the sim with ?log, set the level where you want it, then tell me the value shown in the console. The initial level is currently 0.25, and the range is [0,1].

[jbphet]

Above, @pixelzoom said:

Consider adding a dynamics compressor, which I will defer until @jbphet has investigated a peak detector using AudioWorkletNode. (Do we need a GitHub issue for this?)

I've created an issue, please see phetsims/tambo#133.

[jbphet]

I now have a working peak detector, and it is showing that the output from this sound generator is quite high. The peak detector itself is pretty crude at this point, and doesn't have much for documentation, but I'm pretty sure it's working. I hooked it up by adding the following lines of code to the end of the FourierSoundGenerator constructor:

    const peakDetector = new PeakDetectorAudioNode();
    this.masterGainNode.connect( peakDetector );

Below is a screen capture where several harmonics have been turned on, and the peak readings are above 5. They should be less than or equal to 1.

We definitely should look at either adding some level compensation to this sound generator or putting a compressor/limiter in the signal chain.

Assigning back to @pixelzoom, and we can discuss it more when he is back in town.

[arouinfar]

Consider reducing the default output level, which I will defer until @arouinfar has reviewed.

I agree with @jbphet here. The default output level seems a bit high. It's not too bad at the default configuration (A1 = 1, everything else = 0). The volume is perhaps slightly louder than other UI components like the ResetAllButton or ComboBox. However, once you start adding in more harmonics it can quickly become pretty loud at outputLevel = 0.25. Starting at outputLevel = 0.1 sounds much better to me. It's still reasonably loud for the starting state and not as overwhelming if sound is turned on when there are multiple non-zero harmonics.

That said, I don't know if this would be addressed by adding level compensation suggested by @jbphet in #45 (comment).

[pixelzoom]

@jbphet and I had a Zoom meeting related to Fourier sound today. We noticed a couple of things related to this issue.

First, soundManger.js already adds a limiter to the master output:

    // The final stage is a dynamics compressor that is used essentially as a limiter to prevent clipping.
    const dynamicsCompressor = phetAudioContext.createDynamicsCompressor();

... and we're not hearing any distortion in the output. So adding an additional limiter on FourierSoundGenerator is unlikely to do anything.

Second, adding harmonics should be expected to increase the output level. And trying to main a uniform output level (either by using a limiter or adjusting outputs programmatically based on number on non-zero harmonics) is probably not desirable from a pedagogical standpoint. So I'm going to lower the per-hamonic output level. And hopefully be able to set the default for the slider to a value that's closer to the middle of the range.

[pixelzoom]

... So I'm going to lower the per-hamonic output level. And hopefully be able to set the default for the slider to a value that's closer to the middle of the range.

Done in the above commit. It sounds much better to my ear.

I tried to compare output levels to what was reported in #45 (comment), but PeakDetectorAudioNode was crashing for me (macOS + Chrome + Safari). @jbphet and I discussed a bit on Slack, he is going to have a look.

[pixelzoom]

@jbphet and I had a Zoom call. We couldn't get PeakDetectorAudioNode to work on macOS, so @jbphet ran it on his Win machine. With the configuration shown in #45 (comment), @jbphet previously reported peak=5.33. With my improvements, peak=0.24. And with all 11 amplitudes set to 1.5 (max), and the audio slider at max, the peak was ~0.8. We have a little headroom here, but I don't think it's essential that we get this closer to 1.0 -- it's plenty loud, much louder (and infinitely more annoying :) than the general UI sounds.

The audio slider's default is now at 50%, which is a much nicer starting point.

I'm going to assign this back to @arouinfar to have a listen.

[pixelzoom]

@jbphet also noticed another subtle issue, which I was not aware of. If a SoundGenerator output level is changed while sound is off, the output level is not immediately changed. And the next time it is turned on, you will briefly hear the old (stale) output level. To reproduce this:

1. Go to the Discrete screen
2. Set all amplitudes to 1.5
3. Turn on Fourier series audio
4. Press the Reset All button
5. Turn on Fourier series audio. You should very briefly hear a loud sound.

This is fixed by calling SoundGenerator setOutputLevel with timeConstant=0. This should only be used when the SoundGenerator is off, otherwise you may hear grain/clicky audio. So in the above commit, this is done conditionally, based on enabledProperty. It seems to be working nicely on macOS + Chrome 91.

[arouinfar]

With my improvements, peak=0.24. And with all 11 amplitudes set to 1.5 (max), and the audio slider at max, the peak was ~0.8. We have a little headroom here, but I don't think it's essential that we get this closer to 1.0 -- it's plenty loud, much louder (and infinitely more annoying :) than the general UI sounds.

The audio slider's default is now at 50%, which is a much nicer starting point.

I'm going to assign this back to @arouinfar to have a listen.

The sound levels are much better now @pixelzoom! A default of 50% seems perfect to me.

I'm also unable to reproduce the bug described in #45 (comment) so @pixelzoom's fix took care of things.

I don't think there's anything else left to here, but since the sound implementation isn't complete, it seems best to leave open and unassigned.

[pixelzoom]

I'm going to close this. It will get a review again as part of #54 (UI sound design), during dev testing, etc.