Adding mixing functionality #196

pseeth · 2020-12-15T23:19:35Z

This PR adds convolution reverb functionality to AudioSignal, following discussion in #172. The functionality is:

Convolves signal one with signal two. There are three
cases:

1. s1 is multichannel and s2 is mono.
   -> s1's channels will all be convolved with s2.
2. s1 is mono and s2 is multichannel.
   -> s1 will be convolved with each channel of s2.
3. s1 and s2 are both multichannel.
   -> each channel will be convolved with the matching 
      channel. If they don't have the same number of
      channels, an error will be thrown.

It also adds a method to mix two signals together at a specified signal-to-noise ratio.

Checklist:

Bumped nussl version in setup.py
Updated changelog
Implemented core functionality for convolution
Added test for coverage for convolution
Implemented core functionality for mixing
Added test for coverage for mixing
Tests pass with 100% coverage
Add an impulse response to external file zoo for demo
Add usage example to docstring

nussl/core/mixing.py

ethman

Overall looks good. Comments inline.

ethman · 2020-12-17T22:06:32Z

nussl/core/audio_signal.py

+ ##################################################
+
+ def convolve(self, other, method='auto', normalize=True, 
+ scale=True):


Docs needed for all of these methods

ethman · 2020-12-17T22:08:09Z

nussl/core/constants.py

@@ -102,3 +102,7 @@
 # that use the level_in argument:
 LEVEL_MIN = .015625
 LEVEL_MAX = 64
+
+MIN_LOUDNESS = -70


Maybe add a comment with units.

ethman · 2020-12-17T22:59:22Z

nussl/core/mixing.py

+ direct: The convolution is determined directly from sums, the definition of convolution.
+ fft: The Fourier Transform is used to perform the convolution by calling fftconvolve.
+ auto: Automatically chooses direct or Fourier method based on an estimate of which is faster (default).
+ normalize: Whether to apply a normalization factor which will prevent clipping. Defaults to True.


I think normalize and scale are very similar, and happen at different points in this function, so it'd be nice to have some clarity about the difference between these two args in the docs.

ethman · 2020-12-17T23:00:33Z

nussl/core/mixing.py

+ s1_ch, s2_ch / factor, mode='full', method=method)
+ output.append(convolved_ch)
+ else:
+ for i, s1_ch in enumerate(signal.get_channels()):


Do you need these enumerates()? You're not using the index vars.

ethman · 2020-12-17T23:01:41Z

nussl/core/mixing.py

+ convolved_ch = scipy.signal.convolve(
+ s1_ch, s2_ch / factor, mode='full', method=method)
+ output.append(convolved_ch)
+ else:


Pedantic: Maybe add a comment saying that at least one of these for loops is over 1 item. Wasn't clear to me at first.

ethman · 2020-12-17T23:30:17Z

nussl/core/mixing.py

+ n_loudness = max(MIN_LOUDNESS, bg_signal.loudness())
+ loudness = max(MIN_LOUDNESS, fg_signal.loudness())
+
+ if loudness - snr < MIN_LOUDNESS:


Could this logic be replaced with np.clip()? If not, an explanatory comment would be nice to get an overview.

ethman · 2020-12-17T23:32:16Z

nussl/core/mixing.py

+ bg_signal.zero_pad(0, pad_len)
+ bg_signal.truncate_samples(fg_signal.signal_length)
+
+ n_loudness = max(MIN_LOUDNESS, bg_signal.loudness())


Can you find more descriptive variable names for n_loudness, loudness, and t_loudness? They're hard to keep straight @ line 126 when you do the computation.

ethman · 2020-12-17T23:36:42Z

nussl/core/mixing.py

+ pad_len = max(0, fg_signal.signal_length - bg_signal.signal_length)
+ bg_signal.zero_pad(0, pad_len)
+ bg_signal.truncate_samples(fg_signal.signal_length)
+


Seems like here we should be doing some checks to make sure things like sample_rate, n_channels, & length are OK. Maybe call utils.verify_audio_signal_list_strict([fg_signal, bg_signal]) here.

ethman · 2020-12-17T23:47:14Z

nussl/core/mixing.py

+ fg_signal = copy.deepcopy(fg_signal)
+ bg_signal = copy.deepcopy(bg_signal)
+
+ pad_len = max(0, fg_signal.signal_length - bg_signal.signal_length)


Two things:

I think you should bubble up whether to do min(len(fg), len(bg)) and truncate the longer one or max(len(fg), len(bg)) and pad the shorter one. (I don't think the former is useful, but I figure why not give the option to the user?) I think the opinionated way you've written it now is too subtle and could cause unintended side effects.

Should that be it's own subroutine? Not sure if it's much use outside this function, but maybe def _match_signal_lengths(signal_list, mode='pad') or even something on AudioSignal like signal1.match_length(signal2). Just spitballing, lmk what you think

btw: By "bubble up" I mean add an arg for the user.

ethman · 2020-12-17T23:50:21Z

nussl/core/audio_signal.py

+ normalize=normalize, scale=scale)
+
+ def mix(self, other, snr=10):
+ from . import mixing


I'm assuming this lazy import is to avoid a circular import?

Adding convolution reverb + test.

87e317f

pseeth changed the title ~~Adding convolution reverb function~~ Adding mixing functionality Dec 15, 2020

Adding SNR mixing + test.

ff9c767

ethman reviewed Dec 16, 2020

View reviewed changes

nussl/core/mixing.py Outdated Show resolved Hide resolved

pseeth added 4 commits December 15, 2020 19:27

Moved constants.

882ffac

Adding example usage.

5cd5a2b

Raising atol.

1c91cc9

Raising atol agani.

74c09bb

ethman reviewed Dec 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding mixing functionality #196

Adding mixing functionality #196

pseeth commented Dec 15, 2020 •

edited

Loading

ethman left a comment

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

ethman Dec 17, 2020

Adding mixing functionality #196

Are you sure you want to change the base?

Adding mixing functionality #196

Conversation

pseeth commented Dec 15, 2020 • edited Loading

ethman 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

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

pseeth commented Dec 15, 2020 •

edited

Loading