diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index b786069..ec5c213 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -22,6 +22,15 @@ jobs: python-version: ${{ matrix.python-version }} cache: pip + - name: Install FFmpeg + # tests/test_effect_filters.py renders real audio to verify the filter + # presets. Without FFmpeg those tests skip silently, which would let a + # broken filter through CI. + run: | + sudo apt-get update + sudo apt-get install -y --no-install-recommends ffmpeg + ffmpeg -version | head -1 + - name: Install dependencies run: | python -m pip install --upgrade pip @@ -29,4 +38,5 @@ jobs: - name: Run tests # The suite needs no credentials, no network and no .env. - run: pytest + # -ra surfaces anything that skipped, so silent skips stay visible. + run: pytest -ra diff --git a/cogs/effects.py b/cogs/effects.py index 2608632..5fc3030 100644 --- a/cogs/effects.py +++ b/cogs/effects.py @@ -17,8 +17,12 @@ EFFECTS = { "bass": "equalizer=f=54:width_type=o:width=2:g=5", # gentle low-end lift "bassboost": "equalizer=f=54:width_type=o:width=2:g=10", # heavy low-end lift - "nightcore": "asetrate=48000*1.25,aresample=48000", # +pitch, +speed - "vaporwave": "asetrate=48000*0.8,aresample=48000", # -pitch, -speed + # The leading `aresample=48000` is load-bearing: `asetrate` *reinterprets* a + # stream's declared rate instead of scaling it, so without normalising first + # the speed factor becomes 48000*N/ and differs per track. A + # 44.1 kHz upload ran at 1.36x and a 22 kHz one at 2.72x, not 1.25x. + "nightcore": "aresample=48000,asetrate=48000*1.25,aresample=48000", # +pitch, +speed + "vaporwave": "aresample=48000,asetrate=48000*0.8,aresample=48000", # -pitch, -speed "treble": "equalizer=f=8000:width_type=o:width=2:g=5", # high-end lift "echo": "aecho=0.8:0.88:60:0.4", # short echo "karaoke": "pan=stereo|c0=c0-c1|c1=c1-c0", # cancel centre vocals diff --git a/tests/test_effect_filters.py b/tests/test_effect_filters.py new file mode 100644 index 0000000..108e5b9 --- /dev/null +++ b/tests/test_effect_filters.py @@ -0,0 +1,94 @@ +""" +The FFmpeg filter presets, measured against real FFmpeg. + +The pitch/speed effects are the ones worth measuring: ``asetrate`` +*reinterprets* a stream's declared sample rate rather than scaling it, so a +hardcoded constant silently produces a different speed for every source rate. +Rendering a tone and measuring the output is the only way to catch that - the +filter string looks perfectly reasonable either way. + +Skipped when FFmpeg is not installed. +""" + +import shutil +import subprocess + +import pytest + +from cogs.effects import EFFECTS + +pytestmark = pytest.mark.skipif( + shutil.which("ffmpeg") is None, reason="FFmpeg is not installed" +) + +OUTPUT_RATE = 48000 # what Discord consumes +SOURCE_SECONDS = 2.0 +# Rates real sources actually use: 48 kHz (YouTube Opus), 44.1 kHz (CD-derived +# uploads, much of SoundCloud), and lower ones on older or spoken-word uploads. +SOURCE_RATES = [48000, 44100, 32000, 22050] + + +def rendered_seconds(audio_filter: str, source_rate: int) -> float: + """ + Push a sine through ``audio_filter`` and return how long the result plays. + + Output is raw 16-bit mono at 48 kHz, so duration is just a byte count. + """ + result = subprocess.run( + [ + "ffmpeg", "-hide_banner", "-loglevel", "error", + "-f", "lavfi", + "-i", f"sine=frequency=440:sample_rate={source_rate}:duration={SOURCE_SECONDS}", + "-af", audio_filter, + "-f", "s16le", "-ac", "1", "-ar", str(OUTPUT_RATE), "-", + ], + capture_output=True, check=True, + ) + return len(result.stdout) / (2 * OUTPUT_RATE) + + +def speed_factor(audio_filter: str, source_rate: int) -> float: + """How much faster the filtered audio plays than the source.""" + return SOURCE_SECONDS / rendered_seconds(audio_filter, source_rate) + + +# -- the bug ------------------------------------------------------------ + +@pytest.mark.parametrize("source_rate", SOURCE_RATES) +def test_nightcore_speed_does_not_depend_on_the_source_rate(source_rate): + assert speed_factor(EFFECTS["nightcore"], source_rate) == pytest.approx(1.25, rel=0.02) + + +@pytest.mark.parametrize("source_rate", SOURCE_RATES) +def test_vaporwave_speed_does_not_depend_on_the_source_rate(source_rate): + assert speed_factor(EFFECTS["vaporwave"], source_rate) == pytest.approx(0.8, rel=0.02) + + +def test_the_hardcoded_form_really_was_rate_dependent(): + """ + Documents why the fix exists. The previous filter reinterpreted every source + as 48 kHz, so a 22 kHz upload played at 2.7x instead of 1.25x. + """ + previous = "asetrate=48000*1.25,aresample=48000" + assert speed_factor(previous, 48000) == pytest.approx(1.25, rel=0.02) + assert speed_factor(previous, 44100) == pytest.approx(1.36, rel=0.02) + assert speed_factor(previous, 22050) == pytest.approx(2.72, rel=0.02) + + +# -- every preset must be valid FFmpeg ---------------------------------- + +@pytest.mark.parametrize("name", sorted(EFFECTS)) +def test_every_preset_is_accepted_by_ffmpeg(name): + """A typo in a filter string would only surface as silence at playback.""" + rendered_seconds(EFFECTS[name], 48000) # check=True raises on rejection + + +@pytest.mark.parametrize("name", sorted(set(EFFECTS) - {"nightcore", "vaporwave"})) +def test_non_speed_effects_leave_the_duration_alone(name): + """Only the two pitch effects are meant to change how long a track runs.""" + assert speed_factor(EFFECTS[name], 44100) == pytest.approx(1.0, rel=0.05) + + +@pytest.mark.parametrize("name", sorted(EFFECTS)) +def test_every_preset_produces_audio(name): + assert rendered_seconds(EFFECTS[name], 44100) > 0