[GUIInfoManager] update label from fps to gui fps

[un1versal]

Since #6872 the system information is reporting ~ 4.88fps - 7.5fps which is correct since Default Dirty Regions is 3 which makes this low value slightly confusing at first.
Setting dirty regions to 0 confirms this as FPS go up to ~59.94 - 60

With a label of gui fps to make it clear what the system information value reports.

This isnt runtime tested yet (linux), will test earliest things tomorrow and report back.

Edit Now tested.

@FernetMenta

[fritsch]

That's a start. As this change will confuse a whole lot of users, that even don't know DirtyRegions 3 is active by default, I would suggest to add Dirty Regions: on (x), e.g. Dirty Regions: on (3) to the Sys Info you are altering with your patch.

For the Codec Screen and Debug overlay the fps should either go (as they only help to debug multi layer architectures to see if the gui is running amok) or also DR: X be added.

Wiki needs a lot of documentation here, as it's hard to understand by normal users that will whine: My fps is gone :-(

[FernetMenta]

those kind of issues happen if internals are exposed to users. how often did we explain the meaning of speed on the codec screen? it is amazing how users insist on useless info like this fps counter or temperature of cpu/gpu. how about creating some arbitrary value, give it some name which sounds important, and put it on the codec screen too?

the value fps as printed on the display is taken from an variable called m_fps in GuiInfoManager. I think we should keep those in sync.

[fritsch]

The speed up thing was absolutely not understandable and even wrong (!) remember the fix we pushed lately concerning the round(fps)/round(something)

That value was right at a time, where no 23.976 fps mode existed and so speed up was anavoidable ... but that still made the correspondence between vblank "clock" and frame display not very obvious - I needed a long time to check that ... and if we would talk in detail, i'd still have a lack of knowledge there.

[MartijnKaijser]

I would even say remove the fps from system info. Temperature is still kind of helpful to see if it's overheating

[un1versal]

I would even say remove the fps from system info. Temperature is still kind of helpful to see if it's overheating

That was my first guess too remove the thing but there's no need, if you change DR it shows up normally.

The value fps as printed on the display is taken from an variable called m_fps in GuiInfoManager. I think we should keep those in sync.

Yes that is good.

those kind of issues happen if internals are exposed to users. how often did we explain the meaning of speed on the codec screen? it is amazing how users insist on useless info like this fps counter or temperature of cpu/gpu. how about creating some arbitrary value, give it some name which sounds important, and put it on the codec screen too?

But there is nothing wrong in exposing internals to users, just because it may confuse people at first.. I think its great people know what's going on with their system, and if they dont understand we should endeavour to educate them, by wiki information or any other means.

I spent many hours researching and re-writing up codecinfo page in wiki for this very reason, if 3 people out of 100 read it and understand it, jobs is a good one.
Also fro the greatest time Kodi did not have help text for settings in gui, now it does... For that same reason I think.

These internals you hate exposing, as I see it, are here to help troubleshooting as much as captivate interest for what's under the bonnet, you never know where curiosity will take the next contribution or like me help me develop more interests for kodi..

[fritsch]

The problem is something else. Codec Screen is no benchmark. On e.g. the cubox (before the menu delay) that would even produce frame drop to 35 fps from 50 if opened. So it highly influences measurement. I currently use it most to see the "skipping" indicator.

So that is basically a good info. If it stays in, another DR: (method) should be added, so that even devs are not confused with the new fps. If the m_fps syncing is easy to accomplish it should be fine without additional bikeshedding.

[FernetMenta]

there is no "new" fps. the old value was buggy and this was fixed. the problem is that you associated some buggy value with good/bad operation of whatever.

[fritsch]

Basically all did that. I think there are only two people on this planet that understood the old value, this is including you on weekends and you during weekdays.

[FernetMenta]

Once you get behind the secret of this value, you'll admit that it is useless. And having it with fractional digits is complete nonsense. Users have always been wondering why FPS seemed "unstable" though vsync is active.

[Paxxi]

I'm with @MartijnKaijser that fps should be dropped, looking at your fps on an idle system is hardly useful for anything and raises questions.
DirtyRegions should under no circumstance be shown in the gui, that will only cause pain and suffering on the forums

[fritsch]

@Paxxi If we decide to keep the fps value, then this DR needs to go to the debugging screens. Btw. it's quite useful still, when you are in gui and e.g. the weather addon renders everything dirty ... and a PI gets 100% load - see OpenELEC issue tracker (one of the last ones).

OpenELEC/OpenELEC.tv#4091

[MartijnKaijser]

So it's only useful for devs. Then hide it when debug not enabled

[un1versal]

I can easily make
https://github.com/uNiversaI/kodi/blob/d2e3e566716fe26e884d5bc0627c78ea1b0c3fa5/xbmc/GUIInfoManager.cpp#L1751-L1762

into

      strLabel = StringUtils::Format("%ix%i@%.2fHz - %s",
        CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenWidth,
        CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenHeight,
        CDisplaySettings::Get().GetCurrentResolutionInfo().fRefreshRate,
        g_localizeStrings.Get(244).c_str()
    else
      strLabel = StringUtils::Format("%ix%i - %s (%02.2f gui fps)",
        CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenWidth,
        CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenHeight,
        g_localizeStrings.Get(242).c_str()

So the system info fps is gone from there.

[FernetMenta]

either remove it completely or leave it as is. every further special case on this makes it worse.

[un1versal]

To remove is what I posted in previous comment, right? Shall I do that then? @FernetMenta

https://github.com/uNiversaI/kodi/blob/d2e3e566716fe26e884d5bc0627c78ea1b0c3fa5/xbmc/GUIInfoManager.cpp#L1751-L1762

into

  strLabel = StringUtils::Format("%ix%i@%.2fHz - %s",
    CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenWidth,
    CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenHeight,
    CDisplaySettings::Get().GetCurrentResolutionInfo().fRefreshRate,
    g_localizeStrings.Get(244).c_str()
else
  strLabel = StringUtils::Format("%ix%i - %s (%02.2f gui fps)",
    CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenWidth,
    CDisplaySettings::Get().GetCurrentResolutionInfo().iScreenHeight,
    g_localizeStrings.Get(242).c_str()

[FernetMenta]

hmm, I think there is too much resistance for removal. I would keep it as is. Maybe only round to nearest integer for display.

[Paxxi]

@fritsch it shouldn't be visible during normal use, if its good for debugging then add it to the fps/memory overlay active during debug

[un1versal]

hmm, I think there is too much resistance for removal. I would keep it as is. Maybe only round to nearest integer for display.

I was the one that suggested removing it in first place in IRC.
If someone tell me how to modify the code to do what you propose Ill do it.

@fritsch it shouldn't be visible during normal use, if its good for debugging then add it to the fps/memory overlay active during debug

It already is there. Has always been there.

[un1versal]

Pointless keeping this open. The sys info fps is correct despite appearances.

I think the whole system information needs a refactor, but most of the values aren't, skinable and all hard coded so there's even less chance of improving this on another level.