Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replaygain: target level refactor #3065

Merged
merged 9 commits into from
Jul 25, 2019

Conversation

zsinskri
Copy link
Contributor

@zsinskri zsinskri commented Oct 28, 2018

Remove REPLAYGAIN_* vs R128_* tag handling from the Backends, restrict it to the main plugin logic. That is archieved by passing the target loudness level to the backend with every album/track. It also necessitates that all Backends properly implement the target level.
As the different tag formats call for different default target levels this also seperates the configuration of those levels.

@zsinskri
Copy link
Contributor Author

The latest push merges master (or rather #3056, from which this branch was forked), resolving conflicts. It also fixes the ReplayGainLdnsCliMalformedTest, which caused unrelated tests to fail, due to missing cleanup.

@zsinskri
Copy link
Contributor Author

Is there any guideline on when to merge and when to rebase for this project? I will rework this branch's structure if a rebase is preferred.

zsinskri added a commit to zsinskri/beets that referenced this pull request Jun 19, 2019
When using the ffmpeg replaygain backend to create R128_*_GAIN tags,
automatically set the targetlevel to -23 LUFS.

Note: This will be made configurable by GitHub PullRequest beetbox#3065
zsinskri added a commit to zsinskri/beets that referenced this pull request Jul 14, 2019
Add replaygain backend using ffmpeg's ebur128 filter.

The album gain is calculated as the mean of all BS.1770 gating block powers.
Besides differences in gating block offset, this should be equivalent to a
BS.1770 analysis of a proper concatenation of all tracks.

Just calculating the mean of all track gains (as implemented by the bs1770gain
backend) yields incorrect results as that would:
- completely ignore track lengths
  - just using length in seconds won't work either (e.g. BS.1770 ignores
    passages below a threshold)
- take the mean of track loudness, not power

When using the ffmpeg replaygain backend to create R128_*_GAIN tags, the
targetlevel will be set to -23 LUFS. GitHub PullRequest beetbox#3065 will make this
configurable.
It will also skip peak calculation, as there is no R128_*_PEAK tag.

It is checked if the libavfilter library supports replaygain calculation. Before
version 6.67.100 that did require the `--enable-libebur128` compile-time-option,
after that the ebur128 library is included in libavfilter itself. Thus we
require either a recent enough libavfilter version or the `--enable-libebur128`
option.
zsinskri added a commit to zsinskri/beets that referenced this pull request Jul 19, 2019
Add replaygain backend using ffmpeg's ebur128 filter.

The album gain is calculated as the mean of all BS.1770 gating block powers.
Besides differences in gating block offset, this should be equivalent to a
BS.1770 analysis of a proper concatenation of all tracks.

Just calculating the mean of all track gains (as implemented by the bs1770gain
backend) yields incorrect results as that would:
- completely ignore track lengths
  - just using length in seconds won't work either (e.g. BS.1770 ignores
    passages below a threshold)
- take the mean of track loudness, not power

When using the ffmpeg replaygain backend to create R128_*_GAIN tags, the
targetlevel will be set to -23 LUFS. GitHub PullRequest beetbox#3065 will make this
configurable.
It will also skip peak calculation, as there is no R128_*_PEAK tag.

It is checked if the libavfilter library supports replaygain calculation. Before
version 6.67.100 that did require the `--enable-libebur128` compile-time-option,
after that the ebur128 library is included in libavfilter itself. Thus we
require either a recent enough libavfilter version or the `--enable-libebur128`
option.
This test caused other tests to fail due to missing cleanup.
Configure the replaygain analysis by passing arguments to the Backends. This
avoids the difference between ReplayGain and EBU r128 backends; every Backend
can now fulfil both tasks. Additionally it eases Backend development as the
difference between the two tag formats is now completely handled in the main
Plugin, not in the Backends.
Allow to configure the target level for R128_* tags separately from REPLAYGAIN_*
tags and skip peak calculation for R128_* tags if possible.
Assert that analysing the same track with different target levels yields
different gain adjustments.
Assert that the replaygain plugin does not write REPLAYGAIN_* tags but R128_*
tags, when instructed to do so.

This test is skipped for the `command` backend as it does not support OPUS.
- document `r128_targetlevel`
- explain difference between `targetlevel` and `r128_targetlevel`
- deprecate `method` option: use `targetlevel` instead.
Add changelog entries for the introduction of the `r128_targetlevel`
configuration option, superseding the `method` option.
@zsinskri
Copy link
Contributor Author

Rebased onto master.

@zsinskri
Copy link
Contributor Author

#3056 is merged. This is now ready for review.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Here are just a few suggestions.

beetsplug/replaygain.py Outdated Show resolved Hide resolved
beetsplug/replaygain.py Outdated Show resolved Hide resolved
beetsplug/replaygain.py Outdated Show resolved Hide resolved
beetsplug/replaygain.py Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
docs/plugins/replaygain.rst Outdated Show resolved Hide resolved
zsinskri and others added 2 commits July 26, 2019 01:02
Apply improvements suggested in GitHub PullRequest beetbox#3065:
- be idiomatic
  - 0 is falsy
  - check enum equality, not identity
  - mutate list by constructing a new one
- improve documentation
  - fix a typo
  - do not mention deprecation of a config option
@sampsyo
Copy link
Member

sampsyo commented Jul 25, 2019

Awesome; thank you! This is a heroic effort that required lots of special domain expertise, so I wanted to offer extra appreciation for getting this right. ✨

@sampsyo sampsyo merged commit ddede4e into beetbox:master Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants