RFC: replaygain: R128 support #2560

autrimpo · 2017-05-13T18:16:32Z

Following the discussion in #2557 I've decided to try implement a very WIP, 'proof of concept' support of R128 to the replaygain plugin.

As I'm not very familiar with the codebase nor Python I'm creating a PR already and would appreciate if someone more knowledgable could review the changes.

Things that I'd especially like comments on:

I'm saving and restoring the backend because R128 is only supported by bs1770 as far as I know. Is that needed? If yes, perhaps somehow detect and create the second backend in advance and switch between those two instead of recreating it every time.
There's a lot of shared code between the branches right now, should it branch only for those 2 specific calls or perhaps even branch inside those calls?
How to decide when to branch. My idea was a new plugin setting, a list of formats with a sensible default (so right now only Opus).
The MediaType and db entries - they work, but are they correct?
Removal of REPLAYGAIN_ tags when tagging with override set - right now, my files are incorrectly tagged with these and I'd like the R128_ to replace them. When/where should it be done, should it be configurable?
Expanding on the 'branching on list' and the previous point - is tagging with both systems desirable and how should it be handled?

This only calculates track gain, album gain is still missing. But I've tested it on MPD and DeadBeef and the gain produced with these changes works as expected.

mention-bot · 2017-05-13T18:16:35Z

@autrimpo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @geigerzaehler and @jrobeson to be potential reviewers.

sampsyo

Here are a few comments tied to specific lines of code. I'll also answer some general questions separately…

sampsyo · 2017-05-13T23:31:54Z

beets/mediafile.py

