| Message ID | 20250117-coreaudio-v4-1-f8d4fa4cb5f4@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | coreaudio fixes | expand |
On Fri, 17 Jan 2025 at 07:48, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > init_out_device may only commit some part of the result and leave the > state inconsistent when it encounters a fatal error or the device gets > unplugged during the operation, which is expressed by > kAudioHardwareBadObjectError or kAudioHardwareBadDeviceError. Commit the > result in the end of the function so that it commits the result iff it > sees no fatal error and the device remains plugged. > > With this change, handle_voice_change can rely on core->outputDeviceID > to know whether the output device is initialized after calling > init_out_device. > Apologies if this is a basic question, but I'm seeing this code for the first time: What if init_out_device() is called from handle_voice_change() because the system audio device changed though, will outputDeviceID be valid on those error conditions? I assume those kAudioHardwareBadObjectError checks are defending against the case where the device disappears after coreaudio_get_voice() returned a valid audio device. So for example, we start out with the audio device being one of two devices exposed by a compound device. The compound device is unplugged, but the system audio sub-device appears to the system to be removed first, so the second sub-device briefly becomes the system audio device. We get the handle_voice_change() call, handle_voice_change() returns the second sub-device, but then that one disappears too before we reach the end of init_out_device(). We error out, but in this case outputDeviceID would remain the same as the original device which has actually disappeared. Or have I misunderstood? > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > audio/coreaudio.m | 49 +++++++++++++++++++++++++++---------------------- > 1 file changed, 27 insertions(+), 22 deletions(-) > > diff --git a/audio/coreaudio.m b/audio/coreaudio.m > index cadd729d5053..b9e1a952ed37 100644 > --- a/audio/coreaudio.m > +++ b/audio/coreaudio.m > @@ -355,7 +355,10 @@ static OSStatus audioDeviceIOProc( > static OSStatus init_out_device(coreaudioVoiceOut *core) > { > OSStatus status; > + AudioDeviceID deviceID; > AudioValueRange frameRange; > + UInt32 audioDevicePropertyBufferFrameSize; > + AudioDeviceIOProcID ioprocid; > > AudioStreamBasicDescription streamBasicDescription = { > .mBitsPerChannel = core->hw.info.bits, > @@ -368,20 +371,19 @@ static OSStatus init_out_device(coreaudioVoiceOut > *core) > .mSampleRate = core->hw.info.freq > }; > > - status = coreaudio_get_voice(&core->outputDeviceID); > + status = coreaudio_get_voice(&deviceID); > if (status != kAudioHardwareNoError) { > coreaudio_playback_logerr (status, > "Could not get default output > Device\n"); > return status; > } > - if (core->outputDeviceID == kAudioDeviceUnknown) { > + if (deviceID == kAudioDeviceUnknown) { > dolog ("Could not initialize playback - Unknown Audiodevice\n"); > return status; > } > > /* get minimum and maximum buffer frame sizes */ > - status = coreaudio_get_framesizerange(core->outputDeviceID, > - &frameRange); > + status = coreaudio_get_framesizerange(deviceID, &frameRange); > if (status == kAudioHardwareBadObjectError) { > return 0; > } > @@ -392,31 +394,31 @@ static OSStatus init_out_device(coreaudioVoiceOut > *core) > } > > if (frameRange.mMinimum > core->frameSizeSetting) { > - core->audioDevicePropertyBufferFrameSize = (UInt32) > frameRange.mMinimum; > + audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum; > dolog ("warning: Upsizing Buffer Frames to %f\n", > frameRange.mMinimum); > } else if (frameRange.mMaximum < core->frameSizeSetting) { > - core->audioDevicePropertyBufferFrameSize = (UInt32) > frameRange.mMaximum; > + audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum; > dolog ("warning: Downsizing Buffer Frames to %f\n", > frameRange.mMaximum); > } else { > - core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting; > + audioDevicePropertyBufferFrameSize = core->frameSizeSetting; > } > > /* set Buffer Frame Size */ > - status = coreaudio_set_framesize(core->outputDeviceID, > - > &core->audioDevicePropertyBufferFrameSize); > + status = coreaudio_set_framesize(deviceID, > + &audioDevicePropertyBufferFrameSize); > if (status == kAudioHardwareBadObjectError) { > return 0; > } > if (status != kAudioHardwareNoError) { > coreaudio_playback_logerr (status, > "Could not set device buffer frame > size %" PRIu32 "\n", > - > (uint32_t)core->audioDevicePropertyBufferFrameSize); > + > (uint32_t)audioDevicePropertyBufferFrameSize); > return status; > } > > /* get Buffer Frame Size */ > - status = coreaudio_get_framesize(core->outputDeviceID, > - > &core->audioDevicePropertyBufferFrameSize); > + status = coreaudio_get_framesize(deviceID, > + &audioDevicePropertyBufferFrameSize); > if (status == kAudioHardwareBadObjectError) { > return 0; > } > @@ -425,11 +427,9 @@ static OSStatus init_out_device(coreaudioVoiceOut > *core) > "Could not get device buffer frame > size\n"); > return status; > } > - core->hw.samples = core->bufferCount * > core->audioDevicePropertyBufferFrameSize; > > /* set Samplerate */ > - status = coreaudio_set_streamformat(core->outputDeviceID, > - &streamBasicDescription); > + status = coreaudio_set_streamformat(deviceID, > &streamBasicDescription); > if (status == kAudioHardwareBadObjectError) { > return 0; > } > @@ -437,7 +437,6 @@ static OSStatus init_out_device(coreaudioVoiceOut > *core) > coreaudio_playback_logerr (status, > "Could not set samplerate %lf\n", > streamBasicDescription.mSampleRate); > - core->outputDeviceID = kAudioDeviceUnknown; > return status; > } > > @@ -451,20 +450,24 @@ static OSStatus init_out_device(coreaudioVoiceOut > *core) > * Therefore, the specified callback must be designed to avoid a > deadlock > * with the callers of AudioObjectGetPropertyData. > */ > - core->ioprocid = NULL; > - status = AudioDeviceCreateIOProcID(core->outputDeviceID, > + ioprocid = NULL; > + status = AudioDeviceCreateIOProcID(deviceID, > audioDeviceIOProc, > &core->hw, > - &core->ioprocid); > + &ioprocid); > if (status == kAudioHardwareBadDeviceError) { > return 0; > } > - if (status != kAudioHardwareNoError || core->ioprocid == NULL) { > + if (status != kAudioHardwareNoError || ioprocid == NULL) { > coreaudio_playback_logerr (status, "Could not set IOProc\n"); > - core->outputDeviceID = kAudioDeviceUnknown; > return status; > } > > + core->outputDeviceID = deviceID; > + core->audioDevicePropertyBufferFrameSize = > audioDevicePropertyBufferFrameSize; > + core->hw.samples = core->bufferCount * > core->audioDevicePropertyBufferFrameSize; > + core->ioprocid = ioprocid; > + > return 0; > } > > @@ -548,7 +551,9 @@ static OSStatus handle_voice_change( > fini_out_device(core); > } > > - if (!init_out_device(core)) { > + init_out_device(core); > + > + if (core->outputDeviceID) { > update_device_playback_state(core); > } > > > -- > 2.47.1 > > >
diff --git a/audio/coreaudio.m b/audio/coreaudio.m index cadd729d5053..b9e1a952ed37 100644 --- a/audio/coreaudio.m +++ b/audio/coreaudio.m @@ -355,7 +355,10 @@ static OSStatus audioDeviceIOProc( static OSStatus init_out_device(coreaudioVoiceOut *core) { OSStatus status; + AudioDeviceID deviceID; AudioValueRange frameRange; + UInt32 audioDevicePropertyBufferFrameSize; + AudioDeviceIOProcID ioprocid; AudioStreamBasicDescription streamBasicDescription = { .mBitsPerChannel = core->hw.info.bits, @@ -368,20 +371,19 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) .mSampleRate = core->hw.info.freq }; - status = coreaudio_get_voice(&core->outputDeviceID); + status = coreaudio_get_voice(&deviceID); if (status != kAudioHardwareNoError) { coreaudio_playback_logerr (status, "Could not get default output Device\n"); return status; } - if (core->outputDeviceID == kAudioDeviceUnknown) { + if (deviceID == kAudioDeviceUnknown) { dolog ("Could not initialize playback - Unknown Audiodevice\n"); return status; } /* get minimum and maximum buffer frame sizes */ - status = coreaudio_get_framesizerange(core->outputDeviceID, - &frameRange); + status = coreaudio_get_framesizerange(deviceID, &frameRange); if (status == kAudioHardwareBadObjectError) { return 0; } @@ -392,31 +394,31 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) } if (frameRange.mMinimum > core->frameSizeSetting) { - core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum; + audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum; dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum); } else if (frameRange.mMaximum < core->frameSizeSetting) { - core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum; + audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum; dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum); } else { - core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting; + audioDevicePropertyBufferFrameSize = core->frameSizeSetting; } /* set Buffer Frame Size */ - status = coreaudio_set_framesize(core->outputDeviceID, - &core->audioDevicePropertyBufferFrameSize); + status = coreaudio_set_framesize(deviceID, + &audioDevicePropertyBufferFrameSize); if (status == kAudioHardwareBadObjectError) { return 0; } if (status != kAudioHardwareNoError) { coreaudio_playback_logerr (status, "Could not set device buffer frame size %" PRIu32 "\n", - (uint32_t)core->audioDevicePropertyBufferFrameSize); + (uint32_t)audioDevicePropertyBufferFrameSize); return status; } /* get Buffer Frame Size */ - status = coreaudio_get_framesize(core->outputDeviceID, - &core->audioDevicePropertyBufferFrameSize); + status = coreaudio_get_framesize(deviceID, + &audioDevicePropertyBufferFrameSize); if (status == kAudioHardwareBadObjectError) { return 0; } @@ -425,11 +427,9 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) "Could not get device buffer frame size\n"); return status; } - core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize; /* set Samplerate */ - status = coreaudio_set_streamformat(core->outputDeviceID, - &streamBasicDescription); + status = coreaudio_set_streamformat(deviceID, &streamBasicDescription); if (status == kAudioHardwareBadObjectError) { return 0; } @@ -437,7 +437,6 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) coreaudio_playback_logerr (status, "Could not set samplerate %lf\n", streamBasicDescription.mSampleRate); - core->outputDeviceID = kAudioDeviceUnknown; return status; } @@ -451,20 +450,24 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) * Therefore, the specified callback must be designed to avoid a deadlock * with the callers of AudioObjectGetPropertyData. */ - core->ioprocid = NULL; - status = AudioDeviceCreateIOProcID(core->outputDeviceID, + ioprocid = NULL; + status = AudioDeviceCreateIOProcID(deviceID, audioDeviceIOProc, &core->hw, - &core->ioprocid); + &ioprocid); if (status == kAudioHardwareBadDeviceError) { return 0; } - if (status != kAudioHardwareNoError || core->ioprocid == NULL) { + if (status != kAudioHardwareNoError || ioprocid == NULL) { coreaudio_playback_logerr (status, "Could not set IOProc\n"); - core->outputDeviceID = kAudioDeviceUnknown; return status; } + core->outputDeviceID = deviceID; + core->audioDevicePropertyBufferFrameSize = audioDevicePropertyBufferFrameSize; + core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize; + core->ioprocid = ioprocid; + return 0; } @@ -548,7 +551,9 @@ static OSStatus handle_voice_change( fini_out_device(core); } - if (!init_out_device(core)) { + init_out_device(core); + + if (core->outputDeviceID) { update_device_playback_state(core); }
init_out_device may only commit some part of the result and leave the state inconsistent when it encounters a fatal error or the device gets unplugged during the operation, which is expressed by kAudioHardwareBadObjectError or kAudioHardwareBadDeviceError. Commit the result in the end of the function so that it commits the result iff it sees no fatal error and the device remains plugged. With this change, handle_voice_change can rely on core->outputDeviceID to know whether the output device is initialized after calling init_out_device. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- audio/coreaudio.m | 49 +++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-)