* [PATCH v3 0/2] coreaudio fixes
@ 2025-01-15 12:06 Akihiko Odaki
2025-01-15 12:06 ` [PATCH v3 1/2] coreaudio: Commit the result of init in the end Akihiko Odaki
2025-01-15 12:06 ` [PATCH v3 2/2] coreaudio: Initialize the buffer for device change Akihiko Odaki
0 siblings, 2 replies; 10+ messages in thread
From: Akihiko Odaki @ 2025-01-15 12:06 UTC (permalink / raw)
To: Gerd Hoffmann, Philippe Mathieu-Daudé, Christian Schoenebeck
Cc: qemu-devel, devel, Akihiko Odaki
This series contains two fixes for coreaudio. See each one for details.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (2):
coreaudio: Commit the result of init in the end
coreaudio: Initialize the buffer for device change
audio/audio_int.h | 2 ++
audio/audio.c | 24 ++++++++++++++++++------
audio/coreaudio.m | 50 ++++++++++++++++++++++++++++----------------------
3 files changed, 48 insertions(+), 28 deletions(-)
---
base-commit: 38d0939b86e2eef6f6a622c6f1f7befda0146595
change-id: 20250109-coreaudio-c984607e1d8c
Best regards,
--
Akihiko Odaki <akihiko.odaki@daynix.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 1/2] coreaudio: Commit the result of init in the end 2025-01-15 12:06 [PATCH v3 0/2] coreaudio fixes Akihiko Odaki @ 2025-01-15 12:06 ` Akihiko Odaki 2025-01-15 15:14 ` Christian Schoenebeck 2025-01-15 12:06 ` [PATCH v3 2/2] coreaudio: Initialize the buffer for device change Akihiko Odaki 1 sibling, 1 reply; 10+ messages in thread From: Akihiko Odaki @ 2025-01-15 12:06 UTC (permalink / raw) To: Gerd Hoffmann, Philippe Mathieu-Daudé, Christian Schoenebeck Cc: qemu-devel, devel, Akihiko Odaki init_out_device may only commit some part of the result and leave the state inconsistent when it encounters an error. Commit the result in the end of the function so that it commits the result iff it sees no error. 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(-) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] coreaudio: Commit the result of init in the end 2025-01-15 12:06 ` [PATCH v3 1/2] coreaudio: Commit the result of init in the end Akihiko Odaki @ 2025-01-15 15:14 ` Christian Schoenebeck 2025-01-15 15:37 ` Akihiko Odaki 0 siblings, 1 reply; 10+ messages in thread From: Christian Schoenebeck @ 2025-01-15 15:14 UTC (permalink / raw) To: Gerd Hoffmann, Philippe Mathieu-Daudé, qemu-devel Cc: devel, Akihiko Odaki On Wednesday, January 15, 2025 1:06:55 PM CET Akihiko Odaki wrote: > init_out_device may only commit some part of the result and leave the > state inconsistent when it encounters an error. Commit the result in > the end of the function so that it commits the result iff it sees no Typo "if". > error. > > 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(-) > > 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; I would probably preserve the name 'outputDeviceID' to make it more clear that it's for output. > 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); 'audioDevicePropertyBufferFrameSize' is declared as UInt32, so I guess the cast can be dropped. > 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; Are you sure this should be deferred to the end of the function? > > /* 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); > } > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] coreaudio: Commit the result of init in the end 2025-01-15 15:14 ` Christian Schoenebeck @ 2025-01-15 15:37 ` Akihiko Odaki 2025-01-15 17:10 ` Christian Schoenebeck 0 siblings, 1 reply; 10+ messages in thread From: Akihiko Odaki @ 2025-01-15 15:37 UTC (permalink / raw) To: Christian Schoenebeck, Gerd Hoffmann, Philippe Mathieu-Daudé, qemu-devel Cc: devel On 2025/01/16 0:14, Christian Schoenebeck wrote: > On Wednesday, January 15, 2025 1:06:55 PM CET Akihiko Odaki wrote: >> init_out_device may only commit some part of the result and leave the >> state inconsistent when it encounters an error. Commit the result in >> the end of the function so that it commits the result iff it sees no > > Typo "if". I meant if and only if. > >> error. >> >> 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(-) >> >> 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; > > I would probably preserve the name 'outputDeviceID' to make it more clear that > it's for output. I omitted output because this function is for the output device; every variable in this function is for output and prefixing them with output makes the code verbose. > >> 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); > > 'audioDevicePropertyBufferFrameSize' is declared as UInt32, so I guess the > cast can be dropped. It had a cast even though core->audioDevicePropertyBufferFrameSize is also UInt32. I suspect there are some cases where uint32_t and UInt32 are defined as different types and the compiler complains for "wrong" print format. > >> 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; > > Are you sure this should be deferred to the end of the function? Yes. Setting core->hw.samples only makes sense after AudioDeviceCreateIOProcID() succeeds, which starts generating samples according to the set value. > >> >> /* 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); >> } >> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] coreaudio: Commit the result of init in the end 2025-01-15 15:37 ` Akihiko Odaki @ 2025-01-15 17:10 ` Christian Schoenebeck 2025-01-16 5:17 ` Akihiko Odaki 0 siblings, 1 reply; 10+ messages in thread From: Christian Schoenebeck @ 2025-01-15 17:10 UTC (permalink / raw) To: Gerd Hoffmann, Philippe Mathieu-Daudé, qemu-devel Cc: devel, Akihiko Odaki On Wednesday, January 15, 2025 4:37:28 PM CET Akihiko Odaki wrote: > On 2025/01/16 0:14, Christian Schoenebeck wrote: > > On Wednesday, January 15, 2025 1:06:55 PM CET Akihiko Odaki wrote: > >> init_out_device may only commit some part of the result and leave the > >> state inconsistent when it encounters an error. Commit the result in > >> the end of the function so that it commits the result iff it sees no > > > > Typo "if". > > I meant if and only if. > > > > >> error. > >> > >> 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(-) > >> > >> 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; > > > > I would probably preserve the name 'outputDeviceID' to make it more clear that > > it's for output. > > I omitted output because this function is for the output device; every > variable in this function is for output and prefixing them with output > makes the code verbose. Disagree. When you review audio driver code there are many 'devices', so it is helpful to see the context straight away. Especially on large functions like this one. > > > >> 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); > > > > 'audioDevicePropertyBufferFrameSize' is declared as UInt32, so I guess the > > cast can be dropped. > > It had a cast even though core->audioDevicePropertyBufferFrameSize is > also UInt32. I suspect there are some cases where uint32_t and UInt32 > are defined as different types and the compiler complains for "wrong" > print format. This was introduced by cbc36cb05. While changing the format specifier made sense, the cast was unnecessary, but ... never mind. > > > >> 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; > > > > Are you sure this should be deferred to the end of the function? > > Yes. Setting core->hw.samples only makes sense after > AudioDeviceCreateIOProcID() succeeds, which starts generating samples > according to the set value. OK, I just reviewed the pathes in this function that return zero, and realized that these were added by you (3ba6e3f6), however I don't see any comment on why you have treated them as returning as non-error cases there. In general I highly recommend to be more verbose on your changes. For instance for this patch here it would have helped to see a comment which error path exactly you encountered. Because it makes it more easy for other people to understand what you are trying to fix exactly. /Christian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] coreaudio: Commit the result of init in the end 2025-01-15 17:10 ` Christian Schoenebeck @ 2025-01-16 5:17 ` Akihiko Odaki 2025-01-16 10:31 ` Christian Schoenebeck 0 siblings, 1 reply; 10+ messages in thread From: Akihiko Odaki @ 2025-01-16 5:17 UTC (permalink / raw) To: Christian Schoenebeck, Gerd Hoffmann, Philippe Mathieu-Daudé, qemu-devel Cc: devel On 2025/01/16 2:10, Christian Schoenebeck wrote: > On Wednesday, January 15, 2025 4:37:28 PM CET Akihiko Odaki wrote: >> On 2025/01/16 0:14, Christian Schoenebeck wrote: >>> On Wednesday, January 15, 2025 1:06:55 PM CET Akihiko Odaki wrote: >>>> init_out_device may only commit some part of the result and leave the >>>> state inconsistent when it encounters an error. Commit the result in >>>> the end of the function so that it commits the result iff it sees no >>> >>> Typo "if". >> >> I meant if and only if. >> >>> >>>> error. >>>> >>>> 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(-) >>>> >>>> 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; >>> >>> I would probably preserve the name 'outputDeviceID' to make it more clear that >>> it's for output. >> >> I omitted output because this function is for the output device; every >> variable in this function is for output and prefixing them with output >> makes the code verbose. > > Disagree. When you review audio driver code there are many 'devices', so it is > helpful to see the context straight away. Especially on large functions like > this one. It is not special to the device ID. Other variables like frameRange, audioDevicePropertyBufferFrameSize, and ioprocid are bound to this output device. In theory we could have similar corresponding variables for the input device so they should be prefixed with "output" if it matters. Prefixing only this variable is inconsistent. > >>> >>>> 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); >>> >>> 'audioDevicePropertyBufferFrameSize' is declared as UInt32, so I guess the >>> cast can be dropped. >> >> It had a cast even though core->audioDevicePropertyBufferFrameSize is >> also UInt32. I suspect there are some cases where uint32_t and UInt32 >> are defined as different types and the compiler complains for "wrong" >> print format. > > This was introduced by cbc36cb05. While changing the format specifier made > sense, the cast was unnecessary, but ... never mind. > >>> >>>> 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; >>> >>> Are you sure this should be deferred to the end of the function? >> >> Yes. Setting core->hw.samples only makes sense after >> AudioDeviceCreateIOProcID() succeeds, which starts generating samples >> according to the set value. > > OK, I just reviewed the pathes in this function that return zero, and realized > that these were added by you (3ba6e3f6), however I don't see any comment on > why you have treated them as returning as non-error cases there. kAudioHardwareBadObjectError and kAudioHardwareBadDeviceError imply the device was unplugged. They are not fatal as eventually we will get another active device with the handle_voice_change callback. Regards, Akihiko Odaki > > In general I highly recommend to be more verbose on your changes. For instance > for this patch here it would have helped to see a comment which error path > exactly you encountered. Because it makes it more easy for other people to > understand what you are trying to fix exactly. > > /Christian > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] coreaudio: Commit the result of init in the end 2025-01-16 5:17 ` Akihiko Odaki @ 2025-01-16 10:31 ` Christian Schoenebeck 0 siblings, 0 replies; 10+ messages in thread From: Christian Schoenebeck @ 2025-01-16 10:31 UTC (permalink / raw) To: Gerd Hoffmann, Philippe Mathieu-Daudé, qemu-devel Cc: devel, Akihiko Odaki On Thursday, January 16, 2025 6:17:46 AM CET Akihiko Odaki wrote: > On 2025/01/16 2:10, Christian Schoenebeck wrote: > > On Wednesday, January 15, 2025 4:37:28 PM CET Akihiko Odaki wrote: > >> On 2025/01/16 0:14, Christian Schoenebeck wrote: > >>> On Wednesday, January 15, 2025 1:06:55 PM CET Akihiko Odaki wrote: > >>>> init_out_device may only commit some part of the result and leave the > >>>> state inconsistent when it encounters an error. Commit the result in > >>>> the end of the function so that it commits the result iff it sees no > >>> > >>> Typo "if". > >> > >> I meant if and only if. > >> > >>> > >>>> error. > >>>> > >>>> 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(-) > >>>> > >>>> 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; > >>> > >>> I would probably preserve the name 'outputDeviceID' to make it more clear that > >>> it's for output. > >> > >> I omitted output because this function is for the output device; every > >> variable in this function is for output and prefixing them with output > >> makes the code verbose. > > > > Disagree. When you review audio driver code there are many 'devices', so it is > > helpful to see the context straight away. Especially on large functions like > > this one. > > It is not special to the device ID. Other variables like frameRange, > audioDevicePropertyBufferFrameSize, and ioprocid are bound to this > output device. In theory we could have similar corresponding variables > for the input device so they should be prefixed with "output" if it > matters. Prefixing only this variable is inconsistent. > > > > >>> > >>>> 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); > >>> > >>> 'audioDevicePropertyBufferFrameSize' is declared as UInt32, so I guess the > >>> cast can be dropped. > >> > >> It had a cast even though core->audioDevicePropertyBufferFrameSize is > >> also UInt32. I suspect there are some cases where uint32_t and UInt32 > >> are defined as different types and the compiler complains for "wrong" > >> print format. > > > > This was introduced by cbc36cb05. While changing the format specifier made > > sense, the cast was unnecessary, but ... never mind. > > > >>> > >>>> 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; > >>> > >>> Are you sure this should be deferred to the end of the function? > >> > >> Yes. Setting core->hw.samples only makes sense after > >> AudioDeviceCreateIOProcID() succeeds, which starts generating samples > >> according to the set value. > > > > OK, I just reviewed the pathes in this function that return zero, and realized > > that these were added by you (3ba6e3f6), however I don't see any comment on > > why you have treated them as returning as non-error cases there. > > kAudioHardwareBadObjectError and kAudioHardwareBadDeviceError imply the > device was unplugged. They are not fatal as eventually we will get > another active device with the handle_voice_change callback. It would "imply" a device been unplugged if either two of the constants were documented by Apple exactly that way, which is not the case. What happened instead is that you noticed that this error was returned by the CoreAudio API after you unplugged an audio device. That's called "observed". And that's exactly my point why I think you should be more verbose on your changes in general, so that other people can also understand your changes, without having to ask you. They are definitely not clear enough for other readers. > > In general I highly recommend to be more verbose on your changes. For instance > > for this patch here it would have helped to see a comment which error path > > exactly you encountered. Because it makes it more easy for other people to > > understand what you are trying to fix exactly. > > > > /Christian > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] coreaudio: Initialize the buffer for device change 2025-01-15 12:06 [PATCH v3 0/2] coreaudio fixes Akihiko Odaki 2025-01-15 12:06 ` [PATCH v3 1/2] coreaudio: Commit the result of init in the end Akihiko Odaki @ 2025-01-15 12:06 ` Akihiko Odaki 2025-01-15 15:31 ` Christian Schoenebeck 1 sibling, 1 reply; 10+ messages in thread From: Akihiko Odaki @ 2025-01-15 12:06 UTC (permalink / raw) To: Gerd Hoffmann, Philippe Mathieu-Daudé, Christian Schoenebeck Cc: qemu-devel, devel, Akihiko Odaki Reallocate buffers when the active device change as the required buffer size may differ. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- audio/audio_int.h | 2 ++ audio/audio.c | 24 ++++++++++++++++++------ audio/coreaudio.m | 1 + 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/audio/audio_int.h b/audio/audio_int.h index 2d079d00a259..9ba4a144d571 100644 --- a/audio/audio_int.h +++ b/audio/audio_int.h @@ -187,9 +187,11 @@ struct audio_pcm_ops { void (*volume_in)(HWVoiceIn *hw, Volume *vol); }; +void audio_generic_initialize_buffer_in(HWVoiceIn *hw); void audio_generic_run_buffer_in(HWVoiceIn *hw); void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size); void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size); +void audio_generic_initialize_buffer_out(HWVoiceOut *hw); void audio_generic_run_buffer_out(HWVoiceOut *hw); size_t audio_generic_buffer_get_free(HWVoiceOut *hw); void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size); diff --git a/audio/audio.c b/audio/audio.c index 87b4e9b6f2f3..17c6bbd0ae9e 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg) #endif } +void audio_generic_initialize_buffer_in(HWVoiceIn *hw) +{ + g_free(hw->buf_emul); + hw->size_emul = hw->samples * hw->info.bytes_per_frame; + hw->buf_emul = g_malloc(hw->size_emul); + hw->pos_emul = hw->pending_emul = 0; +} + void audio_generic_run_buffer_in(HWVoiceIn *hw) { if (unlikely(!hw->buf_emul)) { - hw->size_emul = hw->samples * hw->info.bytes_per_frame; - hw->buf_emul = g_malloc(hw->size_emul); - hw->pos_emul = hw->pending_emul = 0; + audio_generic_initialize_buffer_in(hw); } while (hw->pending_emul < hw->size_emul) { @@ -1446,6 +1452,14 @@ void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size) hw->pending_emul -= size; } +void audio_generic_initialize_buffer_out(HWVoiceOut *hw) +{ + g_free(hw->buf_emul); + hw->size_emul = hw->samples * hw->info.bytes_per_frame; + hw->buf_emul = g_malloc(hw->size_emul); + hw->pos_emul = hw->pending_emul = 0; +} + size_t audio_generic_buffer_get_free(HWVoiceOut *hw) { if (hw->buf_emul) { @@ -1477,9 +1491,7 @@ void audio_generic_run_buffer_out(HWVoiceOut *hw) void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size) { if (unlikely(!hw->buf_emul)) { - hw->size_emul = hw->samples * hw->info.bytes_per_frame; - hw->buf_emul = g_malloc(hw->size_emul); - hw->pos_emul = hw->pending_emul = 0; + audio_generic_initialize_buffer_out(hw); } *size = MIN(hw->size_emul - hw->pending_emul, diff --git a/audio/coreaudio.m b/audio/coreaudio.m index b9e1a952ed37..72a6df0f75ee 100644 --- a/audio/coreaudio.m +++ b/audio/coreaudio.m @@ -466,6 +466,7 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) core->outputDeviceID = deviceID; core->audioDevicePropertyBufferFrameSize = audioDevicePropertyBufferFrameSize; core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize; + audio_generic_initialize_buffer_out(&core->hw); core->ioprocid = ioprocid; return 0; -- 2.47.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] coreaudio: Initialize the buffer for device change 2025-01-15 12:06 ` [PATCH v3 2/2] coreaudio: Initialize the buffer for device change Akihiko Odaki @ 2025-01-15 15:31 ` Christian Schoenebeck 2025-01-15 15:40 ` Akihiko Odaki 0 siblings, 1 reply; 10+ messages in thread From: Christian Schoenebeck @ 2025-01-15 15:31 UTC (permalink / raw) To: Gerd Hoffmann, Philippe Mathieu-Daudé Cc: qemu-devel, devel, Akihiko Odaki On Wednesday, January 15, 2025 1:06:56 PM CET Akihiko Odaki wrote: > Reallocate buffers when the active device change as the required buffer > size may differ. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > audio/audio_int.h | 2 ++ > audio/audio.c | 24 ++++++++++++++++++------ > audio/coreaudio.m | 1 + > 3 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/audio/audio_int.h b/audio/audio_int.h > index 2d079d00a259..9ba4a144d571 100644 > --- a/audio/audio_int.h > +++ b/audio/audio_int.h > @@ -187,9 +187,11 @@ struct audio_pcm_ops { > void (*volume_in)(HWVoiceIn *hw, Volume *vol); > }; > > +void audio_generic_initialize_buffer_in(HWVoiceIn *hw); > void audio_generic_run_buffer_in(HWVoiceIn *hw); > void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size); > void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size); > +void audio_generic_initialize_buffer_out(HWVoiceOut *hw); > void audio_generic_run_buffer_out(HWVoiceOut *hw); > size_t audio_generic_buffer_get_free(HWVoiceOut *hw); > void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size); > diff --git a/audio/audio.c b/audio/audio.c > index 87b4e9b6f2f3..17c6bbd0ae9e 100644 > --- a/audio/audio.c > +++ b/audio/audio.c > @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg) > #endif > } > > +void audio_generic_initialize_buffer_in(HWVoiceIn *hw) > +{ > + g_free(hw->buf_emul); > + hw->size_emul = hw->samples * hw->info.bytes_per_frame; > + hw->buf_emul = g_malloc(hw->size_emul); > + hw->pos_emul = hw->pending_emul = 0; > +} > + Better something like "reinit" in the name maybe? > void audio_generic_run_buffer_in(HWVoiceIn *hw) > { > if (unlikely(!hw->buf_emul)) { > - hw->size_emul = hw->samples * hw->info.bytes_per_frame; > - hw->buf_emul = g_malloc(hw->size_emul); > - hw->pos_emul = hw->pending_emul = 0; > + audio_generic_initialize_buffer_in(hw); > } > > while (hw->pending_emul < hw->size_emul) { > @@ -1446,6 +1452,14 @@ void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size) > hw->pending_emul -= size; > } > > +void audio_generic_initialize_buffer_out(HWVoiceOut *hw) > +{ > + g_free(hw->buf_emul); > + hw->size_emul = hw->samples * hw->info.bytes_per_frame; > + hw->buf_emul = g_malloc(hw->size_emul); > + hw->pos_emul = hw->pending_emul = 0; > +} > + > size_t audio_generic_buffer_get_free(HWVoiceOut *hw) > { > if (hw->buf_emul) { > @@ -1477,9 +1491,7 @@ void audio_generic_run_buffer_out(HWVoiceOut *hw) > void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size) > { > if (unlikely(!hw->buf_emul)) { > - hw->size_emul = hw->samples * hw->info.bytes_per_frame; > - hw->buf_emul = g_malloc(hw->size_emul); > - hw->pos_emul = hw->pending_emul = 0; > + audio_generic_initialize_buffer_out(hw); > } > > *size = MIN(hw->size_emul - hw->pending_emul, > diff --git a/audio/coreaudio.m b/audio/coreaudio.m > index b9e1a952ed37..72a6df0f75ee 100644 > --- a/audio/coreaudio.m > +++ b/audio/coreaudio.m > @@ -466,6 +466,7 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) > core->outputDeviceID = deviceID; > core->audioDevicePropertyBufferFrameSize = audioDevicePropertyBufferFrameSize; > core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize; > + audio_generic_initialize_buffer_out(&core->hw); > core->ioprocid = ioprocid; I would have probably separated this change into a separate patch, as changes above were more or less just refactoring, whereas this one changes behaviour. And like my comment in the previous patch, I wonder whether that call comes too late. Keep in mind there are e.g. audio devices where you can't change certain parameters. So not every error here is a fatal error. > > return 0; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] coreaudio: Initialize the buffer for device change 2025-01-15 15:31 ` Christian Schoenebeck @ 2025-01-15 15:40 ` Akihiko Odaki 0 siblings, 0 replies; 10+ messages in thread From: Akihiko Odaki @ 2025-01-15 15:40 UTC (permalink / raw) To: Christian Schoenebeck, Gerd Hoffmann, Philippe Mathieu-Daudé Cc: qemu-devel, devel On 2025/01/16 0:31, Christian Schoenebeck wrote: > On Wednesday, January 15, 2025 1:06:56 PM CET Akihiko Odaki wrote: >> Reallocate buffers when the active device change as the required buffer >> size may differ. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> audio/audio_int.h | 2 ++ >> audio/audio.c | 24 ++++++++++++++++++------ >> audio/coreaudio.m | 1 + >> 3 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/audio/audio_int.h b/audio/audio_int.h >> index 2d079d00a259..9ba4a144d571 100644 >> --- a/audio/audio_int.h >> +++ b/audio/audio_int.h >> @@ -187,9 +187,11 @@ struct audio_pcm_ops { >> void (*volume_in)(HWVoiceIn *hw, Volume *vol); >> }; >> >> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw); >> void audio_generic_run_buffer_in(HWVoiceIn *hw); >> void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size); >> void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size); >> +void audio_generic_initialize_buffer_out(HWVoiceOut *hw); >> void audio_generic_run_buffer_out(HWVoiceOut *hw); >> size_t audio_generic_buffer_get_free(HWVoiceOut *hw); >> void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size); >> diff --git a/audio/audio.c b/audio/audio.c >> index 87b4e9b6f2f3..17c6bbd0ae9e 100644 >> --- a/audio/audio.c >> +++ b/audio/audio.c >> @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg) >> #endif >> } >> >> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw) >> +{ >> + g_free(hw->buf_emul); >> + hw->size_emul = hw->samples * hw->info.bytes_per_frame; >> + hw->buf_emul = g_malloc(hw->size_emul); >> + hw->pos_emul = hw->pending_emul = 0; >> +} >> + > > Better something like "reinit" in the name maybe? > >> void audio_generic_run_buffer_in(HWVoiceIn *hw) >> { >> if (unlikely(!hw->buf_emul)) { >> - hw->size_emul = hw->samples * hw->info.bytes_per_frame; >> - hw->buf_emul = g_malloc(hw->size_emul); >> - hw->pos_emul = hw->pending_emul = 0; >> + audio_generic_initialize_buffer_in(hw); >> } >> >> while (hw->pending_emul < hw->size_emul) { >> @@ -1446,6 +1452,14 @@ void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size) >> hw->pending_emul -= size; >> } >> >> +void audio_generic_initialize_buffer_out(HWVoiceOut *hw) >> +{ >> + g_free(hw->buf_emul); >> + hw->size_emul = hw->samples * hw->info.bytes_per_frame; >> + hw->buf_emul = g_malloc(hw->size_emul); >> + hw->pos_emul = hw->pending_emul = 0; >> +} >> + >> size_t audio_generic_buffer_get_free(HWVoiceOut *hw) >> { >> if (hw->buf_emul) { >> @@ -1477,9 +1491,7 @@ void audio_generic_run_buffer_out(HWVoiceOut *hw) >> void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size) >> { >> if (unlikely(!hw->buf_emul)) { >> - hw->size_emul = hw->samples * hw->info.bytes_per_frame; >> - hw->buf_emul = g_malloc(hw->size_emul); >> - hw->pos_emul = hw->pending_emul = 0; >> + audio_generic_initialize_buffer_out(hw); >> } >> >> *size = MIN(hw->size_emul - hw->pending_emul, >> diff --git a/audio/coreaudio.m b/audio/coreaudio.m >> index b9e1a952ed37..72a6df0f75ee 100644 >> --- a/audio/coreaudio.m >> +++ b/audio/coreaudio.m >> @@ -466,6 +466,7 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) >> core->outputDeviceID = deviceID; >> core->audioDevicePropertyBufferFrameSize = audioDevicePropertyBufferFrameSize; >> core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize; >> + audio_generic_initialize_buffer_out(&core->hw); >> core->ioprocid = ioprocid; > > I would have probably separated this change into a separate patch, as changes > above were more or less just refactoring, whereas this one changes behaviour. That makes sense. I will do so if I have to respin this series. > > And like my comment in the previous patch, I wonder whether that call comes > too late. Keep in mind there are e.g. audio devices where you can't change > certain parameters. So not every error here is a fatal error. A function called late, AudioDeviceCreateIOProcID() is essential to make the output device functional so any error which triggers an early return before that is fatal. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-01-16 10:31 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-15 12:06 [PATCH v3 0/2] coreaudio fixes Akihiko Odaki 2025-01-15 12:06 ` [PATCH v3 1/2] coreaudio: Commit the result of init in the end Akihiko Odaki 2025-01-15 15:14 ` Christian Schoenebeck 2025-01-15 15:37 ` Akihiko Odaki 2025-01-15 17:10 ` Christian Schoenebeck 2025-01-16 5:17 ` Akihiko Odaki 2025-01-16 10:31 ` Christian Schoenebeck 2025-01-15 12:06 ` [PATCH v3 2/2] coreaudio: Initialize the buffer for device change Akihiko Odaki 2025-01-15 15:31 ` Christian Schoenebeck 2025-01-15 15:40 ` Akihiko Odaki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).