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

Add functions to change compression level and bitrate mode. #447

Merged
merged 4 commits into from
Nov 28, 2024

Conversation

ytya
Copy link
Contributor

@ytya ytya commented Nov 11, 2024

Added interface to change compression level and bitrate mode as described in #390.
I was able to test it on Windows, but not on other platforms.

soundfile.py Outdated Show resolved Hide resolved
@bastibe
Copy link
Owner

bastibe commented Nov 18, 2024

Very cool, thank you very much! But I think the write function is the wrong place for this.

@ytya
Copy link
Contributor Author

ytya commented Nov 19, 2024

Thank you for the review. I've moved them to parameter of SoundFile. Is that what you meant?

Copy link
Owner

@bastibe bastibe left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. This looks better already.

But I think there's still a misunderstanding. As far as I can tell, we can't change the bitrate or compression of an already-opened file. Thus, we should make these items constructor arguments, and otherwise read-only.

But please do correct me if I'm misunderstanding this.

Additionally, SoundFile is still predominantly used with uncompressed files, where these settings make no sense. They should probably be hidden from the repr where they're not applicable.

soundfile.py Outdated Show resolved Hide resolved
soundfile.py Show resolved Hide resolved
soundfile.py Outdated Show resolved Hide resolved
@sammlapp
Copy link
Contributor

ah I had just finally started implementing the bitrate in a branch myself, glad to see this is already nearing completion

@sammlapp
Copy link
Contributor

sammlapp commented Nov 20, 2024

also, I tested with OPUS as well as MP3 and am happy to see that setting the compression level in this way works across formats. This will resolve my request in #252 for writing OPUS with variable bit rates as well as resolving #390

@ytya
Copy link
Contributor Author

ytya commented Nov 23, 2024

@bastibe
I followed your advice and changed the code. As for __repr__, i thought there was no problem with the operation as None was entered before the change, but we modified it from the point of view of not displaying extra information.
Please feel free to point out if this is not what you intended.

@sammlapp
Thanks for testing. I also tested opus and flac and it worked.

@bastibe
Copy link
Owner

bastibe commented Nov 24, 2024

This looks really good, thank you very much!

Just one more comment: If I'm reading this correctly, the compression/bitrate information is only available if it was passed to the constructor. But it's None if we opened a pre-existing file, or did not specify the default arguments.

Is there a reasonable way of populating the two properties automatically if they were not given by the user? (Or perhaps I'm misunderstanding the code?)

@ytya
Copy link
Contributor Author

ytya commented Nov 24, 2024

Firstly, if no arguments are specified, this is the default behaviour of libsndfile. This is the same behaviour as the current soundfile, so there should be no problem.

Secondly, if you open pre-existing file, I don't know how to set these parameters. The libsndfile does not have a GET_COMPRESSION_LEVEL command, so I can't get the compression level. There is a GET_BITRATE_MODE command, so I should be able to get it for bitrate mode, but getting just that is not very useful.

The sound quality will deteriorate as the lossy compression is repeated, so I personally think that the case of saving a file with the same compression as the opened file is not important. This function should generally be used when converting formats, or when saving edited sound files as a last step.

@bastibe
Copy link
Owner

bastibe commented Nov 25, 2024

I see. Well, without a GET_COMPRESSION_LEVEL command, we can't really display anything. In that case, the current state is good. Thank you!

Just one more note before we merge: the test is currently broken on Python < 3.9, because it's using numpy.concat, which was not yet available. Could you fix that test?

@ytya
Copy link
Contributor Author

ytya commented Nov 25, 2024

Sorry, not enough testing. This should be fine now.

@bastibe bastibe merged commit 44d2f7a into bastibe:master Nov 28, 2024
23 of 31 checks passed
@bastibe
Copy link
Owner

bastibe commented Nov 28, 2024

Thank you very, very much for your contribution!

@sammlapp
Copy link
Contributor

sammlapp commented Dec 3, 2024

sf.write('max_compression.mp3', data, samplerate, bitrate_mode='VARIABLE', compression_level=1)
gives an error (works fine if 0<=compression_level<1.0), is this expected?

@ytya
Copy link
Contributor Author

ytya commented Dec 3, 2024

It's the libmp3lame specification used by libsndfile. As reported here, setting compression_level=1 with VBR causes libsndfile to return an error.

@sammlapp
Copy link
Contributor

sammlapp commented Dec 3, 2024

Ok, perhaps this could be clarified in the docstring. Also, I'm adding a separate PR that just provides a demo of this functionality in the readme, @bastibe feel free to accept or reject, no offense taken if you reject :)

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.

3 participants