Skip to content

Commit

Permalink
Add play position tracking for all play types
Browse files Browse the repository at this point in the history
  • Loading branch information
hoffie committed Feb 28, 2024
1 parent 5138bb5 commit c4805ce
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 1 deletion.
48 changes: 47 additions & 1 deletion src/jukebox/components/playermpd/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
from jukebox.NvManager import nv_manager
from .playcontentcallback import PlayContentCallbacks, PlayCardState
from .coverart_cache_manager import CoverartCacheManager
from .play_position_tracker import PlayPositionTracker

logger = logging.getLogger('jb.PlayerMPD')
cfg = jukebox.cfghandler.get_handler('jukebox')
Expand Down Expand Up @@ -193,6 +194,9 @@ def __init__(self):
# Change this to last_played_folder and shutdown_state (for restoring)
self.music_player_status['player_status']['last_played_folder'] = ''

play_position_tracker_file = cfg.getn('playermpd', 'play_position_tracker_file', default='../../shared/settings/play_positions.json')

This comment has been minimized.

Copy link
@pabera

pabera Apr 8, 2024

I like the fact your outsourced the entire logic to a different class. I do believe we should keep all relevant logic within this class, including this definition. (We have solved the problem similarly for CoverartCacheManager)

This comment has been minimized.

Copy link
@hoffie

hoffie Apr 8, 2024

Author Owner

I do believe we should keep all relevant logic within this class, including this definition.

Will do.

self.play_position_tracker = PlayPositionTracker(path=play_position_tracker_file)

self.old_song = None
self.mpd_status = {}
self.mpd_status_poll_interval = 0.25
Expand Down Expand Up @@ -270,6 +274,7 @@ def _mpd_status_poll(self):
self.current_folder_status["LOOP"] = "OFF"
self.current_folder_status["SINGLE"] = "OFF"

self.play_position_tracker.handle_mpd_status(self.mpd_status)
# Delete the volume key to avoid confusion
# Volume is published via the 'volume' component!
try:
Expand Down Expand Up @@ -308,11 +313,13 @@ def update_wait(self):
def play(self):
with self.mpd_lock:
self.mpd_client.play()
self.play_position_tracker.flush()

@plugs.tag
def stop(self):
with self.mpd_lock:
self.mpd_client.stop()
self.play_position_tracker.flush()

@plugs.tag
def pause(self, state: int = 1):
Expand All @@ -323,24 +330,28 @@ def pause(self, state: int = 1):
"""
with self.mpd_lock:
self.mpd_client.pause(state)
self.play_position_tracker.flush()

@plugs.tag
def prev(self):
logger.debug("Prev")
with self.mpd_lock:
self.mpd_client.previous()
self.play_position_tracker.flush()

@plugs.tag
def next(self):
"""Play next track in current playlist"""
logger.debug("Next")
with self.mpd_lock:
self.mpd_client.next()
self.play_position_tracker.flush()

@plugs.tag
def seek(self, new_time):
with self.mpd_lock:
self.mpd_client.seekcur(new_time)
self.play_position_tracker.flush()

@plugs.tag
def rewind(self):
Expand All @@ -351,6 +362,7 @@ def rewind(self):
logger.debug("Rewind")
with self.mpd_lock:
self.mpd_client.play(1)
self.play_position_tracker.flush()

@plugs.tag
def replay(self):
Expand All @@ -367,6 +379,7 @@ def toggle(self):
"""Toggle pause state, i.e. do a pause / resume depending on current state"""
with self.mpd_lock:
self.mpd_client.pause()
self.play_position_tracker.flush()

@plugs.tag
def replay_if_stopped(self):
Expand Down Expand Up @@ -466,11 +479,33 @@ def move(self):

@plugs.tag
def play_single(self, song_url):
play_target = ('single', song_url)
with self.mpd_lock:
if self._play_or_pause_current(play_target):
return
self.mpd_client.clear()
self.mpd_client.addid(song_url)
self._mpd_restore_saved_position(play_target)
self.mpd_client.play()

def _play_or_pause_current(self, play_target):
if self.play_position_tracker.is_current_play_target(play_target):
if self.mpd_status['state'] == 'play':
# Do nothing
return True
if self.mpd_status['state'] == 'pause':
logger.debug('Unpausing as the play target is identical')
self.mpd_client.play()
return True
return False

def _mpd_restore_saved_position(self, play_target):
logger.debug(f'Restoring saved position for {play_target}')
playlist_position = self.play_position_tracker.get_playlist_position_by_play_target(play_target) or 0
seek_position = self.play_position_tracker.get_seek_position_by_play_target(play_target) or 0
self.play_position_tracker.set_current_play_target(play_target)
self.mpd_client.seek(playlist_position, seek_position)

@plugs.tag
def resume(self):
with self.mpd_lock:
Expand All @@ -482,11 +517,14 @@ def resume(self):
@plugs.tag
def play_card(self, folder: str, recursive: bool = False):
"""
Main entry point for trigger music playing from RFID reader. Decodes second swipe options before playing folder content
Deprecated (?) main entry point for trigger music playing from RFID reader.

This comment has been minimized.

