Page MenuHome

Fix T86851: PulseAudio randomly asserts in background rendering
ClosedPublic

Authored by Joerg Mueller (nexyon) on Mar 27 2021, 12:27 PM.

Diff Detail

Repository
rB Blender
Branch
T86851_fix (branched from master)
Build Status
Buildable 13757
Build 13757: arc lint + arc unit

Event Timeline

Joerg Mueller (nexyon) requested review of this revision.Mar 27 2021, 12:27 PM
Joerg Mueller (nexyon) created this revision.
Brecht Van Lommel (brecht) requested changes to this revision.Mar 28 2021, 4:54 PM
Brecht Van Lommel (brecht) added inline comments.
extern/audaspace/include/devices/ThreadedDevice.h
75–77

You might want to document that these functions are only allowed to be called from within runMixingThread, otherwise it's not obvious this is correct.

76

I think that technically this should have a mutex lock, to ensure both values are set atomically.

extern/audaspace/src/devices/ThreadedDevice.cpp
45–48

I think this will fail:

  • runMixingThread calls shouldStop() which returns true
  • Main thread calls playing(true)
  • runMixingThread calls doStop()

I think then the thread will stop without being restarted, even though it should be playing.

I would expect shouldStop() and doStop() to be a single atomic function call somehow, and playing() to also use a mutex lock?

This revision now requires changes to proceed.Mar 28 2021, 4:54 PM

I can add some documentation that the functions are only to be called when the device is locked.

What you're missing here is that the device is in fact always locked whenever playing, doStop, and shouldStop are called.

That's actually the reason why I had to rewrite this in the first place, because the lock that is used by the threaded mainloop of Pulse is deadlocking with the lock of the SoftwareDevice.

Ok, that makes sense. It's your code so up to you how you want to document it, but I think it would help to document the implicit assumptions in ThreadedDevice.

This revision is now accepted and ready to land.Mar 28 2021, 10:09 PM

Playback works, but I do have one question about the code.

extern/audaspace/plugins/pulseaudio/PulseAudioDevice.cpp
86–87

I don't know anything about "corking" in this context, so I just look at it from the perspective of my old physics teacher telling the yappy student to go to the chemistry lab, get the biggest cork he could find, bring it back, and put it in his mouth so he'd finally shut up (true story this, it was hilarious). Is it correct that the stream should be corked if it already has a cork in it? Or does passing 0, nullptr, nullptr, contrary to the function name, actuall un-cork it?

Joerg Mueller (nexyon) marked 4 inline comments as done.Mar 29 2021, 11:27 AM
Joerg Mueller (nexyon) added inline comments.
extern/audaspace/plugins/pulseaudio/PulseAudioDevice.cpp
86–87

Yes, the 0 is basically the (C) boolean that states whether it should be corked (1) or uncorked (0).

Joerg Mueller (nexyon) marked an inline comment as done.

Documentation added.