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

UI: Change clip level to 0.0 dB #11230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

cg2121
Copy link
Contributor

@cg2121 cg2121 commented Sep 1, 2024

Description

This changes the clip level of the volume meters from -0.5 dB to 0.0 dB.

Motivation and Context

I think the original intention was to tell users they had too loud of audio without actually clipping the audio. But if audio was intended to be this high of level, such as music, it would show it was clipping without it actually clipping.

Brought up by @PatTheMav off thread.

How Has This Been Tested?

Tested audio with a high dB level, but not clipping, to make sure the volume controls didn't show the clipping indicators.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@cg2121 cg2121 added Enhancement Improvement to existing functionality UI/UX Anything to do with changes or additions to UI/UX elements. labels Sep 1, 2024
@SuslikV
Copy link
Contributor

SuslikV commented Sep 2, 2024

Right now it affects only input meter (small rectangle).

@cg2121
Copy link
Contributor Author

cg2121 commented Sep 3, 2024

Fixed the code now, so the clip level affects both the input and output meters.

@cg2121 cg2121 force-pushed the clip-level branch 2 times, most recently from c8fc97f to 5d5d19c Compare September 3, 2024 05:16
@Warchamp7 Warchamp7 self-assigned this Oct 7, 2024
@DeeDeeG
Copy link

DeeDeeG commented Oct 10, 2024

Hi all, I have a rebased copy of this PR's commits, in case it saves anyone the time/tedium. Feel free to grab from there if it's helpful. (If not, apologies for the noise.) Best regards.

master...DeeDeeG:obs-studio:cg2121_clip-level_PR11230_rebased

Rebase was a little involved due to #9407 landing.

One line originally in this PR has already been done separately in another PR, which has already landed on master, so the line does not appear in the rebased version's diff. #11209

P.S. works well for me, audio from a game mastered to full scale volume no longer indicates clipping during loud sounds. Just red as it should.

EDIT to clarify: I meant that fixing the merge conflict was somewhat difficult/tedious, but unless I missed something, the diff looks effectively identical on my branch vs this PR.

Only the line that was done separately as #11209 is not necessary anymore and disappears when rebasing, as it is 100% redundant -- the same line is already landed verbatim at master branch now. No change to make when rebasing means no diff when rebasing. And of course the line length of surrounding code is now longer and wrapped differently as a consequence of #9407, which is unavoidable to deal with in a rebase. That was why the rebase was difficult/tedious, as the diffing algorithm is unsure about how to resolve differences in the context lines before/after this PR's actual changes, given the different line length and wrapping of the surrounding code pre vs post rebase, so it flags them to be resolved manually. Rebasing the actual code changes of the PR was trivial compared to rebasing the context lines, which was harder/more tedious.

So, now that it's done, the resulting diff looks clean and simple. If it's done right, it should match this original PR. Which I believe it does. Other than the one-line change already in master from #11209 being gone from the diff, as explained above.

I hope my rebase can either be cherry-picked to save time, or else ignored and redone by hand if reviewing my copy constitutes a distraction. Thanks for your understanding, and sorry if it adds any confusion.

Copy link
Member

@Warchamp7 Warchamp7 left a comment

Choose a reason for hiding this comment

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

Fine with this on a conceptual level. The rebased PR above will need to be looked at

@norihiro
Copy link
Contributor

norihiro commented Nov 14, 2024

I manually resolved the conflict with the laster master on local host and tested.
Because of changing peakPosition to peak in the conditions, the meter gets flashing when the audio level is very close to the clipping level.

flashing-error-color.mp4

Screen recording above is tested with Asynchronous Audio Source plugin (an 100-Hz 0-dB sine wave) + Gain Filter (0.1 dB).

@cg2121 cg2121 force-pushed the clip-level branch 2 times, most recently from 5fa1abd to b7a7e68 Compare November 15, 2024 14:00
@cg2121
Copy link
Contributor Author

cg2121 commented Nov 15, 2024

@norihiro could you check if the latest code in this PR fixes the issue you were having?

@norihiro
Copy link
Contributor

The behavior on the horizontal meter looks good. Though, the vertical meter should be updated as well.

This changes the clip level of the volume meters from -0.5 dB to 0.0 dB.
@cg2121
Copy link
Contributor Author

cg2121 commented Nov 23, 2024

Updated to fix the vertical mixer as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality UI/UX Anything to do with changes or additions to UI/UX elements.
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

5 participants