mpv-player
more playback speed changes
Download the artifacts for this pull request:
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Can we just revert https://github.com/mpv-player/mpv/commit/e3af545421322e357eb9355395923710ea93f83b for the time being and rethink this whole thing again? I'm not sure fixing such a minor issue is worth introducing hacks on top of hacks. @DanOscarsson said he had a better fix for it, so we could wait on that or at least wait for more eyes to look at these changes before merging them.
edit: 3c37e18cd1e635b9bdb7918018ff80e498e50fe8 specifically looks good to me though, I'm mostly talking about the av state reset change.
Silently skipping around the playback position when changing speed is something I would consider pretty severe not minor. But then again since nobody ever noticed and this probably existed for the entire of mpv's history, so maybe it's a minor issue by that perspective.
I don't claim to have the best or smartest solutions. But nobody else is interested in working on it so I do what I can even if it takes 10 iterations to get to a good state. Of course if Dan shows up and one ups me with the perfect solution, I will happily click merge. I'm not doubting him, but people have lives and supposedly said patches are pretty old so who knows how long it will take to get it ready. If he's ready by tomorrow or something then yeah sure, I'll close this.
Commits 1 and 3 I'm fine with. 2 needs more work, but I would like to verify if it at least addresses the audio gap problem.
Can we just revert https://github.com/mpv-player/mpv/commit/e3af545421322e357eb9355395923710ea93f83b
Well I ended up doing this anyway in this patchset and completely rewrote it. display-resample and similar modes still have the problem of skipping frames forward when decreasing speed. Still trying to figure out exactly why that happens.
fwiw, I'm fine with just reverting e3af545 for now while a better solution is found. I've managed a way to work around https://github.com/mpv-player/mpv/issues/13513, and for me at least, e3af545 seems to cause more issues than it solves.
Still trying to figure out exactly why that happens.
Think I've mostly dealt with this now. Much happier with this.
until a better solution is found.
Hopefully that would be PR now. Could you give it a try again? I think I've ironed everything out.
I've tested this and it seems to be fine now, I can't reproduce the original issue and there are no obvious regressions.
Made a small adjustment to deal with a fringe case I found. Hopefully, this should be better than the old code and definitely master, so I'll probably merge this later today if I or anyone else doesn't find anything else. I'll handle more issues if/when they inevitably come up.
I got the b9ff661 build from the links above and tried it
AV desync can still be quite high (~500ms), but it resyncs fairly quickly so it's much less noticeable in practice. The effect is fairly similar to using --mc=1 with fa0fccc
For higher speeds (above ~4x), I can manage to get an edge case where the A-V sync number gets "stuck", or in some cases keeps increasing indefinitely, by changing between 2, 4, 8x speeds randomly. It actually seems to trigger when you decrease the speed
AV desync can still be quite high (~500ms), but it resyncs fairly quickly so it's much less noticeable in practice.
It's a tradeoff. When you hit these super high speeds, you can't let the av adjustment go too much or else you'll skip frames around weirdly in a non-monotonic fashion. So that means higher desync on the immediate speed change. It's by no means perfect but for me the av desync message doesn't appear until around 10x speed which is well beyond the point of usuable audio anyways, so I figured it was not too important.
I can manage to get an edge case where the A-V sync number gets "stuck", or in some cases keeps increasing indefinitely, by changing between 2, 4, 8x speeds randomly.
Thanks, I could hit this too. It's pretty easy to address and with the latest push it shouldn't runaway indefinitely anymore. Unfortunately in these weird edge cases, decreasing the speed from say like 16x to 8x will still result in frames skipping forward like before. It doesn't happen when resetting to a sane number around 1x though.
AV desync can still be quite high (~500ms), but it resyncs fairly quickly so it's much less noticeable in practice.
With the latest build 2565bbd, this is further improved to the point of being subjectively unnoticable in all cases and only measurable by screen recording or other analysis. But the a/v desync still seems to adversely affect scripts that depend on firing at precise times; i.e. I seem to be receive some events ~85ms after they fire, which is enough to cause some issues. I'm trying to work around that by adjusting audio-buffer somehow.
I can manage to get an edge case where the A-V sync number gets "stuck", or in some cases keeps increasing indefinitely, by changing between 2, 4, 8x speeds randomly.
This is also fixed on my end now. Hopefully you didn't have to put too many hacks in :P
Since nobody else ever complained, maybe I'm just being too picky, or scripting too far out of scope of what mpv is designed to handle. But I think it's worth pursuing a more robust solution in the future. Shall I open an issue to track further progress on A/V sync + speed change bugs?
But the a/v desync still seems to adversely affect scripts that depend on firing at precise times; i.e. I seem to be receive some events ~85ms after they fire
What was the delay before in the old code (i.e. before e3af545421322e357eb9355395923710ea93f83b)? I don't want to shoot you down but scripts are async/racy so depending on things happening at precise times is not easy.
Shall I open an issue to track further progress on A/V sync + speed change bugs?
Sure after this hopefully is to everyone's satisfaction we can drill down on some even more fringe cases.
What was the delay before in the old code (i.e. before e3af545)? I don't want to shoot you down but scripts are async/racy so depending on things happening at precise times is not easy.
I had a silencedetect filter on the audio and I think I was receiving messages consistently ~30ms ahead of when they happen. Some things were also dependent on audio-buffer length, which was a hacky way for me to play around with that delay.
The thing is, 25-50ms resolution is good enough for me at 1x speed at least for what I'm trying to do. But the problem at higher speeds like 4x is that you start to need an equivalent increase in time resolution to be able to do certain things. So instead of 50ms you now need 12.5ms resolution and your audio-buffer of 200ms now contains 800ms of the audio stream, so you start to notice timing issues if you need to change anything like speed, seeking, filters, etc..
Anyways, I'm not sure I really want to tackle this in my script right now. I may just reconsider doing things differently for now while waiting for a better fix.
That makes sense. This change does increase the duration of the speed change since it tries to smooth out the sync over a long period of time. So there would be more time until whatever message you're waiting on is delivered. I'm not sure what the best approach for your usecase would be. Maybe new events, extending the script API, etc.?
I'm not sure what the best approach for your usecase would be. Maybe new events, extending the script API, etc.?
Yea, I will have to think about that. Right now, the A/V sync would probably help me personally the most, along with refreshing the A/V buffers quickly on filter and speed changes without disrupting playback or introducing gaps.
I played with mp.observe_property for a bit, but resolution was only ~50ms for audio-pts, and multiplied with speedup. If there was a way to pick an arbitrary resolution for audio-pts and make it not depend on playback speed that could help. And/or a kind of timer that fires when audio-pts is greater/less than a user defined value. A corresponding property to fire for video times/frames might also be useful for similar applications. I was able to get higher resolution with mp.add_periodic_timer, but never worked as well for a variety reasons that I can't recall right now.
Another convenience may be a better version of time-pos that isn't dependent on video frame rate or synchronization, but a property that represents where the playback "should be". time-pos is discrete and locked to the frame rate so the resolution isn't always sufficient, and audio-pts isn't guaranteed to be in sync, is nil during seeking, and isn't even guaranteed to exist for files without an audio stream. For example, for a 10fps video, the new property would be able to return something like 0.35317865 when it's on the 4th frame, rather than 0.300. And if a/v gets out of sync, it doesn't return where the audio playback currently is, but the target time the video and/or audio stream is trying to sync up with. So if it skips a few frames or needs to adjust audio or something, the property keeps increasingly at a constant rate as if nothing happened.
Another convenience may be a better version of time-pos that isn't dependent on video frame rate or synchronization, but a property that represents where the playback "should be".
This sounds doable. I dunno what a good name would be essentially it would just be passing back mpv's timer offset by whenever playback of the file starts.
From what I can read I cannot say what parts my code will solve. Some of the problems sounds like they can be related to how video is synced to the audio. The solution I use is to have audio speed filters to output audio with as little as possible of pts holes or overlaps combined with "playing audio pts" returning the current pts of playing audio taking into account the speed changes in the audio data in the ao out buffers and driver delay. So the pts values for the playing audio will normally even when speed changes be a continuous sequence of increasing pts values. Then you need video sync code that will handle the changing speed of audio pts values. I have not tested how good the current mpv code is for that. As I have just reimplemented some of my code from my about three year old mpv to the current version it might be good to wait a little more time for me to find corner cases before releasing it, if you have a working solution now?
I can cherry-pick and push the first three commits from this for now (everyone is good with those) and then later when you're done we can compare approaches and maybe work out what's the best way to go.
Edit: And done.
This sounds doable. I dunno what a good name would be essentially it would just be passing back mpv's timer offset by whenever playback of the file starts.
If it wouldn't break much, perhaps this new property could replace time-pos, since in most cases it should be functionally equivalent or better. And then the current time-pos would get renamed to video-pts since that name doesn't exist yet, and for video files that's basically what time-pos is.
Then you'd end up with video-pts and audio-pts representing the actual time of each stream and time-pos is what they're both getting synced to, if that makes any sense
@DanOscarsson Would you also be willing to share the original patchset (against the older mpv version)?
My original patches are for the mpv code of before 2020. Several things related to audio handling and filters have changed so they does not work well with code of today. Also in the main patch with audio handling is my own video scheduling code that schedule video frames on vsync and handle a/V sync by using pts values. To make movie playing on my TV very smooth. So I would have to separate the old audio pts handling code from the rest and still it would not work on current mpv due to all changes in audio handling code. As my old patched mpv binary still works very well I have not had any need to move to current code. Now I have started to move all my patches I like to the current mpv. But it will take some time to verify that all works well again. So I do not think by original patchset is that easy to use.
@DanOscarsson
My original patches are for the mpv code of before 2020.
I just happen to use such an old version daily, so this isn't an issue for me. [In fact it'd be easier for me to cherrypick from the older patchset rather than trying to backport changes you've forward-ported to the newer version.]
schedule video frames on vsync and handle a/V sync by using pts values
This is interesting, how does it differ from display-resample mode in mpv, which I believe already times video frames against vsync? And could you elaborate more on the pts-based a/v sync? I thought that the a/v delay for sync purposes already operates off of pts:
double a_pts = written_audio_pts(mpctx) + opts->audio_delay - mpctx->delay;
double av_delay = a_pts - v_pts;
Long ago I made patches for mplayer to adjust audio speed to video. When I changed to mpv wm4 decided to instead of my patch create new code. That is the base for display-resample. But I had my code in my version of mpv and though the current video code using "delay" could be done better. So I added code that requires the vo to produce two values: time between two vsyncs and time of a recent vsync. Using these two my code schedules video on best vsync and then adjusts audio speed to match. The standard code in mpv today is much better then in the beginning and probably more or less works as well as my code. But I still prefer to use my code.