Copy link
@pabera

pabera Apr 8, 2024

I reviewed your code and it's a great start.

A few thoughts from my side:

  1. I noticed as well that play_card is not actually used, but has the relevant "Second Swipe" logic (although I am not entirely sure it actually works)
  2. For cards assignments, we currently focus on play_folder, play_album and play_single, but they don't have the Second Swipe functionality included.
  3. My idea was to only focus on play_card for card assignments (cards.yaml), but change the arguments.
-  def play_card(self, folder: str, recursive: bool = False):

+  def play_card(self, type: str, song_url: str, recursive: bool = False):  # type = ['play_folder', 'play_album', 'play_single']
  1. I believe, this would reduce your code to some extend as well.

I have also put a few comments inline.

This comment has been minimized.

Copy link
@hoffie

hoffie Apr 8, 2024

Author Owner

@pabera

Regarding 1-3: I'm fine either way, but this should probably be decided/implemented by someone more involved with the project than me. I'm happy to adjust my code to whatever design wins. ;)

I believe, this would reduce your code to some extend as well.

Yes, I think so, too. I only tried to cover everything which currently seems expected.

This comment has been minimized.

Copy link
@pabera

pabera Apr 9, 2024

I'm happy to adjust my code to whatever design wins.

By contributing, you are already involved ;-)

I will most likely go with the approach above. Once that's merged, I'd love to see this code making its way into the codebase

This comment has been minimized.

Copy link
@pabera

pabera Apr 10, 2024

I tried to streamline the code a bit. There is still some legacy in there (e.g. callbacks), which won't bother much.

MiczFlor#2330

Decodes second swipe options before playing folder content
Checks for second (or multiple) trigger of the same folder and calls first swipe / second swipe action
accordingly.
Note: The Web UI currently uses play_single/album/folder directly.
:param folder: Folder path relative to music library path
:param recursive: Add folder recursively
"""
Expand Down Expand Up @@ -587,8 +625,11 @@ def play_folder(self, folder: str, recursive: bool = False) -> None:
:param recursive: Add folder recursively
"""
# TODO: This changes the current state -> Need to save last state
play_target = ('folder', folder, recursive)
with self.mpd_lock:
logger.info(f"Play folder: '{folder}'")
if self._play_or_pause_current(play_target):
return
self.mpd_client.clear()

plc = playlistgenerator.PlaylistCollector(components.player.get_music_library_path())
Expand All @@ -608,6 +649,7 @@ def play_folder(self, folder: str, recursive: bool = False) -> None:
if self.current_folder_status is None:
self.current_folder_status = self.music_player_status['audio_folder_status'][folder] = {}

self._mpd_restore_saved_position(play_target)
self.mpd_client.play()

