beets
beets copied to clipboard
beetbox
convert: Correctly identify WAVE format as lossless
Description
I've noticed that beet convert with the never_convert_lossy_files: yes just copies .wav files instead of converting them. This fixes that. (see commit message for details)
Isn't copying the wav file the desired option in this case? I am not sure what the bug is here.
The reasoning behind never_convert_lossy_files is that we don't want to re-encode lossy file formats to the target format due to quality loss of repeated conversion. WAVE is a lossless format so should be re-encoded just like FLACs do. (especially since they're uncompressed)
In any case, wav was in the list of lossless formats already, but that was apparently the incorrect key. If I'm reading the code correctly at a glance, the Item.format field is initialized from the human-readable representation of mediafile's format field, which would be WAVE, rather than its internal type field, which would indeed take the value wav.
I prefer that change tbh, since we don't have the test coverage to determine if this is a breaking change on some systems.
@JOJ0 Thanks for finding that. Not sure how I missed it before submitting this one...
Did some git blameing and imo wav should never have been returned by item.format. (unless they manually modified mediafile to fix this bug...) Relevant commits:
- https://github.com/beetbox/mediafile/commit/010a359cd62f7041440a315da873a9b3e27a86e3 (2010) is the commit that added the the
formatproperty and already uses theTYPESdict for the returned value. No support for WAVE. - https://github.com/beetbox/beets/commit/9d55179d2d88e586d1051b6e404659e579e640f7 (2014) added the
never_convert_lossy_filesoption which usesitem.format. - https://github.com/beetbox/mediafile/commit/832f3d895c665edfc29f8cd7fca41ce628b608af (2021) Adds
wav: WAVEto the theTYPESdict for the first time. Previously WAVE files weren't even importable according to https://github.com/beetbox/beets/issues/2300. So I think the last commit just wasn't able to test that WAVE files worked as intended.
No worries, that other PR is abandoned anyway, I'll close it now!
My efforts of moving on with the topic went silent when I couldnt push to that existing PR's branch.
Anyway, thanks for picking this issue up. About time we fix it!
Definitely! I need to take a look at our formatting CI task too, it's malfunctioning.
Thanks for doing the digging for this @Bobo1239, it seems good to merge based on the code. Perhaps you could add a quick entry in the changelog? People might be interested in this and how it impacts their libraries.