Add resume to play_card #1946

votti · 2022-12-30T17:02:24Z

Adds a resume flag to play_card to resume from position in case there is already playback information for a folder.
This is important for audiobooks.

In case the resume fails (eg if the folder changed), a normal playback is done and the error logged.

Addreses issue #1945

votti · 2022-12-31T15:58:40Z

I was wondering, if the the annotation if a folder should be resumed or not shouldnt rather be a property of the folder instead of defined in the RPC linked to a card?
I see that there is already a RESUME Status in playermpd but could it be that it is not used yet?

Maybe even a combination could make sense: allowing the play_card to have a resume argument with options True|False|None (default None).
If set to True|False, this overwrites any property of the folder, giving the power to enforce resume or not for specific cards. In case of None, this resumes or not depending on the RESUME status (ON|OFF) of the folder.

ghost · 2023-01-01T10:57:46Z

I am trying to sharpen the use case a bit:

Case 1: Play something (let's say an audiobook), pause and resume it all with the same card. We currently already have an option to toggle playing on repeating card swipes. This is done via the second swipe option.
Case 2: Play an audiobook, interrupt it and play something different, the come back to the same spot where we left off.
Case 3: Same as case 2, but allow to shut down the Box in between. The only different is that the resume information needs to saved to disc.

Case 2 and 3 is currently not implemented (bits and pieces are there). This is a feature that is available in 2.X and is planned for 3.X as well. But we were planning to rewrite the mpd interface module as there is a couple of issues with it. And then put this and a few other features in.

The way I read your comment, you would add to these cases to function to select on a per-card basis if the resume should actually be executed or not. Correct?

votti · 2023-01-02T20:25:08Z

Thanks for sharpening!
I am going for Case 2 & 3. With my current implementation, these cases work now for me.

As I currently have it, it is configured in the the RPC call associated with a card if a folder is resumed.
I.e. a card has an RPC command play_card associated with an argument resume=True.
In such a case, a card would behave like Case 2 & 3.

What I am now also working on, is that the RESUME status of a folder honored. I.e. if RESUME=ON, it would behave like in Case 2&3 and attempt to resume when playing the folder. If RESUME=OFF not (i.e. the standard behaviour we currently have).
The good thing about this would be, that we could add an RPC toogle_resume that switches the RESUME status. This could then be linked to a button/card/Web GUI button to set this option on a per folder basis.

Does this make sense?

Adds a resume flag to play_card to resume from position in case there is already playback information for a folder. This would be important for audiobooks. In case the resume fails (eg if the folder changed), a normal playback is done and the error logged.

pabera · 2023-01-04T22:42:50Z

@votti I am not entirely following your comment. We do not care about folders in V3 like we did in V2. It all comes down to the card and its assignment.. which can be a folder, an album or a single file. All of these possible options then would have to be supported by the "Resume" flag and especially for Option 3, this needs to be stored on disk.

votti · 2023-01-07T20:12:31Z

Ah thanks a lot for the pointer.
I just was looking at the code and somehow assumed that play_card was the main function that needed to be supported. Eg for the second swipe functionality so far only this one works.

I actually did miss play_song and play_album and that they are not yet logged and persisted in the music_folder_status.json file.

So I guess the steps to go from here is to add status tracking for albums/songs first? (Does this already have an open issue?).

Would you think that in order to get a resume functionality merged it first needs to support all 3 things (songs, album, folder) or would it be OK as well if this would be folder specific first?

pabera · 2023-04-07T21:39:33Z

Would you think that in order to get a resume functionality merged it first needs to support all 3 things (songs, album, folder) or would it be OK as well if this would be folder specific first?

Yes! (sorry for the late answer :-))

tutenchamun · 2023-04-08T14:45:32Z

this is still in request state right ?

DivineDominion · 2023-11-09T13:27:53Z

I want to look into this feature. We very much need playback resume controls, otherwise the user (my grandmother :)) would have to listen to the same intros dozens of times 🤔 Second Swipe on future3 is also not implemented AFAICT, right? Or is it possible to use that in the config e.g. cards.yaml?

pabera · 2023-11-09T14:28:37Z

