Improve support for high-resolution stats #95

andygrundman · 2024-09-14T06:23:39Z

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);

NOTE: this patch depends on a patch to moonlight-common-c, see [url=https://github.com/moonlight-stream/moonlight-common-c/pull/95]PR[/url]. * Adds an audio stats overlay that works with all current renderers, showing common info such as bitrate and packet loss. It is blue and in the upper-right, and will appear whenever the video overlay is enabled. * Audio renderers are able to add more lines to the overlay (the upcoming CoreAudio patch uses this). * Added bitrate/FEC display to both video and audio stats. * Consolidated the 3 FPS lines into one to save a bit of space. * All time-based stats are now microsecond-based, improving accuracy of very fast events.

NOTE: this patch depends on a patch to moonlight-common-c, see [this PR](moonlight-stream/moonlight-common-c#95). * Adds an audio stats overlay that works with all current renderers, showing common info such as bitrate and packet loss. It is blue and in the upper-right, and will appear whenever the video overlay is enabled. * Audio renderers are able to add more lines to the overlay (the upcoming CoreAudio patch uses this). * Added bitrate/FEC display to both video and audio stats. * Consolidated the 3 FPS lines into one to save a bit of space. * All time-based stats are now microsecond-based, improving accuracy of very fast events.

andygrundman · 2024-09-15T08:46:12Z

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;

NOTE: this patch depends on a patch to moonlight-common-c, see [this PR](moonlight-stream/moonlight-common-c#95). * Adds an audio stats overlay that works with all current renderers, showing common info such as bitrate and packet loss. It is blue and in the upper-right, and will appear whenever the video overlay is enabled. * Audio renderers are able to add more lines to the overlay (the upcoming CoreAudio patch uses this). * Added bitrate/FEC display to both video and audio stats. * Consolidated the 3 FPS lines into one to save a bit of space. * All time-based stats are now microsecond-based, improving accuracy of very fast events.

andygrundman · 2024-09-30T11:29:25Z

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

NOTE: this patch depends on a patch to moonlight-common-c, see [this PR](moonlight-stream/moonlight-common-c#95). * Adds an audio stats overlay that works with all current renderers, showing common info such as bitrate and packet loss. It is blue and in the upper-right, and will appear whenever the video overlay is enabled. * Audio renderers are able to add more lines to the overlay (the upcoming CoreAudio patch uses this). * Added bitrate/FEC display to both video and audio stats. * Consolidated the 3 FPS lines into one to save a bit of space. * All time-based stats are now microsecond-based, improving accuracy of very fast events.

src/Platform.c

src/Limelight.h

src/RtspConnection.c

src/Limelight.h

NOTE: this patch depends on a patch to moonlight-common-c, see [this PR](moonlight-stream/moonlight-common-c#95). * Adds an audio stats overlay that works with all current renderers, showing common info such as bitrate and packet loss. It is blue and in the upper-right, and will appear whenever the video overlay is enabled. * Audio renderers are able to add more lines to the overlay (the upcoming CoreAudio patch uses this). * Added bitrate/FEC display to both video and audio stats. * Consolidated the 3 FPS lines into one to save a bit of space. * All time-based stats are now microsecond-based, improving accuracy of very fast events.

src/Platform.c

src/Limelight.h

* 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. Public API in Limelight.h: uint64_t LiGetMicroseconds(void); uint64_t LiGetMillis(void); const RTP_AUDIO_STATS* LiGetRTPAudioStats(void); // provides access to RTP data for the overlay stats const RTP_VIDEO_STATS* LiGetRTPVideoStats(void); Note: Users of this library may need to make changes. If using LiGetMillis() to track the duration of something that is shown to the user, consider switching to LiGetMicroseconds(). Remember to divide by 1000 at time of display to show in milliseconds.

This still holds a millisecond value sourced from RTP timestamp, but since we're changing the API anyway, now is a good time to future-proof this field.

…h_absolute_time() per Apple's recommendation. Move has_monotonic_time variable into the final variant of platform-specific time code (used by Linux/Unix).

andygrundman · 2024-10-23T05:21:02Z

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".

NOTE: this patch depends on a patch to moonlight-common-c, see [this PR](moonlight-stream/moonlight-common-c#95). * Adds an audio stats overlay that works with all current renderers, showing common info such as bitrate and packet loss. It is blue and in the upper-right, and will appear whenever the video overlay is enabled. * Audio renderers are able to add more lines to the overlay (the upcoming CoreAudio patch uses this). * Added bitrate/FEC display to both video and audio stats. * Consolidated the 3 FPS lines into one to save a bit of space. * All time-based stats are now microsecond-based, improving accuracy of very fast events.

andygrundman mentioned this pull request Sep 14, 2024

Support high-resolution stats; add audio stats framework moonlight-stream/moonlight-qt#1410

Open

andygrundman force-pushed the andyg.hires-timing-and-stats branch 5 times, most recently from 54c8d07 to 8795c6c Compare September 15, 2024 01:31

This comment was marked as off-topic.

Sign in to view

andygrundman force-pushed the andyg.hires-timing-and-stats branch 3 times, most recently from 8c8860f to e5b54d7 Compare September 17, 2024 18:41

andygrundman force-pushed the andyg.hires-timing-and-stats branch from e5b54d7 to c386c2e Compare September 30, 2024 11:21

cgutman requested changes Oct 16, 2024

View reviewed changes

src/Platform.c Outdated Show resolved Hide resolved

src/Platform.c Outdated Show resolved Hide resolved

src/Limelight.h Outdated Show resolved Hide resolved

src/RtspConnection.c Outdated Show resolved Hide resolved

src/Limelight.h Show resolved Hide resolved

andygrundman force-pushed the andyg.hires-timing-and-stats branch 2 times, most recently from 309fcbb to 26a0007 Compare October 16, 2024 14:32

cgutman reviewed Oct 19, 2024

View reviewed changes

src/Platform.c Outdated Show resolved Hide resolved

src/Limelight.h Show resolved Hide resolved

andygrundman force-pushed the andyg.hires-timing-and-stats branch from 26a0007 to be19311 Compare October 20, 2024 23:30

andygrundman added 2 commits October 20, 2024 19:41

Use uint64_t for DECODE_UNIT presentationTimeMs

c1a2873

This still holds a millisecond value sourced from RTP timestamp, but since we're changing the API anyway, now is a good time to future-proof this field.

macOS/iOS: use clock_gettime_nsec_np(CLOCK_UPTIME_RAW) instead of mac…

583754f

…h_absolute_time() per Apple's recommendation. Move has_monotonic_time variable into the final variant of platform-specific time code (used by Linux/Unix).

andygrundman force-pushed the andyg.hires-timing-and-stats branch from be19311 to 583754f Compare October 20, 2024 23:41

andygrundman requested a review from cgutman October 23, 2024 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for high-resolution stats #95

Improve support for high-resolution stats #95

andygrundman commented Sep 14, 2024 •

edited

Loading

andygrundman commented Sep 15, 2024

This comment was marked as off-topic.

andygrundman commented Sep 30, 2024

andygrundman commented Oct 23, 2024

Improve support for high-resolution stats #95

Are you sure you want to change the base?

Improve support for high-resolution stats #95

Conversation

andygrundman commented Sep 14, 2024 • edited Loading

andygrundman commented Sep 15, 2024

This comment was marked as off-topic.

andygrundman commented Sep 30, 2024

andygrundman commented Oct 23, 2024

andygrundman commented Sep 14, 2024 •

edited

Loading