Fixes different scaling of scrolling waveforms#15324
Conversation
…veforms. This is no longer necessary.
It turns out to be a good default for -18 LUFS (default) and -16 LUFS (recommended) with modern pop tracks.
|
This compares the types In 2.6 Filtered and Stacked are too tall -- in this PR they're too small, and additionally legacy RGB is much smaller. When I enable High Details, there are additional differences. |
ronso0
changed the title
|
I need to check legacy RGB. This PR aims to use the same scaling for scrolling and preview waveform, at the first place. This allows to adjust them consistently at once. Can you confirm that? My idea was that the global gain needs to be adjusted anyway according to the genre and audio gain settings. This does not mean that we can't do better though. My "new" picture above is from accelerated. It shows for my understanding both extremes. A single frequency sine and a noise containing all frequencies. Real music is between that. You can see that the gain matches for all types with noise. Whenever the music contains only one sound, stacked and filtered become smaller. Sure we can adjust this, but we can't find a value that matches for all genres and all parts in a single track. A real solution would be a compensation on the waveform itself. If the three bands are equal spread low gain if only one band is in the sound high gain. What do you think? |
|
I have originally used white noise with a equal distribution of all frequencies. This has an average level of l = 0.18 m = 0.52 h = 1. But we have much more high frequencies than low using equal step sizes. Music is however more like pink noise. It has l = 0.24 m = 0.24 h = 0.24. |
|
There was an issue with scaling of the RGB signal by using a quadratic transformation. I have removed this and now it behaves the same as simple and HSV. I have also removed the unscaled shaddow from the high detailed waveforms, because there is no use for them. The filtered and stacked waveforms are still significantly smaller. This is expected because in case of pink noise each band contains only 1/3 of the power. I have tested a compensation value that works for real tracks, but have not found one, because there are tracks that are reaching full scale even with one band only like "Downlink - Robo Kitty" |
|
Nice, will test this soon. |
|
For now filterd shows the truth in terms of power on each channel. This is a valid use case IMHO. We may apply compression to "filtered" to mitigate tracks like "Downlink - Robo Kitty". For filtered also the default band gain of 1 is not perfect if the use is to see track details. For instance in case of pink noise you only see the high band. This seems to be a separate PR with a bit or requirements engineering forehead. |
|
For me Filtered and Stacked are now much smaller |
|
You see the removed internal compensation factor of 2 for the filtered types. I have removed it on purpose to make the signals comparable and not clip. See my comparison "after" in the initial post. There are now equal when using a sine wave at 1 kHz which is full in the mid band, while the base and terrible bands are empty. The other extreme is a pink noise signal where the signal is equally distributed among all bands and the filtered signal appears as only 1/3 of the simple waveform. A real track is in between. But we don't know where. That's why my conclusion is to show the truth and allow the user to adjust it and deal with visual clipping. I can confirm that this is not ideal, but at least better than surprising fixed internal scaling. A solution could be like the stacked Diagram in Excel: |
acolombier
left a comment
acolombier
left a comment
There was a problem hiding this comment.
It feels like some of these changes have a significant disruption risks and I am wondering if this is wise to introduce that to 2.6. Changes that concern me are:
- New global gain default changed from
1to2 - Removal of alpha capability on the shader
- Add a renderer using deprecated backend, and introducing a different pattern that the new introduce rendergraph, allowing forward compatibility with QML
I would suggest dropping point 1 and 2, tho I am happy to go ahead if other @mixxxdj/developers think it is the right/safe way forward. For point 3, please change to either use the rendergraph or make it a developer-only type.
| breadth, | ||
| x, | ||
| breadth - | ||
| (int)(heightFactor * |
There was a problem hiding this comment.
Align with the other chnages of this PR, you could use static_cast here and bellow
| #endif | ||
| default: | ||
| return new EmptyWaveformWidget(viewer->getGroup(), viewer); | ||
| return new SimpleSignalWaveformWidget(viewer->getGroup(), viewer); |
There was a problem hiding this comment.
I consider that a regression to the work we have done with @m0dB to remove these renderer and allow compatibility with Rendergraph. Please remove it, and only allow it in developer mode.
Also, please keep the default to empty waveform and add a new type instead.
There was a problem hiding this comment.
We are here in the WaveformWidgetFactory::createSimpleWaveformWidget() function. Using the empty waveform when a Simple is selected but now GL is available, was only a workaround for this situation.
This is no longer required because all renderer types are supported.
The SimpleSignalWaveformWidget() is not restoring the old code. It is based on the new code also used for RGB and HSV waveforms. Since they are not an issue for 2.6. this one is also not an issue.
Maybe @m0dB can confirm this?
The topic in #11838 is about removing all simple waveform altogether because no one is using them. This is a separate topic.
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
Let's look closely to the disruption risks :
- New global gain default changed from 1 to 2
I have moved the hidden gain of 2 to the preferences. This allows the users to see the truth. It continues the work started by @ronso0 in #14463 to make the visual gain more trust worthy. After testing it with different high dynamic tracks, removing the hidden gain of 2 is actually a bug fix: Overview and Scrolling waveform use the same global gain, but only some scrolling types have the additional gain of 2.
There is a small disruption risk, of that users need to re-adjust the visual gain. This is probably anyway required after #14463 and this fix allows to do it properly only one time during the 2.6 upgrade. This needs to be verified.
- Removal of alpha capability on the shader
Some waveform had an broken overlay of the scaled waveform. I consider this information as useless when using replay gain. Without the alpha, waveform have a higher contrast for better visibility. So I have just removed it instead of fixing.
I don't see a disruption risk.
- Add a renderer using deprecated backend, and introducing a different pattern that the new introduce rendergraph, allowing forward compatibility with QML
This does not "introduce" a pettern. We have it also for RGB and HSV waveform, based on CPU rendering. I think we need always provide a CPU rendered waveforms for targets without propper GL hardware/drivers. If this is an issue it is out of scope if this PR
I don't see a disruption risk. Is is a fix for no waveform simple when no GL is detected.
Can one confirm that?
| #endif | ||
| default: | ||
| return new EmptyWaveformWidget(viewer->getGroup(), viewer); | ||
| return new SimpleSignalWaveformWidget(viewer->getGroup(), viewer); |
There was a problem hiding this comment.
We are here in the WaveformWidgetFactory::createSimpleWaveformWidget() function. Using the empty waveform when a Simple is selected but now GL is available, was only a workaround for this situation.
This is no longer required because all renderer types are supported.
The SimpleSignalWaveformWidget() is not restoring the old code. It is based on the new code also used for RGB and HSV waveforms. Since they are not an issue for 2.6. this one is also not an issue.
Maybe @m0dB can confirm this?
The topic in #11838 is about removing all simple waveform altogether because no one is using them. This is a separate topic.
|
I prefer the short-term disruption of some waveforms changing in size to the current constant disruption of having to adjust the scaling for every waveform change. Maybe we can alter people's scaling factors in this upgrade to compensate? I will do some testing -- note that I had added hacks many years ago to make the waveform more useful at the expense of being technically correct. At least in dance music, so much energy is in the low end (often 70% or more) that treating each band equally would produce uniformly red waveforms. One option is we could treat the bands perceptually, the way LUFS does. (the human ear "feels" bass as being quieter than it is) |
|
What's the way forward here. @ywwg and @acolombier did you had the chance to give this a try to see the changes in action? |
acolombier
dismissed
their stale review
Dropping the blocker as I won't be able to look into this for the next few days.
Still not convince this won't impact the QML efforts
Note that is still the case with this PR.
Though I didn't test that, yet. |
|
Who likes to test and merge that? Let's collect test results in the 2.6 branch. |
|
This works well for me! There are slight visual differences but overall the scaling feels consistent. LGTM |
|
@ywwg did you forget to press merge? |
4 participants
fixes #15322
Before:
440 Hz sine:
Noise:
After:
Since after is significant smaller than before, I have added a default visual Gain of 2.0