replaygain: fix #3311 #3314
replaygain: fix #3311 #3314
Conversation
Since commit 95e569a, mediafile takes care of the float -> Q7.8 conversion in R128 GAIN tags by itself. From `store_album_r128_gain` this conversion was already missing, remove it from `store_track_r128_gain`, too. fixes beetbox#3311
|
I'm missing an idea for a test ensuring that #3311 has actually been fixed, as most of that fix has taken place in mediafile. One could duplicate the test added there (checking the expected loss of information). But that only tests the conversion, which is part of mediafile, not beets (and is already tested there). Should we maybe go without an extra test in beets as mediafile is already abstracting over the stored format? If so, this should be ready to merge together with mediafile #16. |
|
How are changes like this that affect both mediafile and beets handled? This will produce incorrect results when updating beets but keeping mediafile at a version prior to these changes---or the otherway around. But I do not see a nice solution to this problem on either end besides just bumping the required version. It might also be worth noting that current |
This reverts commit f3f75b7. This issue is out of scope for this branch and has a much better solution in commit 95e569a (GitHub Pull Request beetbox#3314).
|
This is new territory for us, because MediaFile only recently split off into its own project, but I think we should just do a release and bump the minimum required version. |
|
I've tagged & uploaded v0.2.0 of MediaFile with this new fix, so I think we should be ready to go with the dependency. |
mediafile 0.2.0 includes the changes used by commit f645400. Update setup.py to reflect that new version requirement.
|
Updated the version requirement in |
|
No, that's it—this is before the first release that depends on MediaFile as an external project, so there's no particular notification we have to do for packagers. I think that's all we need (except for a changelog entry)! If this is working, I think we can merge it now. |
|
Merged! Thank you for your help with this!! ✨ |
Fix #3311.
As suggested in the discussion there, the Q7.8 conversion has been moved into mediafile. Thus this depends upon mediafile #16.