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

Extract audio conversion to a separate function and add output logs if conversion fails #10

Merged
merged 20 commits into from
Mar 20, 2023

Conversation

arsaboo
Copy link
Contributor

@arsaboo arsaboo commented Mar 16, 2023

Following up on our discussion in #9, I extracted the ffmpeg conversion process in a separate function and added logging if conversion fails.

Let me know what you think.

fmdpy/download.py Outdated Show resolved Hide resolved
@Liupold
Copy link
Owner

Liupold commented Mar 18, 2023

good work on the log functionality. But the codec option will create problem. If some other format is specified then this would cause the output file extension will be OK but the data format will NOT be OK. I would appreciate if you can write a general function which implements this. Something Like ffmpeg_convert.

@arsaboo
Copy link
Contributor Author

arsaboo commented Mar 18, 2023

Moved to pydub and converted it to a generic function. It still uses ffmpeg under the hood, but we don't have to deal with it. I tested it with both mp3 and opus and it works fine. Any format can be specified as along it is supported by ffmpeg.

The tests are failing for unrelated reasons.

@arsaboo arsaboo changed the title Extract ffmpeg to a separate function and add output logs if conversion fails Extract audio conversion to a separate function and add output logs if conversion fails Mar 18, 2023
@arsaboo
Copy link
Contributor Author

arsaboo commented Mar 18, 2023

Also, if we remove ffmpeg, we can remove

- uses: FedericoCarboni/setup-ffmpeg@v1

Looks like that is causing the failed tests.

@Liupold Liupold mentioned this pull request Mar 20, 2023
@Liupold
Copy link
Owner

Liupold commented Mar 20, 2023

Are you ready to merge ? I have made a shadow branch in which the CI tests is working. The CI test is failing because of the lack of the ffmpeg API key in your repo.

@Liupold
Copy link
Owner

Liupold commented Mar 20, 2023

If you do add more commits, pls change the target branch from main to debug. So your code will be merging into my debug branch and then I will push it to main. BTW, the conversion is working perfectly fine. Good job!

@arsaboo
Copy link
Contributor Author

arsaboo commented Mar 20, 2023

I'm ready to merge...I tested this for a while and it's working well for me for various formats. Feel free to merge this. Thanks!!

@Liupold Liupold merged commit ba92b46 into Liupold:main Mar 20, 2023
@arsaboo arsaboo deleted the debug branch March 20, 2023 20:15
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