Skip to content

Improvements to speed UI (fixes #5147, #5168) #5175

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 7 commits into from
Oct 7, 2024

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Sep 16, 2024

  • I have read CONTRIBUTING.md
  • This implements/fixes issue Change playback speed up to 5 decimal places #5168.
  • Also fixes issue Change playback speed delimiter from "," to "." (x1.5) #5147.
  • Add speedResetBtn which will reset speed to 1x
  • Update osd.speed message; and menu.speed, menu.speed_up, menu.speed_down` menu item titles to support unlimited digits after decimal and also properly localize numeric values.
  • Update slider labels at load using properly localized numeric values for consistency (i.e. issue 5147).
  • Update customSpeedTextField, speedSliderIndicator to support up to 6 digits after decimal (will round to 6th decimal place, rounding down if 7th decimal place is a 5, matching mpv's behavior).
  • Increase width of customSpeedTextField to fit at least 6 total digits
  • Update speed slider action to round to 2 digits after decimal.
  • Fix knob clipping issue with video speed slider & audio delay slider in MacOS Ventura & earlier.

Description:
This attempts to address concerns mentioned in #5168, while striking a good balance between usability, versatility, and technical constraints.

It's not clear how many decimal places mpv will support. It looks like it will allow whatever will fit in a double. But it's doubtful that mpv has thoroughly tested very very precise speeds.
[EDIT] It has been determined that mpv supports 6 digits after the decimal and has a minimum value of 0.01, so this is now enforced when changing the speed.

This PR ensures that the OSD message will display the exact speed mpv is using. It looks like text fields want a maximum number of decimal places, so the customSpeedTextField in the Video Settings sidebar is now 9 decimals instead of 3. This also means that the speedSliderIndicator label above the slider will also be rounded to 9 decimals because it uses the number from customSpeedTextField, but they do not limit the actual value used by mpv. So key bindings can be used to set speed with any precision.

The width of speedSlider had to be decreased slightly to expand the width of customSpeedTextField, which now can comfortably fit 5 digits after the decimal.

I also modified the speed slider's action to round the selected speed to 2 digits after the decimal. This should be more usable because having more precision when dragging the slider wouldn't seem to make much sense and would only make the speed value harder to read.

This also expanded to fix number formatting for everything speed-related, so that locales which use commas instead of decimals are supported. There is more work to do for audio delay & subtitle controls, however.

Localized slider labels Localized menu items

Details:
• Add speedResetBtn which will reset speed to 1x
• Update osd.speed message to support unlimited digits after decimal
• Update customSpeedTextField, speedSliderIndicator to support up to 9
  digits after decimal (will round to 9th decimal place)
• Increase width of customSpeedTextField to fit at least 6 total digits
• Update speed slider action to round to 2 digits after decimal
@low-batt
Copy link
Contributor

Thanks for picking this one up! Will have to see what the other developers think about some of the UI details of this…

I think mpv is truncating speed to 6 digits. When I obtain it directly from mpv through the IPC interface I get 1.042709.

The Playback menu only shows a couple of digits. There is room for more. Should that be changed as well?

I like the reset button. Should that have a tool tip?

For some reason when I set the language to he the OSD messages all show -0.00 for the speed. I checked develop and this didn't happen.

The thumb of the slider is slightly cut off when it is at the end of the slider. This is an existing problem. Happens with develop as well.

OSD

@low-batt low-batt linked an issue Sep 17, 2024 that may be closed by this pull request
@svobs
Copy link
Contributor Author

svobs commented Sep 17, 2024

Thanks for picking this one up! Will have to see what the other developers think about some of the UI details of this…

Sure. I am not super happy about the text field having to scroll. But it should be uncommon for most cases... I suppose I could move the text field onto another row, which would allow the slider to fill the whole width, which would be nice. But it would look awkward aesthetically if the text field is all alone.

Also wanted to highlight that I moved the number formatting out of the localized strings for osd.speed, and replaced it with just a string %@. Seems to me like most localized strings should be formatted like this (or at least the OSD strings) so that changing the number's format doesn't require string changes for every locale (and the number formatter should be localizing the numbers anyway). Unfortunately it does mean that all the localized strings should be migrated to use %@, which is something to do after this PR is merged.

I think mpv is truncating speed to 6 digits. When I obtain it directly from mpv through the IPC interface I get 1.042709.

When I enter 1.23456789 as the custom speed, I do see in the mpv log:

[mpv0][d] Set property: speed=1.23456789

And then MPVController is sent a MPVOption.PlaybackControl.speed message which does indeed contain that exact number.

But it is helpful to know that the IPC interface stops at 6 digits. Maybe that's all anyone has ever needed? Maybe customSpeedTextField should stop at 6 digits as well?

Another issue. I also just now discovered there are problems setting very low speeds.

It looks like 0.01x is the lowest speed which works properly. But trying a lower speed, for example 0.001x, fails. I had to modify IINA's code to check the return value, but it returns -9, which seems to be "unsupported format for accessing property".

Not sure why this particular error code might show up. And there are no problems when higher values are used (for example, 0.010002 is fine). Need to dig in to this more... But at the very least it needs better handling, because right now it will silently fail and the UI will be lying about the actual speed.

The Playback menu only shows a couple of digits. There is room for more. Should that be changed as well?

Mmm yes, forgot about that. Will post an update.

I like the reset button. Should that have a tool tip?

I thought it would be easy enough to guess, since there are buttons like it down below for the equalizers. A tooltip could be added though. Maybe just saying "Reset speed to 1x" or something?

For some reason when I set the language to he the OSD messages all show -0.00 for the speed. I checked develop and this didn't happen.

Sounds like it might be the way I am doing the rounding. Will post an update to fix it. Would be great to learn more about Apple's "negative 0", "NaN", "infinity", etc and how it makes those & stores them.

The thumb of the slider is slightly cut off when it is at the end of the slider. This is an existing problem. Happens with develop as well.

Hmm. My guess is that the slider thumb can be drawn slightly outside the slider's actual bounds. Because it looks similar to an issue which can happen for rounded buttons. Ugly, ugly AppKit... But it only does this for Hebrew? I will see if I can add a workaround.

@svobs svobs marked this pull request as draft September 17, 2024 07:11
@svobs svobs force-pushed the pr-improve-speed-ui branch from 1289fdb to 937dd7f Compare September 17, 2024 07:51
@svobs
Copy link
Contributor Author

svobs commented Sep 17, 2024

The Playback menu only shows a couple of digits. There is room for more. Should that be changed as well?

I added a commit which changes the menu.speed indicator & various menu.speed_up & menu.speed_down speed menu items so they now support unlimited decimal places (at least whatever player.info.playSpeed is).

For some reason when I set the language to he the OSD messages all show -0.00 for the speed. I checked develop and this didn't happen.

I dug in to this, and it's just due to a mismatch in formatting strings, a result of the he locale not yet being updated to use %@ instead of %.2f. For some reason, passing in a String is allowed and is treated as a negative Double. Or something. Something which can be ignored for now.

The thumb of the slider is slightly cut off when it is at the end of the slider. This is an existing problem. Happens with develop as well.

Unable to reproduce. Tried dark & light themes; Hebrew & English; Retina display & regular display; Show scroll bars:: Always & When scrolling. Using MacOS Sonoma 14.6.1 (23G93). [Not sure when a build ID started showing up after double-clicking the version in About This Mac!]

@low-batt
Copy link
Contributor

i will defer to @uiryuu on the change to how the numbers are localized.

When I paste 1.23456789 into the text field the mpv log shows this for me:

[2797.829][v][cplayer] Set property: speed=1.234568 -> 1

Why is the log you showed missing -> 1?

The OSD message shows the full value. It is truncated to just 1.2345678 in the Inspector window when configured to display the speed. Same when pulled using the IPC interface. My guess on this is that the mpv property change event is sending IINA the value IINA sent mpv and mpv is truncating that value afterwards. I'm not liking that behavior. BUT I can't explain why your log message differs. I'm thinking IINA should stop at 6 digits, but it bothers me that I'm not fully understanding what is happening.

The documentation for --speed says:

--speed=<0.01-100>

So the problem with values less than 0.01 is expected mpv behavior.

"Reset speed to 1x" is fine for the tool tip. As for whether the tool tip is needed or not, I rather like tool tips, but I don't know how the other developers would feel about one for this button. They may think it is obvious what it does and shouldn't need one. Lets see what they think.

The problem with the slider thumb being cut off is not specific to he. I see it with en as well and when using default. It is just the very edge of the thumb that is cut off: I am testing under Ventura 13.6.9. I will test on my Mac running Sonoma. Maybe AppKit has fixed AppKit. Tuesday busy for me, so might not get to that until tomorrow.

clip

The hidden macOS build number has been there for a long time. When I was adding build information to IINA's about window I copied that convention where build information was not shown until you clicked on it. But following the macOS convention got a thumbs down from the reviewers.

@svobs
Copy link
Contributor Author

svobs commented Sep 17, 2024

Why is the log you showed missing -> 1?

Good to point this out. My error. I was looking at the IINA log and assumed it was a message output by mpv itself. In my fork I have the mpv logging turned up and I forgot that it's only set to w here. And the msg looks almost identical and uses the same mpvX subsystem as the mpv logs.

OK I am seeing the same behavior as you now. It looks like it rounds to 6 digits (half down), not truncates. IINA should try to match this behavior. But I am seeing consistent numbers in the Inspector window also. I don't see more than 6 digits after the decimal.

So the problem with values less than 0.01 is expected mpv behavior.

Oh ha. Guess I should have read the manual. But how strange that it won't support anything slower. It seems like that they should get the behavior for free, considering the fine-grained control they have at faster speeds. Maybe it starts to generate false positive alerts from their audio/video sync detection / buffer underrun subsystems.

I'll set the minimum to 0.01 then.

The problem with the slider thumb being cut off is not specific to he. I see it with en as well and when using default. It is just the very edge of the thumb that is cut off: I am testing under Ventura 13.6.9. I will test on my Mac running Sonoma. Maybe AppKit has fixed AppKit. Tuesday busy for me, so might not get to that until tomorrow.

Hmm. If it is a legacy problem, it will be hard for me to test. But I can take a shot at a fix.

One more thing which became apparent. I found that numbers in IINA are not being localized properly in lots of places. So if the locale uses a , instead of a . to signify a decimal, that is not being followed.

SCR-20240917-lbkq

In my latest commit I added an extension to Double which adds a string computed property which should properly localize the number.

@svobs
Copy link
Contributor Author

svobs commented Sep 18, 2024

Regarding the slider knob clipping problem, I found it and fixed it. I was able to reproduce it by changing Clips Bounds to YES for the slider's parent view. Recall that it was always YES in older versions of MacOS but defaults to NO now, which is most likely why it is not a problem for more recent versions.

I also found & fixed another minor regression: a horizontal line in the Audio tab did not go all the way across the sidebar.

@svobs svobs marked this pull request as ready for review September 18, 2024 03:25
@svobs svobs changed the title Improvements to speed UI (fixes #5168) Improvements to speed UI (fixes #5147, #5168) Sep 18, 2024
@low-batt
Copy link
Contributor

Glad you found the problem with clipping the thumb. I was just about to report that the clipping did not reproduce for me under macOS Sonoma. I tested the latest commits under Ventura and the clipping is fixed.

This is looking good. Need to check with @uiryuu as to how we can change the format specifiers in the files for other locals.

@uiryuu uiryuu merged commit a6092dd into iina:develop Oct 7, 2024
1 check passed
@uiryuu
Copy link
Member

uiryuu commented Oct 7, 2024

Looks nice, thank you! If there are still unfinished tasks regarding this issue, please open another PR.

MouJieQin pushed a commit to MouJieQin/iina-for-varchive that referenced this pull request Oct 21, 2024
* Improvements to speed UI

Details:
• Add speedResetBtn which will reset speed to 1x
• Update osd.speed message to support unlimited digits after decimal
• Update customSpeedTextField, speedSliderIndicator to support up to 9
  digits after decimal (will round to 9th decimal place)
• Increase width of customSpeedTextField to fit at least 6 total digits
• Update speed slider action to round to 2 digits after decimal

* Support unlimited decimal places for speed numbers in menus

* Use localized formatting of decimals in osd.speed msg & menu speeds

* Fix more holes in custom speed values. Localize decimal fmt of speed slider labels

* Fix clipping of tick sliders. Fix horizontal line widths

* Add tooltip to speedResetBtn

* Fix name, update comments based on feedback
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.

Change playback speed up to 5 decimal places
3 participants