This is a great feature to have. But a few requirements will determine the way to solve the problem. Maybe we can first define the requirements in the format of "As a user, I want to ..." before we go heads down. Maybe we can review the architecture to solve this before someone spends time on it. Thoughts?

DivineDominion · 2023-11-09T19:03:38Z

Side note: I checked out v2.4, too, thinking that might "just solve it", but oh dear future3's RPC and config is much nicer :)

This PR is also probably not the best place to chat about what we (I) wish for.

Let's imagine the next step is to return the double swipe to pause/resume feature, so is there anything here to salvage that, you think? From my noob perspective it looks like the existing functions on future3/develop suffice, only the wiring in the RPC commands is missing?

pabera · 2023-11-10T08:24:53Z

I didn't quite remember which features had been implemented, there was a larger break since I last developed. If it's just to connect the existing feature to the RPC, the give it a try :-)

Mannshoch · 2024-01-16T12:52:24Z

If possible I would like to set the resume with a jump of 10 sec backwards. Like I could do in Antennapod

hoffie · 2024-02-29T22:47:08Z

I've just stumbled upon this PR after playing with code enhancements around this topic myself. My current code is here and has been tested for play_single for now: hoffie@c4805ce

The code currently addresses the above mentioned cases 1, 2 and 3.
I noticed that only the play_card method supports second swipe. play_card does not seem to be used out of the box in the current code base at all. I've added a comment regarding that and added tracking functionality to the more relevant play_single, play_album and play_folder methods.
The tracking is implemented separately from playermpd in order to be able to re-use it in other/different implementations. It is currently implemented to handle mpd-specific data, but that could easily be made more generic.
The tracking re-uses the existing status information from the mpd polling thread.
The tracking only writes to SD card after some grace time or upon user interactions (e.g. stop/next) to keep SD wear low.
The tracking does not act on a per-folder level (which would break for album/single or totally different players). Instead, it uses a play_target, which is basically a combination of method call (e.g. play_single) with its arguments (SomeSong.mp3). That makes it flexible and reliable.
With this code in place, playermpd.status_file may become redundant (but I haven't removed it yet).

If there's any interest I'm happy to polish that commit further and submit it as a PR.

s-martin · 2024-03-02T10:26:02Z

@hoffie, sounds like a cool addition.

Right now we are also working on a multi player support #2164, maybe it’s worth considering.

hoffie · 2024-03-15T22:26:59Z

@s-martin What would be the best way forward? Should I wait for #2164 to be merged and rebase my code on that? Or should I rather submit a PR with my code right now?

s-martin · 2024-03-16T09:04:48Z

We should check with @Groovylein how far the Multiplayer PR is.

Related: MiczFlor#1946

pabera · 2024-04-08T06:46:44Z

@s-martin What would be the best way forward? Should I wait for #2164 to be merged and rebase my code on that? Or should I rather submit a PR with my code right now?

@hoffie: Let's try to get this merged. We'll have to do an extra effort for #2164 anyways.

pabera · 2024-04-08T20:00:04Z

I've just stumbled upon this PR after playing with code enhancements around this topic myself. My current code is here and has been tested for play_single for now: hoffie@c4805ce

@hoffie I added a few comments to your code

hoffie · 2024-04-08T20:50:52Z

@pabera Thanks. I'll try to address your comments in my upcoming PR. I guess a PR will be easier to comment/review than a single commit (which can't change). Will probably get to it by Thursday.

(Just for reference and probably not worth commenting there again: dca9d1b is what I currently use in "production" without issues, even for whole folders)

The tracking is active by default, but the resuming has to be enabled explicitly, either by setting playermpd.play_position_tracking.resume_by_default: true or by calling the play_* functions with the new resume=True kwarg. Related: MiczFlor#1946

hoffie · 2024-04-11T21:44:06Z

I'll try to address your comments in my upcoming PR.

See #2331.

The tracking is active by default, but the resuming has to be enabled explicitly, either by setting playermpd.play_position_tracking.resume_by_default: true or by calling the play_* functions with the new resume=True kwarg. Related: MiczFlor#1946

The tracking is active by default, but the resuming has to be enabled explicitly, either by setting playermpd.resume.resume_by_default: true or by calling the play_* functions with the new resume=True kwarg. Related: MiczFlor#1946

s-martin · 2024-05-03T19:58:58Z

Fixed a merge conflict.

There is a flake8 error, which lets the checks fail, see https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/8944194290/job/24570588323?pr=1946

coveralls · 2024-05-03T20:09:44Z

Pull Request Test Coverage Report for Build 8944306028

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 38.235%

Totals
Change from base Build 8775015840:	0.0%
Covered Lines:	494
Relevant Lines:	1292

💛 - Coveralls

s-martin linked an issue Dec 30, 2022 that may be closed by this pull request

🚀 | Add resume option #1945

Open

s-martin added enhancement future3 Relates to future3 development labels Dec 30, 2022

Add resume to play_card

a700acf

Adds a resume flag to play_card to resume from position in case there is already playback information for a folder. This would be important for audiobooks. In case the resume fails (eg if the folder changed), a normal playback is done and the error logged.

votti force-pushed the future3/feature/resume branch from df12108 to a700acf Compare January 4, 2023 21:17

hoffie added a commit to hoffie/RPi-Jukebox-RFID that referenced this pull request Apr 6, 2024

Add play position tracking for all play types

dca9d1b

Related: MiczFlor#1946

pabera added this to the v3.6 milestone Apr 8, 2024

pabera mentioned this pull request Apr 8, 2024

Release v3.6.0 #2258

Open

hoffie mentioned this pull request Apr 11, 2024

feat: Add resume position tracking for all play types #2331

Draft

3 tasks

Merge branch 'future3/develop' into future3/feature/resume

83fc706

handle exception in logger

3a2e94b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resume to play_card #1946

Add resume to play_card #1946

votti commented Dec 30, 2022

votti commented Dec 31, 2022 •

edited

ghost commented Jan 1, 2023

votti commented Jan 2, 2023

pabera commented Jan 4, 2023 •

edited

votti commented Jan 7, 2023

pabera commented Apr 7, 2023

tutenchamun commented Apr 8, 2023

DivineDominion commented Nov 9, 2023

pabera commented Nov 9, 2023

DivineDominion commented Nov 9, 2023

pabera commented Nov 10, 2023

Mannshoch commented Jan 16, 2024

hoffie commented Feb 29, 2024 •

edited

s-martin commented Mar 2, 2024

hoffie commented Mar 15, 2024

s-martin commented Mar 16, 2024 •

edited

pabera commented Apr 8, 2024

pabera commented Apr 8, 2024

hoffie commented Apr 8, 2024

hoffie commented Apr 11, 2024

s-martin commented May 3, 2024

coveralls commented May 3, 2024

Add resume to play_card #1946

Are you sure you want to change the base?

Add resume to play_card #1946

Conversation

votti commented Dec 30, 2022

votti commented Dec 31, 2022 • edited

ghost commented Jan 1, 2023

votti commented Jan 2, 2023

pabera commented Jan 4, 2023 • edited

votti commented Jan 7, 2023

pabera commented Apr 7, 2023

tutenchamun commented Apr 8, 2023

DivineDominion commented Nov 9, 2023

pabera commented Nov 9, 2023

DivineDominion commented Nov 9, 2023

pabera commented Nov 10, 2023

Mannshoch commented Jan 16, 2024

hoffie commented Feb 29, 2024 • edited

s-martin commented Mar 2, 2024

hoffie commented Mar 15, 2024

s-martin commented Mar 16, 2024 • edited

pabera commented Apr 8, 2024

pabera commented Apr 8, 2024

hoffie commented Apr 8, 2024

hoffie commented Apr 11, 2024

s-martin commented May 3, 2024

coveralls commented May 3, 2024

Pull Request Test Coverage Report for Build 8944306028

Details

💛 - Coveralls

votti commented Dec 31, 2022 •

edited

pabera commented Jan 4, 2023 •

edited

hoffie commented Feb 29, 2024 •

edited

s-martin commented Mar 16, 2024 •

edited