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

[WIP] Endless audio clips #333

Closed
wants to merge 4 commits into from
Closed

[WIP] Endless audio clips #333

wants to merge 4 commits into from

Conversation

frabert
Copy link

@frabert frabert commented May 19, 2019

I've started working on a fix for #331

This is a work in progress, I'm creating the PR to show progress and to receive/provide feedback on the solutions implemented.

Currently, only the OpenAL backed is implemented, and it has some issues. Mainly, the playback does not seems "fluid", I'm guessing the worker thread is not queueing the audio buffers fast enough, and OpenAL automatically stops the source, so that it has to be restarted manually every time a new buffer is enqueued.

@BearishSun
Copy link
Member

Hey. At first glance looks pretty good! Not sure about the buffering issue, it's been a while since I've been in that codebase but if you have trouble figuring it out let me know. I'll leave some comments on the other things I noticed.

* @param[in] desc Descriptor describing the type of the audio stream (format, sample rate, etc.).
* @return Newly created AudioClip. Must be manually initialized.
*/
virtual SPtr<AudioClip> createClip(const SPtr<DataStream>& samples, const AUDIO_CLIP_DESC& desc) = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Documentation might note here it's for creating an endless audio clip. Alternatively we could maybe use the constructor above but assume endless if streamSize and numSamples is 0. Not sure if that's a better option though, probably doesn't matter much.

UINT32 totalNumSamples = mAudioClip->getNumSamples();
UINT32 totalNumSamples;

if(mAudioClip->isEndless())
Copy link
Member

Choose a reason for hiding this comment

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

Code style: Omit {} for single line blocks

@@ -568,8 +582,10 @@ namespace bs

if (fillBuffer(mStreamBuffers[i], info, totalNumSamples))
{
for (auto& source : mSourceIDs)
for (auto& source : mSourceIDs) {
Copy link
Member

Choose a reason for hiding this comment

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

Code style: { should be on a new line

alSourceQueueBuffers(source, 1, &mStreamBuffers[i]);
alSourcePlay(source);
Copy link
Member

Choose a reason for hiding this comment

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

This is the issue you mentioned, that we're forcing OpenAL to start playing again?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, ideally the source should not stop playing. In this case, I have to force the source to keep playing, but I am not yet sure why. As I mentioned before, I suspect it is due to the source running out of buffer contents too early because the audio thread doesn't keep up, but I don't know how to test this theory as of now...

@@ -597,18 +623,20 @@ namespace bs
UINT32 sampleBufferSize = numSamples * (info.bitDepth / 8);

UINT8* samples = (UINT8*)bs_stack_alloc(sampleBufferSize);
std::fill(samples, samples + sampleBufferSize, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we can avoid doing a fill over the entire buffer. It wastes performance as in 99% of the cases it will just get overwritten.

@frabert
Copy link
Author

frabert commented May 19, 2019

I have addressed the stylistic issues

@frabert
Copy link
Author

frabert commented May 22, 2019

I've hit a roadblock in that I could not find an easy way to implement this feature using the FMOD backend, but this is probably mainly due to my inexperience with its API. @BearishSun Do you have any pointers on this?

@BearishSun
Copy link
Member

Current FMOD streaming setup is done in FMODAudioClip::createStreamingSound. It sets up an FMOD_CREATESOUNDEXINFO struct with an pcmReadCallback which gets called by FMOD when it needs new samples - there might be some modifications needed there, not sure. It also has a length parameter which probably needs to be set to 0. Potentially some other params there need changing as well, again not sure.

@frabert
Copy link
Author

frabert commented May 24, 2019

Thanks for the tips! Seems like the FMOD backend mostly works now, but the length field cannot be 0 (it will make createSound fail otherwise), so for now I've just set it to the biggest possible value. This leads me to the question of how to signal that a stream is actually ended: I still don't know, since returning an EOF error when running out of data doesn't seem to make playback stop, just begins a "looping" phase

@BearishSun
Copy link
Member

Looking at this thread: https://qa.fmod.com/t/notifying-user-created-stream-of-end/11417/2

Seems like there is no built-in way to stop an infinite stream. What you could probably do, is to set an atomic in the stream read callback when it ends. The main thread (AudioManager?) can then check the atomic if the stream ended, and call on FMOD to stop playback. Since there is some delay between the two, you can probably fill the stream with zeroes until the main thread actually stops it.

@BearishSun
Copy link
Member

(Although that thread is also quite old. It might be worth checking FMOD API reference for something newer)

@BearishSun
Copy link
Member

FMOD also supports playing DSP's, which can generate PCM samples and don't have the limitation of having to specify the length. api/lowlevel/examples/generate_tone.cpp example provided in FMOD SDK is a good example.

I'm not sure if that's much better than the sound approach though.

@frabert frabert closed this by deleting the head repository Feb 24, 2024
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.

2 participants