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

improving seek speed #791

Closed

Conversation

abdelaziz-mahdy
Copy link
Member

@abdelaziz-mahdy abdelaziz-mahdy commented Apr 27, 2024

image

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:

  • multiple seeks in the same time is not working correctly due to position not being updated

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

@abdelaziz-mahdy
Copy link
Member Author

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.

@user1121114685
Copy link

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.

@abdelaziz-mahdy
Copy link
Member Author

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

@user1121114685
Copy link

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.

// ---------
final value = duration.inMilliseconds / 1000;
final command = 'seek ${value.toStringAsFixed(4)} absolute'.toNativeUtf8();

final command = 'seek ${value.toStringAsFixed(4)}'.toNativeUtf8();
Copy link
Contributor

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.

Copy link
Member Author

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

@alexmercerind
Copy link
Member

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).

@alexmercerind
Copy link
Member

Of course... If one wishes to experiment themselves, they are free to use the hidden setProperty API.

@abdelaziz-mahdy
Copy link
Member Author

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

@default-anton
Copy link
Contributor

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

@alexmercerind
Copy link
Member

alexmercerind commented Apr 29, 2024

Of course... If one wishes to experiment themselves, they are free to use the hidden setProperty API.

^^

FYI: There's a similar command API too: #586

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.

None yet

4 participants