From 19790019c68d1a06fdd818e93f2bd5cda357c9ee Mon Sep 17 00:00:00 2001 From: Ismael Leon Date: Thu, 20 Aug 2026 20:08:08 -0600 Subject: [PATCH] Resume in place when an audio effect changes Asking for a bass boost four minutes into a song threw the listener back to 0:00. An FFmpeg filter chain is fixed for the life of the process, so changing one means respawning the stream, and the respawn always started from the top. The player now tracks playback position and passes it to the new process as an FFmpeg input option, so the effect appears to apply in place. -ss goes in before_options rather than options: as an input option FFmpeg discards packets without decoding them, which on a piped stream costs almost nothing because yt-dlp delivers at network speed rather than in realtime. Measured at 0.03s to skip 25 seconds. Position excludes time spent paused. _start_ts alone would have counted a track paused for a coffee break as still playing and resumed minutes too late, so pause and resume are routed through the player, which stops and restarts its own clock. After a respawn the clock is backdated by the seek, so a second effect change resumes from the real position rather than from the respawn point. Seeking is skipped where it would be wrong: live streams report no duration and have no position to seek to, and a position inside the last two seconds would resume into silence. The load-failure heuristic now measures from the spawn instead of _start_ts, which is no longer wall-clock once backdating is involved. Verified through the real audio path: seeking 10s into a 30s source yields 20.0s of playback and 25s yields 5.0s, with an effect filter applied on top. Closes #9 --- cogs/effects.py | 5 +- cogs/music.py | 12 +- services/media.py | 18 ++- tests/test_effect_filters.py | 52 ++++++++ tests/test_player_seek.py | 245 +++++++++++++++++++++++++++++++++++ utils/player.py | 76 ++++++++++- 6 files changed, 392 insertions(+), 16 deletions(-) create mode 100644 tests/test_player_seek.py diff --git a/cogs/effects.py b/cogs/effects.py index 5fc3030..5d60f05 100644 --- a/cogs/effects.py +++ b/cogs/effects.py @@ -12,8 +12,9 @@ _EFFECT_COOLDOWN = 3.0 -# FFmpeg audio-filter presets. Each restarts the current track through -# ``-af `` (from the beginning). +# FFmpeg audio-filter presets. Applying one respawns the stream through +# ``-af ``, resuming at the current playback position — see +# ``MusicPlayer.apply_effect``. 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 diff --git a/cogs/music.py b/cogs/music.py index d7c114b..639b6f2 100644 --- a/cogs/music.py +++ b/cogs/music.py @@ -124,9 +124,10 @@ def _added_embed(track: dict) -> discord.Embed: @same_voice_channel() async def pause(self, ctx): """Pause the current track.""" - vc = ctx.voice_client - if vc and vc.is_playing(): - vc.pause() + # Routed through the player so it can stop its playback clock; that + # clock is what lets an effect change resume in the right place. + player = players.get(ctx.guild.id) + if player and player.pause(): await ctx.send(embed=success_embed("Paused ⏸")) else: await ctx.send(embed=error_embed("Nothing is playing right now.")) @@ -135,9 +136,8 @@ async def pause(self, ctx): @same_voice_channel() async def resume(self, ctx): """Resume a paused track.""" - vc = ctx.voice_client - if vc and vc.is_paused(): - vc.resume() + player = players.get(ctx.guild.id) + if player and player.resume(): await ctx.send(embed=success_embed("Resumed ▶️")) else: await ctx.send(embed=error_embed("Nothing is paused.")) diff --git a/services/media.py b/services/media.py index c315835..fd12076 100644 --- a/services/media.py +++ b/services/media.py @@ -331,9 +331,21 @@ def spawn_stream(track: dict) -> AudioStream: return AudioStream.launch(cmd) -def make_pipe_source(stdin, *, volume: float = 0.5, ffmpeg_filter: str = ""): - """Build a ``discord.PCMVolumeTransformer`` that reads audio from a pipe.""" +def make_pipe_source(stdin, *, volume: float = 0.5, ffmpeg_filter: str = "", + seek_seconds: float = 0.0): + """ + Build a ``discord.PCMVolumeTransformer`` that reads audio from a pipe. + + ``seek_seconds`` starts playback partway in, which is what lets an effect + change resume where the listener was. It is passed as an *input* option so + FFmpeg discards packets without decoding them; on a pipe that is a + read-and-discard rather than a real seek, but it costs almost nothing + because yt-dlp delivers at network speed rather than in realtime. + """ import discord options = f"-vn -af {ffmpeg_filter}" if ffmpeg_filter else "-vn" - source = discord.FFmpegPCMAudio(stdin, pipe=True, options=options) + before = f"-ss {seek_seconds:.3f}" if seek_seconds > 0 else None + source = discord.FFmpegPCMAudio( + stdin, pipe=True, before_options=before, options=options, + ) return discord.PCMVolumeTransformer(source, volume=volume) diff --git a/tests/test_effect_filters.py b/tests/test_effect_filters.py index 108e5b9..d83a891 100644 --- a/tests/test_effect_filters.py +++ b/tests/test_effect_filters.py @@ -16,6 +16,7 @@ import pytest from cogs.effects import EFFECTS +from services import media pytestmark = pytest.mark.skipif( shutil.which("ffmpeg") is None, reason="FFmpeg is not installed" @@ -92,3 +93,54 @@ def test_non_speed_effects_leave_the_duration_alone(name): @pytest.mark.parametrize("name", sorted(EFFECTS)) def test_every_preset_produces_audio(name): assert rendered_seconds(EFFECTS[name], 44100) > 0 + + +# -- resuming in place, through the real audio path --------------------- + +def played_seconds(source) -> float: + """Drain a discord AudioSource and return how much audio it yielded.""" + frames = 0 + while source.read(): + frames += 1 + source.cleanup() + return frames * 0.02 # discord consumes 20 ms frames + + +@pytest.fixture +def tone_file(tmp_path): + """A 30-second MP3 on disk, to feed the pipe source from a real file.""" + path = tmp_path / "tone.mp3" + subprocess.run( + [ + "ffmpeg", "-hide_banner", "-loglevel", "error", "-y", + "-f", "lavfi", + "-i", "sine=frequency=440:sample_rate=44100:duration=30", + str(path), + ], + check=True, + ) + return path + + +@pytest.mark.parametrize("seek,expected_remaining", [ + (0, 30.0), + (10, 20.0), + (25, 5.0), +]) +def test_seeking_starts_playback_partway_in(tone_file, seek, expected_remaining): + """ + The whole point of #9: an effect change respawns FFmpeg, and the new + process has to pick up where the listener was rather than at 0:00. + """ + with open(tone_file, "rb") as handle: + source = media.make_pipe_source(handle, seek_seconds=seek) + assert played_seconds(source) == pytest.approx(expected_remaining, abs=0.3) + + +def test_seeking_and_an_effect_apply_together(tone_file): + """A resumed track keeps the effect that triggered the respawn.""" + with open(tone_file, "rb") as handle: + source = media.make_pipe_source( + handle, ffmpeg_filter=EFFECTS["bassboost"], seek_seconds=20, + ) + assert played_seconds(source) == pytest.approx(10.0, abs=0.3) diff --git a/tests/test_player_seek.py b/tests/test_player_seek.py new file mode 100644 index 0000000..621b31c --- /dev/null +++ b/tests/test_player_seek.py @@ -0,0 +1,245 @@ +""" +Playback position tracking, and resuming in place across an effect change. + +An FFmpeg filter chain is fixed for the life of the process, so changing an +effect means respawning the stream. These cover the clock that makes the +respawn land where the listener actually was, rather than back at 0:00. + +Time is driven by a fake clock so the tests are exact rather than timing- +dependent. +""" + +from unittest.mock import MagicMock + +import pytest + +import discord +from services import media +from utils import player as player_module +from utils.player import SEEK_TAIL_MARGIN +from tests.conftest import make_track + + +class FakeClock: + def __init__(self, now: float = 1000.0): + self.now = now + + def __call__(self) -> float: + return self.now + + def advance(self, seconds: float) -> None: + self.now += seconds + + +@pytest.fixture +def clock(monkeypatch): + c = FakeClock() + monkeypatch.setattr(player_module.time, "monotonic", c) + return c + + +@pytest.fixture +def playing(player, fake_guild, clock): + """A player mid-track, three minutes into a five-minute song.""" + vc = MagicMock() + vc.is_playing.return_value = True + vc.is_paused.return_value = False + fake_guild.voice_client = vc + + player.current = make_track("Song", duration=300) + player._start_ts = clock.now + clock.advance(180) + return player + + +# -- elapsed ----------------------------------------------------------- + +def test_elapsed_is_zero_before_anything_plays(player, clock): + assert player.elapsed == 0.0 + + +def test_elapsed_tracks_wall_time(playing, clock): + assert playing.elapsed == pytest.approx(180) + clock.advance(45) + assert playing.elapsed == pytest.approx(225) + + +def test_elapsed_excludes_time_spent_paused(playing, clock): + """Otherwise a track paused for a coffee break resumes far too late.""" + playing.pause() + clock.advance(600) # ten minutes paused + playing.voice.is_playing.return_value = False + playing.voice.is_paused.return_value = True + playing.resume() + + assert playing.elapsed == pytest.approx(180), "paused time must not count" + + clock.advance(20) + assert playing.elapsed == pytest.approx(200) + + +def test_elapsed_is_frozen_while_still_paused(playing, clock): + playing.pause() + clock.advance(60) + assert playing.elapsed == pytest.approx(180) + clock.advance(60) + assert playing.elapsed == pytest.approx(180) + + +def test_repeated_pauses_accumulate(playing, clock): + for _ in range(3): + playing.voice.is_playing.return_value = True + playing.voice.is_paused.return_value = False + playing.pause() + clock.advance(10) + playing.voice.is_playing.return_value = False + playing.voice.is_paused.return_value = True + playing.resume() + clock.advance(5) + + # 180 before, then 3 x 5s of actual playback; the 3 x 10s paused is excluded. + assert playing.elapsed == pytest.approx(195) + + +# -- pause / resume guards --------------------------------------------- + +def test_pause_refuses_when_nothing_is_playing(player, fake_guild): + fake_guild.voice_client = None + assert player.pause() is False + + +def test_resume_refuses_when_nothing_is_paused(playing): + assert playing.resume() is False, "already playing" + + +def test_double_pause_does_not_lose_the_clock(playing, clock): + playing.pause() + clock.advance(30) + playing.pause() # a second !pause while paused + clock.advance(30) + playing.voice.is_playing.return_value = False + playing.voice.is_paused.return_value = True + playing.resume() + assert playing.elapsed == pytest.approx(180) + + +# -- where a respawn should pick up ------------------------------------ + +def test_seek_target_is_the_current_position(playing): + assert playing._seek_target() == pytest.approx(180) + + +def test_live_streams_are_never_seeked(playing): + """A live stream reports no duration and has no position to seek to.""" + playing.current["duration"] = None + assert playing._seek_target() == 0.0 + + +def test_a_track_with_unknown_duration_is_never_seeked(playing): + del playing.current["duration"] + assert playing._seek_target() == 0.0 + + +def test_the_last_seconds_of_a_track_are_not_seeked_into(playing, clock): + """Resuming past the end would produce silence, not audio.""" + clock.advance(300 - 180 - SEEK_TAIL_MARGIN) # right at the margin + assert playing._seek_target() == 0.0 + + +def test_a_position_past_the_end_is_not_seeked_to(playing, clock): + clock.advance(500) + assert playing._seek_target() == 0.0 + + +def test_the_very_start_is_not_seeked_to(player, fake_guild, clock): + """Seeking to 0 is a no-op that only costs a slower start.""" + vc = MagicMock() + vc.is_playing.return_value = True + fake_guild.voice_client = vc + player.current = make_track("Song", duration=300) + player._start_ts = clock.now + assert player._seek_target() == 0.0 + + +# -- apply_effect ------------------------------------------------------ + +def test_applying_an_effect_captures_the_position(playing): + assert playing.apply_effect("bassboost", "equalizer=f=54:g=10") is True + assert playing._resume_at == pytest.approx(180) + assert playing._replay is True + assert playing.effect_name == "bassboost" + playing.voice.stop.assert_called_once() + + +def test_applying_an_effect_to_a_live_stream_does_not_seek(playing): + playing.current["duration"] = None + assert playing.apply_effect("bass", "equalizer=f=54:g=5") is True + assert playing._resume_at == 0.0, "a live stream must restart, not seek" + + +def test_clearing_an_effect_also_resumes_in_place(playing): + assert playing.apply_effect(None, "") is True + assert playing._resume_at == pytest.approx(180) + assert playing.effect_filter == "" + + +def test_apply_effect_refuses_when_nothing_is_playing(player, fake_guild): + fake_guild.voice_client = None + assert player.apply_effect("bass", "x") is False + + +def test_apply_effect_works_while_paused(playing, clock): + playing.pause() + playing.voice.is_playing.return_value = False + playing.voice.is_paused.return_value = True + clock.advance(90) + assert playing.apply_effect("echo", "aecho=0.8:0.88:60:0.4") is True + assert playing._resume_at == pytest.approx(180), "paused time must not shift the resume point" + + +# -- the seek reaching FFmpeg ------------------------------------------ + +class FakeFFmpeg(discord.AudioSource): + """Records how discord.FFmpegPCMAudio was configured.""" + last = None + + def __init__(self, source, **kwargs): + FakeFFmpeg.last = kwargs + + def read(self): + return b"" + + +@pytest.fixture +def captured_ffmpeg(monkeypatch): + FakeFFmpeg.last = None + monkeypatch.setattr(discord, "FFmpegPCMAudio", FakeFFmpeg) + return FakeFFmpeg + + +def test_a_seek_becomes_an_ffmpeg_input_option(captured_ffmpeg): + """ + -ss must be an *input* option. As an output option FFmpeg decodes and + discards every frame instead of skipping packets. + """ + media.make_pipe_source(MagicMock(), seek_seconds=182.5) + assert captured_ffmpeg.last["before_options"] == "-ss 182.500" + + +@pytest.mark.parametrize("seek", [0, 0.0, -1]) +def test_no_seek_means_no_input_option(captured_ffmpeg, seek): + media.make_pipe_source(MagicMock(), seek_seconds=seek) + assert captured_ffmpeg.last["before_options"] is None + + +def test_the_filter_and_the_seek_coexist(captured_ffmpeg): + media.make_pipe_source( + MagicMock(), ffmpeg_filter="equalizer=f=54:g=10", seek_seconds=60, + ) + assert captured_ffmpeg.last["before_options"] == "-ss 60.000" + assert captured_ffmpeg.last["options"] == "-vn -af equalizer=f=54:g=10" + + +def test_no_filter_still_strips_video(captured_ffmpeg): + media.make_pipe_source(MagicMock()) + assert captured_ffmpeg.last["options"] == "-vn" diff --git a/utils/player.py b/utils/player.py index 9cb252b..bb79db7 100644 --- a/utils/player.py +++ b/utils/player.py @@ -31,6 +31,8 @@ INACTIVITY_TIMEOUT = 300 # seconds with an empty queue before disconnecting HISTORY_LIMIT = 50 MAX_QUEUE = 500 # hard cap to protect memory on small instances +SEEK_TAIL_MARGIN = 2.0 # never resume into the last seconds of a track +LOAD_FAILURE_SECONDS = 2.0 # a track ending faster than this never really started class MusicPlayer: @@ -53,6 +55,9 @@ def __init__(self, bot: discord.Client, guild: discord.Guild, self.effect_filter: str = "" self._start_ts: float = 0.0 # monotonic clock when current started + self._paused_at: Optional[float] = None # when the current pause began + self._paused_total: float = 0.0 # paused seconds, this track + self._resume_at: float = 0.0 # seek offset for the next spawn self._stream: Optional[media.AudioStream] = None # active yt-dlp stream # Signalling between commands and the playback loop. @@ -129,17 +134,68 @@ def skip(self) -> bool: return True return False + @property + def elapsed(self) -> float: + """Seconds of the current track actually heard, excluding paused time.""" + if not self._start_ts: + return 0.0 + paused = self._paused_total + if self._paused_at is not None: + paused += time.monotonic() - self._paused_at + return max(0.0, time.monotonic() - self._start_ts - paused) + + def pause(self) -> bool: + """Pause playback and stop the clock, so ``elapsed`` stays honest.""" + vc = self.voice + if not (vc and vc.is_playing()): + return False + vc.pause() + if self._paused_at is None: + self._paused_at = time.monotonic() + return True + + def resume(self) -> bool: + vc = self.voice + if not (vc and vc.is_paused()): + return False + vc.resume() + if self._paused_at is not None: + self._paused_total += time.monotonic() - self._paused_at + self._paused_at = None + return True + def apply_effect(self, name: Optional[str], filter_str: str) -> bool: - """Restart the current track with a new FFmpeg filter (from the start).""" + """ + Switch the current track to a new FFmpeg filter, resuming in place. + + A filter chain is fixed for the life of an FFmpeg process, so changing + one means respawning the stream. ``_resume_at`` carries the current + position across that respawn — without it, asking for a bass boost four + minutes into a song threw the listener back to 0:00. + """ vc = self.voice if not (vc and self.current and (vc.is_playing() or vc.is_paused())): return False self.effect_name = name self.effect_filter = filter_str + self._resume_at = self._seek_target() self._replay = True vc.stop() return True + def _seek_target(self) -> float: + """ + Where a respawn should pick up, or 0 when seeking would be wrong. + + Live streams report no duration and cannot be seeked, and a position in + the last couple of seconds would resume into silence or past the end. + """ + duration = (self.current or {}).get("duration") + if not duration: + return 0.0 + position = self.elapsed + return position if 0 < position < duration - SEEK_TAIL_MARGIN else 0.0 + def go_previous(self) -> bool: """Queue the previous track to play next, keeping the current one after it.""" if not self.history: @@ -185,11 +241,19 @@ async def _player_loop(self) -> None: self._stream = stream was_replay = self._replay self._replay = False + seek_to = self._resume_at + self._resume_at = 0.0 source = media.make_pipe_source( - stream.stdout, volume=self.volume, ffmpeg_filter=self.effect_filter, + stream.stdout, volume=self.volume, + ffmpeg_filter=self.effect_filter, seek_seconds=seek_to, ) vc.play(source, after=self._after_play) - self._start_ts = time.monotonic() + # Backdate the clock by the seek so a *second* effect change + # resumes from the real position, not from the respawn point. + self._start_ts = time.monotonic() - seek_to + self._paused_total = 0.0 + self._paused_at = None + spawned_at = time.monotonic() if not silent: await self._safe_send(now_playing_embed( @@ -198,7 +262,9 @@ async def _player_loop(self) -> None: )) await self._next.wait() - played = time.monotonic() - self._start_ts + # Measured from the spawn, not from _start_ts, which is + # backdated when resuming partway into a track. + played = time.monotonic() - spawned_at source.cleanup() # stop FFmpeg # close() waits on the child and reads its stderr, so keep it # off the event loop. @@ -208,7 +274,7 @@ async def _player_loop(self) -> None: # A near-instant end that wasn't a user action means the source # failed to load (bot-check, unavailable). Tell the user. if (not self._destroyed and not self._skip and not was_replay - and played < 2.0): + and played < LOAD_FAILURE_SECONDS): track["error"] = stream.classify_error() # cached by close() await self._safe_send(self._load_error_embed(track)) self.current = None