From 819f5988d32fdaa25588018e71227961529bd23a Mon Sep 17 00:00:00 2001
Message-Id: <819f5988d32fdaa25588018e71227961529bd23a.1714839645.git.mdw@distorted.org.uk>
From: Mark Wooding <mdw@chiark.greenend.org.uk>
Date: Mon, 3 Mar 2008 22:56:22 +0000
Subject: [PATCH] Fix a race between track startup and scratching.  Basically
 if the scratch was too soon then SM_CANCEL would arrive at the speaker before
 SM_PLAY, leaving the speaker thinking this was a queue removal rather than a
 scratch, and therefore not sending a response.
Organization: Straylight/Edgeware

From: Richard Kettlewell <rjk@greenend.org.uk>

The fix is to respond to _all_ SM_CANCELs whatever the speaker thinks
they are, and disorderd to always check the ID against the playing
track.  The responses are distinguished, but the server no longer uses
this information.
---
 lib/speaker-protocol.h |  3 +++
 server/play.c          | 10 +++-------
 server/speaker.c       | 18 ++++++++++++++----
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/lib/speaker-protocol.h b/lib/speaker-protocol.h
index 520a0c6..92ce747 100644
--- a/lib/speaker-protocol.h
+++ b/lib/speaker-protocol.h
@@ -99,6 +99,9 @@ struct speaker_message {
  * initialization. */
 #define SM_READY 132
 
+/** @brief Cancelled track @c id which wasn't playing */
+#define SM_STILLBORN 133
+
 void speaker_send(int fd, const struct speaker_message *sm);
 /* Send a message. */
 
diff --git a/server/play.c b/server/play.c
index d73aec7..c447aa7 100644
--- a/server/play.c
+++ b/server/play.c
@@ -111,13 +111,9 @@ static int speaker_readable(ev_source *ev, int fd,
     D(("SM_PAUSED %s %ld", sm.id, sm.data));
     playing->sofar = sm.data;
     break;
-  case SM_FINISHED:
-    /* the playing track finished */
-    D(("SM_FINISHED %s", sm.id));
-    finished(ev);
-    break;
-  case SM_UNKNOWN:
-    /* we asked for an unknown track to be cancelled */
+  case SM_FINISHED:			/* scratched the playing track */
+  case SM_STILLBORN:			/* scratched too early */
+  case SM_UNKNOWN:			/* scratched WAY too early */
     if(playing && !strcmp(sm.id, playing->id))
       finished(ev);
     break;
diff --git a/server/speaker.c b/server/speaker.c
index 407b1d7..7411a8e 100644
--- a/server/speaker.c
+++ b/server/speaker.c
@@ -553,21 +553,31 @@ static void mainloop(void) {
           report();
 	  break;
 	case SM_CANCEL:
-          D(("SM_CANCEL %s",  sm.id));
+          D(("SM_CANCEL %s", sm.id));
 	  t = removetrack(sm.id);
 	  if(t) {
 	    if(t == playing) {
+              /* scratching the playing track */
               sm.type = SM_FINISHED;
-              strcpy(sm.id, playing->id);
-              speaker_send(1, &sm);
 	      playing = 0;
+            } else {
+              /* Could be scratching the playing track before it's quite got
+               * going, or could be just removing a track from the queue.  We
+               * log more because there's been a bug here recently than because
+               * it's particularly interesting; the log message will be removed
+               * if no further problems show up. */
+              info("SM_CANCEL for nonplaying track %s", sm.id);
+              sm.type = SM_STILLBORN;
             }
+            strcpy(sm.id, t->id);
 	    destroy(t);
 	  } else {
+            /* Probably scratching the playing track well before it's got
+             * going, but could indicate a bug, so we log this as an error. */
             sm.type = SM_UNKNOWN;
-            speaker_send(1, &sm);
 	    error(0, "SM_CANCEL for unknown track %s", sm.id);
           }
+          speaker_send(1, &sm);
           report();
 	  break;
 	case SM_RELOAD:
-- 
[mdw]