Skip to content

Conversation

@ns6089
Copy link
Contributor

@ns6089 ns6089 commented Jul 8, 2025

Related to LizardByte/Sunshine#4019

This introduces two new SDP media attributes x-ml-video.targetFrameRateNum and x-ml-video.targetFrameRateDen, and updates Limelight.h to make full use of them.

@ClassicOldSong
Copy link

ClassicOldSong commented Jul 14, 2025

Came across this PR, but num/den isn't always available on any type of screens(like from Android devices) or some embedded devices that have screens connected directcly to the MCU. Currently Apollo/Artemis handles customized refresh rate by sending the refresh rate value in x1000 form, which should cover most fractional refresh rate scenarios. Yeah it's another poorly designed API but it's the only way to keep compatibility.

In embedded systems, refreshing sometimes is even controlled by a simple RC circuit inside the screen itself which chages with temperature.

Also there're other problems around fractional refresh rates, like this issue in Special-K, which it doesn't read num/den at all.

@cgutman
Copy link
Member

cgutman commented Jul 15, 2025

@ClassicOldSong Are you sending it via SDP like x-nv-video[0].clientRefreshRateX100 or some other way?

I was thinking we could provide both float (converted to decimal x100/x1000) and num/denom type referesh rate options and let the client pick which one to populate based on whatever the OS gives them.

@ClassicOldSong
Copy link

I'm sending it through the ordinary refresh rate, since nobody has monitors that exceeds 1000fps so when the value is greater than 1000, it's fractional.

@ns6089
Copy link
Contributor Author

ns6089 commented Jul 15, 2025

But it's possible to convert float to num/denom format too, probably without even losing precision when it's float32 to int32/int32. I personally don't see a reason for adding some special handling for floats in the protocol, but maybe some helper functions for converting float to num/denom and back in the library is not such a bad idea.

@ns6089
Copy link
Contributor Author

ns6089 commented Jul 15, 2025

probably without even losing precision when it's float32 to int32/int32

Dumb conversion through double precision arithmetic and INT32_MAX numerator is perfectly reversible in 1.0f - 181.035903931f range. Dynamically adjusting numerator raises the ceiling of perfect reversibility to at least 1000.0f (didn't try further because it becomes impractical at that point).

@ns6089
Copy link
Contributor Author

ns6089 commented Jul 15, 2025

Also some interesting info.

Displays use the following formula for refresh rate calculation:
pixel_clock_units * 10'000 / ((vert_resolution + vert_blank) * (horiz_res + horiz_blank))

Now to what makes this relevant to us. Since all of the above variables are integer numbers, it provides limits to what can or can not be encoded losslessly in fractions of different integer sizes.

4K@240 and 8K@60 modes both fall within int32/int32 limits, if barely.

@ns6089
Copy link
Contributor Author

ns6089 commented Jul 15, 2025

Rebased and added utility function for converting floating-point rates to fractions. Everything is probably as good as it can reasonably get. Fraction to floating-point conversion is trivial hence no helper. Heuristic converter that assumes that double precision refresh rate was produced from hardware display mode is possible, but probably out of scope for this library.

@ns6089 ns6089 force-pushed the frac branch 6 times, most recently from 134f9ed to 2787cee Compare July 16, 2025 14:41
@cgutman
Copy link
Member

cgutman commented Aug 1, 2025

Looks like MSVC wants a few more explicit casts to int

@ns6089
Copy link
Contributor Author

ns6089 commented Aug 1, 2025

Looks like MSVC wants a few more explicit casts to int

Fixed.

@andygrundman
Copy link
Contributor

andygrundman commented Oct 10, 2025

I just noticed this PR when updating my other one #4019. I'm sorry but this is missing the point. First off, ffmpeg has a conversion function, it doesn't make sense to reinvent the wheel. Putting the math aside, neither ffmpeg nor this PR account for NTSC special cases, which for 99% of people is the only time they are likely to run into a fractional refresh rate.

(I did see someone in Discord today with a 59.17hz device, so a user like this would want to request 59.17fps. Or really Moonlight should do it automatically.) See PR#4019.

@ns6089
Copy link
Contributor Author

ns6089 commented Oct 10, 2025

First off, ffmpeg has a conversion function, it doesn't make sense to reinvent the wheel.

moonlight-android for example doesn't use ffmpeg. If you think that code should be copied over, I have nothing against that, but it's really not the point of this PR.

Putting the math aside, neither ffmpeg nor this PR account for NTSC special cases, which for 99% of people is the only time they are likely to run into a fractional refresh rate.

NTSC case is covered, and I don't think your estimation is correct. There's no such thing as perfectly even refresh rates in display modes to begin with.

I did see someone in Discord today with a 59.17hz device, so a user like this would want to request 59.17fps

winapi reports 3 digits after the decimal point, not two. So they would want to request at least that. Or add logic to their client to calculate it more precisely from display timings. Android reports refresh rate in float32 precision. Macos in float64. Cropping that to two digits is a crime.

@ClassicOldSong
Copy link

The x100 thing is a Nvidia leftover I think

Honestly, as GameStream has already stopped service, we can move on to more correct and universal solutions.

@ns6089
Copy link
Contributor Author

ns6089 commented Oct 13, 2025

Honestly, as GameStream has already stopped service, we can move on to more correct and universal solutions.

Well, that was the idea... But since in the meantime LizardByte/Sunshine#4019 got quietly merged behind my back, this PR is probably on hold until someone feels like adding support for it to their server/client. Also if anyone believes they could do better, just open another PR and I will close this one in favor of that.

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.

4 participants