Re: [PATCH v3 1/2] coreaudio: Commit the result of init in the end

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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);
>>       }
>>   
>>
>>
> 
>