Skip to content

Fix key bindings for seek exact seeks by keyframes, #3710 #4739

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

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

low-batt
Copy link
Contributor

@low-batt low-batt commented Jan 1, 2024

This commit will:

  • Change MenuController.sameKeyAction to detect the exact flag and pass it on as extra data
  • Change MainMenuActionHandler.menuStep to pass the extra data to PlayerCore.seek if present

The main author of these changes is Matt Svoboda. I merely addressed some review comments.


Description:

This commit will:
- Change MenuController.sameKeyAction to detect the exact flag and pass
  it on as extra data
- Change MainMenuActionHandler.menuStep to pass the extra data to
  PlayerCore.seek if present

The main author of these changes is Matt Svoboda. I merely addressed
some review comments.

Co-authored-by: Matt Svoboda <matt.svoboda@gmail.com>
@low-batt
Copy link
Contributor Author

low-batt commented Jan 1, 2024

@uiryuu I tried to address your review feedback. Please have a look.
@svobs I know you are busy, but if you get a chance look please have a look.

In my testing these key bindings matched:

  • seek 20
  • seek -20 exact
  • seek 20 relative+exact
  • seek 20 exact+relative
  • seek 20 relative exact

These did not match (as expected):

  • seek 20 keyframes
  • seek 20 relative+keyframes
  • seek 20 relative keyframes
  • seek 20 absolute

The keyframe flag can't be supported without dealing with the current code confusion over SeekOption.relative. That value is labeled in the UI as Keyframe seek in the Seek type setting under Control. That makes sense. But internally it is named relative. The code where it is used in PlayerCore.seek looks like this:

    case .relative:
      mpv.command(.seek, args: ["\(relativeSecond)", "relative"], checkError: false)

Trouble is, if you really want to do a keyframe seek you must pass the keyframes flag. Although the seek command will default to keyframes that default can be changed. From the mpv manual entry for hr-seek:

--hr-seek=<no|absolute|yes|default>
Select when to use precise seeks that are not limited to keyframes. Such seeks require decoding video from the previous keyframe up to the target position and so can take some time depending on decoding performance. For some video formats, precise seeks are disabled. This option selects the default choice to use for seeks; it is possible to explicitly override that default in the definition of key bindings and in input commands.

  • no: Never use precise seeks.
  • absolute: Use precise seeks if the seek is to an absolute position in the file, such as a chapter seek, but not for relative seeks like the default behavior of arrow keys.
  • default: Like absolute, but enable hr-seeks in audio-only cases. The exact behavior is implementation specific and may change with new releases (default).
  • yes: Use precise seeks whenever possible.
    always: Same as yes (for compatibility).

So if you have set hr-seek=yes then you must pass the keyframes flag in a seek command if you want to seek by keyframe.

As the issue at hand is about getting exact to work I don't think we need to address this in the current PR.

Copy link
Member

@uiryuu uiryuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed the issue you mentioned when I was reviewing the original PR, and that indeed need to be addressed in another one. The current changes LGTM.

@uiryuu uiryuu merged commit e343c28 into develop Jan 2, 2024
@uiryuu uiryuu deleted the seek-exact branch January 2, 2024 06:00
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

Successfully merging this pull request may close these issues.

Keybindings for seek exact are silently overridden to seek keyframes
2 participants