convert: Correctly identify WAVE format as lossless

[Bobo1239]

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)

[arsaboo]

Isn't copying the wav file the desired option in this case? I am not sure what the bug is here.

[Bobo1239]

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)

[wisp3rwind]

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.

[JOJ0]

Related unmerged PR #4656

[Serene-Arc]

I prefer that change tbh, since we don't have the test coverage to determine if this is a breaking change on some systems.

[Bobo1239]

@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:

• beetbox/mediafile@010a359 (2010) is the commit that added the the format property and already uses the TYPES dict for the returned value. No support for WAVE.
• 9d55179 (2014) added the never_convert_lossy_files option which uses item.format.
• beetbox/mediafile@832f3d8 (2021) Adds wav: WAVE to the the TYPES dict for the first time. Previously WAVE files weren't even importable according to WAV Support #2300. So I think the last commit just wasn't able to test that WAVE files worked as intended.

[JOJ0]

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!

[Serene-Arc]

Definitely! I need to take a look at our formatting CI task too, it's malfunctioning.

[Serene-Arc]

Formatting CI fixed with #5131

[Serene-Arc]

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.