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

Improved VinylControlXwax::establishQuality #3279

Merged
merged 9 commits into from
Dec 15, 2021

Conversation

JoergAtGithub
Copy link
Member

I changed the implementation of VinylControlXwax::establishQuality that it doesn't only consider the position codes, but also the pitch information. This provides control signal quality information also in case of slow spinning control vinyls like cueing (but not in case of heavy scratching).
The intention is to use the control signal quality in follow up PRs as input for other logic, where I need to get a reliable information in short time (e.g. disable the Passthrough-Button, if a control vinyl is on the deck).
The new implementation allowed it to reduce the size of the quality ring buffer from 100 to 32. Which reduces response time and calculation effort.
Currently this information is only used for the color of the vinyl control scope.

@JoergAtGithub JoergAtGithub force-pushed the establishqualitywithpitch branch from 7952c65 to 1b667d7 Compare November 9, 2020 21:25
@Be-ing Be-ing requested a review from ywwg November 10, 2020 01:38
src/vinylcontrol/vinylcontrolxwax.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Dec 8, 2020

Can you rebase this on main to get GitHub Actions to make a Windows build? I could then test this on my friend's timecode setup, although I'm not sure when I'll next get a chance to do so.

… pitch information additional to the position code.

This results in faster response and the function provides also quality information at slow rotation
@JoergAtGithub JoergAtGithub force-pushed the establishqualitywithpitch branch from 6011087 to 9423935 Compare December 8, 2020 20:31
@JoergAtGithub
Copy link
Member Author

@ywwg Do you intend to review this?

@ywwg
Copy link
Member

ywwg commented Jan 20, 2021

I'll try to take a look, but don't block on me. (Note that the PR reports conflicts)

@Holzhaus
Copy link
Member

@mixxxdj/developers Has anyone else have DVS setup to test this with?

I do have timecode vinyl but didn't look into affordable SL-1210 yet, just a consumer record player from the 80s that I only use for listening to my Jazz collection.

@JoergAtGithub JoergAtGithub force-pushed the establishqualitywithpitch branch from fe3f655 to d323cb2 Compare February 13, 2021 11:00
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

thanks for helping improve this old code!

// Position code changed not by more than 5. This indicates a normal spinning conrol vinyl.
positionQualityPercent = 100;
} else {
// Position code changed by more than 5. This indicates a fast spinning control vinyl, a jumping needle or a false signal.
Copy link
Member

Choose a reason for hiding this comment

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

what if we use a simple calculation that gradually reduces the qualitypercent as the velocity increases? Rather than a three-step threshold.

Copy link
Member Author

@JoergAtGithub JoergAtGithub Feb 13, 2021

Choose a reason for hiding this comment

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

I don't see, how this can be expressed by a linear expression:

  • Position code -1 is a special case which needs to be handled
  • +-5 codes difference happen in normal operation due to the sample rate of the quality information. If the vinyl spins a bit faster than 100% nominal speed, it's still a good signal, but we will have missing position codes.
  • Higher code differences, are either a extreme fast spinning vinyls, a jumping needle or a weak signal.

if (pitchStability < 3.0) {
// Unlikely that this pitch difference is from a proper set up control vinyl
pitchQualityPercent = 0;
} else if (pitchStability > 6.0) {
Copy link
Member

Choose a reason for hiding this comment

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

again can you represent this set of thresholds with a simple linear equation?

Copy link
Member Author

Choose a reason for hiding this comment

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

(1 + tanh( pitchstability + 3 ) ) * 50 could work.
But this would require much more calculation effort, in this often executed real time code, than the two comparisations.
Do you think it's worth it?

src/vinylcontrol/vinylcontrolxwax.cpp Outdated Show resolved Hide resolved
@Be-ing Be-ing requested a review from ywwg February 23, 2021 04:21
@Holzhaus
Copy link
Member

Holzhaus commented Apr 2, 2021

Ping @ywwg. I don't have a DVS setup to test this with.

@Holzhaus
Copy link
Member

Holzhaus commented May 5, 2021

@ywwg What's the status of this? Do you agree with @JoergAtGithub's replies to your review comments? Is there more to be done?

@ywwg
Copy link
Member

ywwg commented May 30, 2021

Sorry for the lack of update. I'll try to take a look at this soon

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Aug 29, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Aug 29, 2021

ping @ywwg

@Holzhaus
Copy link
Member

ping @ywwg

Maybe ping him on Zulip, I think he mentioned that he doesn't always see the notifications on GH.

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Aug 30, 2021
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

I'm really sorry for the incredible delay on this. This is good and fine overall, but I do see the following output on the console: warning [Main] QColor::setHsv: HSV parameters out of range. This seems to happen when the quality is low. Double check in wspinny.cpp that we aren't passing crazy values to qual_color.setHsv


void VinylControlXwax::establishQuality(double& pitch) {
m_iQualityRing[m_iQualityRingIndex] = getPositionQuality() + getPitchQuality(pitch);
m_fTimecodeQuality =
Copy link
Member

Choose a reason for hiding this comment

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

Github isn't letting me do a multiline suggestion, but this should be:

m_fTimecodeQuality = std::max<float>(0.0, std::min<float>(1.0,
            static_cast<float>(std::accumulate(
                    m_iQualityRing, m_iQualityRing + m_iQualityRingFilled, 0)) /
            2.0f / 100.0f /
            static_cast<float>(m_iQualityRingFilled))); // Two information in percent per datapoint

clamp the value from 0 to 1 to prevent the HSV warnings I noted below

Copy link
Member

Choose a reason for hiding this comment

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

or actually, we have a math_clamp that can do it

Copy link
Member

Choose a reason for hiding this comment

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

@JoergAtGithub if you're no longer around I'll open a duplicate PR that preserves your commits, and add this one change -- sorry again for the delay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Member Author

Choose a reason for hiding this comment

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

@ywwg This is done!

Copy link
Member

Choose a reason for hiding this comment

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

thank you!

@JoergAtGithub JoergAtGithub force-pushed the establishqualitywithpitch branch from 5075663 to 3139d4b Compare November 26, 2021 22:37
@JoergAtGithub JoergAtGithub force-pushed the establishqualitywithpitch branch from 3139d4b to 49da32a Compare November 26, 2021 22:43
@Holzhaus Holzhaus requested a review from daschuer December 15, 2021 08:22
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

lgtm

@ywwg ywwg merged commit 983df1a into mixxxdj:main Dec 15, 2021
@JoergAtGithub
Copy link
Member Author

Thanks for the review!

@JoergAtGithub JoergAtGithub deleted the establishqualitywithpitch branch December 15, 2021 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants