search

metabrainz / picard-plugins

Picard plugins: use 1.0 branch for Picard < 2.0 (python 2/Qt4) and 2.0 branch for Picard >= 2.0 (python 3/Qt5)
https://picard.musicbrainz.org/plugins/
145 stars 94 forks source link

New ReplayGain Plugin #336

Closed complexlogic closed 1 year ago

complexlogic commented 1 year ago

Following up on the discussion in #335, I created a ReplayGain plugin that uses rsgain as its sole backend. All of the noted issues with the current ReplayGain plugin have been resolved:

I also added a basic progress update to the status message when a user is scanning multiple albums.

The new plugin extends support to the following file formats (in addition to what the current plugin supports):

Settings

The new plugin contains a settings menu which allows advanced users a high level of control over the scan settings.

For example, a user could choose a profile that targets the EBU R128 standard by setting the target loudness to -23 LUFS and true peak calculations. The settings also allow users to chose between ReplayGain tags and the RFC 7845-complaint R128_*_GAIN tags for Opus file.

screen32

New Plugin or Replace Existing?

In #335, there was some discussion whether this should replace the existing ReplayGain plugin, or be a new plugin. After some thought, I think this should be a new plugin because this change is too disruptive for users of the existing plugin - even for a major version update. I have decided to call the new plugin "ReplayGain 2.0" because it implements the ReplayGain 2.0 specification. This name makes it distinct from the current plugin while still clear that ReplayGain is the subject.

To Do

Fixes #335

Sophist-UK commented 1 year ago

I haven't tested this myself, but jolly well done for creating this.

phw commented 1 year ago

Two more things I noticed during testing:

  1. Would you consider writing replaygain_reference_loudness as well? Maybe make it an option to write this tag. This is something the author of loudgain came up with, and I think it can be useful to remember the reference loudness, especially when it was set to something else then the RG2 default. Maybe this again would be something worse considering in rsgain itself, but it can also be done in the plugin.

  2. While using the plugin I got this exception:

E: 23:12:21,695 /home/phw/devel/musicbrainz/picard/picard/util/thread.future_callback:59: Traceback (most recent call last):
  File "/home/phw/devel/musicbrainz/picard/./picard/util/thread.py", line 55, in future_callback
    result = future.result()
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 451, in result
    return self.__get_result()
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
  File "/usr/lib/python3.10/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/phw/.config/MusicBrainz/Picard/plugins/replaygain2/__init__.py", line 121, in calculate_replaygain
    if not track.files:
AttributeError: 'Album' object has no attribute 'files'

I think it happened when I had selected both an album and tracks. You need to filter the objs passed to the plugin callback functions by proper type, see e.g. what the old plugin is doing at https://github.com/metabrainz/picard-plugins/blob/b3e37ce846ca1bdf1a7160e5d27b90de9edee4f8/plugins/replaygain/__init__.py#L144-L147

This also shows some special handling for the "standalone recrdongs" aka "non-album tracks" special album entry. It does not make sense to calculate album gain for this, because it is just a list of random tracks not attached to an album.

As a side note: rsgain easy mode is great! That really makes things easy, and really should work for most. I just had it run over some 4k files, and it worked flawlessly. Looks like I found my new favorite replaygain utility :)

complexlogic commented 1 year ago

Would you consider writing replaygain_reference_loudness as well? Maybe make it an option to write this tag. This is something the author of loudgain came up with, and I think it can be useful to remember the reference loudness, especially when it was set to something else then the RG2 default. Maybe this again would be something worse considering in rsgain itself, but it can also be done in the plugin.

The reference loudness tags aren't part of the ReplayGain standard, and they aren't read by players, so I declined to support them in rsgain. But I can support them in this plugin instead, it's simply the target loudness setting formatted. I just added a checkbox in the plugin settings for it, disabled by default. The formatting is the same used by bs1770gain and loudgain, e.g. "-18.00 LUFS". Let me know if this works for you.

While using the plugin I got this exception:

Fixed.

It does not make sense to calculate album gain for this, because it is just a list of random tracks not attached to an album.

Perhaps we can introduce a new setting such as "Use track values for album tags on non-album tracks"? I think we should always have album tags if the album tags setting is checked, and this could be a good way of doing it.

phw commented 1 year ago

I just added a checkbox in the plugin settings for it, disabled by default. The formatting is the same used by bs1770gain and loudgain, e.g. "-18.00 LUFS". Let me know if this works for you.

That's perfect, thanks.

Perhaps we can introduce a new setting such as "Use track values for album tags on non-album tracks"? I think we should always have album tags if the album tags setting is checked, and this could be a good way of doing it.

Agreed on always setting the album tracks. Not sure about the setting. IMHO it's not needed, it should always set the per-track values for NAT. But if there is a usecase for people wanting to tag all NAT tracks as if they were one album I'm not opposed to having such an option, it just should then be enabled by default.

complexlogic commented 1 year ago

Agreed on always setting the album tracks. Not sure about the setting. IMHO it's not needed, it should always set the per-track values for NAT. But if there is a usecase for people wanting to tag all NAT tracks as if they were one album I'm not opposed to having such an option, it just should then be enabled by default.

Ok, that makes sense. I changed it permanently without setting.

complexlogic commented 1 year ago

@phw This is ready for final review for merging. I added a description, and put a deprecation notice on the old plugin.

There were some minor formatting inconsistencies in rsgain that I had to work around in the plugin (e.g. having an extra \n for album result but not track result). I decided to fix it in rsgain to make the code cleaner in the plugin. The new version of rsgain is 3.0.1, and you will need to upgrade to be compatible with the latest plugin.

Please let me know if you have any other feedback.

complexlogic commented 1 year ago

@phw Good find. I refactored the tagging code into a function that accepts a Metadata object, plus a few other parameters. I pass the File.metadata into into the new function for each File in Track and later call File.update, so Track.update_file_metadata is no longer used. I tested deleting the tags and re-scanning and it's working now.

Let me know what you think of the updated code, and if you find any other issues.

phw commented 1 year ago

Merged, thanks a lot again. The new plugin will show up for download from within Picard sometime in the next 12 hours. @complexlogic would you like to make a post on https://community.metabrainz.org/ to introduce this new plugin and rsgain? I think this would be the best way to get people know about this new plugin.

On a side note I also updated the linter configuration needed for codacy to follow the Picard settings, so the code no longer fails the codacy check.

complexlogic commented 1 year ago

@phw Yes, I will make a post on the forum.

To clean up this repo: I recommend closing #89 and #121.

209 was fixed by Sophist in #290 so that one should definitely be closed.