ReplayGain weird behaviour: a possible fix

[Vittorio Colao]

I fear that the original ReplayGain lib we are using is all but thread-safe.
While working on the vamp branch, I wrote some code which seems to me more suitable for using in analysers.
Please find attached a patch for the current trunk.

[RJ Skerry-Ryan (rryan)]

Hm! Looks like a good move. If multiple analyser queues are running at once (the main track analysis queue and the library 'Analyze' section) then this would likely cause problems since you're right, replay-gain isn't re-entrant since it uses all those static variables.

[Albert Santoni (gamegod)]

Is this why we sometimes see the waveform "pop" as the gain is being automatically adjusted?

[RJ Skerry-Ryan (rryan)]

Since this could explain some segfaults we've seen 1.10.1 is reasonable.

[RJ Skerry-Ryan (rryan)]

Thanks Vittorio!

I noticed that every process() call you allocate a new buffer. This will have significant performance implications since that will allocate 8192 bytes every process() call. Beyond that I don't think it was necessary since AnalyserRg has its own internal buffers so you don't need to create a new one. This will also put a lot of pressure on the heap which could be causing priority inversion with the engine and other parts of MIxxx.

One additional thing I saw you did was hand-unroll a lot of loops. This is the sort of thing that GCC does for us (and it's incredibly rar a programmer can even come close to what GCC can do) so there's no sense in competing with GCC here ;)

I made both of these changes to your patch and committed to 1.10. I think the heap-spray issue may be part of why analysis causes xruns in trunk so I'm hopeful that this change will help that a lot.

[RJ Skerry-Ryan (rryan)]

Sorry, not 8192 bytes, 8192 floats so 32kb... roughly 132MB in total for a
12 minute 48khz track.

On Mon, May 14, 2012 at 4:38 PM, RJ Ryan <email address hidden> wrote:

> Thanks Vittorio!
>
> I noticed that every process() call you allocate a new buffer. This will
> have significant performance implications since that will allocate 8192
> bytes every process() call. Beyond that I don't think it was necessary
> since AnalyserRg has its own internal buffers so you don't need to
> create a new one. This will also put a lot of pressure on the heap which
> could be causing priority inversion with the engine and other parts of
> MIxxx.
>
> One additional thing I saw you did was hand-unroll a lot of loops. This
> is the sort of thing that GCC does for us (and it's incredibly rar a
> programmer can even come close to what GCC can do) so there's no sense
> in competing with GCC here ;)
>
> I made both of these changes to your patch and committed to 1.10. I
> think the heap-spray issue may be part of why analysis causes xruns in
> trunk so I'm hopeful that this change will help that a lot.
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/912258
>
> Title:
> ReplayGain weird behaviour: a possible fix
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/912258/+subscriptions
>

[Vittorio Colao (l0rdt)]

Hey RJ,

Thanks for fixing it!

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6230