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

Soundtouch offset compensation updated #11154

Merged
merged 8 commits into from Aug 9, 2023
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Dec 23, 2022

Replace the outdated offset compensation for soundtouch 1.8 by a comensation for version 2.1.1 to 2.3

Consider the latency before adopting new rate or pitch when calculating the playposition.

@robbert-vdh
Copy link
Contributor

I haven't checked, but does soundtouch not report this information? Just like we can query librubberband for how much padding it requires and what the effective latency is for the current time ratio setting.

@daschuer
Copy link
Member Author

For my undestanding the situation in 2.1.1 was a shortcoming, improved in 2.3.0. I am not aware of a reporting measure.

Unlike in rubberband, this is an offset on the input, not a padding against a ramp up. The ramp is still there. It is even worse, because the frst processed chunk has a different offset.

@daschuer
Copy link
Member Author

daschuer commented May 6, 2023

@robbert-vdh do you have interest to review and check this? (This PR is collecting dusk :-/)

@robbert-vdh
Copy link
Contributor

robbert-vdh commented May 6, 2023

Before I do the same thing and reach the same (lack of) conclusion, did you already check the SoundTouch documentation for a way to get this offset programmatically? Surely there must be a way to get this information without having to measure it. If measuring really is the only way to go, I'd propose implementing a small routine that on startup/first load just runs an impulse through the SoundTouch time stretcher and measures the delay based on that. Then there's no need to hardcode values into Mixxx that may depend on the exact SoundTouch version.

@daschuer daschuer added this to the 2.3.6 milestone May 7, 2023
@daschuer
Copy link
Member Author

There is no way to get timing information out of SoundTouch:
https://codeberg.org/soundtouch/soundtouch/src/branch/master/include/SoundTouch.h

The issue is that the offset was actually bug it SoundTouch < 2.3.0 which has now been fixed.
This PR removes the workaround for recent version of SoundTouch and adjusted the experimental defined offset for older versions. I don't expect that it will break in future again.

@daschuer
Copy link
Member Author

@robbert-vdh can you have a second look here?

Comment on lines 23 to 24
// From V 2.3.0 Soundtouch has no initial offset at unity, but it is to early with
// lowered pitch and to early with raised pitch ~+-2000 farmes = 50 ms.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the comment. also typos

Suggested change
// From V 2.3.0 Soundtouch has no initial offset at unity, but it is to early with
// lowered pitch and to early with raised pitch ~+-2000 farmes = 50 ms.
// From V 2.3.0 Soundtouch has no initial offset at unity, but it is too early with
// lowered pitch and too early with raised pitch ~+-2000 frames = 50 ms.

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this, does the 2000ms just mean latency? how are you estimating time from that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added the description of my test.
Does it help?

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the comment, but it didn't really help. I have a couple questions:

  1. what exactly do you mean with offset in this comment? Latency expressed as a number of frames/samples?
  2. how can that offset be earlier (lowered/raised pitch) than no offset (at unity (which I assume means no change in pitch)).

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 In my test with extrem pitches, the saw tooth was shifted by up to 2000 frames compared to the unity signal. This is relative to the pitch, but I have not investigated how exactly.

2 It can be earlier, because of the settling samples. Soundtouch needs some samples to be settled before you can consume the output even at unity. Mixxx controls the output and recent Soundtouch compensates the input correct only at unity.

src/engine/bufferscalers/enginebufferscalest.cpp Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

Holzhaus commented Aug 8, 2023

Do we need backwards compatibility for old SoundTouch versions? We could try to detect the version with cmake and define some preprocessor variable. But I have no idea if anyone still uses older versions of SoundTouch.

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
@daschuer
Copy link
Member Author

daschuer commented Aug 9, 2023

Our lowest supported version is 2.1.2 form Ubuntu Focal. The runtime check is required, because one may update the SoundTouch version without recompiling Mixxx.

@Holzhaus
Copy link
Member

Holzhaus commented Aug 9, 2023

Ok.

@daschuer
Copy link
Member Author

daschuer commented Aug 9, 2023

OK, done after resolving a distance conflict.

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
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.

LGTM

@Swiftb0y Swiftb0y merged commit 5f86124 into mixxxdj:2.3 Aug 9, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants