diff mbox series

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

Message ID 20250115-coreaudio-v3-1-bdb6bcb5bf9f@daynix.com (mailing list archive)
State New
Headers show
Series coreaudio fixes | expand

Commit Message

Akihiko Odaki Jan. 15, 2025, 12:06 p.m. UTC 
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(-)

Comments

Christian Schoenebeck Jan. 15, 2025, 3:14 p.m. UTC | #1 
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);
>      }
>  
> 
>
diff mbox series

Patch

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