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

Add RemoveDCOffset effect #5917

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

Conversation

PatrickKallenbach
Copy link

Resolves: #2408

The original issue was requesting a DC Offset tool similar to what the Normalize effect can do before normalization. In my university class, we were assigned to find and contribute to an open source project, so I thought I should contribute to this project which I have used myself in the past. The new effect is modified from the existing Normalize effect to automatically remove the offset, bypassing the menu similar to the Invert effect. I am certain improvements could be made upon the modified files, perhaps adding more features, or removing steps that were required for the Normalize effect but not for DC offset removal. I also only added the effects to the src/effects folder, but I do not know if there is something else that needs to know of the updated effects, since it does not currently categorize with the other related effects.

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

@Paul-Licameli
Copy link
Collaborator

This is not a complete code review, but in fact -- the .h file is not needed at all. You can just define the class locally in the .cpp file. More than that -- I would also wrap the whole thing in an anonymous namespace, and save a little bit of space in the symbol tables that the linker has to cope with.

Audacity wasn't originally written this way back in the late twentieth century, but nowadays, static registration objects do a lot of magic to push things into registries (of effects, macro commands, whatever) allowing scattered definitions of details, and making it unnecessary for those details to expose a header file.

@LWinterberg
Copy link
Member

LWinterberg commented Feb 6, 2024

I do not know if there is something else that needs to know of the updated effects, since it does not currently categorize with the other related effects.

That's in resources/EffectsMenuDefaults.xml, probably the best category for this effect would be the "noise removal and repair" category.

auto pOffset = offsets.begin();
auto pExtent = extents.begin();
for (const auto channel : channels) {
const auto extent = oneChannel ? *pExtent++: maxExtent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Patrick, thanks for your contribution!
It looks like you don't use extent, in the end.

@PatrickKallenbach
Copy link
Author

PatrickKallenbach commented Feb 6, 2024

This is not a complete code review, but in fact -- the .h file is not needed at all. You can just define the class locally in the .cpp file. More than that -- I would also wrap the whole thing in an anonymous namespace, and save a little bit of space in the symbol tables that the linker has to cope with.

I will have to research anonymous namespaces, and I'll try to set that up. May not be able to make it work because I don't fully understand what goes on behind the scenes.

That's in resources/EffectsMenuDefaults.xml, probably the best category for this effect would be the "noise removal and repair" category.

Okay, added the effect to that category. I'll test it a bit more and see if everything works.

It looks like you don't use extent, in the end.

That's just a remnant of the Normalize feature that I didn't understand the use of. I'll scan through the program again to get a better understanding and remove unnecessary code.

Edit: For some reason, I cannot get the effect to categorize correctly. Also, once I tried the anonymous namespace tip I kept getting errors, so I feel that would be safer to let someone with more experience handle.

@Paul-Licameli
Copy link
Collaborator

Paul-Licameli commented Feb 6, 2024

Anonymous namespace just means this -- add the line
namespace {
after any #include directives and forward declarations of incomplete types, then at the bottom of the file,
}

@petersampsonaudacity
Copy link

petersampsonaudacity commented Feb 8, 2024

@LWinterberg

If/when this PR gets pulled for this new effect, are you planning to remove the DC offset removal from the Normalize effect or just leave it in there?

@LWinterberg
Copy link
Member

hey @PatrickKallenbach, haven't heard from you in a while. Are you still interested in finishing this PR? Where did you get stuck on?

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.

Add DC Offset removal effect
5 participants