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

Bring back Portaudio (WASAPI) #4465

Closed
jibin1573 opened this issue Jul 5, 2018 · 13 comments · Fixed by #4770
Closed

Bring back Portaudio (WASAPI) #4465

jibin1573 opened this issue Jul 5, 2018 · 13 comments · Fixed by #4770
Assignees
Milestone

Comments

@jibin1573
Copy link

jibin1573 commented Jul 5, 2018

I was using LMMS 1.2.0 RC4 on Windows 10 Home, version:1709, OS build: 16299.402.
I used a USB MIDI keyboard. And my buffer size was 192 frames and latency was 4ms. I tested both SDL and Port Audio (WASAPI) at 140bpm with Piano One vsti, and found that WASAPI was better than SDL, in terms of latency.
I also found that, while exporting the songs at a sample rate higher than 44100hz, some tracks would get detuned (or their volume would decrease), while using SDL. But, no such problems were found in Port audio (WASAPI), while exporting at sample rates higher than 44100hz.
Then later I tried the SDL on 1.2.0 RC6, (on Windows 10 version 1804) and it's SDL has the same problems.
Please consider bringing back Port Audio option. I think WASAPI is getting better.

Thank you.

@tresf
Copy link
Member

tresf commented Sep 29, 2018

Please consider bringing back Port Audio option. I think WASAPI is getting better.

We can bring it back, but there were so many problems with it that I'm surprised it actually worked. Did you have a hacked DLL or using it "vanilla" as bundled?

Related #1173, #4315, #4405, #3889.

Marking for 1.2.0, @Umcaruje I'm putting you on this one because you turned it off. 🤣

@tresf tresf added the response required A response from OP is required or the issue is closed automatically within 14 days. label Sep 29, 2018
@tresf tresf added this to the 1.2.0 milestone Sep 29, 2018
@tresf tresf changed the title Portaudio (WASAPI) Bring back Portaudio (WASAPI) Sep 29, 2018
@DomClark
Copy link
Member

The detuning bugs were fixed in de427bb. @jibin1573 can you test RC7 to make sure this is the case?
I'm happy to write a WASAPI audio backend for master if that would be desirable. Also I believe SDL2, which is used in master, now supports WASAPI.

@jibin1573
Copy link
Author

jibin1573 commented Sep 30, 2018

First of all Kudos to @DomClark for his awesome work! Thanks a lot!

The following VST instruments work fine at sample rates higher than 44000hz:

  1. T-Force Alpha Plus
  2. Bitsonic TB303
  3. DSK Overture

PROBLEMS:

  1. Helm: Normal Helm patches work fine. But, Helm Arpeggiator patches (Eg: Arp1) have some kind of problems (like broken notes, no detuning), while exporting at higher sample rates. These Arpeggiator patches did not create any problem while exporting at 44000hz sample rate.
    Here are the audio files with both, normal and problematic Helm arpeggiator patches:
    Normal and Broken Arp notes with Helm.zip

  2. Piano One: Piano One still gets detuned at sample rates higher than 44000hz

[I'll keep reporting if I find any problems }

@no-response no-response bot removed the response required A response from OP is required or the issue is closed automatically within 14 days. label Sep 30, 2018
@DomClark
Copy link
Member

Helm: Normal Helm patches work fine. But, Helm Arpeggiator patches (Eg: Arp1) have some kind of problems (like broken notes, no detuning), while exporting at higher sample rates. These Arpeggiator patches did not create any problem while exporting at 44000hz sample rate.

I can reproduce the broken notes, but they also occur with Traktion and Reaper as hosts so I think this is a Helm bug. You can report it here: https://github.com/mtytel/helm

Piano One: Piano One still gets detuned at sample rates higher than 44000hz

I can't reproduce this, but we can discuss this more in #4479 so we don't derail this issue. Thank you for testing this.

@jibin1573
Copy link
Author

Oh okay! Sorry, I didn't know that it was Helm's problem.

@zonkmachine
Copy link
Member

IIRC one of the portaudio issues was actually with fluidsynth and it's been fixed.
FluidSynth/fluidsynth#218

Can we just reinstate portaudio as it was and warn the user if they select it?

@DomClark
Copy link
Member

DomClark commented Dec 8, 2018

If I'm reading that issue correctly, it looks like the bug wasn't with FluidSynth, rather they were suffering from the same issue as us. They suggest the PortAudio crashes are down to buggy soundcard drivers, and their fix was not to initialise PortAudio unless it was requested. Currently, if LMMS has PortAudio support compiled in, we initialise it whenever the setup dialog is opened in order to fill out the PortAudio setup widget, so a warning upon selection would be insufficient.

Some possible approaches:

  • Re-enable PortAudio, change things so that it is only initialised when chosen in the combo box, and put a warning on the top of the audio settings page telling the user LMMS may crash if they click it.

    Pros: relatively minimal changes required, existing PortAudio users can continue to use it, low-latency audio support.
    Cons: doesn't actually fix the PortAudio crashes, just puts them behind a "crash me" button. All Windows users see a warning message which may well not be relevant to them.

  • Implement WASAPI support for stable-1.2, stretching the definition of "bug-fix" somewhat.

    Pros: no need to rely on PortAudio any more, shouldn't crash since audio drivers are likely to work properly with native Windows APIs. Low-latency audio support.
    Cons: new, previously untested code in stable branch close to release.

  • Don't do anything, bump milestone to 1.3.0.

    Pros: no crashes, easy solution, nothing needs changing.
    Cons: no low-latency audio on Windows.

Ideas/thoughts?

@tresf
Copy link
Member

tresf commented Dec 8, 2018

WASAPI is Vista and higher. stable-1.2 still carries unofficial XP suport. WASAPI is perhaps a better change on master per: #2576

@zonkmachine
Copy link
Member

zonkmachine commented Dec 15, 2018

  • Re-enable PortAudio...

I think this but bumping it to 1.3 is OK with me.

@musikBear
Copy link

ASIO4ALL support was possible in PortAudio, but afaik only with libportaudio-2.dll, from JACK. Does anyone know a way to support ASIO4ALL?

@DomClark
Copy link
Member

DomClark commented Jan 6, 2019

Moving the calls to PortAudio out of AudioPortAudio::setupWidget's constructor and into its showEvent looks like it should work (tested with a PortAudio DLL built to crash whenever a method from it is called). However I'm not sure what to do about warning the user. Currently I'm considering not showing a warning at all: I have no data, but I would think that most users don't have this bug, and most users don't fiddle with the audio interface setting. If a user does select PortAudio and causes a crash, then the change won't be saved and the current working audio interface will be used when LMMS next starts. Hopefully the user will learn not to do it again. This would still be an improvement over an unconditional crash at any rate.

@tresf
Copy link
Member

tresf commented Jan 6, 2019

I'm not sure what to do about warning the user. Currently I'm considering not showing a warning at all: I have no data, but I would think that most users don't have this bug, and most users don't fiddle with the audio interface setting.

I agree. My issue with PortAudio was that it simply never worked on any of my systems. I'd be happy to re-test but if we have documented evidence that it works, we can simply turn it back on. The fact that SDL is default is the major advantage here as it historically provides a better experience (higher rate of working out of the box) for the average user.

@DomClark
Copy link
Member

In that case I will go ahead and make the changes.

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 a pull request may close this issue.

6 participants