Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

I moved the function to video, and reverted the stuff in utility.h affecting tools. No need to include rational.h now either. The only weird thing left is in src/platform/windows/display_base.cpp where I needed to do this due to video namespace issues.

AVRational fps = ::video::framerateX100_to_rational(config.framerateX100);

Alternatively instead of x100 workaround you can introduce new fractional SDP parameter (or two). This should remove intermediate steps and extend the usability, like creating new virtual display modes in runtime.

Alternatively instead of x100 workaround you can introduce new fractional SDP parameter (or two). This should remove intermediate steps and extend the usability, like creating new virtual display modes in runtime.

I don't follow. You actually need to avoid the use of any floating point numbers because they would introduce rounding errors that get worse over time. NTSC for example if you just tried to stream at 59.94 you would be running slightly off from your TV, as 59.94 is only an approximation. Nvidia had the right idea with this option, we just never implemented it before.

I mean just pass a pair of integer numbers

a=x-ml-video.targetFramerateFractional:60000 1001

or

a=x-ml-video.targetFramerateFractionalNum:60000
a=x-ml-video.targetFramerateFractionalDen:1001

I mean just pass a pair of integer numbers

a=x-ml-video.targetFramerateFractional:60000 1001

or

a=x-ml-video.targetFramerateFractionalNum:60000
a=x-ml-video.targetFramerateFractionalDen:1001

Are you trolling? We have a simpler method and I already got it working. If your comment is about a way to avoid floats please look at the patch. I just hardcoded the few NTSC cases where it matters.

I don't have a problem with supporting the clientRefreshRateX100 option, but I do agree with @ns6089 that the fractional form with separate targetFramerateFractionalNum and targetFramerateFractionalDen is preferable if the client can provide it (SDL3 does).

Are you trolling

I'm not. You're introducing technical debt into client-server contract by adding support for poorly designed GFE attribute. If clients desires particular precision, it should request particular precision without intermediate guesswork.

Either way here are the related changes to moonlight-common-c, so if @cgutman greenlights them, you won't need to write any extra code.

It looks like this PR has been idle for 90 days. If it's still something you're working on or would like to pursue, please leave a comment or update your branch. Otherwise, we'll be closing this PR in 10 days to reduce our backlog. Thanks!

I'd like to resurrect this PR. I've been running it lately while working on Xbox frame pacing, and it's essential. Without it, streaming a 60fps stream to a 59.94 client will have to drop 1 frame every 16 seconds.

Here is the Xbox side of the code, for reference, mostly to try to argue against putting num/den in the request param:

	if (res->GetRefreshRate() > 0.0 && info.vendorId == GAMING_DEVICE_VENDOR_ID_MICROSOFT) {
		// Pass fractional refresh rate to host in case it's supported
		switch (config.fps) {
		case 120:
			config.clientRefreshRateX100 = (int)(res->GetRefreshRate() * 100.0);
			break;
		case 60:
			config.clientRefreshRateX100 = 5994; // enable 60fps stream in 120hz
			break;
		default:
			config.clientRefreshRateX100 = sConfig->FPS * 100;
			break;
		}

If the client is supposed to work out the correct num/den values for something like 119.88 I think it's a mistake. You might think you would just have each client pull in ffmpeg so they can call av_d2q() but this will actually tell you 29.97 is 2997/100 when the value should be 30000/1001. So each client dev will just need to know this one weird trick for the NTSC. I think there are only 2 such special rates, 23.976 and 29.97, the rest are multiples of these. Are there any other cases? I think it's a lot cleaner to let that live in this small Sunshine bit:

  inline AVRational framerateX100_to_rational(const int framerateX100) {
    switch (framerateX100) {
      case 11988:
        return AVRational {120000, 1001};
      case 5994:
        return AVRational {60000, 1001};
      case 2997:
        return AVRational {30000, 1001};
      case 2397:  // assume these mean 23.976 film
      case 2398:
        return AVRational {24000, 1001};
      default:
        return av_d2q((double) framerateX100 / 100.0f, 1 << 26);
    }
  }

Actually I can remove 5994 and 11988 here because they're just doubling 2997, and we can assume in the future we'll see this doubling continue.

Two of my displays in default mode(s). I don't think server-side extrapolation is viable.

If a client is absolutely sure it wants NTSC framerate or similar, it can simply request that in the num/denum form. That's why I pushed for that new rational number parameter.

The alternative is sending all display timings, in this case the server can both calculate the refresh rate and reproduce the mode in virtual display, all with perfect precision. But that's not something every client would want to partake in.

You don't normally see NTSC rates on monitors, it's mostly a North American TV thing. So for your example of 59.95, my LG monitor's 94.98 option, things are easy: simply multiply by 100. Monitors running at arbitrary rates like this will run at exactly their listed fractional rate, and won't have any funny business the way NTSC does.

Bundle Report

Bundle size has no change ✅

Codecov Report

❌ Patch coverage is 50.00000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 12.10%. Comparing base (8dd75c4) to head (fb93ee7).
⚠️ Report is 76 commits behind head on master.

@@            Coverage Diff             @@
##           master    #4019      +/-   ##
==========================================
+ Coverage   12.00%   12.10%   +0.09%     
==========================================
  Files          87       87              
  Lines       17581    17610      +29     
  Branches     8082     8095      +13     
==========================================
+ Hits         2111     2131      +20     
- Misses      14573    14756     +183     
+ Partials      897      723     -174     

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Just to reiterate my point - X100 precision is not enough. Your NTSC rate needed special handling only because X100 precision is not enough. And now, if/when someone finally comes up with a better precision parameter, every server will still be expected to have X100 legacy code block. Not the end of the world I guess, but that's rather annoying kind of technical debt to have.

Just to reiterate my point - X100 precision is not enough. Your NTSC rate needed special handling only because X100 precision is not enough. And now, if/when someone finally comes up with a better precision parameter, every server will still be expected to have X100 legacy code block. Not the end of the world I guess, but that's rather annoying kind of technical debt to have.

Indeed, X100 is not enough for NTSC, in fact no amount of precision would be enough. 60000/1001 is impossible to represent in IEEE floating-point. It's something like 59.940059940059939 and keeps going. This is a relic of the analog age and we don't need to worry about it beyond these few cases I think. "Every server" can just apply this commit, I don't see the problem.

This PR broke several hardcoded config initializer expressions, because of the newly-inserted field in the config.

I'll PR a fix today.

This PR broke several hardcoded config initializer expressions, because of the newly-inserted field in the config.

What are you talking about, this PR doesn't add any config?

Edit: I see what you mean now, the real bug is in the inflexible way those test objects are created.