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