From 968a28ceeffdf4ecca822b4d86752d3cc82df734 Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Sat, 15 Aug 2026 16:19:16 +0100 Subject: [PATCH] fix: guard media template seek/re-entry, add capability flags seek_forward/seek_backward crashed with TypeError when a backend's get_track_position() returned None; they now log and no-op instead. ocp_start is now idempotent while playback is already active, so a re-entrant call no longer double-emits LOADED_MEDIA/PLAYING or calls play() twice. MediaBackend also gains supports_seek and supports_pause class attributes (both default True) so consumers can check backend capability before calling seek/pause. --- ovos_plugin_manager/templates/media.py | 68 ++++++++++++++++++++------ test/unittests/test_media_templates.py | 64 ++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 14 deletions(-) diff --git a/ovos_plugin_manager/templates/media.py b/ovos_plugin_manager/templates/media.py index f769b613..3a360592 100644 --- a/ovos_plugin_manager/templates/media.py +++ b/ovos_plugin_manager/templates/media.py @@ -16,10 +16,21 @@ class MediaBackend(metaclass=ABCMeta): bus (MessageBusClient): Mycroft messagebus emitter """ + #: Whether this backend can seek within a track. Backends that cannot + #: seek (e.g. live streams) should set this to False; consumers may + #: check it before calling seek_forward/seek_backward. + supports_seek = True + + #: Whether this backend can pause/resume playback. Backends that cannot + #: pause (e.g. live streams) should set this to False; consumers may + #: check it before calling ocp_pause/ocp_resume. + supports_pause = True + def __init__(self, config=None, bus=None): if MediaState is None: raise RuntimeError("Please update to ovos-utils~=0.1.") self._now_playing = None # single uri + self._ocp_playing = False # tracks whether ocp_start has run without a matching stop/error self._track_start_callback = None self.supports_mime_hints = False self.config = config or {} @@ -35,13 +46,26 @@ def set_track_start_callback(self, callback_func): def load_track(self, uri: str, metadata: dict = None): self._now_playing = uri + self._ocp_playing = False # new track queued, ocp_start needs to (re)start playback self.meta.update(metadata or {}) LOG.debug(f"queuing for {self.__class__.__name__} playback: {uri}") self.bus.emit(Message("ovos.common_play.media.state", {"state": MediaState.LOADED_MEDIA})) def ocp_start(self): - """Emit OCP status events for play""" + """Emit OCP status events for play. + + Idempotent: if playback was already started by a previous call and + has not since been stopped (ocp_stop) or errored (ocp_error), this + is a no-op. This prevents duplicate LOADED_MEDIA/PLAYING events and + a duplicate call to play() when ocp_start is invoked again while + playback is already active. + """ + if self._ocp_playing: + LOG.debug(f"{self.__class__.__name__}.ocp_start called while " + f"already playing, ignoring") + return + self._ocp_playing = True self.bus.emit(Message("ovos.common_play.player.state", {"state": PlayerState.PLAYING})) self.bus.emit(Message("ovos.common_play.media.state", @@ -52,6 +76,7 @@ def ocp_error(self): """Emit OCP status events for playback error""" if self._now_playing: self._now_playing = None + self._ocp_playing = False self.bus.emit(Message("ovos.common_play.media.state", {"state": MediaState.INVALID_MEDIA})) self.bus.emit(Message("ovos.common_play.player.state", @@ -61,6 +86,7 @@ def ocp_stop(self): """Emit OCP status events for stop""" if self._now_playing: self._now_playing = None + self._ocp_playing = False self.bus.emit(Message("ovos.common_play.player.state", {"state": PlayerState.STOPPED})) self.bus.emit(Message("ovos.common_play.media.state", @@ -172,8 +198,12 @@ def seek_forward(self, seconds=1): seconds (int): number of seconds to seek, if negative rewind """ miliseconds = seconds * 1000 - new_pos = self.get_track_position() + miliseconds - self.set_track_position(new_pos) + position = self.get_track_position() + if position is None: + LOG.debug(f"{self.__class__.__name__} reported no track " + f"position, ignoring seek_forward") + return + self.set_track_position(position + miliseconds) def seek_backward(self, seconds=1): """Rewind X seconds. @@ -182,8 +212,12 @@ def seek_backward(self, seconds=1): seconds (int): number of seconds to seek, if negative jump forward. """ miliseconds = seconds * 1000 - new_pos = self.get_track_position() - miliseconds - self.set_track_position(new_pos) + position = self.get_track_position() + if position is None: + LOG.debug(f"{self.__class__.__name__} reported no track " + f"position, ignoring seek_backward") + return + self.set_track_position(position - miliseconds) def track_info(self): """Get info about current playing track. @@ -210,10 +244,12 @@ def load_track(self, uri, metadata: dict = None): {"state": TrackState.QUEUED_AUDIO})) def ocp_start(self): - """Emit OCP status events for play""" + """Emit OCP status events for play. Idempotent, see MediaBackend.ocp_start.""" + was_playing = self._ocp_playing super().ocp_start() - self.bus.emit(Message("ovos.common_play.track.state", - {"state": TrackState.PLAYING_AUDIO})) + if not was_playing: + self.bus.emit(Message("ovos.common_play.track.state", + {"state": TrackState.PLAYING_AUDIO})) class RemoteAudioPlayerBackend(AudioPlayerBackend): @@ -234,10 +270,12 @@ def load_track(self, uri, metadata: dict = None): {"state": TrackState.QUEUED_VIDEO})) def ocp_start(self): - """Emit OCP status events for play""" + """Emit OCP status events for play. Idempotent, see MediaBackend.ocp_start.""" + was_playing = self._ocp_playing super().ocp_start() - self.bus.emit(Message("ovos.common_play.track.state", - {"state": TrackState.PLAYING_VIDEO})) + if not was_playing: + self.bus.emit(Message("ovos.common_play.track.state", + {"state": TrackState.PLAYING_VIDEO})) class RemoteVideoPlayerBackend(VideoPlayerBackend): @@ -259,10 +297,12 @@ def load_track(self, uri, metadata: dict = None): {"state": TrackState.QUEUED_WEBVIEW})) def ocp_start(self): - """Emit OCP status events for play""" + """Emit OCP status events for play. Idempotent, see MediaBackend.ocp_start.""" + was_playing = self._ocp_playing super().ocp_start() - self.bus.emit(Message("ovos.common_play.track.state", - {"state": TrackState.PLAYING_WEBVIEW})) + if not was_playing: + self.bus.emit(Message("ovos.common_play.track.state", + {"state": TrackState.PLAYING_WEBVIEW})) class RemoteWebPlayerBackend(WebPlayerBackend): diff --git a/test/unittests/test_media_templates.py b/test/unittests/test_media_templates.py index c7913457..1c18b4c7 100644 --- a/test/unittests/test_media_templates.py +++ b/test/unittests/test_media_templates.py @@ -277,6 +277,58 @@ def test_seek_backward(self) -> None: """seek_backward updates track position.""" self.backend.seek_backward(1) # get_track_position() - 1000ms + def test_seek_forward_none_position_does_not_raise(self) -> None: + """seek_forward must not raise when get_track_position() returns None.""" + with patch.object(self.backend, "get_track_position", return_value=None), \ + patch.object(self.backend, "set_track_position") as set_pos: + self.backend.seek_forward(1) # should not raise TypeError + set_pos.assert_not_called() + + def test_seek_backward_none_position_does_not_raise(self) -> None: + """seek_backward must not raise when get_track_position() returns None.""" + with patch.object(self.backend, "get_track_position", return_value=None), \ + patch.object(self.backend, "set_track_position") as set_pos: + self.backend.seek_backward(1) # should not raise TypeError + set_pos.assert_not_called() + + def test_ocp_start_reentry_does_not_double_play(self) -> None: + """Calling ocp_start twice while already playing must not call + play() twice nor re-emit LOADED_MEDIA/PLAYING a second time.""" + self.backend._now_playing = "file:///test.mp3" + with patch.object(self.backend, "play") as play_mock, \ + patch.object(self.backend.bus, "emit") as emit_mock: + self.backend.ocp_start() + self.assertEqual(play_mock.call_count, 1) + first_emit_count = emit_mock.call_count + + self.backend.ocp_start() # re-entry: already LOADED/PLAYING + self.assertEqual(play_mock.call_count, 1) + self.assertEqual(emit_mock.call_count, first_emit_count) + + def test_ocp_start_after_stop_plays_again(self) -> None: + """ocp_start after an intervening ocp_stop must start playback again.""" + self.backend._now_playing = "file:///test.mp3" + with patch.object(self.backend, "play") as play_mock: + self.backend.ocp_start() + self.backend.ocp_stop() + self.backend._now_playing = "file:///test.mp3" + self.backend.ocp_start() + self.assertEqual(play_mock.call_count, 2) + + def test_ocp_start_after_load_track_plays_again(self) -> None: + """ocp_start after a new load_track must start playback again.""" + with patch.object(self.backend, "play") as play_mock: + self.backend.load_track("file:///a.mp3") + self.backend.ocp_start() + self.backend.load_track("file:///b.mp3") + self.backend.ocp_start() + self.assertEqual(play_mock.call_count, 2) + + def test_capability_flag_defaults(self) -> None: + """supports_seek and supports_pause default to True.""" + self.assertTrue(self.backend.supports_seek) + self.assertTrue(self.backend.supports_pause) + def test_track_info(self) -> None: """track_info returns meta dict.""" self.backend.meta = {"artist": "Test Artist"} @@ -310,6 +362,18 @@ def test_ocp_start_emits_playing_audio(self) -> None: self.backend._now_playing = "file:///test.mp3" self.backend.ocp_start() # Should not raise + def test_ocp_start_reentry_does_not_double_emit_track_state(self) -> None: + """Re-entrant ocp_start on AudioPlayerBackend must not re-emit + PLAYING_AUDIO nor call play() again.""" + self.backend._now_playing = "file:///test.mp3" + with patch.object(self.backend, "play") as play_mock, \ + patch.object(self.backend.bus, "emit") as emit_mock: + self.backend.ocp_start() + count_after_first = emit_mock.call_count + self.backend.ocp_start() + self.assertEqual(play_mock.call_count, 1) + self.assertEqual(emit_mock.call_count, count_after_first) + # --------------------------------------------------------------------------- # Tests for VideoPlayerBackend