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

Introduce relativeNormalization option for split channels rendering #2108

Merged
merged 14 commits into from
Nov 19, 2020
Merged

Introduce relativeNormalization option for split channels rendering #2108

merged 14 commits into from
Nov 19, 2020

Conversation

Claudiohbsantos
Copy link
Contributor

Short description of changes:

Introduced new splitChannelsOption parameter relativeNormalization, that allows wavesurfer to normalize channels while keeping them proportional to each other. Currently normalize normalizes each channel independently, so it's impossible to visually tell when a channel is louder than the others after normalization.
Also fixed assignment of default parameters to splitChannelsOptions, so partial options won't cause bugs anymore.

Breaking in the external API:

None

Breaking changes in the internal API:

None. Added parameter to MultiCanvas.prepareDraw that can be left undefined in order to prevent breaking current calls.

Todos/Notes:

Added esdoc documentation for splitChannelsOptions. Still need to update gh-pages if PR is accepted

Related Issues and other PRs:

@Claudiohbsantos Claudiohbsantos changed the title Introduce relativeNormalization option when for split channels rendering Introduce relativeNormalization option for split channels rendering Nov 13, 2020
@coveralls
Copy link

coveralls commented Nov 13, 2020

Coverage Status

Coverage increased (+0.05%) to 82.45% when pulling ba7dca5 on Claudiohbsantos:relativeNormalization into f62ad10 on katspaugh:master.

src/util/absMax.js Outdated Show resolved Hide resolved
src/wavesurfer.js Outdated Show resolved Hide resolved
src/wavesurfer.js Outdated Show resolved Hide resolved
src/drawer.multicanvas.js Show resolved Hide resolved
@osheroff
Copy link
Contributor

might rename this stereoNormalization and default it to false

@osheroff
Copy link
Contributor

otherwise lgtm

@Claudiohbsantos
Copy link
Contributor Author

Claudiohbsantos commented Nov 16, 2020

might rename this stereoNormalization and default it to false

I was struggling with a name for it as well. I'm just not sure about "stereo" because that implies 2 channels, and this would apply to any multichannel audio. The use case inspired this addition for example is when showing audio files recorded on tv/film shoots, which contain any number of channels and are not correlated by level.

Agreed on the default to false. It is already set up that way to avoid affecting current implementations

@thijstriemstra
Copy link
Contributor

can you also add a changelog entry?

@Claudiohbsantos
Copy link
Contributor Author

can you also add a changelog entry?

Sure, I'll add it to Changes.md.

Just realized the @Version tag I added on commit 136f678 is probably wrong, right? I tagged 4.2.0 but it should be tagged for the next release, whichever version it ends up being?

@thijstriemstra
Copy link
Contributor

I tagged 4.2.0 but it should be tagged for the next release, whichever version it ends up being?

4.2.0 is fine as is. you can also rename next in changelog to 4.2.0 if you want.

@Claudiohbsantos
Copy link
Contributor Author

Claudiohbsantos commented Nov 16, 2020

I tagged 4.2.0 but it should be tagged for the next release, whichever version it ends up being?

4.2.0 is fine as is. you can also rename next in changelog to 4.2.0 if you want.

I added under heading Next (unreleased) because I am a bit confused with the versioning since I thought 4.2.0 was already released.

@thijstriemstra
Copy link
Contributor

oops, my bad, i meant 4.3.0

@Claudiohbsantos
Copy link
Contributor Author

oops, my bad, i meant 4.3.0

Cool, I thought i was missing something =]. Updated the version tag and the changelog to 4.3.0

src/wavesurfer.js Outdated Show resolved Hide resolved
src/util/absMax.js Outdated Show resolved Hide resolved
@thijstriemstra thijstriemstra merged commit 3c09bb8 into katspaugh:master Nov 19, 2020
marizuccara pushed a commit to marizuccara/wavesurfer.js that referenced this pull request Nov 28, 2020
…atspaugh#2108)

* Extract absolute maximum as a utility function

* Implement relativeNormalization option for split channels

* assign default parameters for splitChannelsOptions

* Improve example documentation for relativeNormalization

* fix typo

* document purpose of overallAbsMax

* Capitalize normalizedMax parameter documentation

* rewrap long comment

* version tag relativeNormalization property

* unit tests for absMax utility module

* add relativeNormalization to changelog

* corrected version references to 4.3.0

* improve absMax documentation

* mark version for SplitChannelOptions typedef with @SInCE instead of @Version
sandiz pushed a commit to sandiz/wavesurfer.js that referenced this pull request Sep 1, 2021
…atspaugh#2108)

* Extract absolute maximum as a utility function

* Implement relativeNormalization option for split channels

* assign default parameters for splitChannelsOptions

* Improve example documentation for relativeNormalization

* fix typo

* document purpose of overallAbsMax

* Capitalize normalizedMax parameter documentation

* rewrap long comment

* version tag relativeNormalization property

* unit tests for absMax utility module

* add relativeNormalization to changelog

* corrected version references to 4.3.0

* improve absMax documentation

* mark version for SplitChannelOptions typedef with @SInCE instead of @Version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants