Skip to content
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

Improve support for high-resolution stats #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andygrundman
Copy link

@andygrundman andygrundman commented Sep 14, 2024

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 referenced this pull request in andygrundman/moonlight-qt Sep 14, 2024
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.
andygrundman added a commit to andygrundman/moonlight-qt that referenced this pull request Sep 14, 2024
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 andygrundman force-pushed the andyg.hires-timing-and-stats branch 5 times, most recently from 54c8d07 to 8795c6c Compare September 15, 2024 01:31
@andygrundman
Copy link
Author

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

This comment was marked as off-topic.

@andygrundman 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 added a commit to andygrundman/moonlight-qt that referenced this pull request Sep 18, 2024
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 added a commit to andygrundman/moonlight-qt that referenced this pull request Sep 18, 2024
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 added a commit to andygrundman/moonlight-qt that referenced this pull request Sep 20, 2024
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 added a commit to andygrundman/moonlight-qt that referenced this pull request Sep 30, 2024
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
Copy link
Author

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 added a commit to andygrundman/moonlight-qt that referenced this pull request Oct 12, 2024
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 added a commit to andygrundman/moonlight-qt that referenced this pull request Oct 12, 2024
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 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 andygrundman force-pushed the andyg.hires-timing-and-stats branch 2 times, most recently from 309fcbb to 26a0007 Compare October 16, 2024 14:32
andygrundman added a commit to andygrundman/moonlight-qt that referenced this pull request Oct 16, 2024
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 Outdated Show resolved Hide resolved
src/Limelight.h Show resolved Hide resolved
* 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
Copy link
Author

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

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.

3 participants