Further reduce CPU usage when refreshing UI

[skyline75489]

• Don't update progress when player is paused
• Don't update progress when the main window is not visible.
• Replace componentFormatter with simple implementation to reduce overhead.

[skyline75489]

I was also trying to handle the state when the application is completely hidden (covered by other application). This causes the application to become "background" in Instrument:

But I failed to find a way to do so. Any idea?

[NattyNarwhal]

I was also trying to handle the state when the application is completely hidden (covered by other application).

Technically if the window is still open but obscured, it can still be rendered because vibrancy material in a window in front of it, or being pulled in exposé. Maybe occlusionState (which has a notification) can help?

[skyline75489]

Oh yeah, exposé. That's a feature that I haven't been used for quite some time. Maybe we can at lease reduce the refresh rate to 1sec when the window is not visible based on occlusionState. Even without handling occlusionState, the CPU usage is now at ~2% when the window is minimized with this PR. This is about the right amount of CPU usage I'd expect from a music app.

[NattyNarwhal]

Regarding occlusionState with some quick tests:

• You could probably add the windowDidChangeOcclusionState method to the database controller. (There is the applicationDidChangeOcclusionState equivalent for the app delegate, but it's not the right place to put it.)
• Looks like the value is a bitfield; only NSWindowOcclusionStateVisible is documented, there's also 0x2000 set, but it's undocumented. (Means AND it instead of equality.)
• Switching workspaces definitely makes the window be not visible in occlusionState. Switching window via the dock or Cmd+Tab will make it be not visible, but using exposé or selecting another to switch doesn't seem to trigger that occlusion event.
• Miniaturization and closing the window will both trigger non-visible occlusion too.

Overall, it looks like a better option that handles almost all window-not-visible states.

[skyline75489]

Seems that occlusionState is way better than Miniaturization. I'll find some time to update this PR later 🥂

[skyline75489]

You OK with other changes in this PR? Let me know if I'm doing something weird. I haven't really been an expert regarding macOS. I had my time developing on iOS several years ago.

[NattyNarwhal]

I left a comment regarding the change to clearPlaybackProgress; the miniaturization one we just addressed.

[skyline75489]

Could it be that you didn't submit the review? I didn't see any comment. I sometimes also forget the submission :)

[NattyNarwhal]

hahaha I forgot how the code review interface works (I'm usually on the other side of this)

[NattyNarwhal]

Are you sure it's a good idea to remove this? Whenever playback ends, it's usually uninstallProgressTimer calling clearPlaybackProgress, not updateProgress (though the timer might trigger first). If anything, it might be good to remove it from updateProgress since uninstallProgressTimer should take care of it (though it won't hurt).

[skyline75489]

My intention is to make uninstallProgressTimer more focused on, well uninstalling the progress timer and does nothing else. So I just moved this line somewhere else.

[skyline75489]

My intention is to make uninstallProgressTimer more focused on, well uninstalling the progress timer and does nothing else. So I just moved this line somewhere else.

[NattyNarwhal]

FWIW, retested:

• Switching workspaces definitely makes the window be not visible in occlusionState. Switching window via the dock or Cmd+Tab will make it be not visible, but using exposé or selecting another to switch doesn't seem to trigger that occlusion event.

This doesn't seem to be accurate after I checked again; occlusion behaves normally if completely obscured.

[NattyNarwhal]

I think this would install the timer even if it's stopped/paused when it becomes visible (et vice versa)

[NattyNarwhal]

Though I see that updateProgress will try to return early to avoid doing work. That does still make the timer wake up though, but if you're already playing, I guess it doesn't matter too much

[skyline75489]

Yeah, the return early part is to prevent high CPU usage under this scenario: play a song->minimize the window->current song finished, next song started while minimized (update timer will still be installed and start running)

[skyline75489]

Come to think about it, maybe we should only do the early return here and avoid all the timer juggling? Early return reduces the CPU usage basically as much as uninstalling the timer, at least from my testing. Though in the pursuit of perfection, no timer should be running, that I agree.

[NattyNarwhal]

You could check for playback before (un)installing the timer, but another way to do is is centralize the checks into early returns into those methods, so the caller doesn't need to be aware of the taste.

[skyline75489]

You don't like the central check here inside updateProgress? I think it kinda beats early checks in all the playback-related methods. Under the previous scenario I mentioned, only the check inside updateProgress prevents the unnecessary UI refreshing.

[NattyNarwhal]

Good idea, but it might be better to put those checks in the timer installation (or both) and avoid having the timer tick in the first place. I'm fine either way, but it might be worth experimenting to see the impact of a timer that returns early versus uninstalling it.

[skyline75489]

As far as I can tell, I see no obvious difference between early return in updateProgress and uninstalling the timer. The CPU usage are both quite low, at around 1%-2%.

[NattyNarwhal]

In that case it's probably fine then.

[NattyNarwhal]

I think this looks good to me.