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: albumpeak on large collections is calculated as average, not maximum #3008

Closed
vvvrrooomm opened this issue Aug 20, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@vvvrrooomm
Copy link
Contributor

commented Aug 20, 2018

Problem

while working on parallelizing the replaygain command I found
beets/beetsplug/replaygain.py:202

 for chunk in self.isplitter(items, self.chunk_at):

                i += 1

                returnchunk = self.compute_chunk_gain(chunk, is_album)

                albumgaintot += returnchunk[-1].gain

                albumpeaktot += returnchunk[-1].peak

                returnchunks = returnchunks + returnchunk[0:-1]

            return chunks.append(Gain(albumgaintot / i, albumpeaktot / i))

this averages the album gain and album peak but the peak is supposed to be the maximum, right?
It would have to be something like albumpeaktot = max(albumpeaktot ,returnchunk[-1].peak
What about albumgain, I am not sure here?

@jackwilsdon

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Do you think you could please fill out the issue template so we can get a bit more information about your setup?

@vvvrrooomm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

I don't think so. I don't have an album with more than 5000 titles. I was reasoning how to refactor the inner part of the for loop to reuse it while making the code run multiple replaygain encoders in parallel.
github ate the linebreaks in the code section, I fixed that

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Got it; yes, that does pretty much look like a bug! Would you mind opening a pull request to see how the tests do?

@sampsyo sampsyo changed the title replaygain albumpeak on large collections is calculated wrong replaygain: albumpeak on large collections is calculated as average, not maximum Aug 20, 2018

@sampsyo sampsyo added bug and removed needinfo labels Aug 20, 2018

@vvvrrooomm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

I can fix albumpeak but not albumgain. In the meantime I did a test:
``

replaygain: analysis finished: <bs1770gain>
  <album>
    <track total="2" number="1" file="tmplnruql7v&#x2E;ogg">
      <integrated lufs="-15.11" lu="-2.89" />
      <sample-peak spfs="0.30" factor="1.035347" />
    </track>
    <track total="2" number="2" file="tmpogjqseg&#x5F;&#x2E;ogg">
      <integrated lufs="-12.04" lu="-5.96" />
      <sample-peak spfs="0.29" factor="1.033405" />
    </track>
    <summary total="2">
      <integrated lufs="-12.97" lu="-5.03" />
      <sample-peak spfs="0.30" factor="1.035347" />
    </summary>
  </album>
</bs1770gain>

This clearly shows that albumpeak is the max but albumgain is not the average

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Hmm. I’m not sure what I’m supposed to be looking for in the example. Are you suggesting that we shouldn’t be averaging the gain? While that may not be ideal, we might not have any other option if there’s no clear way to combine separate measurements.

@vvvrrooomm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

Sorry, that was unclear.
So this is generated by bs1770gain, the summary section is, well, the summary of the two tracks before. The code in question tries to mimic the behavior of this summary section.
Now, the peak value in the summary actually follows our expectation, it is the maximum of the peaks in both track entries.
BUT the other value, which seems to be used as album gain is not calclulated as the maximum. Neither is it the average, that would be 13,5 in this sample.

I would remove the whole block to prefer correctness over usability cough better to crash with many titles in one 'album' than to have the wrong gain. But I am not sure what beets prefers, also I don't like album mode anyways

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Got it; thank you for explaining!

Maybe one course of action would be look at that tool’s source to see how it does things. For example, it’s plausible that it uses a geometric or harmonic mean instead of the simple arithmetic mean—since those numbers are decibels (right?), which follow a logarithmic scale, one of those averages might be more appropriate anyway.

@vvvrrooomm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

Yeah, that's how I would expect it. I'm sorry to say I won't do that. I don't have enough time for that and I don't use album mode

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

OK; thanks for the fix on the maximum value. I’ll close this issue for now; if anyone wants to dig into the gain averaging, they can open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.