Adjust ReplayGain: Allow user to update the replaygain value based on a deck pregain value. #4031

ywwg · 2021-06-28T15:30:20Z

Currently accessible by right clicking loaded decks, including preview deck.

Also accessible through a new a new per-deck control pushbutton: "[ChannelX]", "update_replaygain_from_pregain". When pressed, the pregain of the associated deck is applied to the replaygain value.

Be-ing · 2021-06-28T22:34:40Z

Thanks for working on this! This feature has been at the back of my to-do list for a long time. WTrackMenu is also used for the right click menu of the library where multiple tracks can be selected. So I think the first commit should be removed and the second one rebased.

Be-ing · 2021-06-28T22:45:26Z

As for the UI, I think adding this to WTrackMenu is okay, but I'd primarily want to use it by mapping a ControlPushButton on a controller.

daschuer

Thank you, in general it looks good. I have added some findings.

src/mixer/basetrackplayer.cpp

src/track/track.cpp

src/widget/wtrackmenu.cpp

daschuer · 2021-06-29T09:38:09Z

That does not work if the replay gain value has been reset before.

src/mixer/basetrackplayer.cpp

ywwg · 2021-07-19T21:49:46Z

WTrackMenu is also used for the right click menu of the library where multiple tracks can be selected.

in cases where more than one track is selected, TrackModel is set. Currently m_trackPointerList is only initialized in code in a single place, inside loadTrack:

    m_trackPointerList = TrackPointerList{pTrack};

It is never initialized from another list, or ranged, etc. Therefore it's never used by more than one track.

ywwg · 2021-07-19T22:15:04Z

As for the UI, I think adding this to WTrackMenu is okay, but I'd primarily want to use it by mapping a ControlPushButton on a controller.

A track can be loaded in more than one deck -- either the button would have to be per-deck, or we would have to guess which pregain the user meant to use as a reference. How would you want to bind the button?

Be-ing · 2021-07-19T22:18:56Z

Give each deck a ControlPushButton.

Be-ing · 2021-07-19T22:27:38Z

in cases where more than one track is selected, TrackModel is set.

Ah, in that case m_trackIndexList is used rather than m_trackPointerList, so I think it may be okay to replace m_trackPointerList with a singular TrackPointer. I'd like to hear @uklotzde's thoughts on this.

src/track/replaygain.h

src/widget/wtrackmenu.cpp

uklotzde · 2021-07-19T23:00:43Z

in cases where more than one track is selected, TrackModel is set.

Ah, in that case m_trackIndexList is used rather than m_trackPointerList, so I think it may be okay to replace m_trackPointerList with a singular TrackPointer. I'd like to hear @uklotzde's thoughts on this.

I wonder why this over-engineering has not been detected yet?! It should better be changed in a separate, preliminary PR.

ywwg · 2021-07-19T23:38:20Z

I wonder why this over-engineering has not been detected yet?! It should better be changed in a separate, preliminary PR.

is the commit wrong as-is? It will be annoying to strip out of this PR, but I'd like to have an actual reason to do so.

Be-ing · 2021-07-19T23:41:13Z

is the commit wrong as-is? It will be annoying to strip out of this PR, but I'd like to have an actual reason to do so.

I agree, let's not be pedantic about splitting up the PR. As long as it's in a cleanly separate commit I think it's fine.

ywwg · 2021-07-19T23:45:36Z

The Real Problem™ is that wtrackmenu should not store important state in one of two places (a model or a pointer). So I could understand this commit as being unnecessary / wasted effort if someone else is going to go through and clean up the file to be consistent. HOWEVER, the change as written is strictly an improvement -- it removes a lot of unused / dead code and leaves things in a more understandable state.

ywwg · 2021-07-19T23:46:25Z

@Be-ing I added pushbuttons, can you test to see if they work as you hope?

coveralls · 2021-07-20T00:29:12Z

Pull Request Test Coverage Report for Build 1071599741

25 of 70 (35.71%) changed or added relevant lines in 3 files are covered.
20 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.01%) to 28.624%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/mixer/basetrackplayer.cpp	17	22	77.27%
src/widget/wtrackmenu.cpp	0	40	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/engine/cachingreader/cachingreaderworker.cpp	2	63.48%
src/widget/wtrackmenu.cpp	2	0%
src/mixxxapplication.cpp	7	75.0%
src/engine/cachingreader/cachingreader.cpp	9	71.38%

Totals
Change from base Build 1065232258:	0.01%
Covered Lines:	20104
Relevant Lines:	70234

💛 - Coveralls

Be-ing · 2021-07-20T00:50:21Z

src/mixer/basetrackplayer.cpp

