Fix stuttering when starting to scroll a Game List #755

XenuIsWatching · 2021-05-21T07:06:58Z

Emulations station would stutter a lot when scorlling through game lists that had artwork with. This was due to it performing multiple reads of the screenshots, marquees, video, etc... For example, even if you were scrolling by 1, it would read these data 3 times. Each time it was calling the function updateInfoPanel which can kick of some File I/O which can be expensive (especially when you have the data on a NAS)

This gets it down to just 1 read. It also changes the behavior a little (for the better). There are three states in an input for scolling through a list: a rising edge where the button is pressed, a held high state where the button is held, and a falling edge where the button is released.

In the case where it was just press quickly, it was doing a read when the button was first pressed of the next item in the list. Then when it was released it would do another read of the next item in the list TWICE! The behavior now is to blank the info panel when the button is pressed, and then perform ONE read of the screenshot, etc. when the button is released.

Fixes #753

XenuIsWatching · 2021-05-21T07:07:23Z

I have this set to a draft because I want to run through some more test cases to make sure I didn't break anything else...

I also have before/after change videos demonstrating the noticeable improvement that will upload later.

pjft · 2021-05-21T09:47:38Z

es-app/src/views/gamelist/DetailedGameListView.cpp

@@ -212,15 +212,19 @@ void DetailedGameListView::initMDValues()

 void DetailedGameListView::updateInfoPanel()
 {
-	FileData* file = (mList.size() == 0 || mList.isScrolling()) ? NULL : mList.getSelected();
+	FileData* file = (mList.size() == 0 || (mList.getScrollingVelocity() != 0)) ? NULL : mList.getSelected();


Why this change? "isScrolling" seems to return a similar result - shouldn't we probably fix it at the isScrolling definition if there's an issue?

isScrolling also checks if ScrollTier is >0. What this means is that the user has held down the button and has now scrolled by at least 2 items. This is what was causing the stutter you begin to scroll, because it would load the the info.

I am considering add another function isSingleScrolling (or a better name)... (notice that mScrollTier is check to be == 0)

bool isSingleScrolling(void) { return (mScrollVelocity != 0 && mScrollTier == 0); }

Right now the current implementation, will blank the info panel momentarily even if the user is just scrolling by one. Another option would be to have it just to do an instant transition (and not blank), but just not update the info panel retain what was there previously, and only update when the user has let go of the button.
It would only blank the info panel if the user has scrolled by at least 2.
Thoughts??

What happens if you remove the mScrollTier check? If it needs to be there, I'd probably rather have isScrolling to receive an argument to distinguish whether you want to check mScrollTier or not as well.

pjft · 2021-05-21T09:50:44Z

es-core/src/components/IList.h

@@ -302,7 +298,7 @@ class IList : public GuiComponent
 			onScroll(absAmt);

 		mCursor = cursor;
-		onCursorChanged((mScrollTier > 0) ? CURSOR_SCROLLING : CURSOR_STOPPED);
+		onCursorChanged((amt != 0) ? CURSOR_SCROLLING : CURSOR_STOPPED);


Tell us more about this change.

I believe the intent of mScrollTier was to control and watch the speed of how much the user has scrolled. However mScrollTier is still 0 when the user has just scrolled by 1, and only increments to 1 after the user has scrolled by 2. It is a better indicator that check that amt is not 0 to indicate that the user is scrolling. As this function is also called when the user has stopped scrolling with amt == 0

XenuIsWatching · 2021-05-21T18:44:24Z

es-core/src/components/IList.h

@@ -191,10 +191,6 @@ class IList : public GuiComponent
 	{
 		PowerSaver::setState(velocity == 0);

-		// generate an onCursorChanged event in the stopped state when the user lets go of the key
-		if(velocity == 0 && mScrollVelocity != 0)
-			onCursorChanged(CURSOR_STOPPED);


I want to make sure that removing this didn't break anything else.

The call to stopScrolling() would also call onCursorChanged() so that is where it would be called once, but believe there are other places that only call listInput(0);

Gemba · 2021-05-22T17:09:36Z

First:
Thanks for documenting the variables. +1 Makes sense to me. However, I guess the wording needs some review (e.g. surplus "the"). My suggestion: Why not renaming mScrollTier to mScrollGear and mScrollVelocity to mScrollStep (as it is literally a fixed value and not a quotient over time)? This way mScrollGearAccumulator would almost explain itself.

Second:
I found a small regression in the DetailGamelistView.

Large gamelist (> 1 Screen):
Single step: Fade out/fade in (differs from master: no fading)
Page step: Fade out/fade in (differs from master: no fading)

Short list (< 1 Screen):
Single step: Fade out/fade in (differs from master: no fading)
Page step: No fading / hard skip (ok as in master)

These are ok:
Continous scrolling / Dimmed with Initials: ok (same as in master)
Roll-over (i.e. pushing up at first list item, pushing down on last list item): ok (same as in master)

The introduced fadeout/fadein for single actions make the ES feel less snappy IMHO.

XenuIsWatching · 2021-05-22T19:45:22Z

First:
Thanks for documenting the variables. +1 Makes sense to me. However, I guess the wording needs some review (e.g. surplus "the"). My suggestion: Why not renaming mScrollTier to mScrollGear and mScrollVelocity to mScrollStep (as it is literally a fixed value and not a quotient over time)? This way mScrollGearAccumulator would almost explain itself.

mScrollVelocity is really the direction you are scrolling. '-1forup, 1 for down, and 0 for stop scrolling. I could be renaming variables all over the place for clairty :/ ... but I didn't want to go down that rabbit hole. That could be rather a separate pull request, as I want this to stay focused on the issue at hand rather than updating the code for easier maintance and readability.

Second:
I found a small regression in the DetailGamelistView.

Large gamelist (> 1 Screen):
Single step: Fade out/fade in (differs from master: no fading)
Page step: Fade out/fade in (differs from master: no fading)

Thanks for testing this out!
About the single step, please see my comment below.
"same reason as above

Short list (< 1 Screen):
Single step: Fade out/fade in (differs from master: no fading)
Page step: No fading / hard skip (ok as in master)

Again, Thanks for testing this out!
Single step, same as above...
Page step, hmm... that's interesting that it was different behavior. I'll try to reproduce it, and investigate why that is happening.

These are ok:
Continous scrolling / Dimmed with Initials: ok (same as in master)
Roll-over (i.e. pushing up at first list item, pushing down on last list item): ok (same as in master)

There is a bug I noticed in master with the Continuous scrolling, and then hitting the very end of the list. When it hits the very end of the list, It will load the info panel, even if the user hasn't let go of the button. But then once the user lets go of the button, it will reload the info panel again. You can notice this if you have a long description, and you hold the button until it begins scrolling after a brief moment. When you let go of it, the description will reset back to beginning indicateing it reloaded.

But this is very minor bug IMO...

The introduced fadeout/fadein for single actions make the ES feel less snappy IMHO.

I thought this at first, and I did experiment with this using that isSingleScrolling function I mentioned earlier. What I did was just force it to retain the state of the info panel when the button was first pressed, and then blank once it is known that the user is holding it down (scrolling by at least 2). But IMHO, I actually felt the opposite and found that this implementation felt less snappier.
My thought went like this....

I press down a button, I see the next item selected... then I see the previous item still on the screen (albeit very briefly) wondering what's going on asking myself "Why am I still seeing old information on the screen, I already see the game selected?", and then I release the button, I finally see the update info after very brief moment again (hitting the file system). All of this happens in less than quarter a second...

For the case I have implemented, my thought went like this...

I press down a button, I see the info panel fade out (and I think to myself it must be doing something and I feel like it's moving), and then I finally release the button, I see the update info (which hits the file system) after very brief moment again. Again, all of this happens in less than quarter a second...

This is just IMHO as well, but if there is consensus from the maintainers/reviewers of this project, then I can easily make it perform the former action.

updateInfoPanel() is an expensive function due to access to file system for reading screenshots, videos, etc. Record what is already up there and if another request comes in for the same thing, then return early.

XenuIsWatching · 2021-05-27T06:20:17Z

I decided to go a simpler route which did no introduce blank in between single scrolling. All this does is record what it just wrote to the info panel, so that when it gets called again (for example, on a button release). it won't waste time updating the panel.

I found that in my system this function can take a while by adding in my own timers ( with <chrono>). I compared both my change and the base code. In the base code it calls it twice for button releases. I found that the first call (when the button is first pressed (rising edge), the time for this function can quite a long time under certain conditions in my system (50ms to 1000ms), but the subsequent calls that happened on the button release would take less (5ms to 35ms each). I can only speculate why the subsequent calls are so much shorter, but I would guess there is either some caching going on or my hard drive controller is doing some "magic". These subsequent calls are just rereading the same data and completely redundant and a waste of time.

Logs of my timing are below...
retropie_infopanel_mod_time.txt
retropie_infopanel_base_time.txt

Gemba · 2022-09-02T15:50:32Z

I tried the suggested change on a installation with the ProfilingUtil (#716).
As expected there was not a significant effect measurable (Rpi4 with UASP USB to SATA adapter):

es-app/src/views/gamelist/DetailedGameListView.cpp
Commit: 52c04d7
Message          	       Calls	  Total Time	    Avg Time	    Min Time	    Max Time	 Internal Total Time	   Internal Avg Time
updateInfoPanel()	         316	    9.973043	    0.031560	    0.000001	    0.108529	            9.973043	            0.031560

With this PR:
Message          	       Calls	  Total Time	    Avg Time	    Min Time	    Max Time	 Internal Total Time	   Internal Avg Time
updateInfoPanel()	         314	   10.098609	    0.032161	    0.000001	    0.104449	           10.098609	            0.032161

Could you @XenuIsWatching pls try on your setup with the NAS? And: Are you using a wired or wireless connection to your NAS?

To use the profile, in a nutshell:
cmake . -DPROFILING=on -DRPI=on -DGL=on, #include "utils/ProfilingUtil.h" in object under test, then ProfileScope("..."); in the method to profile. Start ES with --debug.

pjft reviewed May 21, 2021

View reviewed changes

XenuIsWatching commented May 21, 2021

View reviewed changes

XenuIsWatching changed the title ~~optimize calls to updateInfoPanel~~ Fix stuttering when starting to scroll a Game List May 21, 2021

XenuIsWatching force-pushed the optimize_game_list branch from 1a0775b to 083547c Compare May 26, 2021 07:21

return early if info panel already the same info

9f2ff94

updateInfoPanel() is an expensive function due to access to file system for reading screenshots, videos, etc. Record what is already up there and if another request comes in for the same thing, then return early.

XenuIsWatching force-pushed the optimize_game_list branch from 083547c to 9f2ff94 Compare May 27, 2021 06:09

XenuIsWatching marked this pull request as ready for review May 27, 2021 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stuttering when starting to scroll a Game List #755

Fix stuttering when starting to scroll a Game List #755

XenuIsWatching commented May 21, 2021

XenuIsWatching commented May 21, 2021 •

edited

pjft May 21, 2021

XenuIsWatching May 21, 2021

pjft May 21, 2021

pjft May 21, 2021

XenuIsWatching May 21, 2021 •

edited

pjft May 21, 2021

XenuIsWatching May 21, 2021

Gemba commented May 22, 2021 •

edited

XenuIsWatching commented May 22, 2021 •

edited

XenuIsWatching commented May 27, 2021

Gemba commented Sep 2, 2022 •

edited

Fix stuttering when starting to scroll a Game List #755

Are you sure you want to change the base?

Fix stuttering when starting to scroll a Game List #755

Conversation

XenuIsWatching commented May 21, 2021

XenuIsWatching commented May 21, 2021 • edited

pjft May 21, 2021

Choose a reason for hiding this comment

XenuIsWatching May 21, 2021

Choose a reason for hiding this comment

pjft May 21, 2021

Choose a reason for hiding this comment

pjft May 21, 2021

Choose a reason for hiding this comment

XenuIsWatching May 21, 2021 • edited

Choose a reason for hiding this comment

pjft May 21, 2021

Choose a reason for hiding this comment

XenuIsWatching May 21, 2021

Choose a reason for hiding this comment

Gemba commented May 22, 2021 • edited

XenuIsWatching commented May 22, 2021 • edited

XenuIsWatching commented May 27, 2021

Gemba commented Sep 2, 2022 • edited

XenuIsWatching commented May 21, 2021 •

edited

XenuIsWatching May 21, 2021 •

edited

Gemba commented May 22, 2021 •

edited

XenuIsWatching commented May 22, 2021 •

edited

Gemba commented Sep 2, 2022 •

edited