+    r128_track_gain = MediaField(
+        StorageStyle(
+            u'R128_TRACK_GAIN',
+            float_places=0


You don't need float_places if you don't intend to store floating-point numbers. Is this field supposed to store an integer?

sampsyo · 2017-05-13T23:33:13Z

beets/mediafile.py

+        StorageStyle(
+            u'R128_TRACK_GAIN',
+            float_places=0
+        ),


With just a StorageStyle, this will only support "free-form" tag formats. On MP3, MPEG-4, and WMA, it will crash unceremoniously. I think the right thing to do is to fill in logical names for these other formats, even if support isn't widespread yet.

I've added support for other formats, with names more or less correspoinding to their replaygain equivalents.

sampsyo · 2017-05-13T23:36:09Z

beetsplug/replaygain.py

+        method = self.method
+        self.method = '--ebu'
+        output = self.compute_gain(items, False)
+        self.method = method


Hmm, yes—mutating and restoring the object's state is a bit hacky. Maybe this decision should be made inside compute_gain? The idea would be to write a clear policy about when the "format-specific default" overrides the global default or the user's configuration.

sampsyo · 2017-05-13T23:43:06Z

beetsplug/replaygain.py

+                self._log.info(u"ReplayGain error: {0}", e)
+            except FatalReplayGainError as e:
+                raise ui.UserError(
+                    u"Fatal replay gain error: {0}".format(e))


Hmm; I see what you mean about duplicate structure between the two branches. It does seem like a good idea to trim this down, but I don't have any non-obvious suggestions at the moment…

sampsyo · 2017-05-13T23:47:40Z

beetsplug/replaygain.py

-                    u"ReplayGain backend failed for track {0}".format(item)
+        self._log.debug(u'format: {0}', item.format)
+
+        if item.format == 'Opus':


Maybe some manner of global function to make this decision (e.g., should_use_r128) would be useful to encapsulate the policy here?

sampsyo · 2017-05-13T23:52:41Z

Removal of REPLAYGAIN_ tags when tagging with override set - right now, my files are incorrectly tagged with these and I'd like the R128_ to replace them. When/where should it be done, should it be configurable?

Good question. I don't have a strong feeling whether it should be configurable (maybe some people want both sets of tags for maximum compatibility? Or maybe we should force compliance with the RFC's recommendation, so adding R128 tags clobbers RG tags but not the other way around?). I imagine it should be done in the store_track_gain/store_album_gain functions (or rather, the new versions of the same).

autrimpo · 2017-05-15T12:28:46Z

I've cleaned up the code a little, the decision making is now done by a method and controlled by a setting (whitelist of formats). Unfortunately I can't delete the REPLAYGAIN_ tags, only set their value to be empty. Any hints on how to do that would be appreciated.

sampsyo · 2017-05-15T15:01:40Z

Sadly, actually deleting the tag is a little more difficult than it should be. (Real, general support for deleting tags is a long-standing debacle—see #919, for example.) The actual answer is that we need a way to invoke del mediafile.rg_track_gain inside the Item.write() method when the corresponding Item field is None. As pedantic as this sounds, can we perhaps address this in a separate PR? Adding this tag-deletion ability would be really useful in general.

sampsyo

Awesome! This design seems quite a bit simpler, and the configurable list of format names seems like the right thing.

A note for the future: it looks like the only trouble in the tests so far is that we need to add the new field name to the MediaFile tests. https://travis-ci.org/beetbox/beets/jobs/232405195#L894

sampsyo · 2017-05-15T15:02:38Z

beetsplug/replaygain.py

@@ -822,6 +823,9 @@ def __init__(self):
        if self.config['auto']:
            self.import_stages = [self.imported]

+        # Formats to use R128.
+        self.r128_whitelist = self.config['r128'].as_str_seq()


Cool! This configurable format list makes sense to me. (We'll need to add it to the documentation eventually—unless we want to leave it undocumented, if we think people shouldn't be fiddling with it.)

sampsyo · 2017-05-15T15:02:54Z

beetsplug/replaygain.py

+        self.r128_backend_instance = ''
+
+    def should_use_r128(self, item):
+        return item.format in self.r128_whitelist


A docstring for this method might be useful.

sampsyo · 2017-05-15T15:03:44Z

beetsplug/replaygain.py

+                raise ui.UserError(
+                    u'replaygain initialization failed: {0}'.format(e))
+
+            self.r128_backend_instance.method = '--ebu'


This method body looks like it might have gotten indented one step too far.

autrimpo · 2017-05-15T15:35:12Z

I see, in that case I'll skip the tag removal functionality for now. I'll add the tests, and I of course plan to update the plugin documentation to reflect the final implementation.

autrimpo · 2017-05-15T19:04:55Z

I've added album gain, so this can be considered a 'final draft'.
However, the ALAC and MP4 tests are failing with Python 3. I've tried to debug it but don't know what the problem is.

I'm also making an assumption that all the files in an album are in the same format, but I think it's a fairly safe one to make and I can't really think of a way how to handle multiple formats and gain standards in one album in an elegant way. Maybe exiting gracefully and notifying the user would be a bit better if it happens?

sampsyo · 2017-05-16T17:42:22Z

beets/mediafile.py

+        MP4SoundCheckStorageStyle(
+            '----:com.apple.iTunes:iTunR128',
+            index=0
+        ),


I'd recommend dropping the SoundCheck storage styles (this and the MP3 variant above). The SC format is included for the RG tags for compatibility with the iTunes-specific normalization metadata, which we should probably (eventually) break off as a third standard that deserves a third set of tags.

We can also cross our fingers and hope that this solves the crash on Python 3…

sampsyo · 2017-05-16T17:44:41Z

Great! I think the error probably has to do with the SoundCheck conversion stuff, so we may be able to sidestep it altogether. In short, the MediaFile abstraction layer is trying to data stored as bytes in the MPEG-4 format as a Unicode string, which won't do.

Good point about heterogenous albums. Yeah, just logging an error and "giving up" on the album (raising a ReplayGainError) would be nice—and slightly better than silently assuming the format from the first track on the album.

autrimpo · 2017-05-17T10:39:24Z

Alright, I've finally fixed the tests.

MP4 tags starting with ----: are encoded as unicode. The float branch of _safe_cast handled the decoding fine but the int branch had no such logic, that's why REPLAYGAIN_ passed fine but R128_ struggled. I've also taken the liberty of adjusting the regex a little, to support a sign prefix. I think the spec allows both in the tag and - is actually very common, as with replaygain values.

If everything looks fine I'll rewrite the history to something prettier (I was thinking 3 commits - the _safe_cast fix, the tests and finally the implementation itself) and update the documentation (fourth commit).

sampsyo

Nice work tracking that down! This looks great—thanks for all your effort.

I personally don't mind a "messy" git history, but if you're inclined to clean things up, that sequence of 3 commits sounds great.

sampsyo · 2017-05-17T17:17:08Z

beets/mediafile.py

-            if not isinstance(val, six.string_types):
+            if isinstance(val, bytes):
+                val = val.decode('utf-8', 'ignore')
+            else:


Cool; this looks right. Just to clarify what's going on, may I recommend elif not isinstance(val, six.string_types) here?

sampsyo · 2017-05-17T17:17:48Z

beets/mediafile.py

                val = six.text_type(val)
            # Get a number from the front of the string.
-            val = re.match(r'[0-9]*', val.strip()).group(0)
+            val = re.match(r'[\+-]?[0-9]*', val.strip()).group(0)


👍 Good call.

sampsyo · 2017-05-17T17:25:35Z

beets/mediafile.py

+        ),
+        MP3DescStorageStyle(
+            u'r128_track_gain'
+        ),


It belatedly occurs to me to recommend that we don't try use capitalizations here. It was useful for RG, where there are apparently both styles in use, but here we're essentially "inventing a standard"—I don't see any prior art yet indicating one or the other. We can always add both later if someone complains.

The only problem is I'm not sure which to choose… maybe lower case?

Well, the RFC always refers to it in upper case, so I guess we should go with that for consistency.

autrimpo · 2017-05-17T20:14:02Z

Cleaned up, updated the docs.

RFC: replaygain: R128 support

sampsyo · 2017-06-11T00:35:38Z

Thank you for your patience! I've finally done a complete review and merged. This looks great. ✨

autrimpo · 2017-06-11T08:00:55Z

Glad to hear that!

sampsyo reviewed May 13, 2017

View reviewed changes

sampsyo reviewed May 15, 2017

View reviewed changes

autrimpo force-pushed the r128 branch 4 times, most recently from 2b9ab76 to 2cde5c0 Compare May 15, 2017 19:01

sampsyo reviewed May 16, 2017

View reviewed changes

autrimpo force-pushed the r128 branch 2 times, most recently from 13eca81 to 856fc9b Compare May 16, 2017 19:34

sampsyo reviewed May 17, 2017

View reviewed changes

autrimpo added 4 commits May 17, 2017 21:37

properly safe cast unicode as int

9fe171c

r128 gain tags in mediafile test

ddfe442

replaygain: support r128

2685f13

update replaygain docs with info about r128

8168a88

autrimpo force-pushed the r128 branch from 7e50fa5 to 8168a88 Compare May 17, 2017 20:00

sampsyo merged commit 8168a88 into beetbox:master Jun 11, 2017

sampsyo added a commit that referenced this pull request Jun 11, 2017

Merge pull request #2560 from autrimpo/r128

0e47095

RFC: replaygain: R128 support

sampsyo added a commit that referenced this pull request Jun 11, 2017

Changelog for #2560 (fixes #2557)

7cacae5

sampsyo mentioned this pull request Jun 29, 2017

replaygain: Check for R128 tags fails on opus files when values are missing #2615

Closed

autrimpo mentioned this pull request Dec 19, 2017

replaygain: Sets R128 fields to zero when R128 values are not calculated #2757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: replaygain: R128 support #2560

RFC: replaygain: R128 support #2560

autrimpo commented May 13, 2017

mention-bot commented May 13, 2017

sampsyo left a comment

sampsyo May 13, 2017

sampsyo May 13, 2017

autrimpo May 15, 2017

sampsyo May 13, 2017

sampsyo May 13, 2017

sampsyo May 13, 2017

sampsyo commented May 13, 2017

autrimpo commented May 15, 2017

sampsyo commented May 15, 2017

sampsyo left a comment

sampsyo May 15, 2017

sampsyo May 15, 2017

sampsyo May 15, 2017

autrimpo commented May 15, 2017

autrimpo commented May 15, 2017 •

edited

Loading

sampsyo May 16, 2017

sampsyo commented May 16, 2017

autrimpo commented May 17, 2017 •

edited

Loading

sampsyo left a comment

sampsyo May 17, 2017

sampsyo May 17, 2017

sampsyo May 17, 2017

autrimpo May 17, 2017

autrimpo commented May 17, 2017

sampsyo commented Jun 11, 2017

autrimpo commented Jun 11, 2017

RFC: replaygain: R128 support #2560

RFC: replaygain: R128 support #2560

Conversation

autrimpo commented May 13, 2017

mention-bot commented May 13, 2017

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampsyo commented May 13, 2017

autrimpo commented May 15, 2017

sampsyo commented May 15, 2017

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autrimpo commented May 15, 2017

autrimpo commented May 15, 2017 • edited Loading

Choose a reason for hiding this comment

sampsyo commented May 16, 2017

autrimpo commented May 17, 2017 • edited Loading

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autrimpo commented May 17, 2017

sampsyo commented Jun 11, 2017

autrimpo commented Jun 11, 2017

autrimpo commented May 15, 2017 •

edited

Loading

autrimpo commented May 17, 2017 •

edited

Loading