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

fix a lot of issues related to no sound on mac #3492

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

anatoly-os
Copy link
Contributor

If we don't specify framesPerBuffer parameter in Pa_OpenStream, PortAudio will choose (randomly?) the value. In Voice::write, we assume that RestN which initially keeps the value of frames from pa_callback will be bigger than env_data->count. The last is 43 on my Mac (47 on my Ubuntu machine), but default frames count from pa callback on failed MacBook 12" is 39. So, we cannot correctly process all frames and envelop and produce sound, specifically amplitude calculation fails and Voice::write returns under this if statement 'if (volenv_section == FLUID_VOICE_ENVDELAY)'.

So, I hardcoded 630 since it is the value from my Mac. Also I've got 1027 for Ubuntu.

@anatoly-os
Copy link
Contributor Author

Let's open a discussion. I've fixed the bug, sound appeared on MacBook 12".

  • But I'm not sure neither about hardcoded value nor that we should modify framesPerBuffer, but leave envelope calculations.

  • I don't know why port audio on that MacBook processes only about 30 frames, not hundreds.

  • I'm also not aware about performance issues which we can face with after specify frames directly.

@Jojo-Schmitz
Copy link
Contributor

If only Mac is affected by that problem, why making that change for all OSs?
Esp. as just a few lines above we have a Mac specific change already?

@anatoly-os
Copy link
Contributor Author

anatoly-os commented Feb 24, 2018

In general, I agree. We know about Mac issues, also we know about Bluetooth issues, which may be related. But, if we don't know, it doesn't mean that no issues exist. Since we have specific limit in our code, we cannot expect any value here. So, we have to control it somehow. I would prefer to set minimal value, but there is no option.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 24, 2018

AFAIK the bluetooth issues are Mac only too?
And from portaudio.h.:
@param framesPerBuffer The number of frames passed to the stream callback
function, or the preferred block granularity for a blocking read/write stream.
The special value paFramesPerBufferUnspecified (0) may be used to request that
the stream callback will receive an optimal (and possibly varying) number of
frames based on host requirements and the requested latency settings.
Note: With some host APIs, the use of non-zero framesPerBuffer for a callback
stream may introduce an additional layer of buffering which could introduce
additional latency. PortAudio guarantees that the additional latency
will be kept to the theoretical minimum however, it is strongly recommended
that a non-zero framesPerBuffer value only be used when your algorithm
requires a fixed number of frames per stream callback.

mscore/pa.cpp Outdated
@@ -125,11 +125,15 @@ bool Portaudio::init(bool)
#endif
out.hostApiSpecificStreamInfo = 0;

err = Pa_OpenStream(&stream, 0, &out, double(_sampleRate), 0, 0, paCallback, (void*)this);
int defaultFramesToProcessFromCallback = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

unsigned long rather than int

@polarbreeze
Copy link

@lasconic
Copy link
Contributor

lasconic commented Feb 25, 2018

Awesome. Very glad we have a solid explanation for the problem.

I don't think it's a good idea to hard code the framesPerBuffer value. It's discouraged by PortAudio. The problem has been introduced in Fluid... it needs to be fixed in Fluid...

I'm also pretty sure that it's related with "Bluetooth not working" I believe bluetooth will typically use smaller framesPerBuffer to cope with latency.

@polarbreeze
Copy link

Is there something to learn from what Audacity does? It is working reliably for me on MacBook 12", for built-in and BT and USB.
Some things I noticed:

  1. Audacity separates the input selector from the output selector. Good idea because otherwise it's too easy to select an input device for the output (which results in silence) - in MS the input device has the same name as the output device and they both appear in the device selector menu - so you can't tel which is which. (Maybe the input device should not be presented at all?)
  2. In Audacity it's possible to switch back and forth between different output devices without restarting the program, which is easier and less error prone ("did I remember to restart the program?")
  3. Portaudio version 19.5.0-devel.

screen shot 2018-02-25 at 06 08 03

@polarbreeze
Copy link

I'm wondering... if this is actually consistent with the symptoms I am seeing on my MacBook 12":

  1. The dropdown are not blank - they are properly populated
  2. When "built-in" audio is selected:
    a) With both 2.0.3 and 2.1: this console coreaudio error message is repeated every few seconds, even if no score is playing:
    "HALS_IOA1Engine.cpp:365:EndWriting: HALS_IOA1Engine::EndWriting: got an error from the kernel trap, Error: 0xE00002EE"
    b) With 2.1, there is no audio from the score or from note entry; however, the metronome can be heard normally.
    c) With 2.0.3, the audio works properly.
  3. When "Bluetooth, "HDMI" or "USB" audio is selected:
    a) There is no error message of that sort either with 2.0.3 or with 2.1
    b) The audio works properly with both 2.0.3 and 2.1

@anatoly-os
Copy link
Contributor Author

Don't afraid huge changes in voice.cpp - it is just extracting huge write logic to separate methods. What I've done actually is generating envelope for minimum 100 frames. If framesBuf from pa is less, we generate envelope for 100 anyway but send only framesBuf number of frames. It results in weak attack and ending sound of notes. I'm trying to use cache data, but still no success.

To emulate no sound case as on MacBook 12 or bluetooth, set fifth argument of Pa_OpenStream in pa.cpp to desired small buff size (39 - for MacBook, 14 - for bluetooth from experiments).

@lasconic lasconic added the work in progress not finished work or not addressed review label Feb 27, 2018
@polarbreeze
Copy link

polarbreeze commented Feb 27, 2018 via email

@anatoly-os
Copy link
Contributor Author

Finished implementation of the cache. Reverted all changes in pa.cpp to keep optimal buffer size. So, for now, I cannot imagine a case when there is no sound because of buffer size. This PR is supposed to fix MacBook 12" no sound issues, bluetooth issues. According to my investigation, slow computers also could be affected.

