Feature: Add DeepFilterCommandBuilder to MediaProcessor #42
Conversation
There was a problem hiding this comment.
Hi @georgeolson92, thanks for the changes! I added a few comments addressing minor points.
Could you also take a look at other flags that may be interesting to have, for a better coverage of what's available?
For instance, I remember using a post filter with a post filter beta. There was also a field for adjusting the filtering limits in dB.
We'll be exposing the filtering behavior to the end-user, so these can come in handy.
I haven't tested the --noise-reduction flag so far though, maybe it works great on its own?
Thanks a lot for taking another look!
omeryusufyagci
added
feature
|
Made updates based on recommendations from review, let me know how everything looks! Thank you for all of the helpful feedback! |
There was a problem hiding this comment.
Hi @georgeolson92, thanks for the changes! One of the mandatory checks failed as the code was not clang-format'ed. I also noticed a couple of other things for you to check.
Could you please address the 2 points and run the clang-format? Should be good to go after. Many thanks!
Edit: we provide a .clang-format at the project root, in case you havent' seen it yet
|
Thanks for the review, made requested updates and ran clang-format, let me know how it's looking! |
Added a method to pass --compensate-delay flag which is required to ensure the filtered audio remains in sync with the source media. Minor refactor for general consistency Added doxy docs to the header Discovered a user-side problem; see omeryusufyagci#52 for details
There was a problem hiding this comment.
Hi @georgeolson92, many thanks for the changes!
The only important thing that was missing was a method to set the --compensate-delay flag for the filter. This ensures audio remains in sync with the original media after filtering.
I also made some minor refactors for consistency across the codebase.
One issue we'll need to address in the future is that this filter does not currently support specifying an input flag, and it expects the input file to be provided after the options. This approach is not ideal for a builder, as it forces users to follow a specific sequence of operations. I didn't find a straightforward solution to fix this without compromising const-correctness, so we'll revisit it later.
We will most likely structure the DeepFilter related classes similar to what we discussed at #45. Please let me know if you'd be interested in continuing this work further.
Thanks again!
omeryusufyagci
added
the
hacktoberfest-accepted
|
Hi @omeryusufyagci, I can certainly continue working on the Deep Filter classes, if you have any issues open feel free to assign me and I can create a new branch for that! |
|
Hi @georgeolson92, fantastic to hear! : ) We have #65, and #52 that are DeepFilter specific. I am not able to assign you directly -for some reason can't find you on the list. If you could leave a comment on these I can assign them to you. As usual, I am happy to clarify anything that may come as we progress. Thanks a lot! |
This is my first time working in the code base (and with anything related to deep filtering), hope this will address the new feature implementation in issue #32! Let me know what needs to be adjusted, or if there is anything else I should add!