From 1ba76765c3d1e0b728c8d76c82942ca01c6e7786 Mon Sep 17 00:00:00 2001 From: Paul Walker Date: Tue, 7 Jan 2020 21:41:46 -0500 Subject: [PATCH] Fixed to ADSR Analog Mode ADSR analog mode had two bugs 1. It misused _mm_and_ps so was incorrectly rounding decay time 2. It didn't allow for swells in sustain once the sustain stage was underway This fixes both those problems. In some cases the fix to #1 will change the sound of patches by making the decay time slower (but more correct). Closes #1439 Closes #825 --- src/common/dsp/AdsrEnvelope.h | 16 ++- src/headless/UnitTests.cpp | 259 +++++++++++++++++++++++++--------- 2 files changed, 207 insertions(+), 68 deletions(-) diff --git a/src/common/dsp/AdsrEnvelope.h b/src/common/dsp/AdsrEnvelope.h index 30a04a24013..64b8cdfbbab 100644 --- a/src/common/dsp/AdsrEnvelope.h +++ b/src/common/dsp/AdsrEnvelope.h @@ -163,8 +163,13 @@ class AdsrEnvelope : public ModulationSource bool gate = (envstate == s_attack) || (envstate == s_decay); __m128 v_gate = gate ? _mm_set_ss(v_cc) : _mm_set_ss(0.f); + __m128 v_is_gate = _mm_cmpgt_ss( v_gate, _mm_set_ss( 0.0 ) ); - discharge = _mm_and_ps(_mm_or_ps(_mm_cmpgt_ss(v_c1_delayed, one), discharge), v_gate); + // The original code here was + // _mm_and_ps(_mm_or_ps(_mm_cmpgt_ss(v_c1_delayed, one), discharge), v_gate); + // which ored in the v_gate value as opposed to the boolean + discharge = _mm_and_ps( _mm_or_ps(_mm_cmpgt_ss(v_c1_delayed, one), discharge), v_is_gate ); + v_c1_delayed = v_c1; __m128 S = _mm_load_ss(&lc[s].f); @@ -174,7 +179,14 @@ class AdsrEnvelope : public ModulationSource __m128 v_release = v_gate; __m128 diff_v_a = _mm_max_ss(_mm_setzero_ps(), _mm_sub_ss(v_attack, v_c1)); - __m128 diff_v_d = _mm_min_ss(_mm_setzero_ps(), _mm_sub_ss(v_decay, v_c1)); + + // This change from a straight min allows sustain swells + __m128 diff_vd_kernel = _mm_sub_ss(v_decay, v_c1); + __m128 diff_vd_kernel_min = _mm_min_ss(_mm_setzero_ps(), diff_vd_kernel ); + __m128 dis_and_gate = _mm_and_ps(discharge, v_is_gate ); + __m128 diff_v_d = _mm_or_ps(_mm_and_ps(dis_and_gate, diff_vd_kernel), + _mm_andnot_ps(dis_and_gate, diff_vd_kernel_min)); + __m128 diff_v_r = _mm_min_ss(_mm_setzero_ps(), _mm_sub_ss(v_release, v_c1)); // calculate coefficients for envelope diff --git a/src/headless/UnitTests.cpp b/src/headless/UnitTests.cpp index b9a4e8ebd44..7925ec415dc 100644 --- a/src/headless/UnitTests.cpp +++ b/src/headless/UnitTests.cpp @@ -509,7 +509,9 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) auto runAdsr = [surge](float a, float d, float s, float r, int a_s, int d_s, int r_s, bool isAnalog, - float releaseAfter, float runUntil ) + float releaseAfter, float runUntil, + float pushSusAt = -1, + float pushSusTo = 0 ) { auto* adsrstorage = &(surge->storage.getPatch().scene[0].adsr[0]); std::shared_ptr adsr( new AdsrEnvelope() ); @@ -549,6 +551,7 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) adsr->attack(); bool released = false; + bool pushSus = false; int i = 0; std::vector> res; res.push_back(std::make_pair(0.f, 0.f)); @@ -565,6 +568,13 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) adsr->release(); released = true; } + + if( pushSusAt > 0 && ! pushSus && t > pushSusAt ) + { + pushSus = true; + svn(&(adsrstorage->s), pushSusTo); + copyScenedataSubset(&(surge->storage), 0, ids, ide); + } adsr->process_block(); res.push_back( std::make_pair( (float)t, adsr->output ) ); @@ -669,6 +679,7 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) } }; +#if 0 SECTION( "Test the Digital Envelope" ) { for( int as=0;as<3;++as ) @@ -711,7 +722,7 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) if( obs.first > a + d + d * 0.5 && obs.first < holdFor && s > 0.05 ) // that 0.1 lets the delay ring off { - REQUIRE( obs.second < s * s * 1.02 ); + REQUIRE( obs.second == Approx( s * s ).margin( 1e-3 ) ); heldPeriod.push_back(obs.second); } if( obs.first > a + d && obs.second < 1e-6 && zerot == 0 ) @@ -748,7 +759,7 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) REQUIRE( valAtRelEnd < s * 0.025 ); } }; - + testAnalog( 0.1, 0.2, 0.5, 0.1 ); testAnalog( 0.1, 0.2, 0.0, 0.1 ); for( int rc=0;rc<50; ++rc ) @@ -759,91 +770,207 @@ TEST_CASE( "ADSR Envelope Behaviour", "[mod]" ) auto r = rand() * 1.0 / RAND_MAX; testAnalog( a, d, s, r); } - + } + +#endif + // This is just a rudiemntary little test of this in digital mode + SECTION( "Test Digital Sus Push" ) + { + auto testSusPush = [&]( float s1, float s2 ) + { + auto digPush = runAdsr( 0.05, 0.05, s1, 0.1, 0, 0, 0, false, 0.5, s2, 0.25, s2 ); + int obs = 0; + for( auto s : digPush ) + { + if( s.first > 0.2 && obs == 0 ) + { + REQUIRE( s.second == Approx( s1 ).margin( 1e-5 ) ); + obs++; + } + if( s.first > 0.3 && obs == 1 ) + { + REQUIRE( s.second == Approx( s2 ).margin( 1e-5 ) ); + obs++; + } + } + }; + for( auto i=0; i<10; ++i ) + { + auto s1 = 0.95f * rand() / RAND_MAX + 0.02; + auto s2 = 0.95f * rand() / RAND_MAX + 0.02; + testSusPush( s1, s2 ); + } + } + /* ** This section recreates the somewhat painful SSE code in readable stuff */ - SECTION( "Clone the Analog" ) - { - auto analogClone = [](float a_sec, float d_sec, float s, float r_sec, float releaseAfter, float runUntil ) - { - float a = log(a_sec)/log(2.0); - float d = log(d_sec)/log(2.0); - float r = log(r_sec)/log(2.0); + auto analogClone = [](float a_sec, float d_sec, float s, float r_sec, float releaseAfter, float runUntil, float pushSusAt = -1, float pushSusTo = 0 ) + { + float a = limit_range((float)( log(a_sec)/log(2.0) ), -8.f, 5.f); + float d = limit_range((float)( log(d_sec)/log(2.0) ), -8.f, 5.f); + float r = limit_range((float)( log(r_sec)/log(2.0) ), -8.f, 5.f); - int i = 0; - bool released = false; - std::vector> res; - res.push_back(std::make_pair(0.f, 0.f)); - - auto v_c1 = 0.f; - auto v_c1_delayed = 0.f; - bool discharge = false; - const float v_cc = 1.5f; + int i = 0; + bool released = false; + std::vector> res; + res.push_back(std::make_pair(0.f, 0.f)); + + float v_c1 = 0.f; + float v_c1_delayed = 0.f; + bool discharge = false; + const float v_cc = 1.5f; - while( true ) + while( true ) + { + float t = 1.0 * (i+1) * BLOCK_SIZE * dsamplerate_inv; + i++; + if( t > runUntil || runUntil < 0 ) + break; + + if( t > releaseAfter && ! released ) { - auto t = 1.0 * (i+1) * BLOCK_SIZE * dsamplerate_inv; - i++; - if( t > runUntil || runUntil < 0 ) - break; + released = true; + } + + if( pushSusAt > 0 && t > pushSusAt ) + s = pushSusTo; + + auto gate = !released; + float v_gate = gate ? v_cc : 0.f; + + // discharge = _mm_and_ps(_mm_or_ps(_mm_cmpgt_ss(v_c1_delayed, one), discharge), v_gate); + discharge = ( ( v_c1_delayed > 1 ) || discharge ) && gate; + v_c1_delayed = v_c1; + + float S = s * s; + + float v_attack = discharge ? 0 : v_gate; + // OK so this line: + // __m128 v_decay = _mm_or_ps(_mm_andnot_ps(discharge, v_cc_vec), _mm_and_ps(discharge, S)); + // The semantic intent is discharge ? S : v_cc + // but in the ADSR discharge has a value of v_gate which is 1.5 (v_cc) not 1.0. + // That bitwise and with 1.5 acts as a binary filter (the mantissa) and a rounding operation + // (from the .5) so I need to duplicate it exactly here. + /* + __m128 sM = _mm_load_ss(&S); + __m128 dM = _mm_load_ss(&v_cc); + __m128 vdv = _mm_and_ps(dM, sM); + float v_decay = discharge ? vdv[0] : v_cc; + */ + // Alternately I can correct the SSE code for discharge + float v_decay = discharge ? S : v_cc; + float v_release = v_gate; + + float diff_v_a = std::max( 0.f, v_attack - v_c1 ); + float diff_v_d = ( discharge && gate ) ? v_decay - v_c1 : std::min( 0.f, v_decay - v_c1 ); + // float diff_v_d = std::min( 0.f, v_decay - v_c1 ); + float diff_v_r = std::min( 0.f, v_release - v_c1 ); + + const float shortest = 6.f; + const float longest = -2.f; + const float coeff_offset = 2.f - log( samplerate / BLOCK_SIZE ) / log( 2.f ); + + float coef_A = pow( 2.f, std::min( 0.f, coeff_offset - a ) ); + float coef_D = pow( 2.f, std::min( 0.f, coeff_offset - d ) ); + float coef_R = pow( 2.f, std::min( 0.f, coeff_offset - r ) ); + + v_c1 = v_c1 + diff_v_a * coef_A; + v_c1 = v_c1 + diff_v_d * coef_D; + v_c1 = v_c1 + diff_v_r * coef_R; + - if( t > releaseAfter && ! released ) - { - released = true; - } + // adsr->process_block(); + float output = v_c1; + if( !gate && !discharge && v_c1 < 1e-6 ) + output = 0; + + res.push_back( std::make_pair( (float)t, output ) ); + } + return res; + }; - auto gate = !released; - auto v_gate = gate ? v_cc : 0.f; + SECTION( "Clone the Analog" ) + { + auto compareSrgRepl = [&](float a, float d, float s, float r ) + { + auto t = a + d + 0.5 + r * 3; + auto surgeA = runAdsr( a, d, s, r, 0, 0, 0, true, a + d + 0.5, t ); + auto replA = analogClone( a, d, s, r, a + d + 0.5, t ); - // discharge = _mm_and_ps(_mm_or_ps(_mm_cmpgt_ss(v_c1_delayed, one), discharge), v_gate); - discharge = ( ( v_c1_delayed > 1 ) || discharge ) && gate; - v_c1_delayed = v_c1; + REQUIRE( surgeA.size() == replA.size() ); + + for( auto i=0; i 0.2 && obs == 0 ) + { + REQUIRE( s.second == Approx( s1 * s1 ).margin( 1e-3 ) ); + obs++; + } + if( s.first > 0.4 && obs == 1 ) + { + REQUIRE( s.second == Approx( s2 * s2 ).margin( 1e-3 ) ); + obs++; + } + } - float coef_A = pow( 2.f, std::min( 0.f, coeff_offset - a ) ); - float coef_D = pow( 2.f, std::min( 0.f, coeff_offset - d ) ); - float coef_R = pow( 2.f, std::min( 0.f, coeff_offset - r ) ); - v_c1 = v_c1 + diff_v_a * coef_A; - v_c1 = v_c1 + diff_v_d * coef_D; - v_c1 = v_c1 + diff_v_r * coef_R; - - - // adsr->process_block(); - auto output = v_c1; - if( !gate && !discharge && v_c1 < 1e-6 ) - output = 0; - - res.push_back( std::make_pair( (float)t, output ) ); + obs = 0; + INFO( "Analog SSE passes sus push" ); + for( auto s : aSurgePush ) + { + if( s.first > 0.2 && obs == 0 ) + { + REQUIRE( s.second == Approx( s1 * s1 ).margin( 1e-3 ) ); + obs++; + } + if( s.first > 0.4 && obs == 1 ) + { + REQUIRE( s.second == Approx( s2 * s2 ).margin( 1e-5 ) ); + obs++; + } } - return res; }; - auto surgeA = runAdsr( 0.1, 0.2, 0.5, 0.1, 0, 0, 0, true, .5, 1.0 ); - auto replA = analogClone( 0.1, 0.2, 0.5, 0.1, 0.5, 1.0 ); - - REQUIRE( surgeA.size() == replA.size() ); - - for( auto i=0; i