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: ZeroDivisionError: float division by zero #2387

Closed
lazka opened this Issue May 18, 2017 · 16 comments

Comments

Projects
None yet
3 participants
@lazka
Member

lazka commented May 18, 2017

https://sentry.io/share/issue/3134323431352e323735363939363437/

How: Quickly disable/enable the waveform seekbar plugin multiple times.

@ptitjes

I guess it's hard to hit, so not that urgent

@lazka lazka added the bug label May 18, 2017

@lazka lazka changed the title from waveformseekbar: ZeroDivisionErrorquodlibet to waveformseekbar: ZeroDivisionError: float division by zero May 18, 2017

@lazka lazka added the sentry label May 18, 2017

@ptitjes ptitjes self-assigned this May 18, 2017

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented May 18, 2017

The sentry report is nice ! :)
I'll have a look into it this week end.

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented May 19, 2017

@lazka I can't reproduce it here... so I would need some more input from you.

If there is a division by zero, this means that width * pixel_ratio = 1. So if you are in HiDPI mode that would mean that width = .5px, and otherwise that would mean that width = 1px.

Should I do a blind fix and just test for width * pixel_ratio being 1 ?

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented May 19, 2017

I could also test for the visibility of the component, but as I can't reproduce it, I won't know if that fixes the problem.

@urielz

This comment has been minimized.

Contributor

urielz commented May 19, 2017

I can reproduce it. It took me 5 seconds of checking/unchecking the waveform seek bar plugin quickly to get the crash. Did you try that @ptitjes?

It seems your suggestion of testing when width * pixel_ratio = 1 should work... this is the error I got:

Traceback (most recent call last):

.../quodlibet/ext/events/waveformseekbar.py", line 122, in _on_bus_message
self._update_redraw_interval()

.../quodlibet/ext/events/waveformseekbar.py", line 138, in _update_redraw_interval
interval = self._waveform_scale.compute_redraw_interval()

.../quodlibet/ext/events/waveformseekbar.py", line 256, in compute_redraw_interval
return length * 1000 / (width * pixel_ratio - 1)

ZeroDivisionError: float division by zero

@lazka

This comment has been minimized.

Member

lazka commented May 20, 2017

screenshot from 2017-05-20 07-02-17

No HiDPI, maybe that's the difference.

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented May 20, 2017

I see ! So I planted a if width * pixel_ratio - 1 == 1: print("Crash") in my compute_redraw_interval code and I can indeed trigger it when enabling/disabling quickly.

So I have a fix for this enabling/disabling quickly, which is to correctly cancel the pipeline in the destroy method:

     def _on_destroy(self, *args):
+        if self._pipeline:
+            self._pipeline.set_state(Gst.State.NULL)
+            self._clean_pipeline()
         self._label_tracker.destroy()
         self._redraw_tracker.destroy()

I think that correctly cancelling the pipeline is good practice anyway so I'll add a commit just for that in my PR. However, here, the "Crash" is also triggered at startup, as the seekbar component is not yet visible but the waveform computation has finished. Do you have that too ? (or my laptop is too fast ;p)

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented May 20, 2017

The two commits in #2392 should fix the issue.

@lazka lazka closed this in #2392 May 20, 2017

@lazka

This comment has been minimized.

Member

lazka commented May 26, 2017

Still a problem it seems (assuming I correctly applied the fixes on quodlibet-3.9): https://sentry.io/share/issue/3134323431352e323735363939363437/

@lazka lazka reopened this May 26, 2017

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented May 29, 2017

@lazka Is this also happening by quickly enabling/disabling multiple times ? Also can you tell me what length your currently playing song has (as this changes the time needed to process the waveform) ?

@lazka

This comment has been minimized.

Member

lazka commented May 29, 2017

That's a user submitted report, so I don't know. The song length is "158.29333333333332". I'll have a look later.

@lazka lazka added this to the 3.9.1 Release milestone Jun 1, 2017

@lazka

This comment has been minimized.

Member

lazka commented Jun 4, 2017

What's the reason for the "-1" in compute_redraw_interval() and draw_waveform() ?

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Jun 4, 2017

@lazka The reason for using width * pixel_ratio - 1 in draw_waveform() is that with Cairo you have to draw in the middle of pixels [1], else everything is blurry. Then, in compute_redraw_interval(), this is just to align the redraws to the pixel count that have to be drawn.

I guess the question is more why does the component has a width of 1 ?

[1] https://www.cairographics.org/FAQ/#sharp_lines

@lazka

This comment has been minimized.

Member

lazka commented Jun 4, 2017

Why does drawing in the middle of pixels change the amount of pixels being drawn?

I guess the question is more why does the component has a width of 1 ?

The initial allocation seems to be 1x1, so maybe there is some racy stuff going on where the widget is not mapped but still called. Either way, a width of 1 is a valid size and should be handled.

lazka added a commit that referenced this issue Jun 4, 2017

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Jun 4, 2017

Well, thinking of it now, it indeed seems dumb. I was thinking of inclusive intervals whereas the allocated width is a right-open interval. I think you can remove both - 1...

@lazka

This comment has been minimized.

Member

lazka commented Jun 4, 2017

done, thanks

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Jun 13, 2017

Thank you @lazka !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment