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

Use mpv's new create_osd_overlay API

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

FichteFoll
Copy link
Contributor

@FichteFoll FichteFoll commented Apr 14, 2020

The new API was added in mpv-player/mpv@0728726 for mpv 0.31 and doesn't add new functionality, but it makes the existing private interface publicly available, so it should be used eventually. In my journey of fixing the performance regression, I also ported syncplay's code to use this new API.

This was rebased on top of #295 in order to merge cleanly.

I did not change any code regarding the minimum version requirement of mpv because I didn't know how you would like to do that. I see that there is still code supporting some old mpv client with a version lower than 0.6. Please decide for yourself how you want to handle this or whether you want to accept it at all. There is a legacy layer that will map to the new API without any functional differences (at least as of now).

FichteFoll added 3 commits Apr 14, 2020
In mpv-player/mpv@0728726, a strcmp on
the previously displayed and the new text has been removed, causing
excessive GPU usage especially on idle frames when playback was paused.
I will submit a patch to upstream, but mpv versions 0.31 & 0.32 are
already affected by this.

See torque/mpv-progressbar#56 for a similar report to a different
script.
The internal and previously used set_osd_ass API has been deprecated.
@daniel-123
Copy link
Contributor

daniel-123 commented Apr 14, 2020

#261 is already going to have to bump minimum supported version to at least 0.17.

That said the policy we'd like to keep is to support player versions at least as old as found in latest LTS releases of popular distros. Which in case of mpv right now means 0.27 - so before this is merged it needs to cleanly handle older version.

Initial workaround on the other hand probably just needs a quick review.

@FichteFoll
Copy link
Contributor Author

FichteFoll commented Apr 15, 2020

Since there is a compatibility layer on the side of mpv, I would rather not want to duplicate this even into scripts unless necessary (I. e. removed by mpv) and reject it.

@daniel-123
Copy link
Contributor

daniel-123 commented Apr 18, 2020

I agree. Though instead of rejecting this outright I think it makes sense to simply keep it around until it's needed (mpv removes/changes old API) or until the point where raising minimum supported mpv version to 0.31 won't be a problem.

@FichteFoll
Copy link
Contributor Author

FichteFoll commented Apr 18, 2020

Sure, we can leave it floating around here. I have my forked repositories for pull requests in a separate org anyway for easier organization.

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

Successfully merging this pull request may close these issues.

None yet

3 participants