replaygain: Assorted refactoring #4851
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wisp3rwind
force-pushed
the
pr_rg_refactor_2
branch
from
635ac2f
to
9742f11
Compare
sampsyo
approved these changes
Jul 18, 2023
ybnd
suggested changes
Jul 18, 2023
There was a problem hiding this comment.
I don't have much to add to @sampsyo's comments in terms of the refactor itself.
Ran beet replaygain with the command, ffmpeg and gstreamer backends (I could not get audiotools installed for some reason)
Confirmed that all of the track/album gain calculations and write operations remain identical compared to the master branch.
Found a tiny typo with album gain in ffmpeg, see below.
wisp3rwind
force-pushed
the
pr_rg_refactor_2
branch
2 times, most recently
from
d2c90d9
to
fd770ae
Compare
to check for an optional ThreadPool. Seems more idiomatic and readable.
doesn't seem to be very useful to log the _list of items_ here
...album gains. This is in preparation for parallelizing the track analysis, and computing the album values in the plugin's "main thread" once all items are done.
now that we have the `track_results` upfront, some simplifications are possible
This is a preparation for moving the Gst calculation to multiprocessing worker threads. Passing only the file paths to the worker threads instead of synchronizing the entire `Item`s (i.e. minimizing the data that is shared between the processes) hopefully helps to prevent any issues with this approach.
…ests by adding files which are not completely silent, thus hitting a different code path in some calculations The sample files were generated using > sox -n whitenoise.flac synth 00:00:02 whitenoise > ffmpeg -i whitenoise.flac whitenoise.opus > ffmpeg -i whitenoise.flac whitenoise.mp3
wisp3rwind
force-pushed
the
pr_rg_refactor_2
branch
from
fd770ae
to
3965858
Compare
|
Thanks for the feedback! This should be good to go now: I've added a version of the ffmpeg tests with non-silent files (turns out there's SoX which makes it really easy to generate audio files with some synthesized sounds) and addressed all of your comments. |
ybnd
approved these changes
Jul 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm trying to revive my work on improved parallelization (in particular for replaygain, cf. #3893). A while ago, I've been exploring an alternative approach to the the elaborate async approach of #3893 using beets's pipeline threads + an additional pool of worker threads for CPU intense computation. In this way, no intrusive refactoring of beets' pipeline is required and the resulting code feels easier to understand.
This PR is preparatory work for the new approach, which is however quite independent from the actual rewrite of parallelization; and could be merged already.