I tried to describe all the changes in commit log, but logic in Voice and dip generation is too complex. BTW, I enriched my knowledge :)

This PR should be thoroughly tested in terms of sound quality. There should be no glitches, no rattling, no noise. Everything we find, I will fix...

I'm happy now. Long weekends are waiting for me :)

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Mar 1, 2018

Would be great to also fix the display of input devices in the output list, as selecting those would cause no sound also. Furthermore fix the truncation of the entries in that list.

@lasconic
Copy link
Contributor

lasconic commented Mar 1, 2018

fix the display of input devices in the output list, as selecting those would cause no sound also

This is a totally different bug. So please, not in this PR.

@anatoly-os
Copy link
Contributor Author

@lasconic for sure. I was not even going to :)

@polarbreeze
Copy link

polarbreeze commented Mar 1, 2018 via email

@anatoly-os
Copy link
Contributor Author

Well, there is no build with fix I suppose until this PR will be merged to master... Are you a developer? Are you building MuseScore sources?

@polarbreeze
Copy link

polarbreeze commented Mar 1, 2018 via email

@anatoly-os
Copy link
Contributor Author

But since Travis and AppVeyor work on this build, I believe they should create build with this PR. @lasconic, am I right?

@Jojo-Schmitz
Copy link
Contributor

@lasconic
Copy link
Contributor

lasconic commented Mar 2, 2018

Only Appveyor creates downloadable builds from PR. So only for windows.

@lasconic
Copy link
Contributor

lasconic commented Mar 2, 2018

The good news, it sounds ok with internal speakers and I have sound via bluetooth (after a MuseScore restart but that's expected)

The not so good news, Bluetooth playback is not as good than the internal speakers, I hear some glitches.

@anatoly-os
Copy link
Contributor Author

@lasconic did you use MacOS debug build for testing using code from this PR or downloaded AppVeyor's package and ran MuseScore on virtual machine?

I've checked sound on both Windows debug build and downloaded AppVeyor's build with my JBL Charger 3 Bluetooth speaker. Sound is good, without noise or glitches with the default (1024 even with Bluetooth) number of frames and the hardcoded 14 frames as you mentioned you had for bluetooth headphones, Sound is still great even after switching devices on the fly.

Maybe, it is about computers with less memory or CPU resources...

@lasconic
Copy link
Contributor

lasconic commented Mar 5, 2018

I tested on MacOS debug. Tried now with release. Same problem : glitches with bluetooth headphones.
Internal speakers or wired headphones sound ok.

@lasconic
Copy link
Contributor

lasconic commented Mar 5, 2018

With my QC35 headphones, n = 14 in Voice::write()

@polarbreeze
Copy link

polarbreeze commented Mar 5, 2018 via email

@anatoly-os
Copy link
Contributor Author

@lasconic, I've reproduced the case on MacOS. My CPU utilisation directly correlates with number of glitches. While playing tremolos for Viola, it glitches hardly and CPU usage is about 100%...

@anatoly-os
Copy link
Contributor Author

@polarbreeze, we are discussing not merged changes to master and 2.2. After I finish with it, you can test it using nightly build.

Speaking about switching devices, the issue exists, indeed. But it is another issue, so I'll switch there later.

@polarbreeze
Copy link

polarbreeze commented Mar 6, 2018 via email

@anatoly-os
Copy link
Contributor Author

@lasconic I've added comments and fixed bluetooth devices issue.

There is last issue with small noise on stopping playback, but it is not critical. I'll find way to fix it.

@lasconic
Copy link
Contributor

lasconic commented Mar 6, 2018

Awesome. Let me test.

anatoly-os added a commit to anatoly-os/MuseScore that referenced this pull request Mar 7, 2018
Hardcoded frames buffer size is seemed to solve the issues, but it may lead to performance issues. Correct soultion is in progress on master (see musescore#3492).
… framesBuff from PortAudio is too small

If we don't specify framesPerBuffer parameter in Pa_OpenStream, PortAudio will choose optimal value for particular callback call. It can vary from run to run even on the same hardware depending on available system resources.

While generating signal, interpolating and applying effects, we assume that framesBuffer contains more than minimal number of frames to generate envelope. BTW, it is not true. If framesBuffer is smaller, algos cannot generate correct sound and just keep silence.

I've implemented cache which keeps generated values from dsp algorithms and applies it step-by-step to buffer values from pa_callback. Cache is filled each time algos generate dsp values. If buffer frames are not enough to generate envelope, algos generate values for further calls and keep it in cache.

Required number of frames has been selected as a number of frames for one phase multiplying by number of phases. Actually, smaller numbers of this value generates good results, but it is better to keep it as max as possible to provide perfect sound.

Code changes:
 - Replaced C-like variables with std containers for comfortable debugging and better usage
 - Extracted similar code calls to separate methods
 - Implemented cache as std constainers, so also implemented convertion from std::vector to C-like float* to fill the pa buffer
 - Changed the logic of applying effects and interpolation, it is now possible to use them separately. This is required to fill effects several times after calculating the interpolation is finished.

Removed std::vector<float> to keep cache - process buff values on the fly. Performance is better, but still glitches on https://musescore.com/user/166080/scores/175421. BuffSize = 64 with my wired headphones.
@lasconic
Copy link
Contributor

lasconic commented Mar 9, 2018

met's align master with 2.2 by merging this one on master too. See #3518

@lasconic lasconic merged commit ee37c81 into musescore:master Mar 9, 2018
@anatoly-os anatoly-os removed the work in progress not finished work or not addressed review label Jul 17, 2018
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.

None yet

4 participants