@@ -192,6 +192,13 @@ BaseTrackPlayerImpl::BaseTrackPlayerImpl(

    m_pRateRatio = make_parented<ControlProxy>(getGroup(), "rate_ratio", this);
    m_pPitchAdjust = make_parented<ControlProxy>(getGroup(), "pitch_adjust", this);
+
+    m_pUpdateReplayGainFromDeckGain = std::make_unique<ControlPushButton>(
+            ConfigKey(getGroup(), "update_replaygain_from_pregain"));


This is quite a verbose name for a ControlObject... how about just update_replaygain... or set_replaygain? store_replaygain?

I like the name. Our other CO names are often extremely ambiguous (so this PR introduces inconsistency 😅). You suggestions sound like I can set a the replaygain value directly, not like a button CO that takes only 1 and 0.

yeah I don't know how to make it shorter and include the ideas of both the deck pregain and the track replaygain. "apply_pregain_to_replaygain" ?

I don't see how to remove any more words without making it confusing, and I don't think we have any limitations on CO name length

I haven't thought of any better name, so... 🤷 I won't object to merging this over finding a better name.

Be-ing · 2021-07-20T00:55:37Z

This works, yay! I used it to manually set the ReplayGain for a track which I remembered to play way louder than others. The automatic reset of the gain knob to unity was surprising at first but I think it makes sense.

Be-ing · 2021-07-20T00:55:57Z

Auuuugh the "close and comment" strikes again...

ronso0 · 2021-07-20T16:53:04Z

yaij, finally a co for this!
I don't have unmapped buttons anymore so I'll probably map it to Shift+gain knob: the first incoming value triggers the gain adoption (within 2 seconds following values are ignored), then I center the knob, release shift. voila, replay gain adjusted, knob sync'ed.

ywwg · 2021-07-21T03:00:40Z

I believe all comments are addressed

ronso0 · 2021-07-21T13:51:05Z

@ywwg Please document the behaviour and the new CO in the PR decription.
Later on we also need to add it to the manual https://manual.mixxx.org/2.4/en/chapters/appendix/mixxx_controls.html#decks-preview-decks-and-samplers

src/widget/wtrackmenu.cpp

…ne track.

… a deck pregain value. Currently accessible by right clicking loaded decks, including preview deck

… places

src/track/track.h

Always refer to "pregain" instead of "deck gain".

src/widget/wtrackmenu.h

uklotzde · 2021-07-23T21:07:36Z

Merge now or after the manual has been updated?

uklotzde · 2021-07-23T21:10:52Z

@ywwg Please create and link a PR for the manual, then we can merge.

Holzhaus · 2021-07-24T00:23:09Z

Not a blocker for merge, but coveralls points out that a bunch of lines are not covered by any test:

4 of 70 (5.71%) changed or added relevant lines in 3 files are covered.

116 unchanged lines in 6 files lost coverage..

Overall coverage decreased (-0.005%) to 28.621%

Would be nice to add a test for this.

ywwg · 2021-07-27T15:45:07Z

Added a test. Manual PR: mixxxdj/manual#417

ywwg · 2021-07-28T17:25:58Z

PR appears to be approved -- who wants to merge?

uklotzde · 2021-07-28T18:37:50Z

Thank you! LGTM

uklotzde · 2021-08-20T10:06:47Z

https://bugs.launchpad.net/mixxx/+bug/1940589

github-actions bot added the ui label Jun 28, 2021

Be-ing requested a review from uklotzde June 28, 2021 22:34

daschuer requested changes Jun 29, 2021

View reviewed changes

daschuer reviewed Jun 29, 2021

View reviewed changes

src/mixer/basetrackplayer.cpp Show resolved Hide resolved

uklotzde suggested changes Jul 19, 2021

View reviewed changes

src/track/replaygain.h Outdated Show resolved Hide resolved

src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved

src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved

src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 20, 2021

View reviewed changes

Be-ing closed this Jul 20, 2021

Be-ing reopened this Jul 20, 2021

ywwg requested review from daschuer and uklotzde July 21, 2021 02:59

uklotzde suggested changes Jul 21, 2021

View reviewed changes

src/widget/wtrackmenu.cpp Show resolved Hide resolved

src/widget/wtrackmenu.cpp Show resolved Hide resolved

src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved

src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved

Refactor unnecessary list of tracks in wtrackmenu -- it's only ever o…

afbbe72

…ne track.

ywwg added 4 commits July 21, 2021 14:53

Adjust ReplayGain: Allow user to update the replaygain value based on…

f6e6b68

… a deck pregain value. Currently accessible by right clicking loaded decks, including preview deck

Replaygain: rework implementation to put the right logic in the right…

7cd5ded

… places

ReplayGain: Use debug asserts for situations that shouldn't occur

6a2721e

Update ReplayGain: address small notes

a7a49d0

ywwg force-pushed the update-replaygain branch from 3f74538 to a7a49d0 Compare July 21, 2021 21:36

uklotzde reviewed Jul 21, 2021

View reviewed changes

src/track/track.h Outdated Show resolved Hide resolved

ywwg added 2 commits July 21, 2021 22:14

Merge branch 'main' into update-replaygain

cdf5f35

Update Replaygain: normalize naming style

460124b

Always refer to "pregain" instead of "deck gain".

daschuer approved these changes Jul 22, 2021

View reviewed changes

uklotzde reviewed Jul 22, 2021

View reviewed changes

src/widget/wtrackmenu.h Outdated Show resolved Hide resolved

Update ReplayGain: Use consistent case for ReplayGain

77e95fb

uklotzde approved these changes Jul 23, 2021

View reviewed changes

ywwg added 2 commits July 27, 2021 10:13

Merge branch 'main' into update-replaygain

2d92e5a

Update Replaygain: Add a test

0b624a0

github-actions bot added the code quality label Jul 27, 2021

ywwg added the changelog This PR should be included in the changelog label Jul 27, 2021

ywwg mentioned this pull request Jul 27, 2021

Add entries for replaygain update functionality. mixxxdj/manual#417

Merged

uklotzde merged commit 460f3a8 into mixxxdj:main Jul 28, 2021

Be-ing mentioned this pull request Aug 31, 2021

Kontrol S4 Mk3 ES6 mapping #4056

Closed

mixxxbot mentioned this pull request Aug 23, 2022

AdjustReplayGainTest sometimes segfaults #10497

Closed

ronso0 mentioned this pull request Mar 22, 2023

remember gain adjustments values #8268

Closed

daschuer added this to the 2.4.0 milestone Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust ReplayGain: Allow user to update the replaygain value based on a deck pregain value. #4031

Adjust ReplayGain: Allow user to update the replaygain value based on a deck pregain value. #4031

ywwg commented Jun 28, 2021 •

edited

Loading

Be-ing commented Jun 28, 2021

Be-ing commented Jun 28, 2021

daschuer left a comment

daschuer commented Jun 29, 2021

ywwg commented Jul 19, 2021

ywwg commented Jul 19, 2021

Be-ing commented Jul 19, 2021

Be-ing commented Jul 19, 2021

uklotzde commented Jul 19, 2021 •

edited

Loading

ywwg commented Jul 19, 2021

Be-ing commented Jul 19, 2021

ywwg commented Jul 19, 2021

ywwg commented Jul 19, 2021

coveralls commented Jul 20, 2021 •

edited

Loading

Be-ing Jul 20, 2021

Holzhaus Jul 20, 2021

ywwg Jul 20, 2021

ywwg Jul 20, 2021

Be-ing Jul 23, 2021

Be-ing commented Jul 20, 2021

Be-ing commented Jul 20, 2021

ronso0 commented Jul 20, 2021 •

edited

Loading

ywwg commented Jul 21, 2021

ronso0 commented Jul 21, 2021

uklotzde commented Jul 23, 2021

uklotzde commented Jul 23, 2021

Holzhaus commented Jul 24, 2021

ywwg commented Jul 27, 2021

ywwg commented Jul 28, 2021

uklotzde commented Jul 28, 2021

uklotzde commented Aug 20, 2021

Adjust ReplayGain: Allow user to update the replaygain value based on a deck pregain value. #4031

Adjust ReplayGain: Allow user to update the replaygain value based on a deck pregain value. #4031

Conversation

ywwg commented Jun 28, 2021 • edited Loading

Be-ing commented Jun 28, 2021

Be-ing commented Jun 28, 2021

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Jun 29, 2021

ywwg commented Jul 19, 2021

ywwg commented Jul 19, 2021

Be-ing commented Jul 19, 2021

Be-ing commented Jul 19, 2021

uklotzde commented Jul 19, 2021 • edited Loading

ywwg commented Jul 19, 2021

Be-ing commented Jul 19, 2021

ywwg commented Jul 19, 2021

ywwg commented Jul 19, 2021

coveralls commented Jul 20, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1071599741

💛 - Coveralls

Be-ing Jul 20, 2021

Choose a reason for hiding this comment

Holzhaus Jul 20, 2021

Choose a reason for hiding this comment

ywwg Jul 20, 2021

Choose a reason for hiding this comment

ywwg Jul 20, 2021

Choose a reason for hiding this comment

Be-ing Jul 23, 2021

Choose a reason for hiding this comment

Be-ing commented Jul 20, 2021

Be-ing commented Jul 20, 2021

ronso0 commented Jul 20, 2021 • edited Loading

ywwg commented Jul 21, 2021

ronso0 commented Jul 21, 2021

uklotzde commented Jul 23, 2021

uklotzde commented Jul 23, 2021

Holzhaus commented Jul 24, 2021

ywwg commented Jul 27, 2021

ywwg commented Jul 28, 2021

uklotzde commented Jul 28, 2021

uklotzde commented Aug 20, 2021

ywwg commented Jun 28, 2021 •

edited

Loading

uklotzde commented Jul 19, 2021 •

edited

Loading

coveralls commented Jul 20, 2021 •

edited

Loading

ronso0 commented Jul 20, 2021 •

edited

Loading