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

Audio compressor plugin #288

Merged
merged 3 commits into from
Oct 24, 2021
Merged

Audio compressor plugin #288

merged 3 commits into from
Oct 24, 2021

Conversation

thymue
Copy link
Contributor

@thymue thymue commented May 17, 2021

This plugin adds DynamicsCompressorNode onto the audio that is being played which should help with the case where there are two videos with different volume in the video's audio itself.

Example:
vid1 (which is loud) compared to
vid2
without the plugin:
audacity screenshot of both without the plugin
with the plugin:
audacity screenshot of both with the plugin

@Araxeus
Copy link
Collaborator

Araxeus commented May 18, 2021

It looks like you are watching any changes in the video element, including volume change / pause etc
is this actually necessary?

or can this be applied only once on load?

if that doesn't work then maybe only when each song load?

const applyCompressor = (videoElement) => { 
   ...
	 const source = audioContext.createMediaElementSource(this)
   ...
}
document.querySelector('video').addEventListener("canplay", applyCompressor);

or maybe im completely wrong 🙃

you should know that front.js is called after DOMContentLoaded

@thymue
Copy link
Contributor Author

thymue commented May 18, 2021

you should know that front.js is called after DOMContentLoaded

image
shouldn't this work then? I'm not very experienced so correct me if I'm doing something wrong.
but regardless, you're right that using watchDOMElement is probably unnecessary. I'll look into some other solution.

@Araxeus
Copy link
Collaborator

Araxeus commented May 18, 2021

You're right - DOMContentLoaded isn't very reliable in our case
It does actually takes a few milliseconds before the video element is loaded

a possible solution is having a timeout to check if video is loaded (even with load event it takes a few more milliseconds)

checkVideoLoaded()
// or if it gets reset on page reload (unlikely from what i've observed)
window.addEventListener('load', checkVideoLoaded)

function checkVideoLoaded() {
	const video = document.querySelector("video");
	if (video) {
		applyCompressor(video); // or video.addEventListener("canplay", applyCompressor) if it doesn't persist
	} else {
		setTimeout(checkVideoLoaded, 500);
	}
}

video.canplay event fires every time a new song is loaded inside the video element - which might be unnecessary since the dom element itself isn't reloaded)

this is pretty much exactly what happens in precise-volume plugin, which also modify the video element

/** Restore saved volume and setup tooltip */
function firstRun(options) {
const video = $("video");
// Those elements load abit after DOMContentLoaded
if (video) {
setupVolumeOverride(video, options);
if (typeof options.savedVolume === "number") {
// Set saved volume as tooltip
setTooltip(options.savedVolume);
}
} else {
setTimeout(firstRun, 500, options); // Try again in 500 milliseconds
}
}

from #275

@Araxeus
Copy link
Collaborator

Araxeus commented May 19, 2021

or the even shorter version that im using in the new Chromecast plugin:

module.exports = function checkVideoLoaded() {
    const video = document.querySelector("video");
    video ? setup(video) : setTimeout(checkVideoLoaded, 500);
}

@thymue
Copy link
Contributor Author

thymue commented May 19, 2021

I personally don't like using setTimeout as a solution but it's certainly better than using watchDOMElement and creating unnecessary function calls. Thanks a lot for your effort in helping me <3

@Araxeus
Copy link
Collaborator

Araxeus commented May 19, 2021

And I dont like watching a whole dom element when you don't care about any of the changes 😝

Speaking of, I think there is a memory leak in plugins/playback-speed/front because of watchDOMElement funcction

module.exports = () => {
watchDOMElement(
"video",
(document) => document.querySelector("video"),
(element) => {
videoElement = element;
updatePlayBackSpeed();
}
);
watchDOMElement(
"menu",
(document) => getSongMenu(document),
(menuElement) => {
if (!menuElement.contains(slider)) {
menuElement.prepend(slider);
}
const playbackSpeedElement = document.querySelector(
"#playback-speed-slider #sliderKnob .slider-knob-inner"
);
const playbackSpeedObserver = new MutationObserver((mutations) => {
mutations.forEach(function (mutation) {
if (mutation.type == "attributes") {
const value = playbackSpeedElement.getAttribute("value");
playbackSpeedPercentage = parseInt(value, 10);
if (isNaN(playbackSpeedPercentage)) {
playbackSpeedPercentage = 50;
}
updatePlayBackSpeed();
return;
}
});
});
playbackSpeedObserver.observe(playbackSpeedElement, {
attributes: true,
});

First dom watcher is just unnecessary,
but the second? it looks like its creates a new mutation observer every time there is a change in the document?

I hope i'm reading it wrong somehow..

the problem is that watchDOMElement watch the whole document. so any change in the document execute the callback
(and the document changes all the time...)

observer.observe(document, {
childList: true,
subtree: true,

@th-ch Am I wrong or should this be fixed?

Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the contribution!

@Araxeus:

Am I wrong or should this be fixed?

Nice catch, it looks like it's the case indeed, and observing/watching the menu should only be needed when the page/music changes so there is probably room for improvement indeed for the playback speed plugin!

@th-ch th-ch merged commit 7b064c1 into th-ch:master Oct 24, 2021
@Araxeus
Copy link
Collaborator

Araxeus commented Oct 26, 2021

I personally don't like using setTimeout as a solution but it's certainly better than using watchDOMElement and creating unnecessary function calls. Thanks a lot for your effort in helping me <3

@thymue #458 make this plugin use the updated apiLoaded event 🙂

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.

3 participants