VideoCommon: New FrameTime/VBlank Analyzer + Graph #11326
Conversation
|
An audio stretching change in a videocommon PR is very peculiar. Why is that here and not in a different PR? |
Because of the way that gamespeed calculations are integral to the audiostretching, and the fact that the gamespeed is now calculated in a new way, the audio stretching is affected. |
|
I'm unsure about the "FTimes" and "VTimes" terms and can't even think how to translate that when accounting for localization. I was going to suggest changing "Show FTimes" to "Show Frame Times" but then there's the VTimes setting which is also related to the frame times (AFAICT). Any suggestion for distinguishing them? |
|
The graphs need measures to be useful. Here's Digital Foundy's small frametime graph as a good example.
Though I'd recommend placing the measures on the inside of the box or something for aesthetic reasons. Also you don't need to invert it (0 at the top) the way they do if you don't want to, I'm just showing it as a good reference for a small graph like this. Also I concur that FTimes and Vtimes is silly. You have plenty of space so you should just say Frame Times and V-blank Times.
The exact same thing can be said about showing V-Blanks Per Second and Frames Per Second. They are very closely related so there's going to be a lot of overlap. Especially in a 50 or 60fps titles where they are -exactly the same-. I don't think there is really anything that can be done about that in this PR without challenging the paradigm, and I don't think this PR is the place to do that. One day a dedicated PR will challenge it and that is it certainly going to be a big discussion. The only solution I can recommend for this is to move to one big graph. So have the three options along the top horizontally, and then one big graph below them that's just frametimes. If you want more graphs, you could add a second graph below that that is based on the speed%. While the frametimes graph would be the most important for catching stuttering, the speed graph would act as a trendline for those battling to get to fullspeed. (not that a lot of people are doing that in Dolphin these days but you know) Especially if the speed graph has a longer sample window than the frametime graph (the size of the graphs would be the same, just, you know, more data in the same space to benefit their uses). |
This is enough to solve my concerns with the translation/localization. "Show Frame Times" and "Show V-Blank Times" are 100% more workable than just "Show FTimes" and "Show VTimes"... |
|
These all sound like good ideas, I'll probably be switching over to ImPlot to make these graphs and I'll add a single big graph underneath the FPS and VPS counters. I'll keep the standard deviation in the fps box, but the graph can be separate. |
Sam-Belliveau
changed the title
|
Is it possible to add a way to change the size of the graph? On small displays, the graph takes up a lot of space. |
|
I think it might be a good start to add some transparency to make it easier to see through, however I'll be sure to think about how we could resize things. 1X - Native: 2X - 720p: 3X - 1080p: This depends on the UI scale of your device, so it wont look like this for everyone. |
|
I'm concerned about that scaling you set up and how it will work on the extremely high resolutions that I use for screenshots and such. I'll give it a test after you rebase the PR. |
Ok I would like to clarify that the size of the FPS counter does not have anything to do with internal resolution. It depends on too things, the size of your window and the scaling of your desktop environment. I used "automatically fit windows to resolution" to get a few standard resolutions to test it, but if your window remains the same size, the graph will not change. |
|
This refactor allows the frame time graph to have relatively messy code, but have that messy code be isolated from the rest of the code base. I think that this PR is finally good for review. |
|
@Sam-Belliveau I need to get in touch with you, please contact me via IRC or MayImilae#2884 on Dolphin's unofficial discord. |
|
The performance graph looks really useful. It’s a nice addition. I have a question though. I’m running Ventura and I can get the frame pacing locked in through a fixed refresh rate. It’s plainly visible if the pacing is off through the 240 test suite drop shadow test. If it doubles up frames, there’s a visible discontinuity. No games flickers frames like this but it’s only to show how well paced it is. In the following video, Metal performance hud is enabled to show that I have my refresh rate set to 59.94Hz with good pacing. My question is about the numbers reported. It would make sense for the V-Blank numbers to match the display refresh rate since I can visually confirm that the pacing is perfect. pacing.movPerfect pacing to will also depend on your video viewer of course. You can confirm the video is valid by stepping through each frame. Is the mismatch due to some weirdness within Dolphin or the sampling code for getting the numbers? It doesn’t seem accurate. |
|
Maybe the point at which |
|
Need to ask someone who is a lot more familiar with the graphics system. It looks like v-blanks are consistently syncing up with the refresh rate but the performance graph/numbers says otherwise. |
| PerformanceMetrics(PerformanceMetrics&&) = delete; | ||
| PerformanceMetrics& operator=(PerformanceMetrics&&) = delete; | ||
|
|
||
| public: // Count Functions |
There was a problem hiding this comment.
No need to have public 4 times, just have it once
|
|
||
| void ImPlotPlotLines(const char* label) const; | ||
|
|
||
| private: // Functions for managing dt queue |
There was a problem hiding this comment.
Same here with the private/public. Also it's helpful if you group the functions by access type (where possible).
|
|
|
I would suggest to first make a pr to implement the fix relevant to ImmediateXFB , then add the implot submodule with the graph additions. A third pr could be the one aboit std:chrono. |
Sam-Belliveau
force-pushed
the
video-common-frame-pacing
branch
2 times, most recently
from
a938cee
to
0859b27
Compare
Sam-Belliveau
force-pushed
the
video-common-frame-pacing
branch
from
53ad688
to
673f81c
Compare
|
@Sam-Belliveau when you have a chance, could you address the review comments from @iwubcode so we can move forward with this PR? Thanks! |
I addressed the one where he said i should remove the GetAverageFrameTimeMS(), but the public / private, i just like to split up my classes between methods and types, I'm not too sure about that one. |
FWIW I also think the redundant "public"/"private" specifiers are unusual and don't fit the project's normal style. I think keeping the // comments you already have to logically distinguish the groups of methods is a good idea though. Any strong objection? (In general if you disagree with a code review comment - which is fine, many things in code are subjective preferences - please reply to it still so we know it was looked at and not just ignored.) |
|
Yeah sorry about that, I'll remove the redundant specifiers so we can get to merging. |
also to clarify, the reason the two seem so mismatched is because they have vsync on, and the timings get thrown out of wack with VSync, but thats an issue for the next PR. |
There was a problem hiding this comment.
Did a round of reviews myself, overall this change looks fine to me. I have a bunch of optional comments for you to look at and maybe fix in a future PR, I don't want to block this from being merged now (@JMC47 really wants it in before christmas :P). Many of these comments are very general and might apply in future code you write for the project, please try to incorporate this feedback into future PRs you contribute.
Thank you!
| PerformanceTracker m_speed_counter{nullptr, 6000000}; | ||
| }; | ||
|
|
||
| extern PerformanceMetrics g_perf_metrics; |
There was a problem hiding this comment.
Not a huge fan of adding more global state, in the future please attempt to avoid it as much as possible (by e.g. moving it to be a member of some other object that uses it).
|
|
||
| double PerformanceTracker::GetHzAvg() const | ||
| { | ||
| std::lock_guard lock{m_mutex}; |
There was a problem hiding this comment.
A bunch of these could be reader locks (std::shared_lock) to save a bit on contention. In practice it's unlikely to make a difference here, this seems like it's mostly single reader / single writer in the current workload. But using reader locks doesn't make anything more complicated, so in general I prefer doing it regardless.
std::shared_mutex is also marginally better in general for various historical reasons: https://stackoverflow.com/questions/69990339/why-is-stdmutex-so-much-worse-than-stdshared-mutex-in-visual-c
| int m_on_state_changed_handle; | ||
|
|
||
| // Name of log file and file stream | ||
| const char* m_log_name; |
There was a problem hiding this comment.
IMO having this be std::string (and thus having ownership of the string data) would be better, vs. the user of this object having to maintain the filename alive until this object is deleted. Right now it doesn't make a huge difference because (I think) this is only ever being used with constant strings, but in the future it could lead to confusion.
|
This seems to have caused the % speed indicator to change too slowly. Both FPS and VPS update about at the rate they should, but % speed takes a lot longer to change. This is easiest seen on the homebrew channel or the safety screen on the Wii Menu, where you can hold TAB to fast-forward to ~500 FPS. If you hold it for a few seconds and then release, the FPS and VPS values will quickly update to match, but the % speed will slowly increase even after you release and then will start to drop down afterwards. I suspect this also causes bad results with audio stretching, but I haven't tested extensively. |
By moving some functions around, I was able to count the FPS / VPS in a much more consistent way that fixed the issue where ImmediateXFB would report an incorrect speed. I decided to detach the Speed counter from the VPS counter as the Audio Stretcher relies on the speed values that the speed counter reports.
I also added a "Time Counter" along with a Standard Deviation meant to better help developers determine if the frame pacing is consistent or not. This is useful because the FPS is not the only measure of smoothness, but so is the consistency of the frames being presented.
Here is what the UI looks like.
Frame.Time.Graph.mp4
Because audio stretching now uses this new performance tracker, it is able to more smoothly transition in and out of different game speeds, further reducing the stutters that occur during gameplay.
This new frametime system can reveal interesting behavior, such as how Super Mario Sunshine runs at unlimited speed:
Those shapes are caused by the VPS having a "fast cycle" than a "slow cycle." This type of analysis is impossible with something like MangoHud because that is unable to record Frames vs VBlanks.
It can show frame consistency with VSync on/off. [this data is specific to macOS, but should also apply elsewhere]
VSync On (fullscreen): [notice 60fps😳 ]
VSync Off (fullscreen):
It can scale to make sure that everything fits on screen.
