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

[postprocessor/ffmpeg] Allow options for mp3 #32507

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

Conversation

a-rich
Copy link

@a-rich a-rich commented Aug 15, 2023

Why?
Options like preferredquality are ignored when postprocessing mp3s.

What?
Remove mp3 from the list of filecodec values to check for in the "lossless" condition.

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])
  • Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

Why?
Options like preferredquality are ignored when postprocessing mp3s.

What?
Remove mp3 from the list of filecodec values to check for in the "lossless" condition.

@dirkf
Copy link
Contributor

dirkf commented Aug 16, 2023

I'm not convinced about this.

The existing logic tries to avoid re-encoding the audio unless the target container requires it. As I see it, this change on its own will generally result in worse audio quality than what the site offered: if you ask for lower quality you'll get it and if you ask for higher quality you'll get a degraded version of the original with a higher nominal bitrate. Unless you always want to dumb down the MP3 quality to reduce filesize, that doesn't sound good.

You could try this

-            elif filecodec in ['aac', 'flac', 'mp3', 'vorbis', 'opus']:
+            elif filecodec in ['aac', 'flac', 'vorbis', 'opus'] or (filecodec == 'mp3' and self._preferredquality is None):

If you specify --audio-quality ..., that should be passed into self._preferredquality and cause the re-encoding that you want to occur. A more complex change could extend the same processing to the other audio formats.

The FFMPEG audio option processing is definitely a target for updates. yt-dlp has some more sophisticated processing but I haven't studied it fully.

Why?
Options like `preferredquality` are ignored when postprocessing mp3s.

What?
Remove `mp3` from the list of `filecodec` values to check for in the
"lossless" condition unless `preferredquality` is unspecified.
@a-rich a-rich force-pushed the fix-ffmpeg-mp3-postprocessing branch from 4a433f6 to e8bebe0 Compare August 17, 2023 14:50
@a-rich
Copy link
Author

a-rich commented Aug 17, 2023

I see. I've amended the commit to incorporate your suggestion which I feel achieves both aims...

if you ask for lower quality you'll get it
which satisfies my need to ensure a standardized bit rate

and [preventing asking] for higher quality [and getting] a degraded version of the original with a higher nominal bitrate
which avoids the needless and unexpected audio degradation

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.

None yet

2 participants