-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
improving seek speed #791
improving seek speed #791
Conversation
i cant figure out why the test case is failing since the seek works, but looks like its not as accurate as absolute in exchange for speed i think thats a good trade. |
Precise jump positioning is really not necessary. Users don’t click very accurately. I agree with this update. In addition, I have a question that is not related to this update. I would like to ask about whether the automatic next episode is triggered by mpv? Or media-kit. I didn't find the relevant code for the next episode in the media-kit. |
its handled by mpv, and can you test this pr and see if the seek is faster or not in your case |
Thanks, I'd like to test it, but one of these days I'll be back in a small town where I don't have a computer to program with. I can test it after I come back, which will take 3 days. Thank you for your enthusiastic reply. |
…proving-seek-speed
// --------- | ||
final value = duration.inMilliseconds / 1000; | ||
final command = 'seek ${value.toStringAsFixed(4)} absolute'.toNativeUtf8(); | ||
|
||
final command = 'seek ${value.toStringAsFixed(4)}'.toNativeUtf8(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, mpv does seek relative+keyframes <number>
. The tests are failing because seeking is no longer absolute
. You can do the following:
final command = 'seek ${value.toStringAsFixed(4)} absolute+keyframes'.toNativeUtf8();
I don't think this should be a default though, but it might be worth adding a new player configuration to either use accurate but expensive or inaccurate but fast seeking method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try this configuration it fails very badly if you seek multiple times.
For the configuration part I agree that it can be optional, but I don't have any idea how to expose the option cleanly
No. This is not a solution. It's not really ideal to have 2-3 seconds mismatch, many of our users actually rely on accurate seek (which in-fact is default in most applications). |
Of course... If one wishes to experiment themselves, they are free to use the hidden |
from my test the mismatch was close to 0.5 sec but if you got higher values i dont think this can work, so i agree with you |
I tried this yesterday. It couldn’t let me seek 15s forward on a long video file. It was jumping back to the closest keyframe. For certain use cases that’s probably not an issue, so we can at least file an issue to make this configurable |
^^ FYI: There's a similar |
we were using
absolute
which uses exact and from the docs looks like that is slow, when testing with the new code it seems faster, but i dont feel that the workaround i did to allow such logic is good,so this needs further testing and a better/cleaner position logic
issues i found which needs to be fixed before merging:
from my testing for now it should be all good and seeking is faster when called multiple times (that was a problem in general )
and seeking is 50% faster from what i feel