obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

Add 8-band Equalizer

Open phkahler opened this issue 3 years ago • 7 comments

Description

This PR adds an 8-channel equalizer to OBS studio

Motivation and Context

I used OBS once and wanted an EQ badly. It turns out this is a common request for many years.

How Has This Been Tested?

I tested this by playing an .mp3 in the default gnome music player, applying this filter to the desktop audio in OBS and monitoring in headphones. All sliders work.

Types of changes

Strictly the addition of a new audio filter. Similar to the recently added 3-band.

Checklist:

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

phkahler avatar Dec 23 '22 16:12 phkahler

The property labels should be localized. This means you'll have to add entries to en-us.ini in the data subfolder. Check the other filters to have a model.

pkviet avatar Dec 23 '22 16:12 pkviet

The property labels should be localized. This means you'll have to add entries to en-us.ini in the data subfolder. Check the other filters to have a model.

I was wondering why my eq8. prefix kept appearing in the UI when that didn't happen in the 3-band one. I ended up removing it in the code. I'll put it back and do the same - removing during localization.

phkahler avatar Dec 23 '22 16:12 phkahler

The commit title needs a space after the module prefix.

notr1ch avatar Dec 23 '22 18:12 notr1ch

Is the "appropriate documentation" checkbox for PRs for the code or user docs? I don't really see a place for either and the code seems much like the other filters.

Updated the commit message and localized the properties.

phkahler avatar Dec 23 '22 18:12 phkahler

Documentation is primarily for libobs changes, e.g. if you introduce a new API or change behavior of an existing API. Not necessary for internal components like this.

notr1ch avatar Dec 23 '22 18:12 notr1ch

@pkviet Is there anything more you'd like from me on this?

phkahler avatar Jan 02 '23 17:01 phkahler

Fixed the commit message.

phkahler avatar Feb 05 '23 16:02 phkahler

@pkviet I consider this done, unless someone finds an issue with it or has suggestions to make it better. I'm not sure how this project operates, but it might be good to merge after a major release so some set of people (developers) get to test it prior to the next one. I'll be around if they find anything even though this is kind of drive-by PR from me ;-)

phkahler avatar Mar 22 '23 18:03 phkahler

After internal discussion and now that we have a basic 3 band EQ, we don't feel this is adding anything meaningful to do natively in OBS, and users who want that extra level of control will probably have a preferred VST that would work better anyway.

Thank you for the submission.

Fenrirthviti avatar Mar 22 '23 19:03 Fenrirthviti