Fix ReplayGain metaflac backend altering input files - #6916
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull request overview
This PR fix ReplayGain metaflac backend so it no longer write ReplayGain tags into input/source files during import. It switch to reading gain data via metaflac --scan-replay-gain output, then write tags only when beets later writes library file.
Changes:
- Switch
MetaflacBackendfrom--add-replay-gain+ tag readback to--scan-replay-gain+ stdout parsing. - Update docs to note FLAC tools version requirement for
metaflacbackend. - Update tests to cover new
--scan-replay-gainoutput parsing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| beetsplug/replaygain.py | Use --scan-replay-gain, parse its output, and compute album/track Gain without mutating files. |
| docs/plugins/replaygain.rst | Document minimum FLAC tools version for metaflac backend. |
| test/plugins/test_replaygain.py | Update unit test to validate new metaflac output parsing behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| track_gains = [] | ||
| for item in filter(self.format_supported, task.items): | ||
| self._add_replay_gain([item]) | ||
| track_gains.append( | ||
| self._read_gain(item, "TRACK", task.target_level) | ||
| ) | ||
| result = self._read_gain([item], task.target_level) | ||
|
|
||
| track_gain_value = result[item][1] |
There was a problem hiding this comment.
I'll fix this in my next commit.
| """Run ``metaflac --scan-replay-gain`` on the given files""" | ||
| paths = [str(item.filepath) for item in items] | ||
| call([self.command, "--add-replay-gain", *paths], self._log) | ||
| output = call( | ||
| [self.command, "--scan-replay-gain", *paths], self._log | ||
| ).stdout.decode("utf-8", "ignore") |
There was a problem hiding this comment.
I'll fix this in my next commit.
| output = ( | ||
| b"REPLAYGAIN_TRACK_GAIN=-11.55 dB\nREPLAYGAIN_TRACK_PEAK=0.99998772\n" | ||
| "01.flac: -1.234567 1.234567 1.987654 -1.987654\n" | ||
| "02.flac: -1.234567 1.234567 -1.987654 1.987654\n" | ||
| ) |
There was a problem hiding this comment.
I'll fix this in my next commit.
| for item in items: | ||
| path = str(item.filepath) | ||
|
|
||
| album_gain, album_peak, track_gain, track_peak = gain_by_path[path] | ||
|
|
There was a problem hiding this comment.
I'll fix this in my next commit.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6916 +/- ##
==========================================
- Coverage 75.69% 75.68% -0.01%
==========================================
Files 163 163
Lines 21412 21428 +16
Branches 3384 3387 +3
==========================================
+ Hits 16208 16218 +10
- Misses 4405 4410 +5
- Partials 799 800 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/plugins/test_replaygain.py:414
- grug see test use album_peak 1.234567, but code say peak part of full scale (FS is 1.0). peak > 1.0 not real and make reader confuse. grug want peak <= 1.0 in sample output.
"01.flac: -1.234567 1.234567 1.987654 0.123456\n"
"02.flac: -1.234567 1.234567 -1.987654 0.987654\n"
docs/changelog.rst:54
- grug see changelog say 🐛
6916but PR say fix #6915. wrong bug number make release note point wrong place. grug want use 6915.
- :doc:`plugins/replaygain`: Fix ReplayGain metaflac backend altering input
files. :bug:`6916`
|
I have fixed all elements raised by GitHub Copilot. I did try to stay consistent and to keep it simple. Furthermore, I left the conversation opened by GitHub Copilot as-is so you can close them when reviewing the changes I made from its input. Let me know if you need other changes. |
| for item in items: | ||
| track_gain_value = results[item][1] | ||
| track_gains.append(track_gain_value) | ||
|
|
There was a problem hiding this comment.
It just occurred to me that my current implementation might not be the cleanest.
With the current implementation, if multiple tracks are passed to the compute_track_gain function, it will calculate the album gain across all tracks, even though they are not in the same album (e.g. singletons).
The next implementation partially reverts to the previous implementation, fixing this issue:
diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py
index 748de174b..916623af4 100644
--- a/beetsplug/replaygain.py
+++ b/beetsplug/replaygain.py
@@ -688,11 +688,16 @@ class MetaflacBackend(Backend):
task.track_gains = None
return task
- results = self._read_gain(items, task.target_level)
-
track_gains: list[Gain] = []
+
+ # Each item has to be processed individually to avoid metaflac's album
+ # gain calculation across multiple tracks that are not part of the same
+ # album (e.g., when importing singletons).
for item in items:
- track_gain_value = results[item][1]
+ result = self._read_gain([item], task.target_level)
+
+ track_gain_value = result[item][1]
+
track_gains.append(track_gain_value)
task.track_gains = track_gainsIf my assumptions are correct, I can rebase with the main branch and push this new commit.
Description
Fixes #6915.
I was able to try out this PR and it works as expected. The ReplayGain tags are now only written to the copy of the file in the library, and not to the original file that was imported from.
To Do
Remaining questions
As this is my first contribution to beets and that I do not consider myself very experienced with Python, I would like to ask for feedback on the following:
Should I add a check if the- GitHub Copilot seems to have answered my question.--scan-replay-gainoption is available and throw aFatalReplayGainErrorexception if not, or is it sufficient to mention it in the documentation?track_gainslist? I find the current implementation more readable, but I am not sure if it is the best practice.Thank you for your time and for reviewing my PR.
AI disclaimer
I have used AI to help me write this PR.