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

waveformseekbar: Enhance the redraw performance and frequence #2289

Merged
merged 9 commits into from Mar 4, 2017

Conversation

@ptitjes
Copy link
Collaborator

@ptitjes ptitjes commented Mar 2, 2017

I carefully enhanced the performance by using Cairo's clipping mechanism. Now, only the minimum required region of the waveform is queued for redraw.

Then the interval time between the redraws is computed depending on the width of the waveform and the length of the song. With a 2'50 song rendered on half of a MacBookPro 13", the redraw happens every ~250 milliseconds (4 times more than currently). On a full screen MacBookPro 13" it happens every ~90 milliseconds. Due to the careful handling of the clip rectangle, the occupation of the CPU shows no real difference.

ptitjes added 2 commits Feb 26, 2017
Fixes #2255.

* add a mechanism to update TimeTracker's interval
* enhance waveform redraw by using Cairo's clip rectangle
* shorten redraw interval to make the position travel smoothly
Fixes #2255.

* use separate time trackers for label and waveform
* do less updates depending on the incoming events
@@ -69,7 +82,7 @@ def __paused(self, player, paused):
else:
self.__stop = False
if self.__id is None:
self.__id = GLib.timeout_add_seconds(1, self.__update)
self.__id = GLib.timeout_add(self.__interval, self.__update)

This comment has been minimized.

@lazka

lazka Mar 2, 2017
Member

TimeTracker uses timeout_add_seconds() because that gets synced with all other timeout_add_seconds() calls (also from other apps) and allows us to wake up only once per second.

So in case it is 1000 it should still call timeout_add_seconds().

ptitjes added 4 commits Mar 2, 2017
The computation of the minimal area to redraw was expecting too much
regularity from the wake up from GLib. This was failing by one pixel
sometimes on very short songs (~10 seconds).
@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 2, 2017

@lazka OK, I fixed the reuse of the existing waking up every seconds. Also, I fixed two bugs in my code.
I think it is good now, if you want to review my additions. Thanks.

@lazka
Copy link
Member

@lazka lazka commented Mar 3, 2017

Thanks. Just one thing (I haven't looked at the drawing part), the invalidated area looks like its width is based on the elapsed time, is that on purpose?

out

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

@lazka Thanks, was a big bug ! :)
BTW, how do you highlight the invalidated areas like that ? Can't find that on my search engine...

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

OK, finally found it: GTK_DEBUG=updates in the environment.
So I confirm it now works as expected.

redraws

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

There are still some full redraws that could be avoided when seeking through the song.
But still it is far better than what it was before.
I will enhance the redraw behaviours on seeking later.

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

Well later is now...

redraws2

@lazka
Copy link
Member

@lazka lazka commented Mar 3, 2017

👍

@lazka
Copy link
Member

@lazka lazka commented Mar 3, 2017

If you press "ctrl+shift+i" while running QL, the gtk inspector will start where you can also enable visual updates etc..

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

"ctrl+shift+i" doesn't trigger the inspector here !

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

OK, to have Ctrl-Shift-D and Ctrl-Shift-I toggle the inspector, I needed that first:

gsettings set org.gtk.Settings.Debug enable-inspector-keybinding true
@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 3, 2017

After some more testing it seems to me that maybe one of _song_started or _song_changed could be ignored. I don't know which one however as I am not sure to fully understand the difference.

Other than that, I'm quite happy with the result.

@lazka
Copy link
Member

@lazka lazka commented Mar 4, 2017

After some more testing it seems to me that maybe one of _song_started or _song_changed could be ignored. I don't know which one however as I am not sure to fully understand the difference.

song-started is after app.player.song changes, song-changed is after any tags of the provided songs have changed.

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 4, 2017

song-started is after app.player.song changes, song-changed is after any tags of the provided songs have changed.

Yeah! I finally figured out as you can see in my last commit :)

@lazka lazka merged commit 8e45222 into quodlibet:master Mar 4, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Mar 4, 2017

Thank you!

@ptitjes ptitjes deleted the ptitjes:waveformredraw branch Mar 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants