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

Adds option to adjust the audio volume on a per podcast basis #8

Merged
merged 12 commits into from
Jan 17, 2024

Conversation

maltebaer
Copy link
Contributor

As discussed in #7, this PR adds the ability to adjust the volume.

This is done using pydub, which in turn requires ffmpeg. So I guess it is not a very lightweight solution.

By the way, is there a method to list the required dependencies in a python project and install them all at once? Like with npm for Node.js projects?

@alexhartm
Copy link
Owner

alexhartm commented Jan 13, 2024

By the way, is there a method to list the required dependencies in a python project and install them all at once? Like with npm for Node.js projects?

Hi @maltebaer Yes, the project uses poetry for package management. If a new dependency is consumed, it should be made available via e.g. poetry add pydub. Poetry is then also used for build and publishing it to the pypi package repository.
I think this is also something that can be automated. I'll try to set this up eventually via github actions.

@alexhartm
Copy link
Owner

On a quick test on my laptop (with the missing ffmpeg dependency), it looked like it was stuck during the download part. I think this might be because __cache_episode was stuck when doing __adjust_volume__.
I did some quick workaround via

if (ep.volume_adjustment != 0):
    adjusted_content = self.__adjust_volume__(r.content, ep.volume_adjustment)
else:
    adjusted_content=r.content

and that seemed to work.
I think it would be good if the thing keeps working without having ffmpeg available locally. (As this is something that a user has to install separately outside of pip.) But maybe there is a more clever approach than mine.
Ideally, we could also highlight the adjust_volume part at bit (in the terminal), because on constrained devices, this might take some time. However, this might also be something for a follow-up improvement.
Will try it out in the next days in more detail. And am already looking forward to this feature! 😃 👍

@maltebaer
Copy link
Contributor Author

I updated the code to avoid potential issues with missing ffmpeg. I still get an error from the linter about S603 subprocess call: check for execution of untrusted input. I think we can ignore it in this case, however I'm not able to add the exception.

try:
# Safe to use untrusted input: executable is hardcoded
# noqa: S603
executable = "ffmpeg" if platform.system().lower() != "windows" else "ffmpeg.exe"
Copy link
Owner

Choose a reason for hiding this comment

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

Ah, nice that you considered platform-independent'ness! I was wondering if this works on Windows as well, as I noticed another user running this on a Windows host. Unfortunately, I have no option to test it on Windows...
Do you have a Windows setup where you could try it out already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I can't try it either. To be honest, I only asked Chat GPT to make it Windows safe.

toniepodcastsync.py Outdated Show resolved Hide resolved
@alexhartm
Copy link
Owner

This is a great change. Thanks for contributing! I'm going to release this as a new 2.1 version after the merge

@alexhartm alexhartm merged commit 1285ca8 into alexhartm:main Jan 17, 2024
2 checks passed
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