Vernoxvernax / Puddler-RS

Jellyfin & Emby & Plex command line client, powered by mpv. Written in Rust.
MIT License
14 stars 2 forks source link

Sane handling of early playback termination. #8

Closed calmcacil closed 1 year ago

calmcacil commented 1 year ago

Here I go again...

Should be able to handle playback termination a little better, while yes you can just close the console output but if mid episode close (quit in mpv) to the point where it sends updated playtime rather than marking as played, it would be better if we didn't immediately start the next episode, instead dropping us back to the "chose episode" screen, bonus point if handling w as an input to mark the previously watched episode as watched if it was done in error. It's a minor thing for sure, but for me i sometimes just close the player instead of pausing it if i have to go away for a bit or doing something else, so just bypassing autoplay in these cases seems like a sane handling of it, its very rare you cancel playback of an episode only to want to watch the next one.

Vernoxvernax commented 1 year ago

to mark the previously watched episode as watched if it was done in error

But why mark it as played, if you accidentally left the player and didn't see the entire video?


The player now falls back to questioning when the item has not been marked as played. Pretty genius idea. The user now also has the option to go back to the menu without fully exiting puddler when there are still items queued. I've never spent enough time streaming something to even experience issues like these. This is soo helpful!!!

Do the changes match what you had in mind?

calmcacil commented 1 year ago

I binge a lot so for me it ends up being alot, like if im playing a game, doing idle stuff i watch shows then suddenly friends wanna do stuff so i just exit the player and continue watching it.

As for why ask if it should mark it as watched and continue on, sometimes i rewatch old shows from when I was growing up, and sometimes i just see an episode i remember to well, and exit my player and go off to do something else, in these cases just quickly marking it as watching and moving on when i get back would be helpful. Would just be after being returned to puddler after exiting mpv you could potentially have enter to resume playing when you get back to it, or if you want move on and mark the previous one as watched for the reason, it could accept input like w for "Mark as watched and go to next" Now i get this might be a very... "me" problem, but im sure it might apply to others and it seems like a low impact thing to have.

calmcacil commented 1 year ago

Just did a quick test, of the newest code changes, currently it jumps to the next file rather than the one you were just on, I'd like to see it be "Do you wanna continue watching: insert media" rather than going to the next file if it wasnt fully watched.

Proposed options:

Vernoxvernax commented 1 year ago

Unfortunately life had me busy for the past days. Could you check out the new changes?

calmcacil commented 1 year ago

Oh no worries, your turn around is way better than most projects so i appreciate you handling requests when you get them.

This looks good, seems to work like expected.

calmcacil commented 1 year ago

Scratch that, Finish episode does not start at the point you left off, starts the file from the start again, if playback is handle as if "do you want to continue at XX:XX" by default it'd be perfect.

Vernoxvernax commented 1 year ago

Hm it works for me. I am even retrieving the item again for this exact issue. Did the script tell you that the "playback progress has been sent" to the server?

calmcacil commented 1 year ago

Found the error, seems if you accidentally give it an invalid input before you send the F to finish watching it ignores he continue prompt. Ideally since its a rewatching thing, maybe skip the Continue prompt all together?

Welcome back. Do you want to finish the current episode or play the next one?:
   Grand Designs (2004) - Season 4 - The Modernist Sugar Cube
 (F)inish episode | (M)ark watched | (N)ext episode | (R)eturn to menu | (E)xit
:
Invalid input, please try again.
: f

Playback progress (00:20:53) has been sent to your server.

Welcome back. Do you want to finish the current episode or play the next one?:
   Grand Designs (2004) - Season 4 - The Modernist Sugar Cube
 (F)inish episode | (M)ark watched | (N)ext episode | (R)eturn to menu | (E)xit
: F

Do you want to continue at: 00:20:53?
  (Y)es | (N)o (start from a different position)
:
Vernoxvernax commented 1 year ago

Nope. Emby just takes years to process the playstate. Working for me when waiting a little bit before clicking 'F'.

calmcacil commented 1 year ago

The file should already have had watch history since i resumed it to begin with though, but yeah might be a timing thing.

Vernoxvernax commented 1 year ago

The emby dashboard logs playback finishes as "$user has finished playing $what on Puddler". At least for me, these take quite a while (several seconds) after quiting mpv, to show up, which is most likely the issue right now. Hm, I'll think of a fix.

calmcacil commented 1 year ago

Maybe add a 5 sec wait before it allows a resume?

Vernoxvernax commented 1 year ago

Maybe add a 5 sec wait before it allows a resume?

Does the most recent commit fix the issue for you?

calmcacil commented 1 year ago

negative, but for context i did find out why the invalid inputs happen, it listens for inputs still during playback and when playback ends it handles all inputs, so should add handling to discard any inputs gotten while playback is happening.

Vernoxvernax commented 1 year ago

negative, but for context i did find out why the invalid inputs happen, it listens for inputs still during playback and when playback ends it handles all inputs, so should add handling to discard any inputs gotten while playback is happening.

Due to the nature of stdin and terminals it's extremely complicated to achieve this without multi-threading, which would in turn create problems with mpv. (macOS and certain OS's only allow window creation on the main thread).

Nevertheless, this can't be the cause for the resume problem. I'm rather puzzled atm. I've tested the changes of the last commit several times and it still seems to work flawlessly for me.

Just to be safe: When you watch something and end the playback during the episode. It writes the log "Playback progress ...", with the correct timestamp. But then when "finishing" it, it resumes at a different time, than where you left off?

calmcacil commented 1 year ago

Correct, what I've been seeing when trying to resume playback it starts from the beginning rather than resume it does seem like it only happens when its got bad inputs prior to trying to resume, I'll spend some more time testing when i get a chance.

calmcacil commented 1 year ago

It's been handling resumes somewhat okay today, though i did have this happen when i closed the player after it was resumed once.

As for handling inputs when things are playing, as for handling ghost inputs, maybe discard all inputs when for the first 250ms after it goes to the welcome back screen?

thread 'main' panicked at 'Failed to seek: MPV_ERROR_COMMAND', src/player.rs:369:82
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Vernoxvernax commented 1 year ago

Please always check if you're running the most up to date code. I've tested the binaries on a windows vm, and couldn't reproduce the thread panic you experienced. The script only seeks, once libmpv sends the "FileLoaded" event. Not a lot to optimize there. Have you tried updating your mpv library?

As for the ghost inputs: It is absolutely not possible to implement this as of right now. I've already attempted this multiple times in the past. There is no stdin blocking, discarding or locking. We will get there in the future.

calmcacil commented 1 year ago

Not possible to make all inputs received in for the first 500ms after playback be handled to do "null"? I will admit i have no clue how this works, just figured maybe you could at least prevent the "ghost inputs" for having a negative affect by handling all actions for the first 500 as just, an empty task.

Vernoxvernax commented 1 year ago

Not possible to make all inputs received in for the first 500ms after playback be handled to do "null"? I will admit i have no clue how this works, just figured maybe you could at least prevent the "ghost inputs" for having a negative affect by handling all actions for the first 500 as just, an empty task.

Puddler uses two types of input prompts. The one where you enter multiple characters, like for example when configuring the ip address of your emby instance. And then the one where you only enter a single char like for the (y/n) prompts or after finishing a video.

The first one would already complicated enough to do this for, as it only "sends" when clicking enter. The second one uses getch. A unix and windows compatible library that only waits for a single character.

The problem with both of those methods: You can't put a timeout on them. So there is no way to listen to ghost inputs ONLY for the first 250ms.

From an entirely different point of view: I sometimes connect to one of my media servers remotely, so it happens that the "PlaybackFinished" request takes about or more than one second to send. During that times I often find myself already entering either E or M into the console while the prompt hasn't appeared yet.

Discarding-input reminds me too much of those javascript libraries on amazon or whatever that load in too slowly and end up deselecting the search box. Which is probably why I seem so reluctant to rewrite half of the program for this change.

calmcacil commented 1 year ago

Ah, gotcha.

Vernoxvernax commented 1 year ago

Further tests have shown the following: When playing a file from a clean start, exiting and "finishing" it. Puddler correctly seeks to the location where playback was left off. Issues are only appearing when repeating this process multiple times within 30 seconds. Usually at the 4th attempt, mpv will play the file, but won't send the playback information that Puddler needs, for at least 30 seconds. After that, other issues like the thread panick can occur.

This is a perfect example of undefined behaviour. Nowhere in mpv's documentation is there any mention of commands to explicitly kill the libmpv thread. I will go on and harrass the mpv devs with this next issue and see if there is any way of exiting libmpv correctly.

Great catch by the way.

Edit: Maybe it's just the outdated mpv bindings that I am using right now.

calmcacil commented 1 year ago

mpv is by far my favorite way to watch things (suppose it makes sense why the 3 big media center solutions all use it under the hood) but it certainly can break in unexpected ways, usually due to weird edge cases like the things we ran into with the stuff you explained.

Even with something like jf-mpv-shim when i was using that (boy i wish someone would port the JF version to fully support emby) there were edge cases where it would just crash in unexpected ways so I do believe it could be very related to weird edge cases.

Vernoxvernax commented 1 year ago

Well. I found the culprit. It's actually not our beloved mpv. The discord presence has some timeout logic, so when sending updates too quickly it blocks and ends up halting the entire program. In an attempt to fix this I changed to a different library, but that didn't change anything. Ideas are already popping up in my head, which I will test in the following days.

PS: Since this is all going so well, could you please test if the new libmpv library compiles/works for you?

calmcacil commented 1 year ago

Sneaky little bonus features causing issues :p

As for testing, i can confirm that the latest build worked fine on my end, didnt seem to run into any issues watching a few episodes.

Vernoxvernax commented 1 year ago

So. All discord stuff is now multi-threaded. This comes with the advantage that it should never interfere with mpv or any other part of the program. But it also doesn't guarantee that each update gets sent to the discord presence. Which I imagine is less of a problem.

calmcacil commented 1 year ago

I mean I would definitively say that DiscordRPC is a low-priority thing. Obviously having it is a nice-have but if its not 100% spot on, its barely an inconvenience.

Vernoxvernax commented 1 year ago

The situation we are talking about (playing and exiting within 3 seconds consecutively) is so unusual that I would consider this issue as solved.

calmcacil commented 1 year ago

Agreed.