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 playback speed issue on certain detected media #137

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

Quantizr
Copy link
Contributor

@Quantizr Quantizr commented Mar 9, 2023

Fixes #136 where certain media types would not be detected by
document.querySelectorAll("video, audio")

)

this.elements.forEach((element) => {
inspectMediaElements((element: MediaElement) => {
Copy link

Choose a reason for hiding this comment

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

This creates a new MutationObserver each time it is called, which should result in a resource leak.

Copy link
Owner

@vantezzen vantezzen left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

)

this.elements.forEach((element) => {
inspectMediaElements((element: MediaElement) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Using inspectMediaElements inside setPlaybackRate will call the function every time the speed should change and attach a new MutationObserver.
Due to this, we might have 100s to 1000s of MutationObservers after long videos which would be a performance issue and we'll also have a memory leak in the callback function as it will be called every time a new element appears for each observer.

I'd suggest instead creating a central inspectMediaElements in the SpeedController constructor and have it update this.elements dynamically instead.

Moved inspectMediaElements to the constructor where it dynamically updates this.elements
@Quantizr Quantizr requested a review from vantezzen March 9, 2023 10:05
@Quantizr
Copy link
Contributor Author

Quantizr commented Mar 9, 2023

Hopefully that's what you meant? I'm not really familiar with JS

@vantezzen
Copy link
Owner

Yes looks good now - thank you again for the contribution!

@vantezzen vantezzen merged commit be57451 into vantezzen:master Mar 10, 2023
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.

Kaltura Player Incompatible service
3 participants