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

Use the real track length to scale waveforms #11162

Merged
merged 11 commits into from
Feb 20, 2023

Conversation

daschuer
Copy link
Member

Without this PR, the waveform signal is scaled from the length of the analysis data, while beats and cue points are placed relative to the actual length of the track. In case the track length changes for some reasons the waveform signal will have a building up offset towards the end of the track. This is a visual issue that the beat marker are no longer on the beats visible in the waveform.
This PR fixes the issue by using the loaded track length for all renders in the waveform and waveform overview.

The original decoding issue that may lead to an offset between waveform and audio is not fixed.

@github-actions github-actions bot added the ui label Dec 28, 2022
@robbert-vdh
Copy link
Contributor

This fixes #11159, or at least the visual aspect of it. Not sure why Mixxx stripping ID3v1.1 tags would cause the track's length to change. How is the track's length computed? It sounds like it might consider the ID3v1.1 tag chunk as part of the track's audio when computing the file's total length, or something like that.

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2023

Not sure why Mixxx stripping ID3v1.1

This has been fixed here:
#11163

How is the track's length computed?

The library scanner read the length for the ID3 tag. When the rpm is loaded, all frame headers are read and a lookup table for random seeks is build. That the real length is available and replaces the value form the ID3 Tag.

@@ -36,7 +37,7 @@ bool WOverviewRGB::drawNextPixmapPart() {
// by total_gain
// We keep full range waveform data to scale it on paint
m_waveformSourceImage = QImage(
dataSize / 2,
static_cast<int>(getTrackSamples() / audioVisualRatio / 2),
static_cast<int>(2 * 255 * m_devicePixelRatio),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this multiplied with m_devicePixelRatio for RGB, but not for the HSV and LMH versions of the overview waveform?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh Yes that can be removed.

const auto lastVisualIndex = static_cast<GLfloat>(
m_waveformRenderer->getLastDisplayedPosition() * dataSize / 2.0);
m_waveformRenderer->getLastDisplayedPosition() * trackSamples / audioVisualRatio / 2.0);

// const int firstIndex = int(firstVisualIndex+0.5);
// firstVisualIndex = firstIndex - firstIndex%2;
Copy link
Member

Choose a reason for hiding this comment

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

There is a check above, that the integer datasize can't be <=1. Therefore dataSize / 2.0 can't be smaller than 1.0. I wonder if a similar check for trackSamples / audioVisualRatio / 2.0 is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

trackSamples and audioVisualRatio are also checked individual.

@daschuer
Copy link
Member Author

Done.

Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

Code looking good and the changes make sense to me!

@daschuer
Copy link
Member Author

Great, can we merge that to 2.3 before release?

@JoergAtGithub JoergAtGithub merged commit cfcd265 into mixxxdj:2.3 Feb 20, 2023
@JoergAtGithub
Copy link
Member

Thank you!

@daschuer daschuer added this to the 2.3.4 milestone Feb 20, 2023
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 23, 2023
napaalm added a commit to napaalm/mixxx that referenced this pull request Mar 24, 2023
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.

3 participants