From c88c6cfff57a7885e93d0458ddfc54d880fa12ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx@sigxcpu.org>
Date: Fri, 15 Oct 2021 14:49:45 +0200
Subject: [PATCH] fbd-dev-sound: Delay sending `ended` signal

Remove the feedback and the associated callback data before invoking the
callback (which then triggers FbdFeebackBased:ended) since we otherwise
remove the data from the currently playing sound from the hash table
instead of removing the data from the sound that just finished playing.

Call flow for a repeating sound before:

  on_sound_play_finished_callback()
    data->callback()
      fbd_feedback_get_ended()
        on_fb_ended()
          fbd_feedback_run()
            g_hash_table_insert(..., feedback, data)
            fbd_dev_sound_play()
              gsound_context_play_full() [async]
    g_hash_table_remove (..., data->feedback) <- removes wrong feedback

So (since the feedback is still the same but only the callback data
changes) we end up removing the data from the currently playing sound
from the hash table and any calls to fbd_dev_sound_stop() would fail
to find data and hence not cancel the playing sound.

Call flow for a repeating sound now:

  on_sound_play_finished_callback()
    g_hash_table_remove (..., data->feedback)
    data->callback()
      fbd_feedback_get_ended()
        on_fb_ended()
          fbd_feedback_run()
            g_hash_table_insert(..., feedback, data)
            fbd_dev_sound_play()
              gsound_context_play_full() [async]

So we remove the data of the currently playing sound before adding
the feedback to the hash table again. No ownership is transfered to
the hash table so no cleanusp need to be changed.

Closes: https://source.puri.sm/Librem5/feedbackd/-/issues/46
---
 src/fbd-dev-sound.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/fbd-dev-sound.c b/src/fbd-dev-sound.c
index 1052de1..f7861d8 100644
--- a/src/fbd-dev-sound.c
+++ b/src/fbd-dev-sound.c
@@ -187,9 +187,11 @@ on_sound_play_finished_callback (GSoundContext *ctx,
     }
   }
 
+  /* Order matters here. We need to remove the feedback from the hash table before
+     invoking the callback. */
+  g_hash_table_remove (data->dev->playbacks, data->feedback);
   (*data->callback)(data->feedback);
 
-  g_hash_table_remove (data->dev->playbacks, data->feedback);
   fbd_async_data_dispose (data);
 }
 
@@ -204,7 +206,8 @@ fbd_dev_sound_play (FbdDevSound *self, FbdFeedbackSound *feedback, FbdDevSoundPl
 
   data = fbd_async_data_new (self, feedback, callback);
 
-  g_hash_table_insert (self->playbacks, feedback, data);
+  if (!g_hash_table_insert (self->playbacks, feedback, data))
+    g_warning ("Feedback %p already present", feedback);
 
   gsound_context_play_full (self->ctx, data->cancel,
                             (GAsyncReadyCallback) on_sound_play_finished_callback,