Skip to content

fix: guard media template seek/re-entry, add capability flags #430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JarbasAl wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

fix: guard media template seek/re-entry, add capability flags #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 54 additions & 14 deletions ovos_plugin_manager/templates/media.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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):
Expand All @@ -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):
Expand All @@ -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):
Expand Down
64 changes: 64 additions & 0 deletions test/unittests/test_media_templates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down Expand Up @@ -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
Expand Down
Loading