Skip to content

Fix canplay listener and interval accumulation in doUseCamera()#5847

Closed
kartikktripathi wants to merge 5 commits intosugarlabs:masterfrom
kartikktripathi:fix-camera-interval-accumulation
Closed

Fix canplay listener and interval accumulation in doUseCamera()#5847
kartikktripathi wants to merge 5 commits intosugarlabs:masterfrom
kartikktripathi:fix-camera-interval-accumulation

Conversation

@kartikktripathi
Copy link
Contributor

@kartikktripathi kartikktripathi commented Feb 20, 2026

Summary

This PR fixes event listener and interval accumulation in doUseCamera() (js/utils/utils.js).

Previously, each invocation of doUseCamera() attached a new "canplay" listener using addEventListener and could create multiple active setInterval loops without reliably clearing the previous one. This could lead to:

  • Event listener accumulation
  • Multiple concurrent render intervals
  • Increased CPU usage
  • Potential performance degradation over repeated camera toggles

Changes

Replaced:

video.addEventListener("canplay", ...)

with:

video.oncanplay = ...

This ensures only a single canplay handler is active at any time.

&

Added defensive cleanup before starting a new interval:

if (cameraID !== null) {
     window.clearInterval(cameraID);
}

This prevents interval stacking when the camera is re-triggered.

Testing

  • Connected Start → Show → Camera block in UI.
  • Repeatedly toggled Start/Stop 6–8 times.
  • Confirmed:
    • No console errors
    • No performance degradation
    • No interval acceleration
    • Stable CPU usage
    • No flickering
  • Passed all npm test test suites.

Notes

The fix prevents resource accumulation while preserving existing camera behaviour.

PR Category

  • Bug Fix

Thanks!

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@kartikktripathi
Copy link
Contributor Author

Hey @walterbender, this PR is ready for review. Please take a look.
Thanks!

Copy link

@shashank03-dev shashank03-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dude this code looks solid, the tests are green, and the PR description is very clear. Approving this from my end

@kartikktripathi
Copy link
Contributor Author

@shashank03-dev, thanks for the review!

@walterbender
Copy link
Member

please address the merge conflict

@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

mathutils.test.js

@github-actions github-actions bot added the bug fix Fixes a bug or incorrect behavior label Mar 14, 2026
@kartikktripathi
Copy link
Contributor Author

Hi @walterbender, I have resolved the conflicts which had appeared. Please take a look when you get time. Thanks!

@kartikktripathi
Copy link
Contributor Author

This issue has been solved in #5837.

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

Labels

bug fix Fixes a bug or incorrect behavior NEEDS WORK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants