mpv icon indicating copy to clipboard operation
mpv copied to clipboard

player: add video-sync=display-tempo

Open christoph-heinrich opened this issue 1 year ago • 12 comments

So far there was no way to sync video to display and have audio sync to video without changes in pitch.

With this option the audio does not get resampled (pitch change) and instead the corrected audio speed is applied to audio filters.

This allows higher values for video-sync-max-video-change without changes in pitch.

Closes #10210

christoph-heinrich avatar Aug 03 '22 22:08 christoph-heinrich

This seems to get out of sync sometimes, I'll have to look into that.

Edit: I think I fixed it, but I'll do some more testing before pushing.

christoph-heinrich avatar Aug 04 '22 15:08 christoph-heinrich

man page generation was broken

christoph-heinrich avatar Aug 04 '22 23:08 christoph-heinrich

I have created a conditional profile in my config to avoid the problems that arise when speed is set to 1 with this sync mode (scaletempo2 doesn't like close to 1).

[video-sync]
profile-desc=change sync when speed close to 1 to avoid intermittent audio artifacts
profile-cond=(speed * audio_speed_correction) > 0.99 and (speed * audio_speed_correction) < 1.01
video-sync=display-resample
profile-restore=copy-equal

But now I'm wondering if the sync mode itself should switch to resample when corrected speed gets close enough to 1. And then how close is close enough? Maybe create a new option so users can set it to whatever they think is best? Something similar already exists for interpolation with interpolation-threashold. Afaik the whole reason why display-resample resamples audio instead of applying it to speed directly is to avoid exactly that problem with scaletempo (and rubberband). If this really goes down that road, this effectively obsoletes display-resample.

christoph-heinrich avatar Aug 06 '22 22:08 christoph-heinrich

Do you want to be able to sync a 24 Hz movie to a 30 Hz display? The original intention of display-resample is to do what my own patches in mplayer and mpv does. To enable smooth playing of video without video frame drops even when audio and video speed does not exactly match display refresh rate. From what I have read a change of speed of about 4% (that which is needed to play a 25 Hz movie on a 24 Hz display or the other way around) does not change pitch so much that it is noticeable for most people. Though the normal way mpv changes speed to match display rate may be to quick and then you hear when speed changes due to pitch changing. This will not happen if you do the change slowly. Anyway, the reason to use resample is quality. scaletempo does distort sound. The normal case for display sync is to play a video at its normal speed and just do minimal changes in audio speed so that it matches the video which plays in sync with display refresh. In that case I can see no reason to use scaletempo as resample handles small changes much better and with much higher quality. But if you want to play video at a rate much different from original and sync audio to it, then you need to use scaletempo. In what cases are "display-speed" (should maybe be called "display-scaletempo") intended to be used?

DanOscarsson avatar Aug 10 '22 07:08 DanOscarsson

This new option, just like display-resample, tries to change the playback speed of video to better match the refresh rate of the display -> smoother playback. Higher values for video-sync-max-video-change give it more wiggle room for that, resulting in smoother playback more often. The problem with that is that any adjustment made to synchronize with the display gets applied to resample, and thus results in a change of pitch, and so higher values for video-sync-max-video-change will also result in a greater change to the pitch. I use video-sync-max-video-change=11.501333195111 in my config, and the pitch change from that is very noticeable. Btw, higher values for that work particularly well with #10495.

And just to clarify, this changes nothing in how the synchronization to the display refresh rate works, it does exactly the same as display-resample, the only difference is on the audio side.

Imagine you want to watch a 24fps movie on a 60hz display. With e.g. video-sync-max-video-change=4 you can simply press ] twice, resulting in speed=1.21 (with the default settings). Since display-resample has enough breathing room for changing the speed, it will now effectively play the video at speed=1.25, so every frame of the video will line up with a refresh of the display (or at least it should, but without #10495 it might not actually, depending on your setting for video-sync-max-factor). Now display-resample sets a speed of 1.21 for scaletempo/scaletempo2 and makes up the rest of the difference with resample. But what's the point in throwing resample on top of that when scaletempo/scaletempo2 is already used anyway? It's just putting a change in pitch on top of whatever scaletempo/scaletempo2 already does to the audio. It would be better to just set a speed of 1.25 for scaletempo/scaletempo2 and be done with it.

That is what display-speed is for.

christoph-heinrich avatar Aug 10 '22 19:08 christoph-heinrich

While I normally want to watch a movie in its normal speed, I can skip through it faster if its bad. But then it would normally be 2 or 3 times normal speed as that is smoothest when you watch in on a TV or other display that can do 24 or 50 Hz. If you want to skip through a movie faster when using a 60 Hz display, a speed of 1.25 or 2.5 would be good as it will then match vsyncs. I have not tested but have looked at the code for resample and scaletempo/scaletempo2, and I suspect that the resampler is much better at doing small changes in speed. That and the fact that scaletempo is not good at a speed of near normal speed, is probably why resampler is used for the small speed corrections when doing display sync. So I think the resampler definitely be used near normal speed (less then 5% deviation) but probably also at any speed you do display sync as it is good a small speed changes.

I still think you should call your version display-scaletempo as that is what you want to use. Changing audio speed is also what the resampler does.

DanOscarsson avatar Aug 12 '22 13:08 DanOscarsson

As this isn't specific to scaletempo/scaletempo2 I don't want to name it after those (also works with rubberband). Rubberband, SoundTouch and Audacity all use the term "tempo" for changing speed without changing pitch, so maybe display-tempo?

Edit: scaletempo2 does fine with small changes in speed btw, after all audio speed gets constantly adjusted to account for drift and I haven't noticed anything bad so far (I have used it in my installed mpv build ever since I made this). It only has problems with speed close to 1, hence my comment with the conditional profile.

christoph-heinrich avatar Aug 12 '22 13:08 christoph-heinrich

I think this option will just confuse people. display-resample works fine for what it is intended. Sync video to display at normal speed to get smooth playing is what probably most people want. display-resample does that fine. A speed up/down of about 4 % of a movie does result in a change of pitch which is noticed by most people. If some user want to do different things with sync to display vsync, they should clearly describe what they want and why. Then it is possible to look at a solution. Most issues I have seen about pitch changes are for very special cases not for normal playing of movies.

DanOscarsson avatar Aug 13 '22 11:08 DanOscarsson

If some user want to do different things with sync to display vsync, they should clearly describe what they want and why. Then it is possible to look at a solution.

I am a user and I use mpv for watching YouTube videos, which come at all sorts at frame rates and I usually watch them at >1 and <=2 speed. To increase smoothness of playback I would like to use bigger values for video-sync-max-video-change (e.g. video-sync-max-video-change=11.5), but the change in pitch is bothering me.

Do I have to open an issue about this, just to have an issue to get closed by this PR? I don't see why you're so against this change.

If the name is the problem, we can certainly change it to something better. I still like display-tempo, considering the existence of display-resample. I can also add a warning to the documentation that some audio filters don't do well with only slight deviations from speed=1 and thus using this with speed set to 1 (or close to is) can lead to undesirable results.

christoph-heinrich avatar Aug 13 '22 13:08 christoph-heinrich

I've changed it to display-tempo and updated the documentation. Although I'm not sure about the speed * video_speed_correction recommendation, because with vfr video video_speed_correction might get updated every frame, which means that condition might get reevaluated every frame. It's been fine for me so far, but I haven't watched any vfr video.

christoph-heinrich avatar Aug 13 '22 18:08 christoph-heinrich

Rebased to lastest master because it conflicted with f0011552e7bd0d7ecaac02069781ef23973d411e.

christoph-heinrich avatar Aug 14 '22 18:08 christoph-heinrich

I don't have any feelings on this. Code looks fine to me, and not as invasive as I was fearing.

haasn avatar Aug 14 '22 22:08 haasn