reload track metadata changes gain of a playing track

By the way: The new feature also suffers the ctrl-A problem. If you try to reload the metatdata of the whole library, mixxx GUI is freezed very very long.

Why not making the library scanner do this work? (latest version with patch from bug #801700)

Do we have a already bug number for the ctrl-A problem of the other context menu features? Or should I file a new one?

While this is valid, I think it would be preferable to use the ReplayGain value in the metadata if it's present when "reload from default" is selected. Otherwise, yes, leave it alone.

(As for the Ctrl-A problem, if you can't find a relevant bug using a few search terms, please file a new one.)

I'm not sure what we could do to fix this. It's intended behavior that the track gain changes once the replaygain is calculated. This is so that you can load a track, and once the analyser finishes (hopefully soon after) you get the replaygain adjustment on the fly.

If you reload metadata, ideally we will not reset the replaygain we pre-calculated /unless/ there is replaygain stored in the file. But if the file had different replaygain information then it would be nice if we could prevent it from changing on the fly but there is currently no easy way.

For 1.10, I added a simple warning message about this problem when reload track metadata. That should be a good enough band-aid until we can figure out a good way to actually solve the problem.

I am attaching a patch to fade volume on reload track metadata.
This is another band aid.
Indeed, replaygain currently is worked into enginepregain.* and this is good as we are calling SampleUtil::copyWithGain only one time. On the other side, enginepregain is a deck stuff, while replaygain belongs to track...

Thank you Vittorio for the patch. Fading to the new volume will make the Volume drop less irritating for the audience.

But for me the best solution is one without any change of volume during on track.
This issue will happen when the replay gain analysis is finished and when reload meta data is selected.
Most likely it will happen if you play an unanalysed track and the new replay gain is adopted in the middle of the Track.
I this case the DJ will most likely have manual adjusted the gain so the change to the calculated replay gain will always produce an unintended volume shift.

I have prepared a solution that simply does not adopt the replay gain when a track is played. This is combined with the patch from Bug #1008721 so the DJ can see from the overview waveform if the new replay gain analysis is finished. So its up to him to wait until the replay gain is adopted or play the song before and adjust gain manual. In any case the there is no unintended volume change and the annoying warning pop-up from 1.10.1 is unnecessary.

The attached patch is against lp:mixxx/1.11 #3298

Hi Daniel,

I agree with the overall idea of not changing the gain on a playing track and think it should go in 1.11.

Here are my comments on the patch:

* Instead of blocking TrackInfoObject's ReplayGainUpdated signal, I think that instead it should be blocked in BaseTrackPlayer. See the connection

connect(m_pLoadedTrack.data(), SIGNAL(ReplayGainUpdated(double)), m_pReplayGain, SLOT(slotSet(double)));

This connection is what communicates the track's replay-gain to the engine replaygain control object.

Instead of hooking this up to slotSet(), instead you should make a 'BaseTrackPlayer::slotReplayGainUpdated(double)' slot and hook it up to that. In that slot, check if the deck is playing, and if it is not playing then call m_pReplayGain->slotSet(value);

This removes the need for using PlayerInfo in TrackInfoObject and also lets other parts of Mixxx that want to be notified when the replaygain changes (but for which it doesn't matter if the track is playing or not) can listen to the track's signal.

* Could you explain why you made the analysis report 10% slower? I don't see why this is needed. Is it because some Analysers (such as AnalyserWaveform) take a while to finalise() ? If it's about visual-feedback that RG analysis is done, the waveform will grow or shrink once the replaygain has been applied in EnginePregain so I think the user has adequate feedback that analysis is done.

* Could you explain the need for sleep(1) in AnalyserGain? Why do we need to wait until the replaygain has been updated in EnginePregain?

* Instead of using the waveform progress, could you add a TrackInfoObject setAnalysisProgress/getAnalysisProgress (and maybe isAnalyzing(), setAnalyzing) method and then in AnalyserQueue::doAnalysis, call AnalyserQueue setAnalyzing(true) when the track is being analyzed and then set the progress every time through the analysis loop in AnalyserQueue::doAnalysis?

It would be great if we could bring back the 1.10.x-style progress bar and make it independent of the waveform analysis. This way if replaygain is analyzing but the track waveform is already analyzed then we can still render a progress bar. Mabye we can make a progress-bar widget or re-introduce the progress-bar into the waveform overview.

Attached you will find a patch issuing the comments from #7:

* ReplayGainUpdated signal is still emitted but blocked in BaseTrackPlayer when track is played. This is just fine, but it does not solve the corner cases when track goe to the output with other functions than play.

* The waveform overview painting is now hooked to the analysisProgress value in 0,1 % steps. This signal is transmited at most each 60 ms. Now more update polling timer is required in overview. Doing this we have a visual over all analyser progress in the waveform overview even if the waveform is already analysed.

* The analysis process functions are finished at 90 % progress. 100 % progress is emitted after finalise. This is required to not have the waveform at 100 % before replay gain is adopted. The visual-feedback of replay gain by growing or shrinkin is not suitable for 100% analysis because this happens only in the case we have actual a replay gain change.
I like this solution more than having a block progress bar from 1.10 or a separated control.

* I have changed EnginePregain so that an initial replay gain set is adopted without fade. So there is now no more need for wait in AnlayserGain (sleep(1)).

* Since replay Gain changes are not adopted on playing decks, the fading only happens on Replay gain changes by boost or possible bypassing BaseTrackPlayer (does not happened yet). So we should consider removing fading at all for a better performance.

* I have also noticed that the mostly constant total gain is calculated in every process cycle. I am not sure if it worth the effort to pre-calculate it in BaseTrackPlayer or in a new gain controller class from the main tread.

* I like the decision to apply replaygain being decided by BaseTrackPlayer
* I like adding an analyser progress meter back into WOverview (so you can see progress even if the overview waveform is pre-calculated)
* I like the change to EnginePregain to immediately apply the initial replaygain.
* The total-gain calculation could be pre-calculated but that's probably a very minor performance optimization.
* I think whether replaygain should take effect even if a track is playing should be a ReplayGain preference defaulted to off.

My main objection to the patch is still the artificial delay of the progress. It seems to me this adds a lot of complexity for only a little bit of a benefit.

Hi RJ,

thank you for reviewing this.

For me the main issue is solved without having a progress visualisation for the finalise part of the analysis queue,
because of the Replay gain is not adopted when playing.
So if you like, I will prepare a patch which only visualise the process function part.

Although I am still in favour for a solution which visualise the whole analysis time until finished. It is a more general solutions for all upcoming analyser tasks which have a long finalise function like replay gain analysis has today.

I do not see a valid use case for replay gain adoption on a playing deck. So I think we need not implement a preference option, until we have one.

What do you think?

Kind regards,

Daniel

On Thu, Sep 6, 2012 at 5:04 PM, Daniel Schürmann
<email address hidden>wrote:

> Hi RJ,
>
> thank you for reviewing this.
>
> For me the main issue is solved without having a progress visualisation
> for the finalise part of the analysis queue,
> because of the Replay gain is not adopted when playing.
> So if you like, I will prepare a patch which only visualise the process
> function part.
>

Sounds good -- let's go with that for now.

>
> Although I am still in favour for a solution which visualise the whole
> analysis time until finished. It is a more general solutions for all
> upcoming analyser tasks which have a long finalise function like replay
> gain analysis has today.
>

In practice I'm seeing ReplayGain's finalize step taking less than 1ms on
average. How long are you seeing it take?

>
> I do not see a valid use case for replay gain adoption on a playing
> deck. So I think we need not implement a preference option, until we
> have one.
>

OK we can hold off on that.

Thanks a lot,
RJ

> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/840860
>
> Title:
> reload track metadata changes gain of a playing track
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/840860/+subscriptions
>

Attached you find a patch as announced.

additional changes:
* #define FINALISE_PERCENT 0 for no progress during finalise
* unused AnalyserQueue::numQueuedTracks() removed
* Analyser thread wait for Progress update slot finished (for dejerk waveforms during analysis)
* Progress information are not transmitted through the QT event queue (better performance)
* fixed possible raise conditions with waveform data in tio. They are directly updated though a pointer now.
* Prepare analyser progress no shows the current track intdex in queue like "5/5 70 %"

Info: On my Atom Netbook, the finalise step takes about 3 s for usual tracks with replay gain analysis enabled.

Updated patch without deadlock on exit mixxx with running analysis.

Whew! I finally got to reviewing this huge patch. I managed to split it up into some more bite-sized chunks.

* I reworded the warning a little bit in the replaygain dialog.

* RE: the changes to EnginePregain, I think using QTime in the callback is not OK since QTime does a lot of (potentially blocking) work to tell what the time is. I think clock() (what we had before) is much more efficient. I'm not sure if there was any change here beyond switching to QTime so let me know if I left something out.

Thanks a lot for the fixes!

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