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

Fix Waveform Seekbar: now it slides #2138

Closed
wants to merge 3 commits into from
Closed

Fix Waveform Seekbar: now it slides #2138

wants to merge 3 commits into from

Conversation

qwhex
Copy link
Collaborator

@qwhex qwhex commented Dec 3, 2016

Update seekbar position for every pixel "elapsed", so update frequency is based on the seekbar width. TimeTracker was replaced by a custom solution.

Update seekbar position for every pixel "elapsed", so update frequency is based on the seekbar width. TimeTracker was replaced by a custom solution.
@qwhex qwhex mentioned this pull request Dec 3, 2016
Let's you change the render speed. Render for every X pixel elapsed
@lazka
Copy link
Member

lazka commented Dec 4, 2016

I don't like the approach.

  1. Why would one want to change the config value?
  2. This queries the player backend more often and results in more wakups (TimeTracker syncs all wakups in the app)
  3. The time labels get updated multiple times per second which results in lots of uneven update intervals due to rounding.

My suggestion:

  1. Use the TimeTracker and estimate the current position in a Gtk.Widget.add_tick_callback callback
  2. If it crosses a pixel boundary queue a draw with the estimated value
  3. Leave the label updates as they are now.

@qwhex
Copy link
Collaborator Author

qwhex commented Dec 4, 2016

Hello,

I didn't realize that it modifies the label update behavior as I was so focused on getting it to work with my non-existent GObject knowledge. :)

The config option is more of a debug thing, so I can experiment with it without modifying code / restarting quodlibet (and stopping music in the process).

So aside from the unnecessary label updates (which is clearly a bug) why is add_tick_callback a better option? The current solution calculates the exact time when a redraw is needed, also it recalculates the update frequency when you resize the window, so no unnecessary redraws.

I didn't find any examples of add_tick_callback in the quodlibet source code, could you point me to some well designed implementations to get inspiration?

@lazka
Copy link
Member

lazka commented Dec 5, 2016

I didn't realize that it modifies the label update behavior as I was so focused on getting it to work with my non-existent GObject knowledge. :)

The config option is more of a debug thing, so I can experiment with it without modifying code / restarting quodlibet (and stopping music in the process).

ok, in the end if there is no value for the user it shouldn't be there.

So aside from the unnecessary label updates (which is clearly a bug) why is add_tick_callback a better option? The current solution calculates the exact time when a redraw is needed, also it recalculates the update frequency when you resize the window, so no unnecessary redraws.

I'm not sure what the exact differences are. It just looks easier since most of the update/timing stuff is handled by gtk this way, and you get called before each frame. If your time stuff works, there is no reason the switch I guess.

I didn't find any examples of add_tick_callback in the quodlibet source code, could you point me to some well designed implementations to get inspiration?

We've never done any animation in QL (there are some hacks in the osd plugin and the treeview DnD code, but that was coded before gtk+3.0 where this API didn't exist)

@qwhex
Copy link
Collaborator Author

qwhex commented Dec 8, 2016

Refactored the code to update the labels separately with the TimeTracker class, but it doesn't seem to affect the CPU usage much.

The main performance bottleneck is that the whole waveform gets redrawn every time. My idea is to render the waveform only when needed (window resize, song change) and update the position with one of the following methods:

  • The waveform is a transparent hole and a coloured rectangle gets resized in the background avoiding complex redraws
  • The waveform is being tinted with a foreground rectangle

Examples: https://www.cairographics.org/operators/

The thing is: I don't know how performant Cairo compositing is. I'm going to run some isolated tests to see. If anybody can give me hints, please contact me.

@ptitjes
Copy link
Collaborator

ptitjes commented Feb 26, 2017

@knockoutMice Hi, are you still contributing on this ?
Sorry to jump in, but I have some code that uses Cairo's clipping there :
ptitjes@0f26f84
ptitjes@4756bda

@ptitjes
Copy link
Collaborator

ptitjes commented Mar 5, 2017

@knockoutMice @lazka I think we can close this PR now that #2289 is in master.

@lazka lazka closed this Mar 5, 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants