Fix audio render thread thread safety issues

[ferjm]

…d tick

This is a different approach for #27.

There is one issue here that can be noticeable running the play example. It is supposed to play 2 seconds of two sine waves with different frequencies. With this patch the first sine wave steals most of the time of the second sine wave. I believe it is caused by the difference between the JS clock and the (currently not existent) WebAudio clock (there is a nice blog post about this). If that's the case this should be fixed by using the scheduling feature of AudioScheduledSourceNodes in the play example once #21 is done.

[ferjm]

@Manishearth @sdroege WDYT of this approach?

[Manishearth]

I like this approach. The lack of blocking in the event loop is a bit concerning though.

[Manishearth]

We shouldn't need to clone here. Call .as_ref().unwrap()

[Manishearth]

There's a slight problem here; this loop will eat up CPU instead of blocking because there's nothing to block on.

[ferjm]

Hmm, isn't this (not blocking) what's supposed to happen for the rendering thread? In any case, we could use appsrc's block property here instead of the custom has_enough_data check.

[Manishearth]

Now we have the problem that the event loop won't process events when the buffer is full 😄

Which is ... okay, I guess?

[ferjm]

The other option may be to use the previous has_enough_data and sleep the thread for a few ms if the buffer is full to avoid 100% CPU usage.

[Manishearth]

This can work. I'm not sure which approach is better at this point. I guess avoiding the synchronization overhead is good. Do you/@sdroege have opinions? On May 24, 2018 2:28 PM, "Fernando Jiménez Moreno" <notifications@github.com> wrote: The other option may be to use the previous has_enough_data and sleep the thread for a few ms if the buffer is full to avoid 100% CPU usage. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSKDABxeMvxLm3zAf3QdEIG5bGDq6ks5t1yXsgaJpZM4UMVM2> .

[ferjm]

Until we have performance tests or we can manually test with more nodes or real WebAudio apps, it's hard to make an informed decision here, but I think the has_enough_data + sleep(5ms), while it's not very elegant, it's a good middle ground solution. We have no messaging overhead, the render thread can continue processing control thread messages and the blocking situations are controlled and for a known and configurable amount time.

[Manishearth]

Sounds good. We can continue with that model then 😄

…

On Fri, May 25, 2018, 10:31 AM Fernando Jiménez Moreno < ***@***.***> wrote: Until we have performance tests or we can manually test with more nodes or real WebAudio apps, it's hard to make an informed decision here, but I think the has_enough_data + sleep(5ms), while it's not very elegant, it's a good middle ground solution. We have no messaging overhead, the render thread can continue processing control thread messages and the blocking situations are controlled and for a known and configurable amount time. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSMU6BVa_rvrWXhehTQE96FydoL_aks5t18FugaJpZM4UMVM2> .

[ferjm]

Actually, I want to try yet another approach, but I am not sure if it'll work.

I think we can have an extra thread where we wait for appsrc callbacks. We can share the control <-> render thread channel with this thread. If the appsrc buffer is full, we will block on the control <-> render thread channel reception. When an appsrc.need_data callback is triggered, a message will be sent through this channel to unblock and switch to the previous non-blocking event loop.

[sdroege]

but I think the has_enough_data + sleep(5ms)

I didn't review anything in detail here yet (later!) but a) sleeping 5ms will drift slowly if done just like that as it does not consider the amount of time taken for the actual code, b) you'll sleep according to the system clock and not according to the audio clock, so you either produce too much or too little data over a longer time (the audio clock will run at different speed).

[ferjm]

I just pushed the commit with the approach I mentioned above. We no longer sleep the thread and we switch between a non-blocking and a blocking event loop depending on the audio sink data needs

[ferjm]

r? @Manishearth

[Manishearth]

Ah, yeah, this new approach is closer to #28 in the sense that the app src still can request stuff through the queue, but backpressure is better dealt with. I like it.