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

DE-6596: VU meter component #15

Merged
merged 8 commits into from
Oct 13, 2023
Merged

DE-6596: VU meter component #15

merged 8 commits into from
Oct 13, 2023

Conversation

fingerartur
Copy link
Contributor

It's a meter that shows the current levels of volume (left and right channels separately).

Screenshot 2023-09-18 at 22 32 50

@fingerartur fingerartur requested a review from doubeka September 18, 2023 19:35
@fingerartur fingerartur marked this pull request as ready for review October 9, 2023 09:48
@fingerartur fingerartur force-pushed the feat/DE-6596-vu-meter branch from 21d3a9a to dd31ffd Compare October 9, 2023 09:52
Copy link
Contributor

@thasso thasso left a comment

Choose a reason for hiding this comment

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

Nice VU meter :) just had some comments and questions.

src/components/MuteButton.tsx Show resolved Hide resolved
src/react.ts Show resolved Hide resolved
src/services/volumeMeterService.ts Outdated Show resolved Hide resolved
src/services/volumeMeterService.ts Show resolved Hide resolved
types/presto.d.ts Outdated Show resolved Hide resolved
Copy link

@doubeka doubeka left a comment

Choose a reason for hiding this comment

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

Looks great in storybook! :)
Few comments to look into.

story/stories/components/VuMeter.mdx Outdated Show resolved Hide resolved
types/presto.d.ts Outdated Show resolved Hide resolved
src/services/volumeMeterService.ts Outdated Show resolved Hide resolved
src/Player.ts Outdated Show resolved Hide resolved
@fingerartur fingerartur force-pushed the feat/DE-6596-vu-meter branch from b28dff7 to 44bba76 Compare October 13, 2023 09:00
@fingerartur
Copy link
Contributor Author

@thasso Thanks for pushing this forward! :)

Fun fact

PS: One thing you may have noticed is that instead of taking the average of the amplitudes/volumes I decided to split the frequencies into a couple of bins, average each bin and take the maximum of that. This is because if I take the average over all frequencies even loud audio produces max only cca 50% amplitude/volume in a normal audio track for a movie or something (it doesn't happen that all frequencies would be at max amplitued/volume, not even close).

const average = Math.max(...averages)

So that's my hack on how to make sure the volume bar goes up to the top when I as a human ear hear loud audio 😄.

I think it works pretty well at this point, so I would not like to mess with it more inside this PR, but it's a fun exercise to think about how to work with the volume.

freq

@fingerartur fingerartur requested review from thasso and doubeka October 13, 2023 09:26
@fingerartur fingerartur dismissed doubeka’s stale review October 13, 2023 13:31

I adressed the issues and we want to merge it now.

@fingerartur fingerartur merged commit faadc0f into main Oct 13, 2023
1 check passed
@fingerartur fingerartur deleted the feat/DE-6596-vu-meter branch October 13, 2023 13:33
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