Hz rounding and toggle for 'auto' option#13
Open
GeorgeK07 wants to merge 1 commit into
Open
Conversation
…integer frame rate (eg: 59.977hz, 60fps), also adds the ability to toggle automatically changing the screen's hz to the video's frame rate with a script binding.
CogentRedTester
left a comment
CogentRedTester
left a comment
Owner
There was a problem hiding this comment.
Glad you like the script. I don't really have the time to maintain it anymore, but I've done a quick code review.
| and not ( tostring(var.current_height) == height and | ||
| tostring(var.current_width) == width and | ||
| tostring(math.floor(mp.get_property_number('display-fps'))) == rate | ||
| tostring(math.floor(mp.get_property_number('display-fps') + 0.5)) == rate |
Owner
There was a problem hiding this comment.
This seems a little risky, the framerate of the video is also rounded down, because that is what nircmd expects. Rounding up here could cause comparison issues. Shouldn't this be solved by removing 60 from the list of valid rates in your config options? If the highest is 59 then it should detect that the display is already at this rate and not try to change anything.
Comment on lines
+485
to
+498
| --toggles between auto being on or off | ||
| function toggleAuto() | ||
| if options.auto then | ||
| options.auto = false | ||
| osdMessage("[Change-Refresh] auto fps change is disabled") | ||
| msg.info("now disabled auto fps change") | ||
| else | ||
| options.auto = true | ||
| osdMessage("[Change-Refresh] auto fps change is enabled") | ||
| msg.info("now enabled auto fps change") | ||
| end | ||
| return | ||
| end | ||
|
|
Owner
There was a problem hiding this comment.
Have you tested how this interacts with script-opt on_update changes to the auto option?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello, I am a fan of your changerefresh script for mpv, however, I had an issue where the video would pause despite the frame rate and the screen's Hz matching. I was initially confused by this as it seemed this issue had been solved in an issue.
Looking into it, the way that the code checks if the screen's Hz and FPS are the same does a floor rounding. This means that if the screen's Hz is slightly below the video's FPS or the screen's Hz that the FPS has been configured to change into, the video will still pause.
My laptop's screen's 60Hz mode is at 59.977Hz, which while using nircmd it rounds to 60, because the comparison in your script always rounds down, the video pauses. To solve this, I added 0.5 to the screen Hz in the comparison to simulate rounding to the nearest integer. (eg: 59.977Hz -> 59Hz, but 60.477Hz -> 60Hz) This solved the problem for me.
The second addition this PR adds is the ability to toggle the 'auto' option via script binding, which means that automatically changing the screen's Hz to the video's FPS can be enabled or disabled using a keybind.
This is useful as there are some videos where I do not want my screen's Hz to change automatically to its FPS, such as some mathematics videos that are sometimes encoded at 25 FPS or 30 FPS, where the motion smoothness does not matter. In addition, I watch the videos at a faster speed, so even if it changes the Hz to match the FPS, the video's FPS will not divide into the Hz evenly anyway.
I hope you can look over this soon and merge the changes so that others can benefit. Thanks again for the script.