@plugs.tag
Expand All @@ -621,10 +663,14 @@ def play_album(self, albumartist: str, album: str):
:param albumartist: Artist of the Album provided by MPD database
:param album: Album name provided by MPD database
"""
play_target = ('album', albumartist, album)
with self.mpd_lock:
logger.info(f"Play album: '{album}' by '{albumartist}")
if self._play_or_pause_current(play_target):
return
self.mpd_client.clear()
self.mpd_retry_with_mutex(self.mpd_client.findadd, 'albumartist', albumartist, 'album', album)
self._mpd_restore_saved_position(play_target)
self.mpd_client.play()

@plugs.tag
Expand Down
105 changes: 105 additions & 0 deletions src/jukebox/components/playermpd/play_position_tracker.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
"""
Keeps track of playlist and in-song position for played single tracks,
albums or folders.
Syncs to disk every FLUSH_INTERVAL seconds.
Provides methods to retrieve the stored values to resume playing.
"""
import time
import os
import logging
import threading
import json


NO_SEEK_IF_NEAR_START_END_CUTOFF = 5
FLUSH_INTERVAL = 30

This comment has been minimized.

Copy link
@pabera

pabera Apr 8, 2024

Should maybe be configurable in jukebox.yml.

Background is: Writing to the SD card regularly reduces their lifetime (by general knowledge, we don't have actual user data on that). 30 seconds does not lead to a lot of writes but I found the interval quite long (especially for short music tracks, for audiobooks, this doesn't seem to be a problem).

I am not sure what the happy path is, but in a configurable way, we could at least collect different type of feedback.

This comment has been minimized.

Copy link
@hoffie

hoffie Apr 8, 2024

Author Owner

@pabera

Writing to the SD card regularly reduces their lifetime (by general knowledge, we don't have actual user data on that). 30 seconds does not lead to a lot of writes

Yep. I haven't checked how journald is configured, but there is a constant flow of messages anyway, so it's hard to tell what frequency is ok and what isn't.

but I found the interval quite long (especially for short music tracks, for audiobooks, this doesn't seem to be a problem).

This is only the non-happy-path though. The happy path (pausing, next, prev, card swipe) should always cause a write, since this is the active user interaction which should be saved. The flush timer is solely there to avoid losing too much information in case that software shuts down unexpectedly during playing (due to crashes, battery shortage, etc.).

I am not sure what the happy path is, but in a configurable way, we could at least collect different type of feedback.

Yeah, I guess we could make it configurable.

This comment has been minimized.

Copy link
@pabera

pabera Apr 9, 2024

software shuts down unexpectedly during playing (due to crashes, battery shortage, etc.).

The most common and very likely scenario is that kids switch off the device by randomly pressing buttons (or their siblings) :-)

This comment has been minimized.

Copy link
@hoffie

hoffie Apr 11, 2024

Author Owner

The most common and very likely scenario is that kids switch off the device by randomly pressing buttons (or their siblings) :-)

Probably depends on the hardware, but if the power bank is not accessible and all other buttons are regular buttons (next, prev, vol up, vol down, power off), then nothing should break, no matter how many buttons one presses.


logger = logging.getLogger('jb.PlayerMPD.PlayPositionTracker')


def play_target_to_key(play_target):
return '|'.join([str(x) for x in play_target])


class PlayPositionTracker:
flush_interval = 30
_last_flush_timestamp = 0

def __init__(self, path):
self._lock = threading.RLock()
self._path = path
self._tmp_path = path + '.tmp'
self._current_play_target = None
with self._lock:
self._load()

def _load(self):
logger.debug(f'Loading from {self._path}')
try:
with open(self._path) as f:
d = json.load(f)
except FileNotFoundError:
logger.debug('File not found, assuming empty list')
self._play_targets = {}
self.flush()
return
self._play_targets = d['positions_by_play_target']
logger.debug(f'Loaded {len(self._play_targets.keys())} saved target play positions')

def set_current_play_target(self, play_target):
with self._lock:
self._current_play_target = play_target_to_key(play_target)

def is_current_play_target(self, play_target):
return self._current_play_target == play_target

def get_playlist_position_by_play_target(self, play_target):
return self._play_targets.get(play_target_to_key(play_target), {}).get('playlist_position')

def get_seek_position_by_play_target(self, play_target):
return self._play_targets.get(play_target_to_key(play_target), {}).get('seek_position')

def handle_mpd_status(self, status):
if not self._current_play_target:
return
playlist_position = status.get('song')
if playlist_position is not None:
with self._lock:
if self._current_play_target not in self._play_targets:
self._play_targets[self._current_play_target] = {}
self._play_targets[self._current_play_target]['playlist_position'] = playlist_position
elapsed = status.get('elapsed')
duration = status.get('duration')
if elapsed is not None:
elapsed = float(elapsed)
duration = float(duration)
if (elapsed < NO_SEEK_IF_NEAR_START_END_CUTOFF or
((duration - elapsed) < NO_SEEK_IF_NEAR_START_END_CUTOFF)):
# restart song next time:
elapsed = 0
with self._lock:
if self._current_play_target not in self._play_targets:
self._play_targets[self._current_play_target] = {}
self._play_targets[self._current_play_target]['seek_position'] = elapsed
self._flush_if_necessary()

def _flush_if_necessary(self):
now = time.time()
if self._last_flush_timestamp + FLUSH_INTERVAL < now:
return self.flush()
with self._lock:
self._dirty = True

def flush(self):

This comment has been minimized.

Copy link
@pabera

pabera Apr 8, 2024

I couldn't test this locally, but. I'd be interested about the behaviour

  1. Let's say, I have Song A playing
  2. I then swipe a card to play Song B.
  • Will it save the position for Song A?
  1. After a while, I will swipe Song A again.
  • Will it save the position for Song B?
  • Will it play Song A from the start or from the save position?

Or will we only save the latest played item?

This comment has been minimized.

Copy link
@hoffie

hoffie Apr 8, 2024

Author Owner

@pabera

Let's say, I have Song A playing
I then swipe a card to play Song B.
Will it save the position for Song A?

Yes, it should.

After a while, I will swipe Song A again.
Will it save the position for Song B?

Yes, it should.

Will it play Song A from the start or from the save position?

It should play from the saved position.

Or will we only save the latest played item?

No, all play positions (track number + elapsed time) should be saved by play target (single/album/folder).

This comment has been minimized.

Copy link
@pabera

pabera Apr 9, 2024

Alright. This must be combined with Second Swipe or a similar feature, I believe.
Otherwise, it would not be possible to play an album which is assigned to a card from the beginning.

This comment has been minimized.

Copy link
@hoffie

hoffie Apr 11, 2024

Author Owner

@pabera

Alright. This must be combined with Second Swipe or a similar feature, I believe.
Otherwise, it would not be possible to play an album which is assigned to a card from the beginning.

I use it without it. If the card has never played, it will start at the beginning. If the end of the playlist has been reached, the next swipe of the card will start at the beginning. Prev functionality will also work regularly to move to the beginning.

But I guess the activation of the feature must be configurable anyway.

with self._lock:
self._dirty = False
self._last_flush_timestamp = time.time()
with open(self._tmp_path, 'w') as f:
json.dump({
'positions_by_play_target': self._play_targets,
}, f, indent=2, sort_keys=True)
os.rename(self._tmp_path, self._path)
logger.debug(f'Flushed state to {self._path}')

def __del__(self):
self.flush()

0 comments on commit c4805ce

Please sign in to comment.