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

VSTEffect samplePos should be a value in number of samples, not in seconds #1628

Closed
nd-bl opened this issue Sep 6, 2021 · 6 comments · Fixed by #1636
Closed

VSTEffect samplePos should be a value in number of samples, not in seconds #1628

nd-bl opened this issue Sep 6, 2021 · 6 comments · Fixed by #1636
Labels
bug An error, undesired behaviour, or missed functionality P4 Low priority

Comments

@nd-bl
Copy link

nd-bl commented Sep 6, 2021

Describe the bug
VSTEffect mTimeInfo.samplePos should be a value in number of samples, not in seconds

In the file "aeffectx.h" from the Steinberg vst 2.4 SDK, it is written:


struct VstTimeInfo
{
	double samplePos;				///< current Position in audio samples (always valid)

In the file VSTEffect.cpp, the value is computed in seconds:


mTimeInfo.samplePos += ((double) blockLen / mTimeInfo.sampleRate);

It should be:


mTimeInfo.samplePos += blockLen;

To Reproduce
Steps to reproduce the behavior:

  1. Inside a VST plugin, get the time info with a callback using "audioMasterGetTime"
  2. Get the samplePos from the time info

Expected behavior
The samplePos should be in samples, not in seconds

Additional information (please complete the following information):

  • OS: Ubuntu 20.10
  • Version Audacity 3.0.4
@LWinterberg LWinterberg added bug An error, undesired behaviour, or missed functionality P4 Low priority labels Sep 7, 2021
@LWinterberg
Copy link
Member

This line has been there since forever (since we no longer use svn anyway), and we're not aware of VST2 problems arising from this, so I'm marking this P4.

(If you want to make a PR that fixes this, feel free to!)

@nd-bl
Copy link
Author

nd-bl commented Sep 7, 2021

Pull request done! (#1636)

Some additional information: the VST plugin used for testing is a kind of scrolling spectrogram viewer, with a scrolling time axis at the bottom. It has been tested on a certain number of DAWs, getting the sample pos from the DAW in order to synchronize the time axis. For all these other tests, the sample pos unit is in number of samples.

@LWinterberg LWinterberg linked a pull request Sep 8, 2021 that will close this issue
6 tasks
@LWinterberg
Copy link
Member

Cool, cool! I'll try to get someone who actually knows C++ to review this.

While you're at it, are you aware of other bugs/spec violations of our VST2 implementation that we maybe haven't discovered yet? I'm asking because we're planning to add proper real-time effects support for 3.1 (see also #992)

@Paul-Licameli
Copy link
Collaborator

No! 3.2 is the target for that.

@nd-bl
Copy link
Author

nd-bl commented Sep 8, 2021

Thank you :)

No, no particular problem detected after having tested with more than 20 VST plugins (with some of them having GUI resize features). I find that your VST support is great! And your plan to make real-time stackable effects looks awesome!

@Paul-Licameli
Copy link
Collaborator

Reviewed, approved, merged.

It was just one line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error, undesired behaviour, or missed functionality P4 Low priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants