[2/2] coreaudio: Handle output device change

Commit Message

Akihiko Odaki March 1, 2021, 11:45 a.m. UTC

An output device change can occur when plugging or unplugging an
earphone.

Signed-off-by: Akihiko Odaki <[e-mail]>
---
 audio/coreaudio.c | 327 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 236 insertions(+), 91 deletions(-)

Comments

Gerd Hoffmann March 3, 2021, 9:27 a.m. UTC | #1

Hi,

>      status = coreaudio_get_voice(&core->outputDeviceID);
>      if (status != kAudioHardwareNoError) {
> -        coreaudio_logerr2 (status, typ,
> -                           "Could not get default output Device\n");
> -        return -1;
> +        coreaudio_playback_logerr (status,
> +                                   "Could not get default output Device\n");
> +        return status;

This "pass status code to caller" change can and should be splitted to a
separate patch.

Likewise with the logging changes.

That makes the patch with the actual functional change to deal with the
device changes smaller and easier to review.

thanks,
  Gerd

Akihiko Odaki March 3, 2021, 1:20 p.m. UTC | #2

2021年3月3日(水) 18:27 Gerd Hoffmann <[e-mail]>:
>
>   Hi,
>
> >      status = coreaudio_get_voice(&core->outputDeviceID);
> >      if (status != kAudioHardwareNoError) {
> > -        coreaudio_logerr2 (status, typ,
> > -                           "Could not get default output Device\n");
> > -        return -1;
> > +        coreaudio_playback_logerr (status,
> > +                                   "Could not get default output Device\n");
> > +        return status;
>
> This "pass status code to caller" change can and should be splitted to a
> separate patch.
>
> Likewise with the logging changes.
>
> That makes the patch with the actual functional change to deal with the
> device changes smaller and easier to review.
>
> thanks,
>   Gerd
>

Actually the code was extracted from coreaudio_init_out to
init_out_device in this patch. init_out_device "passes status code to
caller", but coreaudio_init_out still doesn't. A new executor of the
procedure, handle_voice_change only uses the status code returned by
init_out_device.

Logging changes are alike; Now "playback" type logs are recorded by
both of coreaudio_init_out and handle_voice_change.
handle_voice_change is a function required only for device change.

Regards,
Akihiko Odaki

Gerd Hoffmann March 11, 2021, 12:50 p.m. UTC | #3

On Wed, Mar 03, 2021 at 10:20:20PM +0900, Akihiko Odaki wrote:
> 2021年3月3日(水) 18:27 Gerd Hoffmann <[e-mail]>:
> >
> >   Hi,
> >
> > >      status = coreaudio_get_voice(&core->outputDeviceID);
> > >      if (status != kAudioHardwareNoError) {
> > > -        coreaudio_logerr2 (status, typ,
> > > -                           "Could not get default output Device\n");
> > > -        return -1;
> > > +        coreaudio_playback_logerr (status,
> > > +                                   "Could not get default output Device\n");
> > > +        return status;
> >
> > This "pass status code to caller" change can and should be splitted to a
> > separate patch.
> >
> > Likewise with the logging changes.
> >
> > That makes the patch with the actual functional change to deal with the
> > device changes smaller and easier to review.
> >
> > thanks,
> >   Gerd
> >
> 
> Actually the code was extracted from coreaudio_init_out to
> init_out_device in this patch.

It is likewise good practice to handle such refactorings as separate
patch, i.e. have a patch which reorganizes the code (in this case move
code to the new init_out_device() function) without functional change,
then go add the new handle_voice_change which fixes the actual issue
in the next patch.  Finally send the two (or more) changes as patch
series.

take care,
  Gerd

Patch

diff --git a/audio/coreaudio.c b/audio/coreaudio.c
index c5f0b615d64..578ec9b8b2e 100644
--- a/audio/coreaudio.c
+++ b/audio/coreaudio.c
@@ -36,22 +36,26 @@  typedef struct coreaudioVoiceOut {
     HWVoiceOut hw;
     pthread_mutex_t mutex;
     AudioDeviceID outputDeviceID;
+    int frameSizeSetting;
+    uint32_t bufferCount;
     UInt32 audioDevicePropertyBufferFrameSize;
     AudioStreamBasicDescription outputStreamBasicDescription;
     AudioDeviceIOProcID ioprocid;
+    bool enabled;
 } coreaudioVoiceOut;
 
+static const AudioObjectPropertyAddress voice_addr = {
+    kAudioHardwarePropertyDefaultOutputDevice,
+    kAudioObjectPropertyScopeGlobal,
+    kAudioObjectPropertyElementMaster
+};
+
 static OSStatus coreaudio_get_voice(AudioDeviceID *id)
 {
     UInt32 size = sizeof(*id);
-    AudioObjectPropertyAddress addr = {
-        kAudioHardwarePropertyDefaultOutputDevice,
-        kAudioObjectPropertyScopeGlobal,
-        kAudioObjectPropertyElementMaster
-    };
 
     return AudioObjectGetPropertyData(kAudioObjectSystemObject,
-                                      &addr,
+                                      &voice_addr,
                                       0,
                                       NULL,
                                       &size,
@@ -253,17 +257,8 @@  static void GCC_FMT_ATTR (3, 4) coreaudio_logerr2 (
     coreaudio_logstatus (status);
 }
 
-static inline UInt32 isPlaying (AudioDeviceID outputDeviceID)
-{
-    OSStatus status;
-    UInt32 result = 0;
-    status = coreaudio_get_isrunning(outputDeviceID, &result);
-    if (status != kAudioHardwareNoError) {
-        coreaudio_logerr(status,
-                         "Could not determine whether Device is playing\n");
-    }
-    return result;
-}
+#define coreaudio_playback_logerr(status, ...) \
+    coreaudio_logerr2(status, "playback", __VA_ARGS__)
 
 static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
 {
@@ -336,6 +331,11 @@  static OSStatus audioDeviceIOProc(
         return 0;
     }
 
+    if (inDevice != core->outputDeviceID) {
+        coreaudio_unlock (core, "audioDeviceIOProc(old device)");
+        return 0;
+    }
+
     frameCount = core->audioDevicePropertyBufferFrameSize;
     pending_frames = hw->pending_emul / hw->info.bytes_per_frame;
 
@@ -368,175 +368,320 @@  static OSStatus audioDeviceIOProc(
     return 0;
 }
 
-static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,
-                              void *drv_opaque)
+static OSStatus init_out_device(coreaudioVoiceOut *core)
 {
     OSStatus status;
-    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
-    int err;
-    const char *typ = "playback";
     AudioValueRange frameRange;
-    Audiodev *dev = drv_opaque;
-    AudiodevCoreaudioPerDirectionOptions *cpdo = dev->u.coreaudio.out;
-    int frames;
-    struct audsettings obt_as;
-
-    /* create mutex */
-    err = pthread_mutex_init(&core->mutex, NULL);
-    if (err) {
-        dolog("Could not create mutex\nReason: %s\n", strerror (err));
-        return -1;
-    }
-
-    obt_as = *as;
-    as = &obt_as;
-    as->fmt = AUDIO_FORMAT_F32;
-    audio_pcm_init_info (&hw->info, as);
 
     status = coreaudio_get_voice(&core->outputDeviceID);
     if (status != kAudioHardwareNoError) {
-        coreaudio_logerr2 (status, typ,
-                           "Could not get default output Device\n");
-        return -1;
+        coreaudio_playback_logerr (status,
+                                   "Could not get default output Device\n");
+        return status;
     }
     if (core->outputDeviceID == kAudioDeviceUnknown) {
-        dolog ("Could not initialize %s - Unknown Audiodevice\n", typ);
-        return -1;
+        dolog ("Could not initialize playback - Unknown Audiodevice\n");
+        return status;
     }
 
     /* get minimum and maximum buffer frame sizes */
     status = coreaudio_get_framesizerange(core->outputDeviceID,
                                           &frameRange);
+    if (status == kAudioHardwareBadObjectError) {
+        return 0;
+    }
     if (status != kAudioHardwareNoError) {
-        coreaudio_logerr2 (status, typ,
-                           "Could not get device buffer frame range\n");
-        return -1;
+        coreaudio_playback_logerr (status,
+                                    "Could not get device buffer frame range\n");
+        return status;
     }
 
-    frames = audio_buffer_frames(
-        qapi_AudiodevCoreaudioPerDirectionOptions_base(cpdo), as, 11610);
-    if (frameRange.mMinimum > frames) {
+    if (frameRange.mMinimum > core->frameSizeSetting) {
         core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum;
         dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum);
-    } else if (frameRange.mMaximum < frames) {
+    } else if (frameRange.mMaximum < core->frameSizeSetting) {
         core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum;
         dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum);
     } else {
-        core->audioDevicePropertyBufferFrameSize = frames;
+        core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting;
     }
 
     /* set Buffer Frame Size */
     status = coreaudio_set_framesize(core->outputDeviceID,
                                      &core->audioDevicePropertyBufferFrameSize);
+    if (status == kAudioHardwareBadObjectError) {
+        return 0;
+    }
     if (status != kAudioHardwareNoError) {
-        coreaudio_logerr2 (status, typ,
-                           "Could not set device buffer frame size %" PRIu32 "\n",
-                           (uint32_t)core->audioDevicePropertyBufferFrameSize);
-        return -1;
+        coreaudio_playback_logerr (status,
+                                    "Could not set device buffer frame size %" PRIu32 "\n",
+                                    (uint32_t)core->audioDevicePropertyBufferFrameSize);
+        return status;
     }
 
     /* get Buffer Frame Size */
     status = coreaudio_get_framesize(core->outputDeviceID,
                                      &core->audioDevicePropertyBufferFrameSize);
+    if (status == kAudioHardwareBadObjectError) {
+        return 0;
+    }
     if (status != kAudioHardwareNoError) {
-        coreaudio_logerr2 (status, typ,
-                           "Could not get device buffer frame size\n");
-        return -1;
+        coreaudio_playback_logerr (status,
+                                    "Could not get device buffer frame size\n");
+        return status;
     }
-    hw->samples = (cpdo->has_buffer_count ? cpdo->buffer_count : 4) *
-        core->audioDevicePropertyBufferFrameSize;
+    core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize;
 
     /* get StreamFormat */
     status = coreaudio_get_streamformat(core->outputDeviceID,
                                         &core->outputStreamBasicDescription);
+    if (status == kAudioHardwareBadObjectError) {
+        return 0;
+    }
     if (status != kAudioHardwareNoError) {
-        coreaudio_logerr2 (status, typ,
-                           "Could not get Device Stream properties\n");
+        coreaudio_playback_logerr (status,
+                                    "Could not get Device Stream properties\n");
         core->outputDeviceID = kAudioDeviceUnknown;
-        return -1;
+        return status;
     }
 
     /* set Samplerate */
-    core->outputStreamBasicDescription.mSampleRate = (Float64) as->freq;
-
     status = coreaudio_set_streamformat(core->outputDeviceID,
                                         &core->outputStreamBasicDescription);
+    if (status == kAudioHardwareBadObjectError) {
+        return 0;
+    }
     if (status != kAudioHardwareNoError) {
-        coreaudio_logerr2 (status, typ, "Could not set samplerate %d\n",
-                           as->freq);
+        coreaudio_playback_logerr (status,
+                                   "Could not set samplerate %lf\n",
+                                   core->outputStreamBasicDescription.mSampleRate);
         core->outputDeviceID = kAudioDeviceUnknown;
-        return -1;
+        return status;
     }
 
     /* set Callback */
     core->ioprocid = NULL;
     status = AudioDeviceCreateIOProcID(core->outputDeviceID,
                                        audioDeviceIOProc,
-                                       hw,
+                                       &core->hw,
                                        &core->ioprocid);
+    if (status == kAudioHardwareBadDeviceError) {
+        return 0;
+    }
     if (status != kAudioHardwareNoError || core->ioprocid == NULL) {
-        coreaudio_logerr2 (status, typ, "Could not set IOProc\n");
+        coreaudio_playback_logerr (status, "Could not set IOProc\n");
         core->outputDeviceID = kAudioDeviceUnknown;
-        return -1;
+        return status;
     }
 
     return 0;
 }
 
-static void coreaudio_fini_out (HWVoiceOut *hw)
+static void fini_out_device(coreaudioVoiceOut *core)
 {
     OSStatus status;
-    int err;
-    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
+    UInt32 isrunning;
 
     /* stop playback */
-    if (isPlaying(core->outputDeviceID)) {
-        status = AudioDeviceStop(core->outputDeviceID, core->ioprocid);
+    status = coreaudio_get_isrunning(core->outputDeviceID, &isrunning);
+    if (status != kAudioHardwareBadObjectError) {
         if (status != kAudioHardwareNoError) {
-            coreaudio_logerr(status, "Could not stop playback\n");
+            coreaudio_logerr(status,
+                             "Could not determine whether Device is playing\n");
+        }
+
+        if (isrunning) {
+            status = AudioDeviceStop(core->outputDeviceID, core->ioprocid);
+            if (status != kAudioHardwareBadDeviceError && status != kAudioHardwareNoError) {
+                coreaudio_logerr(status, "Could not stop playback\n");
+            }
         }
     }
 
     /* remove callback */
     status = AudioDeviceDestroyIOProcID(core->outputDeviceID,
                                         core->ioprocid);
-    if (status != kAudioHardwareNoError) {
+    if (status != kAudioHardwareBadDeviceError && status != kAudioHardwareNoError) {
         coreaudio_logerr(status, "Could not remove IOProc\n");
     }
     core->outputDeviceID = kAudioDeviceUnknown;
-
-    /* destroy mutex */
-    err = pthread_mutex_destroy(&core->mutex);
-    if (err) {
-        dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
-    }
 }
 
-static void coreaudio_enable_out(HWVoiceOut *hw, bool enable)
+static void update_device_playback_state(coreaudioVoiceOut *core)
 {
     OSStatus status;
-    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
+    UInt32 isrunning;
 
-    if (enable) {
+    status = coreaudio_get_isrunning(core->outputDeviceID, &isrunning);
+    if (status != kAudioHardwareNoError) {
+        if (status != kAudioHardwareBadObjectError) {
+            coreaudio_logerr(status,
+                             "Could not determine whether Device is playing\n");
+        }
+
+        return;
+    }
+
+    if (core->enabled) {
         /* start playback */
-        if (!isPlaying(core->outputDeviceID)) {
+        if (!isrunning) {
             status = AudioDeviceStart(core->outputDeviceID, core->ioprocid);
-            if (status != kAudioHardwareNoError) {
+            if (status != kAudioHardwareBadDeviceError && status != kAudioHardwareNoError) {
                 coreaudio_logerr (status, "Could not resume playback\n");
             }
         }
     } else {
         /* stop playback */
-        if (isPlaying(core->outputDeviceID)) {
+        if (isrunning) {
             status = AudioDeviceStop(core->outputDeviceID,
                                      core->ioprocid);
-            if (status != kAudioHardwareNoError) {
+            if (status != kAudioHardwareBadDeviceError && status != kAudioHardwareNoError) {
                 coreaudio_logerr(status, "Could not pause playback\n");
             }
         }
     }
 }
 
+static OSStatus handle_voice_change(
+    AudioObjectID in_object_id,
+    UInt32 in_number_addresses,
+    const AudioObjectPropertyAddress *in_addresses,
+    void *in_client_data)
+{
+    OSStatus status;
+    coreaudioVoiceOut *core = in_client_data;
+
+    if (coreaudio_lock(core, __func__)) {
+        abort();
+    }
+
+    if (core->outputDeviceID) {
+        fini_out_device(core);
+    }
+
+    status = init_out_device(core);
+    if (!status) {
+        update_device_playback_state(core);
+    }
+
+    coreaudio_unlock (core, __func__);
+    return status;
+}
+
+static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,
+                              void *drv_opaque)
+{
+    OSStatus status;
+    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
+    int err;
+    Audiodev *dev = drv_opaque;
+    AudiodevCoreaudioPerDirectionOptions *cpdo = dev->u.coreaudio.out;
+    struct audsettings obt_as;
+
+    /* create mutex */
+    err = pthread_mutex_init(&core->mutex, NULL);
+    if (err) {
+        dolog("Could not create mutex\nReason: %s\n", strerror (err));
+        goto mutex_error;
+    }
+
+    if (coreaudio_lock(core, __func__)) {
+        goto lock_error;
+    }
+
+    obt_as = *as;
+    as = &obt_as;
+    as->fmt = AUDIO_FORMAT_F32;
+    audio_pcm_init_info (&hw->info, as);
+
+    core->frameSizeSetting = audio_buffer_frames(
+        qapi_AudiodevCoreaudioPerDirectionOptions_base(cpdo), as, 11610);
+
+    core->bufferCount = cpdo->has_buffer_count ? cpdo->buffer_count : 4;
+    core->outputStreamBasicDescription.mSampleRate = (Float64) as->freq;
+
+    status = AudioObjectAddPropertyListener(kAudioObjectSystemObject,
+                                            &voice_addr, handle_voice_change,
+                                            core);
+    if (status != kAudioHardwareNoError) {
+        coreaudio_playback_logerr (status,
+                                   "Could not listen to voice property change\n");
+        goto listener_error;
+    }
+
+    if (init_out_device(core)) {
+        goto device_error;
+    }
+
+    coreaudio_unlock(core, __func__);
+    return 0;
+
+device_error:
+    status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
+                                               &voice_addr,
+                                               handle_voice_change,
+                                               core);
+    if (status != kAudioHardwareNoError) {
+        coreaudio_playback_logerr(status,
+                                  "Could not remove voice property change listener\n");
+    }
+
+listener_error:
+    coreaudio_unlock(core, __func__);
+
+lock_error:
+    err = pthread_mutex_destroy(&core->mutex);
+    if (err) {
+        dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
+    }
+
+mutex_error:
+    return -1;
+}
+
+static void coreaudio_fini_out (HWVoiceOut *hw)
+{
+    OSStatus status;
+    int err;
+    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
+
+    if (coreaudio_lock(core, __func__)) {
+        abort();
+    }
+
+    status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
+                                               &voice_addr,
+                                               handle_voice_change,
+                                               core);
+    if (status != kAudioHardwareNoError) {
+        coreaudio_logerr(status, "Could not remove voice property change listener\n");
+    }
+
+    fini_out_device(core);
+
+    coreaudio_unlock(core, __func__);
+
+    /* destroy mutex */
+    err = pthread_mutex_destroy(&core->mutex);
+    if (err) {
+        dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
+    }
+}
+
+static void coreaudio_enable_out(HWVoiceOut *hw, bool enable)
+{
+    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
+
+    if (coreaudio_lock(core, __func__)) {
+        abort();
+    }
+
+    core->enabled = enable;
+    update_device_playback_state(core);
+
+    coreaudio_unlock(core, __func__);
+}
+
 static void *coreaudio_audio_init(Audiodev *dev)
 {
     return dev;