-
-
Notifications
You must be signed in to change notification settings - Fork 51
fix: prevent memory leak in Visualizer component #114
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: prevent memory leak in Visualizer component #114
Conversation
8dcecdd
to
0d1c658
Compare
I haven't been able to intensively test the CPU usage, but I do hope that it reduces some 🤞 |
0d1c658
to
607c1ef
Compare
Hmm. I'm guessing that the AudioContext didn't finish resetting at the time we pressed play again. I'm actually thinking about keeping the Visualizer in the DOM when the music is paused or when the user switches tab, but stop the draw function instead, so that we won't have to reinitialize the audio context again. I think this approach will help keep the animation transition smooth. |
0606504
to
86bd752
Compare
15279bb
to
f83348c
Compare
@ahmadabdolsaheb I found that apparently This PR is ready for another round of review 🙂 |
9c1e4a6
to
f83348c
Compare
if (nextProps.playing && !this.state.eq.context) { | ||
this.initiateEQ(); | ||
componentDidUpdate(prevProps, prevState) { | ||
if (prevProps.playing === this.props.playing && prevState.isTabVisible === this.state.isTabVisible) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do (prevProps.playing !== this.props.playing && prevState.isTabVisible !== this.state.isTabVisible) so all of the conditions can stay in one block.
rafId = null; | ||
timerId = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since they are changing, should we set the to states?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what rafId stands for.
Thanks for the pr. see my nitpicks above, other than that LGMT |
This PR:
The issue can be reproduce by making the following changes to the code:
playing
stateExpected behavior: When the user presses pause, the Visualizer component is unmounted, and there shouldn't be anything logged to the browser console.
Actual behavior: When the user presses pause, the Visualizer component is unmounted, and there are logs constantly printed in the browser console.
My fix for this is to move the recursion outside of the draw function, and implement
start
andstop
functions that helps initiate and clean up animation loops instead.References: