Beets: replaygain: Don't crash when albums have a mix of ReplayGain and EBU R128 formats

[myfreeweb]

beetsplug.replaygain.ReplayGainError: Mix of ReplayGain and EBU R128 detected for some tracks in album  -

The message isn't accurate: pretty much all of my library is Opus files with both tags — this is not about the actual tags in the files, this is about formats, the code checks should_use_r128 — this actually seems to mean "you've mixed Opus and not-Opus in the same album". Which.. shouldn't be a fatal error??

[sampsyo]

Indeed, that message does seem to not describe what's going on there! We should definitely make it clear.

Separately, I'm not entirely sure why this was made an error in the first place… it seems like a reasonable thing to do might be to print out the warning, but just pick one or the other (probably R128) just so the plugin does something vaguely useful. But to be honest, I am far from an expert on this stuff and I know there are lots of rules out there about where you can and cannot use R128, so maybe I'm oversimplifying.

The change was introduced in 2685f1331. Perhaps @autrimpo can clarify? 🙏

[autrimpo]

It's been some years since the original implementation, but the motivation behind this is as follows, as far as I remember:

Let's assume the user's configuration contains replaygain.r128 = [ formatA ] and they're importing an album that is a mix of formatA and formatB.
Based on this, there are two interpretations of user intent; I will call these _strict_ and _lenient_.

In the _strict_ variant, files in the library of formatA _must_ use R128 and everything else _must_ use ReplayGain. Since the album being imported contains a mixture of these formats, it's impossible [1] to preserve both of these invariants, resulting in an error.

In the _lenient_ variant, this configuration signifies a _preference_ when both options are possible. We therefore pick a common format that's usable for the whole album [2] and use that for both formats.

The downside of the _strict_ approach is that it will refuse to normalize volume on mixed-format albums with conflicting target methods.

The downside of the _lenient_ approach is the inconsistency within the library, which some users might find undesirable. Different files of the same format will have different volume normalization method applied to them; knowing which one has which requires knowing the import context of the track.

Personally, I don't have strong feelings on the topic, as I don't use mixed-format albums so this corner case does not affect me. I've implemeted the strict variant because it is simpler - figuring out how to handle an album is straightforward.

I think this is mostly a matter of user UX and beet's design philosophy, more than being a technical issue, even though there are some technical points to be considered. No matter the decision, I agree that the error message should be rewritten to be clearer.

[1] We could convert the R128 value to a ReplayGain value (or vice-versa) and write mixed tags: this would preserve library consistency but leads to a whole other set of issues.
ReplayGain uses peak values while R128 uses averages, so while numerically it's possible, they're not _really_ convertible semantically.
Handling of the target loudness level also needs to be considered - ReplayGain and R128 use different target levels by default. With RG I believe it's configurable, with R128 it's specified by the standard. Not only will a direct conversion not target the desired loudness level of the other, but due to the different methods used (peak/avg) the applied results when playing the tracks will be different.
This assumes the backend cannot compute both for given format which I think was the case with some RG implementations which didn't support Opus.
I think the new ffmpeg backend is able to handle this, which simplifies it a bit (calculate both R128 and RG with R128's target level and pick which to write as needed); however the issue with peak/average remains.

[2] This requires having some other way of being able to answer "Can I use method X for format Y" besides the user config, which was the original meaning of the config option.
This is also a difficult question to answer for every format, and there are two levels: _possible_ and _desirable_.
The Opus spec says it SHOULD NOT contain any RG tags, and from my light research when implementing the original R128 support, most players do not handle RG tags for Opus (I know mpd acts like this for sure), as the meaning of two (conflicting) volume normalization schemes is ambiguous.
So while it is _possible_ to use RG for Opus, it's not clear whether it is _desirable_.
We would need to do research like this for every format to first determine whether it's _possible_ and store that in some sort of database within beets to use when handling multi-format albums.
The _desirability_ part is mostly dependent on the implementation of the user's choice of music player. While we could also build a database of format support for that, that's an even bigger and faster moving target than the file formats.
The alternative is to have the user specify if it can be used or not, which actually circles us back to the current model, as you can simply add formatB to replaygain.r128 - we just introduced a lot of additional complexity along the way.
A possible synthesis could be to have explicit RG and R128 format lists, and introduce another option which sets the preference in case a format is in both. This moves a lot of the complexity to the user, but if the user has such needs they probably know what they're doing and can configure it accordingly.

[myfreeweb]

IMO strict is fine, this just shouldn't be an uncaught exception. The mixed album I have is a junk album that was accidentally picked up.

[sampsyo]

Hey, thanks for the really detailed explanation, @autrimpo! (And for the thoughts on how to proceed.)

Indeed, it seems like the thing to do is just make sure the error is handled. It currently looks like it's a bug that the ReplayGainError is not being caught… notice that the error is thrown here in handle_album:
https://github.com/beetbox/beets/blob/43d58a77d9b8f1fa7223ad9e1c19403a46dce6c3/beetsplug/replaygain.py#L1329-L1332

But the except handler for ReplayGainError that is supposed to take these things and turn them into non-fatal log messages is also in handle_album, farther down in the method:
https://github.com/beetbox/beets/blob/43d58a77d9b8f1fa7223ad9e1c19403a46dce6c3/beetsplug/replaygain.py#L1362-L1363

I think we either need to expand that try block, relocate the "mix" detection farther down inside that handler, or avoid the exception altogether and just log a message and return.