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

Waveform tweaks #12466

Merged
merged 6 commits into from
Jan 2, 2024
Merged

Waveform tweaks #12466

merged 6 commits into from
Jan 2, 2024

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Dec 25, 2023

Adjust the scaling of the waveform data: removing a * 2 factor which doesn't seem justified and undo a scaling curve applied to the waveform.filtered.all data which also lacks an explanation.

See my analysis here #12448

@m0dB m0dB changed the base branch from main to 2.4 December 25, 2023 19:45
@JoergAtGithub
Copy link
Member

The scaling for looks good now, but there seems to be something, that clamps the visual amplitude. See the following video of AllShader RGB R/L and notice the Peak Indicator of the VU-Meter:

VisualClampingAllshadersRGB_RL.mp4

For comparision, the next video is recorde with exactly the same setting but GLSL RGB stacked waveforms:

NoVisualClampingRGB_stacked.mp4

@m0dB
Copy link
Contributor Author

m0dB commented Dec 27, 2023

I see what you mean. Can you send me this track so I can have a look at the values?

@m0dB
Copy link
Contributor Author

m0dB commented Dec 27, 2023

@JoergAtGithub the scaling curve seems intentional. See the discussion @Swiftb0y dug up in #12448

@ronso0 ronso0 added this to the 2.4.0 milestone Dec 27, 2023
@m0dB
Copy link
Contributor Author

m0dB commented Dec 28, 2023

What visual gain are you using? On what Replay Gain do you have on that track? I got -9.59306 dB.

Here is looks like this:
Screenshot 2023-12-28 at 01 45 12

Which seems correct, considering the ReplayGain. If I boost the visual gain with the replay gain (9.59306 dB =~ x3) I get this:

Screenshot 2023-12-28 at 01 49 24

@m0dB
Copy link
Contributor Author

m0dB commented Dec 30, 2023

I now made the gain compensation optional, so each renderer can decide whether to apply it or not. With this the amplitude is pretty consistent, only the "filtered" types are a bit different depending on the content, but I think for now this is as good as it gets. Should be merged to 2.4. Ping @ronso0 @JoergAtGithub

@mxmilkiib
Copy link
Contributor

mxmilkiib commented Dec 30, 2023

Cool cool. (On my phone and busy for a while)

Btw, because it hasn't been stated; the fact that the test track just above looks clipped before any gain.. is that just accidental? Like, that's the waveform like Audacity or whatever shows? Because it's maybe not the best example to use when specifically looking into waveform gain/height :) i.e. if the render somehow gets clipped itself, there's no telling that apart from the true waveform, or maybe the fact it looks clipped is a problem itself.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

little modernization opportunities.

@m0dB
Copy link
Contributor Author

m0dB commented Dec 30, 2023

@mxmilkiib wrote:

Btw, because it hasn't been stated; the fact that the test track just above looks clipped before any gain.. is that just accidental?

This is caused be the scaling curve that is being applied to the waveform and this is why it looks different in Audacity than in mixxx. I think it's a bad idea, but it's unclear to me if this is a bug or a feature, so I didn't want to change it.

Screenshot 2023-12-30 at 19 36 31

@mxmilkiib
Copy link
Contributor

mxmilkiib commented Dec 30, 2023

Aah right, thank you, had kinda got the gist from the convo but that makes it explicit. I have always had this niggling feeling that Mixxx waveforms were ever so sightly.. "fatter".

Does feel like, if intentional, it would be for some take re UX. Maybe there's a convo somewhere from the date of the blame commit, if no-one remembers the rational.

It is kind of an imposition though, with no option to toggle it, or/tho is that what you've just added? I did wonder how this relates to the existing waveform scaling preferences..

I guess this in a way relates to #11833

(2p ramble; I've still not settled on a preference for scaling yet. Tbh, again, Pioneer is the club standard, they'll have had certified and paid UI people come up with those, certainly by the recent start of the era of 3000s, though obvs. their exact magic is private. Figuring that out using, like, a series pure tone samples, would be a fun for some but a super low priority side quest..)

Edit: to be clear; a (previously?) unavoidable scaling curve for the waveform graphic that makes some tracks look like they're clipping, that's.. really not very good, must be and have been confusing sooo many people.

@ronso0
Copy link
Member

ronso0 commented Dec 30, 2023

@m0dB Thank you, this is much more consistent now!

@m0dB m0dB requested a review from Swiftb0y January 2, 2024 01:05
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, though in the future, I'd really like to gain back some consistency by removing the scaling in the analyzer. Are you interested in looking into that in the future?

@Swiftb0y Swiftb0y merged commit 77c175c into mixxxdj:2.4 Jan 2, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants