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

Implement --lower-only #271

Closed
wants to merge 5 commits into from
Closed

Conversation

ahmetsait
Copy link
Contributor

No description provided.

@ahmetsait ahmetsait force-pushed the lower-only branch 2 times, most recently from b635435 to 56e542a Compare November 19, 2024 02:03
@slhck
Copy link
Owner

slhck commented Nov 19, 2024

Thanks for the PR. I'll review the functionality later, but can you please undo the version change? This is done separately. Thanks.

@ahmetsait ahmetsait marked this pull request as draft November 19, 2024 09:27
@ahmetsait
Copy link
Contributor Author

Looks like naively removing the filter causes a bunch of issues for parser code so it's gonna take a bit more time to flesh this PR out. In the meantime please mention if you see better ways to handle things in the code-base.

@slhck
Copy link
Owner

slhck commented Nov 19, 2024

I think you will have to return the original input labels in the _get_audio_filter_cmd() method as output_labels, because the output mapping expects the -filter_complex to output the audio streams and the particular output labels used for the -map option.

Because later in the second pass we do:

        for ol in output_labels:
            cmd.extend(["-map", ol])

and if no normalization filter was added, these labels do not exist.

@ahmetsait ahmetsait marked this pull request as ready for review November 19, 2024 14:51
@ahmetsait
Copy link
Contributor Author

@slhck Should be ready for review.

I opted for replacing loudnorm with a no-op acopy filter if input loudness is already lower than target.

@slhck
Copy link
Owner

slhck commented Nov 19, 2024

Thanks! I see you changed a few other things related to parsing. On first sight it looks reasonable but I'll do a thorough test in the next few days. (The test suite is not good, coverage should be higher.)

@slhck
Copy link
Owner

slhck commented Nov 22, 2024

In principle everything looks good and works as intended with respect to the --lower-only option, but there is one failing test when parsing the loudnorm output. Since you changed this part quite significantly, could you please check?

@slhck slhck self-requested a review November 22, 2024 08:12
ffmpeg_normalize/_streams.py Show resolved Hide resolved
@slhck
Copy link
Owner

slhck commented Nov 22, 2024

You used some typing syntax that does not work in Python 3.8. If you rebase onto master, it should work — I just removed 3.8 support because it's EOL anyway. Merged manually now since all tests pass, but I'll do some manual further tests before finally making a release later. Thanks for the contribution!

@slhck slhck closed this Nov 22, 2024
@slhck
Copy link
Owner

slhck commented Nov 22, 2024

See commit 7730205

@slhck
Copy link
Owner

slhck commented Nov 22, 2024

Released as v1.30.0

@ahmetsait ahmetsait deleted the lower-only branch November 23, 2024 20:56
@ahmetsait
Copy link
Contributor Author

Awesome thanks!
Any documentation on your release steps? Things like gitchangelog > CHANGELOG.md and commands to run for generating docs etc.

@slhck
Copy link
Owner

slhck commented Nov 23, 2024

I used to have a script per repository and then eventually settled on a bash script that does this: https://github.com/slhck/dotfiles/blob/master/scripts%2Frelease-python.sh

It's not perfect, and I would like to migrate all of my projects away from setup.py eventually, using virtual environments, pyproject.toml with development dependencies, etc., but this gets the job done for now.

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.

2 participants