-
Notifications
You must be signed in to change notification settings - Fork 90
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
flac settings #184
Comments
That isn't currently supported (rationale in #121). The easier thing to do would be to re-encode whipper's resulting FLAC files with the settings you prefer, otherwise you could directly hack whipper's sources (unsupported scenario): https://github.com/JoeLametta/whipper/blob/master/whipper/program/flac.py#L15 check_call(['flac', '--silent', '--verify', '-o', outfile, to check_call(['flac', '-3', '--silent', '--verify', '-o', outfile,
Until it gets fixed you can check whipper's inbuilt help from the CLI, like this:
Currently that's broken #99.
You're welcome! |
Thanks a lot! What about adding an option? I guess your syntax is ...
|
I don't think we should add an option for this functionality. |
Personally, I'm OK with supporting custom compression, as long as the default is the default that FLAC uses, and not the default that we have in our argparser. So I would say default=None, and if not None, pass a custom option. |
If we're going to bother with this at all we should probably just allow users to define a custom flac command in the configuration file as opposed to adding single options for single flac options. But I'm not sure it's necessary, because you can get the same effect by re-encoding the flac file after running whipper. |
It's not necessary, but it sure would be convenient. |
I just assumed that flac would be compressing with --best, which is common for ripping. I guess I'll patch the flac config until this is default. It does make sense to me to expose the flac encoder settings as an executable option. |
My original assumption was also that I ran the following script over a folder of .wav files (Dark Side of the Moon in this case):
Results (cleaned up) --best
default
Delta
Of course, this isn't a scientifically rigorous benchmark, but I think the results are quite stark. The You could reasonably argue that the bulk of whipper's time is spent reading from disc, so the additional encode time doesn't matter, but (for me at least) this is a case of diminishing returns and really not something to worry about. |
@calumchisholm I just drop here a post I wrote a while ago. It basically covers what you just wrote. |
Nice write-up! Glad to see that your (much more comprehensive) findings agree with my own quick-and-dirty test. |
I have a fast new CPU (Intel Core i7 8550U), so even double the encoding time is trivial. I will encode the files once and listen to them perhaps hundreds or thousands of times over a span of years or decades. Even a 0.5% reduction in storage space is worth the cost of encoding time. In practical terms, 0.5% would mean I could fit a few more albums on the 128 GB microSD card in my phone. If no one else here cares enough to work on this, I understand, but I don't think there would be any reason someone else couldn't make a pull request to add a command line option for this. |
Sorry, for me it is a no go, using flac at all. Is there a way to rip only as "pure" wav? |
No, nor is this a desired feature. Feel free to edit your own copy of whipper to skip the flac encoding step. |
I'm closing this issue due to the creation of issue #244 which addresses the underlying problem. |
Hey,
is there a way to set the compression settings for flac? I would like to go for level 3 but can't find an option.
Btw what's the state of the documentation? I just found whipper and it seems to to do really well -- closes the gap of easy to use but funtional accurate terminal CD rippers for me! But formatting and possible options of the config file remain a mystery to me.
Thanks a lot!
Cheers
Stephan
The text was updated successfully, but these errors were encountered: