IOStream properties and thread safety

[levs42]

Audio mixer invalidates the output format after updating the settings from the main thread. At the same time, it still performs conversion that is called from append method and uses mixer.audioIO.lockQueue. This leads to a race condition for tracks. Should IOStream.audioMixerSettings get/set and other public audio properties be wrapped by a queue too?

This is similar to #1496 where IOVideoUnit.captures property is getting modified from main thread and IOStream.lockQueue. Would it makes sense to remove IOStream.lockQueue and only use mixer.videoIO.lockQueue and mixer.audioIO.lockQueue?

Thanks in advance.

[shogo4405]

I couldn’t determine the specific issue, so I’m not sure whether or not to use a queue.

Does a data race mean that the program crashes? Or does it mean that, due to the order of execution, the values might not be applied to the subsequent processing?

Regarding issue #1496, lockQueue is used for making changes to the AVCaptureSession system. Changing AVCaptureSession values is a time-consuming process. The videoIO.lockQueue is used for capturing CMSampleBuffer from the device and eliminating data conflicts. Therefore, as they serve different purposes, I don't think removing the lockQueue is appropriate.

[levs42]

Yes the program crashes when user tries to change sample rate or number of channels before or after going live. Let's say user changes the settings from some UI menu. Here's a code that would be called when selecting new settings:

final class IngestViewController: UIViewController {
  func updateSampleRateFromUI(_ sampleRate: Float64) {
    stream.audioMixerSettings = stream.audioMixerSettings.copy(sampleRate: sampleRate)
  }
  
  func updateSampleRateFromUI_simulateCrash() {
    for _ in 0..<10000 {
      let sampleRate: Float64 = Int(stream.audioMixerSettings.sampleRate) == 44100 ? 48000 : 44100
      stream.audioMixerSettings = stream.audioMixerSettings.copy(sampleRate: sampleRate)
    }
  }
}

private extension IOAudioMixerSettings {
  /// Helper function to build new settings based on the current values
  func copy(sampleRate: Float64? = nil, channels: UInt32? = nil) -> IOAudioMixerSettings {
    var settings = IOAudioMixerSettings(sampleRate: sampleRate ?? self.sampleRate,
      channels: channels ?? self.channels)
    settings.mainTrack = mainTrack
    settings.tracks = tracks
    settings.isMuted = isMuted
    settings.maximumNumberOfChannels = maximumNumberOfChannels
    return settings
  }
}

This leads to all kind of random crashes for the properties that are getting accessed from the both threads:

AudioNode: 240

buffer.frameLength = numberOfFrames
Object 0x3008f8c00 of class OutputNode deallocated with non-zero retain count 2. This object's deinit, or something called from it, may have created a strong reference to self which outlived deinit, resulting in a dangling reference.

IOAudioMixerByMultiTrack: 181

buffers[track.id]?.append(audioPCMBuffer, when: when)
Exception NSException * "-[__NSTaggedDate objectForKey:]: unrecognized selector sent to instance 0x8000000000000000" 0x0000000302723de0

Proposed solution would be to make all audio mixer properties be modified inside of mixer.audioIO.lockQueue:

public var audioMixerSettings: IOAudioMixerSettings {
  get {
    mixer.audioIO.lockQueue.sync { self.mixer.audioIO.mixerSettings }
  }
  set {
    mixer.audioIO.lockQueue.async {
      self.mixer.audioIO.mixerSettings = newValue
    }
  }
}

[shogo4405]

Indeed. I assumed that IOAudioMixerSettings is thread-safe because it is a struct. However, buffers[] and others are not thread-safe. I realized this is necessary.

[shogo4405]

[FYI] #1510

[levs42]

Similar to the audio properties, here is the code that simulates video orientation change crash:

func configureCaptureDevice_simulateCrash() {
  for _ in 0..<10000 {
    let positions: [AVCaptureDevice.Position] = [.back, .front]
    stream.attachCamera(AVCaptureDevice.default(.builtInWideAngleCamera, for: .video, position: positions.randomElement()!))
    let orientations: [AVCaptureVideoOrientation] = [.portrait, .landscapeLeft]
    stream.videoOrientation = orientations.randomElement()!
  }
}

IOVideoCaptureUnit.swift:56
> $0.videoOrientation = videoOrientation
EXC_BAD_ACCESS:

Thread 1 Queue : com.apple.main-thread (serial)
#0	objc_msgSend ()
#1	-[NSDictionary isEqualToDictionary:] ()
#2	-[AVCaptureVideoDataOutput updateVideoSettingsForConnection:] ()
#3	-[AVCaptureConnection _setVideoRotationAngle:forceUpdate:] ()
#4	-[AVCaptureConnection setVideoOrientation:] ()
#5	-[NSObject(NSKeyValueObservingPrivate) _changeValueForKeys:count:maybeOldValuesDict:maybeNewValuesDict:usingBlock:] ()
#6	-[NSObject(NSKeyValueObservingPrivate) _changeValueForKey:key:key:usingBlock:] ()
#7	_NSSetLongLongValueAndNotify ()
#8	closure #2 in IOVideoCaptureUnit.videoOrientation.didset at HaishinKit/Sources/IO/IOVideoCaptureUnit.swift:57
#9	partial apply for closure #2 in IOVideoCaptureUnit.videoOrientation.didset ()
#10	Sequence.forEach(_:) ()
#11	IOVideoCaptureUnit.videoOrientation.didset at HaishinKit/Sources/IO/IOVideoCaptureUnit.swift:56
#12	IOVideoCaptureUnit.videoOrientation.setter ()
#13	closure #1 in IOVideoUnit.videoOrientation.didset at HaishinKit/Sources/IO/IOVideoUnit.swift:111
#14	partial apply for closure #1 in IOVideoUnit.videoOrientation.didset ()
#15	IOCaptureSession.configuration(_:) at HaishinKit/Sources/IO/IOCaptureSession.swift:127
#16	IOVideoUnit.videoOrientation.didset at HaishinKit/Sources/IO/IOVideoUnit.swift:108
#17	IOVideoUnit.videoOrientation.setter ()
#18	IOStream.videoOrientation.setter at HaishinKit/Sources/IO/IOStream.swift:174
#19	SampleViewController.configureCaptureDevice_simulateCrash() at SampleViewController.swift:102

output in IOVideoCaptureUnit is modified from both threads. Wrapping accessors with lockQueue could be a solution:

public var videoOrientation: AVCaptureVideoOrientation {
  get {
    return lockQueue.sync { self.mixer.videoIO.videoOrientation }
  }
  set {
    lockQueue.async {
      self.mixer.videoIO.videoOrientation = newValue
    }
  }
}

[shogo4405]

I remember implementing it this way because there were some side effects and I couldn't completely eliminate the possibility of crashes. If the frequency is low, I would prefer to leave it as it is. Currently, I am replacing DispatchQueue with Swift Concurrency, so I am not motivated to make changes considering the adjustments needed after the replacement.

[levs42]

Sounds good. lockQueue is also public so it can be used when setting any IOStream property.