Improve support for high-resolution stats

andygrundman commented 2 months ago

NOTE: Users of this library may need to make changes. I've also submitted a sister patch for moonlight-qt that supports these changes and implements a few new features like an audio overlay.

This patch adds a new microsecond-resolution function call, LiGetMicroseconds(), to complement the existing LiGetMillis(). Many variables used by stats have been updated to work at this higher resolution and now provide better results when displaying e.g. sub-millisecond frametime stats. To try and avoid confusion, variables that now contain microseconds have been renamed with a suffix of 'Us', and those ending in 'Ms' contain milliseconds. I originally experimented with nanoseconds but it felt like overkill for our needs.
Since this library is designed to be mostly standalone, I reorganized Platform.c a bit to make it compatible with SDL's GetTicks64(), which starts its ticker at program start. A lot of the stats here are used with those in moonlight-qt so I tried to simplify the functions as much as possible. Each platform now has its own few smaller functions, instead of trying to fit a complex set of ifdef's inside the same function.
I added a simple gtest suite for the Platform.c changes, and this test suite should be easy to extend to other areas of the code.

Internal API:

void Plt_TicksInit(void);          // store initial timestamp
uint64_t Plt_GetTicks64_us(void);  // The most precision, in microseconds
uint64_t Plt_GetTicks64_ms(void);  // Plt_GetTicks64_ms returns the value in milliseconds
uint64_t Plt_GetTicks64(void);     // Alias to _ms() and compatible with SDL_GetTicks64 (ms since program start)
uint64_t PltGetMillis(void);       // replaced by Plt_GetTicks64_ms()

Public API in Limelight.h:

uint64_t LiGetMicroseconds(void);
uint64_t LiGetMillis(void);
PRTP_AUDIO_STATS LiGetRTPAudioStats(void);  // provides access to RTP data for the overlay stats PRTP_VIDEO_STATS LiGetRTPVideoStats(void);

andygrundman commented 2 months ago

I think the build issues might be fixed now for Win/Mac/Linux.

I wanted to mention a new error I ran into thanks to the -fanalyzer mode on one of the Linux builds. It ended up really impressing me with what the compiler is able to spot now. In this bit of code there is a loop from 0-7 accessing an array of speaker channels.

for (i = 0; i < opusConfig->channelCount; i++) {
        opusConfig->mapping[i] = CHAR_TO_INT(*paramStr);
        paramStr++;
}

The code was correct of course, but the compiler had no way to know what channelCount might be and that we wouldn't overflow the array. This caused the following error that was pretty confusing, especially when I hadn't changed anything nearby. (I think it was just part of an appveyor gcc update since the last time this got built.) Luckily -Werror=stringop-overflow= was a helpful starting point.

/home/appveyor/projects/moonlight-common-c/src/RtspConnection.c: In function ‘parseOpusConfigFromParamString’:
/home/appveyor/projects/moonlight-common-c/src/RtspConnection.c:691:32: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
  691 |         opusConfig->mapping[i] = CHAR_TO_INT(*paramStr);
      |                                ^
In file included from /home/appveyor/projects/moonlight-common-c/src/Platform.h:81,
                 from /home/appveyor/projects/moonlight-common-c/src/Limelight-internal.h:3,
                 from /home/appveyor/projects/moonlight-common-c/src/RtspConnection.c:1:
/home/appveyor/projects/moonlight-common-c/src/Limelight.h:334:19: note: at offset 8 into destination object ‘mapping’ of size 8
  334 |     unsigned char mapping[AUDIO_CONFIGURATION_MAX_CHANNEL_COUNT];
      |                   ^~~~~~~

The fix turned out to be exactly the kind of safety code that should exist here. Now it's much more obvious how the loop variable works and the compiler was happy with it. This new if statement is at the beginning of the function, about 20 lines away from the loop. Pretty neat!

    if (channelCount > AUDIO_CONFIGURATION_MAX_CHANNEL_COUNT) {
        channelCount = AUDIO_CONFIGURATION_MAX_CHANNEL_COUNT;
    }
    opusConfig->channelCount = channelCount;

Rhea4life commented 2 months ago

Hey I will not be using this email for you anymore so please just email me at @.***

please and thank you.

From: Andy Grundman @.> Sent: Sunday, September 15, 2024 4:46 AM To: moonlight-stream/moonlight-common-c @.> Cc: Subscribed @.***> Subject: Re: [moonlight-stream/moonlight-common-c] Improve support for high-resolution stats (PR #95)

I think the build issues might be fixed now for Win/Mac/Linux.

I wanted to mention a new error I ran into thanks to the -fanalyzer mode on one of the Linux builds. It ended up really impressing me with what the compiler is able to spot now. In this bit of codehttps://github.com/moonlight-stream/moonlight-common-c/blob/8795c6c28d61671cedbf473e478700ae3e1f491a/src/RtspConnection.c#L667-L696 there is a loop from 0-7 accessing an array of speaker channels.

for (i = 0; i < opusConfig->channelCount; i++) { opusConfig->mapping[i] = CHAR_TO_INT(*paramStr); paramStr++; }

The code was correct of course, but the compiler had no way to know what channelCount might be and that we wouldn't overflow the array. This caused the following error that was pretty confusing, especially when I hadn't changed anything nearby. (I think it was just part of an appveyor gcc update since the last time this got built.) Luckily -Werror=stringop-overflow= was a helpful starting point.

/home/appveyor/projects/moonlight-common-c/src/RtspConnection.c: In function ‘parseOpusConfigFromParamString’: /home/appveyor/projects/moonlight-common-c/src/RtspConnection.c:691:32: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] 691 | opusConfig->mapping[i] = CHAR_TO_INT(*paramStr); | ^ In file included from /home/appveyor/projects/moonlight-common-c/src/Platform.h:81, from /home/appveyor/projects/moonlight-common-c/src/Limelight-internal.h:3, from /home/appveyor/projects/moonlight-common-c/src/RtspConnection.c:1: /home/appveyor/projects/moonlight-common-c/src/Limelight.h:334:19: note: at offset 8 into destination object ‘mapping’ of size 8 334 | unsigned char mapping[AUDIO_CONFIGURATION_MAX_CHANNEL_COUNT]; | ^~~

The fix turned out to be exactly the kind of safety code that should exist here. Now it's much more obvious how the loop variable works and the compiler was happy with it. This new if statement is at the beginning of the function, about 20 lines away from the loop. Pretty neat!

if (channelCount > AUDIO_CONFIGURATION_MAX_CHANNEL_COUNT) {
    channelCount = AUDIO_CONFIGURATION_MAX_CHANNEL_COUNT;
}
opusConfig->channelCount = channelCount;

— Reply to this email directly, view it on GitHubhttps://github.com/moonlight-stream/moonlight-common-c/pull/95#issuecomment-2351469658, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BGIDZXTCGKYMMRZNVERX6VDZWVCO5AVCNFSM6AAAAABOGSAQ6WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJRGQ3DSNRVHA. You are receiving this because you are subscribed to this thread.Message ID: @.***>

andygrundman commented 2 months ago

I've removed the test suite in my patch for a few reasons:

couldn't get it to build on win64, some MSVC/cmake/appveyor conflict, I dunno.
Probably weird to have a C++ test suite for a C library.
the tests were time-based which means they will break and annoy someone in the future

andygrundman commented 1 month ago

Apologies for the delay, I am not entirely sure if marking everything resolved lets you know or if I am supposed to use "re-request review".

moonlight-stream / moonlight-common-c

Improve support for high-resolution stats #95