Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seek position flickers between keyframes when dragging OSC on short videos #4183

Closed
ghost opened this issue Feb 24, 2017 · 13 comments
Closed

Comments

@ghost
Copy link

ghost commented Feb 24, 2017

mpv version and platform

mpv git master
macOS 10.11.6

Reproduction steps

  1. Open an mp4 video less than 10 minutes in length with HD dimensions.

  2. Slowly drag the OSC seek bar between any 2 adjacent key frames.

Expected behavior

When dragging the seek bar on short videos, the seek bar and video position should consistently display the key frame for the active GOP at the seeked time.

It should not flicker 'randomly' between adjacent keyframes.

Actual behavior

The seek bar and video position oscillate between the previous and the next key frame (bounding the seeked time). This oscillation cycles for each 1-pixel cursor movement between the corresponding slider positions.

Example demo

osc_drag

Log file

http://sprunge.us/CcQY

Why this is an issue (imo)

  1. It's visually jarring, e.g. in a presentation or demo.
  1. It's difficult for the user to stop at a specific keyframe, since every 1-pixel mouse movement jumps away from it. (The gif is slowed down, does not show how fast it occurs.)

Sample file

https://0x0.st/JPP.mp4

Edit

I'm guessing this might be an ffmpeg seek API issue, not mpv. Feel free to close if that's the case.

As a workaround I changed "keyframes" to "exact" at this line:
https:/mpv-player/mpv/blob/master/player/lua/osc.lua#L1740

But that's not a solution, as it can make seeking slower / laggy.

@wiiaboo
Copy link
Member

wiiaboo commented Feb 24, 2017

This is an "intended feature". Seeking with mouse dragging uses fast keyframe seeking, not hr-seek like single click seeks. You're not supposed to be using it for precise seeks.

@ghost
Copy link
Author

ghost commented Feb 24, 2017

Are you saying the flickering back and forth is intended?

I agree it is an intended feature that mouse dragging should use fast keyframe seeking.

That is not an issue. That is a 100% intended feature.

The issue is that video position flickers rapidly between adjacent keyframes, for each 1 pixel moved by the mouse, so that the user cannot seek to a keyframe without difficulty.

"The position should consistently display the key frame for the active GOP at the seeked time."

^^ The above statement is consistent with keyframe seeking. I see my edit may have confused the issue, sorry. Again, if this is an issue with the ffmpeg seek API, I understand it's not easily fixed.

@wiiaboo
Copy link
Member

wiiaboo commented Feb 24, 2017

It basically flickers between the closest keyframe from where your pointer is. If you're about in the middle between keyframes it'll flicker between one or the other.

@ghost
Copy link
Author

ghost commented Feb 24, 2017

"if you're about in the middle"

^ That's not quite accurate.
On Mac it's flickering for every pixel across the entire span of the 2 keyframe times. (Note it occurs much faster than shown in the GIF animation)

The user should not have to play "catch-the-keyframe" with their mouse.

It is practically a random behavior, from the user's perspective.

A clear violation of the principle of least astonishment:
https://en.wikipedia.org/wiki/Principle_of_least_astonishment

The behavior of least astonishment would be to seek to the keyframe for the current GOP (group of pictures)
...not jump/leap 'randomly' between keyframes.

No other media player, that I've ever used, has this unpredictable behavior. I'm not bashing mpv. I love mpv. That's why I'd like to find / work on a solution, and why I reported this.

Didn't expect pushback :p

@wiiaboo
Copy link
Member

wiiaboo commented Feb 24, 2017

I agree it's buggy behavior, but you'd get the same if you spammed seek 50% too, I think. So, not really a bug in the OSC but in how mpv/ffmpeg deals with these kind of seeks being spammed. I can be wrong.

@ghost
Copy link
Author

ghost commented Feb 24, 2017

Thanks for the validation (I mean that sincerely).

Yes, spamming seek 50 absolute-percent keyframes does the same thing.

I'll look into this more and follow-up, nntr

@ChrisK2
Copy link
Member

ChrisK2 commented Feb 24, 2017

It is certainly ugly, but also only an issue with rather short files. The longer the file is the less noticeable it becomes.

@ghost
Copy link
Author

ghost commented Feb 25, 2017

@ChrisK2, some people like their cat videos.

This seems to be fixed by exclusively using SEEK_BACKWARD for seek type MPSEEK_FACTOR.

i.e. change:
https:/mpv-player/mpv/blob/master/player/playloop.c#L274
to demux_flags = SEEK_BACKWARD;

This [change] causes a %-seek "keyframes" to always seek to the keyframe prior to the seek time, i.e. to the keyframe for the seeked GOP. That is, imo, the expected behavior of a seek bar. It also eliminates the ugly oscillation.

This seems to work fine with "exact"/hrseek as well, since (if I'm reading it right) that just works by dropping frames forward.

An edge case is when a keyframes seek factor computes to EXACTLY a keyframe PTS, in which case the previous keyframe will be seeked-to. I'm not sure if adding a minor forward offset to correct this would be considered a hack, or not. But considering that a %-seek is not typically intended as an exact seek operation, this case could be overlooked imo.

I might be missing something here, as I'm not familiar with mpv source.

@ghost
Copy link
Author

ghost commented Feb 25, 2017

@ChrisK2 (and anyone else who got email) the source link I gave was wrong, fixed.

@ghost
Copy link

ghost commented Feb 25, 2017

The line you pointed out is probably nonsense. Is the behavior fixed if you change it?

@ghost
Copy link
Author

ghost commented Feb 25, 2017

Yes, it is fixed with that change. That's what I meant to say.

And I haven't observed any side-effects, besides the edge case mentioned.

@ghost
Copy link
Author

ghost commented Jun 17, 2017

FYI, since the source file has changed, if you guys ever want to fix it, the source line is now 287.

i.e.:
https:/mpv-player/mpv/blob/master/player/playloop.c#L287
should be demux_flags = SEEK_BACKWARD;

It'd be a shame if this is a won't-fix. Would it be bad etiquette for me to do a pull-request for this as an outsider?

@wiiaboo
Copy link
Member

wiiaboo commented Jun 17, 2017

Go for it.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants