mp3parse: Refactor checking for sync. Make resyncing more reliable.
authorMichael Smith <msmith@songbirdnest.com>
Tue, 22 Sep 2009 19:13:38 +0000 (12:13 -0700)
committerMichael Smith <msmith@songbirdnest.com>
Tue, 22 Sep 2009 19:17:18 +0000 (12:17 -0700)
Previously, we could get false sync relatively easily - it sometimes happened
on real files. This cleans the code up a fair bit, and makes it require more
confirmation that we've found valid sync before continuing.

gst/mpegaudioparse/gstmpegaudioparse.c

index 6a2836a..40eadb9 100644 (file)
@@ -48,6 +48,10 @@ GST_DEBUG_CATEGORY_STATIC (mp3parse_debug);
 #define GST_READ_UINT24_BE(p) (p[2] | (p[1] << 8) | (p[0] << 16))
 #endif
 
+/* Minimum number of consecutive, valid-looking frames to consider
+   for resyncing */
+#define MIN_RESYNC_FRAMES 3
+
 static inline MPEGAudioSeekEntry *
 mpeg_audio_seek_entry_new ()
 {
@@ -121,6 +125,8 @@ static void gst_mp3parse_get_property (GObject * object, guint prop_id,
     GValue * value, GParamSpec * pspec);
 static GstStateChangeReturn gst_mp3parse_change_state (GstElement * element,
     GstStateChange transition);
+static GstFlowReturn
+gst_mp3parse_handle_data (GstMPEGAudioParse * mp3parse, gboolean at_eos);
 
 static gboolean mp3parse_bytepos_to_time (GstMPEGAudioParse * mp3parse,
     gint64 bytepos, GstClockTime * ts, gboolean from_total_time);
@@ -198,8 +204,9 @@ mp3_type_frame_length_from_header (GstMPEGAudioParse * mp3parse, guint32 header,
 
   bitrate = (header >> 12) & 0xF;
   bitrate = mp3types_bitrates[lsf][layer - 1][bitrate] * 1000;
-  if (bitrate == 0)
-    return 0;
+  /* The caller has ensured we have a valid header, so bitrate can't be
+     zero here. */
+  g_assert (bitrate != 0);
 
   samplerate = (header >> 10) & 0x3;
   samplerate = mp3types_freqs[lsf + mpg25][samplerate];
@@ -565,9 +572,16 @@ gst_mp3parse_sink_event (GstPad * pad, GstEvent * event)
       res = gst_pad_push_event (mp3parse->srcpad, event);
       break;
     case GST_EVENT_EOS:
+      /* If we haven't processed any frames yet, then make sure we process
+         at least whatever's in our adapter */
       if (mp3parse->frame_count == 0) {
-        GST_ELEMENT_ERROR (mp3parse, STREAM, WRONG_TYPE,
-            ("No valid frames found before end of stream"), (NULL));
+        gst_mp3parse_handle_data (mp3parse, TRUE);
+
+        /* If we STILL have zero frames processed, fire an error */
+        if (mp3parse->frame_count == 0) {
+          GST_ELEMENT_ERROR (mp3parse, STREAM, WRONG_TYPE,
+              ("No valid frames found before end of stream"), (NULL));
+        }
       }
       /* fall through */
     default:
@@ -1219,233 +1233,278 @@ done:
   gst_query_unref (query);
 }
 
-static GstFlowReturn
-gst_mp3parse_chain (GstPad * pad, GstBuffer * buf)
+/* Flush some number of bytes and update tracked offsets */
+static void
+gst_mp3parse_flush_bytes (GstMPEGAudioParse * mp3parse, int bytes)
 {
-  GstFlowReturn flow = GST_FLOW_OK;
-  GstMPEGAudioParse *mp3parse;
-  const guchar *data;
-  guint32 header;
-  int bpf;
-  guint available;
-  GstClockTime timestamp;
+  gst_adapter_flush (mp3parse->adapter, bytes);
+  if (mp3parse->cur_offset != -1)
+    mp3parse->cur_offset += bytes;
+  mp3parse->tracked_offset += bytes;
+}
 
-  mp3parse = GST_MP3PARSE (GST_PAD_PARENT (pad));
+/* Perform extended validation to check that subsequent headers match
+   the first header given here in important characteristics, to avoid
+   false sync. We look for a minimum of MIN_RESYNC_FRAMES consecutive
+   frames to match their major characteristics.
 
-  GST_LOG_OBJECT (mp3parse, "buffer of %d bytes", GST_BUFFER_SIZE (buf));
+   If at_eos is set to TRUE, we just check that we don't find any invalid
+   frames in whatever data is available, rather than requiring a full
+   MIN_RESYNC_FRAMES of data.
 
-  timestamp = GST_BUFFER_TIMESTAMP (buf);
+   Returns TRUE if we've seen enough data to validate or reject the frame.
+   If TRUE is returned, then *valid contains TRUE if it validated, or false
+   if we decided it was false sync.
+ */
+static gboolean
+gst_mp3parse_validate_extended (GstMPEGAudioParse * mp3parse, guint32 header,
+    int bpf, gboolean at_eos, gboolean * valid)
+{
+  guint32 next_header;
+  const guint8 *data;
+  guint available;
+  int frames_found = 1;
+  int offset = bpf;
 
-  mp3parse->discont |= GST_BUFFER_IS_DISCONT (buf);
+  while (frames_found < MIN_RESYNC_FRAMES) {
+    /* Check if we have enough data for all these frames, plus the next
+       frame header. */
+    available = gst_adapter_available (mp3parse->adapter);
+    if (available < offset + 4) {
+      if (at_eos) {
+        /* Running out of data at EOS is fine; just accept it */
+        *valid = TRUE;
+        return TRUE;
+      } else {
+        return FALSE;
+      }
+    }
 
-  /* If we don't yet have a next timestamp, save it and the incoming offset
-   * so we can apply it to the right outgoing buffer */
-  if (GST_CLOCK_TIME_IS_VALID (timestamp)) {
-    gint64 avail = gst_adapter_available (mp3parse->adapter);
+    data = gst_adapter_peek (mp3parse->adapter, offset + 4);
+    next_header = GST_READ_UINT32_BE (data + offset);
+    GST_DEBUG_OBJECT (mp3parse, "At %d: header=%08X, header2=%08X, bpf=%d",
+        offset, (unsigned int) header, (unsigned int) next_header, bpf);
 
-    mp3parse->pending_ts = timestamp;
-    mp3parse->pending_offset = mp3parse->tracked_offset + avail;
+/* mask the bits which are allowed to differ between frames */
+#define HDRMASK ~((0xF << 12)  /* bitrate */ | \
+                  (0x1 <<  9)  /* padding */ | \
+                  (0xf <<  4)  /* mode|mode extension */ | \
+                  (0xf))        /* copyright|emphasis */
 
-    /* If we have no data pending and the next timestamp is
-     * invalid we can use the upstream timestamp for the next frame.
-     *
-     * This will give us a timestamp if we're resyncing and upstream
-     * gave us -1 as offset. */
-    if (avail == 0 && !GST_CLOCK_TIME_IS_VALID (mp3parse->next_ts))
-      mp3parse->next_ts = timestamp;
+    if ((next_header & HDRMASK) != (header & HDRMASK)) {
+      /* If any of the unmasked bits don't match, then it's not valid */
+      GST_DEBUG_OBJECT (mp3parse, "next header doesn't match "
+          "(header=%08X (%08X), header2=%08X (%08X), bpf=%d)",
+          (guint) header, (guint) header & HDRMASK, (guint) next_header,
+          (guint) next_header & HDRMASK, bpf);
+      *valid = FALSE;
+      return TRUE;
+    } else if ((((next_header >> 12) & 0xf) == 0) ||
+        (((next_header >> 12) & 0xf) == 0xf)) {
+      /* The essential parts were the same, but the bitrate held an
+         invalid value - also reject */
+      GST_DEBUG_OBJECT (mp3parse, "next header invalid (bitrate)");
+      *valid = FALSE;
+      return TRUE;
+    }
 
-    GST_LOG_OBJECT (mp3parse, "Have pending ts %" GST_TIME_FORMAT
-        " to apply in %lld bytes (@ off %lld)",
-        GST_TIME_ARGS (mp3parse->pending_ts), avail, mp3parse->pending_offset);
+    bpf = mp3_type_frame_length_from_header (mp3parse, next_header,
+        NULL, NULL, NULL, NULL, NULL, NULL, NULL);
+
+    offset += bpf;
+    frames_found++;
   }
 
-  /* Update the cur_offset we'll apply to outgoing buffers */
-  if (mp3parse->cur_offset == -1 && GST_BUFFER_OFFSET (buf) != -1)
-    mp3parse->cur_offset = GST_BUFFER_OFFSET (buf);
+  *valid = TRUE;
+  return TRUE;
+}
 
-  /* And add the data to the pool */
-  gst_adapter_push (mp3parse->adapter, buf);
+static GstFlowReturn
+gst_mp3parse_handle_data (GstMPEGAudioParse * mp3parse, gboolean at_eos)
+{
+  GstFlowReturn flow = GST_FLOW_OK;
+  const guchar *data;
+  guint32 header;
+  int bpf;
+  guint available;
+  guint bitrate, layer, rate, channels, version, mode, crc;
+  gboolean caps_change;
 
   /* while we still have at least 4 bytes (for the header) available */
   while (gst_adapter_available (mp3parse->adapter) >= 4) {
-    /* search for a possible start byte */
+    /* Get the header bytes, check if they're potentially valid */
     data = gst_adapter_peek (mp3parse->adapter, 4);
-    if (*data != 0xff) {
-      /* It'd be nice to make this efficient, but it's ok for now; this is only
-       * when resyncing */
+    header = GST_READ_UINT32_BE (data);
+
+    if (!head_check (mp3parse, header)) {
+      /* Not a valid MP3 header; we start looking forward byte-by-byte trying to
+         find a place to resync */
       mp3parse->resyncing = TRUE;
-      gst_adapter_flush (mp3parse->adapter, 1);
-      if (mp3parse->cur_offset != -1)
-        mp3parse->cur_offset++;
-      mp3parse->tracked_offset++;
+      gst_mp3parse_flush_bytes (mp3parse, 1);
+      GST_DEBUG_OBJECT (mp3parse, "wrong header, skipping byte");
       continue;
     }
 
+    /* We have a potentially valid header.
+       If this is just a normal 'next frame', we go ahead and output it.
+
+       However, sometimes, we do additional validation to ensure we haven't
+       got false sync (common with mp3 due to the short sync word).
+       The additional validation requires that we find several consecutive mp3
+       frames with the same major parameters, or reach EOS with a smaller
+       number of valid-looking frames.
+
+       We do this if:
+       - This is the very first frame we've processed
+       - We're resyncing after a non-accurate seek, or after losing sync
+       due to invalid data.
+       - The format of the stream changes in a major way (number of channels,
+       sample rate, layer, or mpeg version).
+     */
     available = gst_adapter_available (mp3parse->adapter);
 
-    /* construct the header word */
-    header = GST_READ_UINT32_BE (data);
-    /* if it's a valid header, go ahead and send off the frame */
-    if (head_check (mp3parse, header)) {
-      guint bitrate = 0, layer = 0, rate = 0, channels = 0, version = 0, mode =
-          0, crc = 0;
-      gboolean caps_change = FALSE;
-
-      if (!(bpf = mp3_type_frame_length_from_header (mp3parse, header,
-                  &version, &layer, &channels, &bitrate, &rate, &mode, &crc)))
-        goto header_error;
-
-      if (channels != mp3parse->channels ||
-          rate != mp3parse->rate || layer != mp3parse->layer ||
-          version != mp3parse->version)
-        caps_change = TRUE;
-
-      /*************************************************************************
-      * robust seek support
-      * - This performs additional frame validation if the resyncing flag is set
-      *   (indicating a discontinuous stream), or if the caps are changing 
-      *   (different sample rate, channels, layer, version)
-      * - The current frame header is not accepted as valid unless the NEXT 
-      *   frame header has the same values for most fields.  This significantly
-      *   increases the probability that we aren't processing random data.
-      * - It is not clear if this is sufficient for robust seeking of Layer III
-      *   streams which utilize the concept of a "bit reservoir" by borrowing
-      *   bitrate from previous frames.  In this case, seeking may be more 
-      *   complicated because the frames are not independently coded.
-      *************************************************************************/
-      if (mp3parse->resyncing || caps_change) {
-        guint32 header2;
-        const guint8 *data2;
-
-        /* wait until we have the the entire current frame as well as the next 
-         * frame header */
-        if (available < bpf + 4)
-          break;
+    bpf = mp3_type_frame_length_from_header (mp3parse, header,
+        &version, &layer, &channels, &bitrate, &rate, &mode, &crc);
+    g_assert (bpf != 0);
 
-        data2 = gst_adapter_peek (mp3parse->adapter, bpf + 4);
-        header2 = GST_READ_UINT32_BE (data2 + bpf);
-        GST_DEBUG_OBJECT (mp3parse, "header=%08X, header2=%08X, bpf=%d",
-            (unsigned int) header, (unsigned int) header2, bpf);
+    if (channels != mp3parse->channels ||
+        rate != mp3parse->rate || layer != mp3parse->layer ||
+        version != mp3parse->version)
+      caps_change = TRUE;
+    else
+      caps_change = FALSE;
 
-/* mask the bits which are allowed to differ between frames */
-#define HDRMASK ~((0xF << 12)  /* bitrate */ | \
-                  (0x1 <<  9)  /* padding */ | \
-                  (0xf <<  4)  /* mode|mode extension */ | \
-                  (0xf))        /* copyright|emphasis */
+    if (mp3parse->resyncing || caps_change) {
+      gboolean valid;
+      if (!gst_mp3parse_validate_extended (mp3parse, header, bpf, at_eos,
+              &valid)) {
+        /* Not enough data to validate; wait for more */
+        break;
+      }
 
-        /* require 2 valid matching headers in a row */
-        if ((header2 & HDRMASK) != (header & HDRMASK)) {
-          GST_DEBUG_OBJECT (mp3parse, "next header doesn't match "
-              "(header=%08X (%08X), header2=%08X (%08X), bpf=%d)",
-              (guint) header, (guint) header & HDRMASK, (guint) header2,
-              (guint) header2 & HDRMASK, bpf);
-        } else if ((((header2 >> 12) & 0xf) == 0) ||
-            (((header2 >> 12) & 0xf) == 0xf)) {
-          /* optimized validity check for almost equal headers;
-           * only bitrate needs checking */
-          GST_DEBUG_OBJECT (mp3parse, "next header invalid (bitrate)");
-        } else {
-          goto valid;
-        }
-        /* This frame is invalid.  Start looking for a valid frame at the
-         * next position in the stream */
+      if (!valid) {
+        /* Extended validation failed; we probably got false sync.
+           Continue searching from the next byte in the stream */
         mp3parse->resyncing = TRUE;
-        gst_adapter_flush (mp3parse->adapter, 1);
-        if (mp3parse->cur_offset != -1)
-          mp3parse->cur_offset++;
-        mp3parse->tracked_offset++;
+        gst_mp3parse_flush_bytes (mp3parse, 1);
         continue;
       }
+    }
 
-    valid:
-      /* if we don't have the whole frame... */
-      if (available < bpf) {
-        GST_DEBUG_OBJECT (mp3parse, "insufficient data available, need "
-            "%d bytes, have %d", bpf, available);
-        break;
-      }
+    /* if we don't have the whole frame... */
+    if (available < bpf) {
+      GST_DEBUG_OBJECT (mp3parse, "insufficient data available, need "
+          "%d bytes, have %d", bpf, available);
+      break;
+    }
 
-      if (caps_change) {
-        GstCaps *caps;
+    if (caps_change) {
+      GstCaps *caps;
 
-        caps = mp3_caps_create (version, layer, channels, rate);
-        gst_pad_set_caps (mp3parse->srcpad, caps);
-        gst_caps_unref (caps);
+      caps = mp3_caps_create (version, layer, channels, rate);
+      gst_pad_set_caps (mp3parse->srcpad, caps);
+      gst_caps_unref (caps);
 
-        mp3parse->channels = channels;
-        mp3parse->rate = rate;
-      }
+      mp3parse->channels = channels;
+      mp3parse->rate = rate;
 
-      if (layer != mp3parse->layer || version != mp3parse->version) {
-        mp3parse->layer = layer;
-        mp3parse->version = version;
-
-        /* see http://www.codeproject.com/audio/MPEGAudioInfo.asp */
-        if (mp3parse->layer == 1)
-          mp3parse->spf = 384;
-        else if (mp3parse->layer == 2)
-          mp3parse->spf = 1152;
-        else if (mp3parse->version == 1) {
-          mp3parse->spf = 1152;
-        } else {
-          /* MPEG-2 or "2.5" */
-          mp3parse->spf = 576;
-        }
-      }
+      mp3parse->layer = layer;
+      mp3parse->version = version;
 
-      mp3parse->bit_rate = bitrate;
+      /* see http://www.codeproject.com/audio/MPEGAudioInfo.asp */
+      if (mp3parse->layer == 1)
+        mp3parse->spf = 384;
+      else if (mp3parse->layer == 2)
+        mp3parse->spf = 1152;
+      else if (mp3parse->version == 1) {
+        mp3parse->spf = 1152;
+      } else {
+        /* MPEG-2 or "2.5" */
+        mp3parse->spf = 576;
+      }
 
       mp3parse->max_bitreservoir = gst_util_uint64_scale (GST_SECOND,
           ((version == 1) ? 10 : 30) * mp3parse->spf, mp3parse->rate);
+    }
 
-      /* Check the first frame for a Xing header to get our total length */
-      if (mp3parse->frame_count == 0) {
-        /* For the first frame in the file, look for a Xing frame after 
-         * the header, and output a codec tag */
-        gst_mp3parse_handle_first_frame (mp3parse);
+    mp3parse->bit_rate = bitrate;
 
-        /* Check if we're seekable */
-        gst_mp3parse_check_seekability (mp3parse);
-      }
+    /* Check the first frame for a Xing header to get our total length */
+    if (mp3parse->frame_count == 0) {
+      /* For the first frame in the file, look for a Xing frame after 
+       * the header, and output a codec tag */
+      gst_mp3parse_handle_first_frame (mp3parse);
 
-      /* Update VBR stats */
-      mp3parse->bitrate_sum += mp3parse->bit_rate;
-      mp3parse->frame_count++;
-      /* Compute the average bitrate, rounded up to the nearest 1000 bits */
-      mp3parse->avg_bitrate =
-          (mp3parse->bitrate_sum / mp3parse->frame_count + 500);
-      mp3parse->avg_bitrate -= mp3parse->avg_bitrate % 1000;
-
-      if (!mp3parse->skip) {
-        mp3parse->resyncing = FALSE;
-        flow = gst_mp3parse_emit_frame (mp3parse, bpf, mode, crc);
-      } else {
-        GST_DEBUG_OBJECT (mp3parse, "skipping buffer of %d bytes", bpf);
-        gst_adapter_flush (mp3parse->adapter, bpf);
-        if (mp3parse->cur_offset != -1)
-          mp3parse->cur_offset += bpf;
-        mp3parse->tracked_offset += bpf;
-        mp3parse->skip--;
-      }
-    } else {
-      mp3parse->resyncing = TRUE;
-      gst_adapter_flush (mp3parse->adapter, 1);
-      if (mp3parse->cur_offset != -1)
-        mp3parse->cur_offset++;
-      mp3parse->tracked_offset++;
-      GST_DEBUG_OBJECT (mp3parse, "wrong header, skipping byte");
+      /* Check if we're seekable */
+      gst_mp3parse_check_seekability (mp3parse);
     }
 
-    if (GST_FLOW_IS_FATAL (flow))
-      break;
+    /* Update VBR stats */
+    mp3parse->bitrate_sum += mp3parse->bit_rate;
+    mp3parse->frame_count++;
+    /* Compute the average bitrate, rounded up to the nearest 1000 bits */
+    mp3parse->avg_bitrate =
+        (mp3parse->bitrate_sum / mp3parse->frame_count + 500);
+    mp3parse->avg_bitrate -= mp3parse->avg_bitrate % 1000;
+
+    if (!mp3parse->skip) {
+      mp3parse->resyncing = FALSE;
+      flow = gst_mp3parse_emit_frame (mp3parse, bpf, mode, crc);
+      if (GST_FLOW_IS_FATAL (flow))
+        break;
+    } else {
+      GST_DEBUG_OBJECT (mp3parse, "skipping buffer of %d bytes", bpf);
+      gst_mp3parse_flush_bytes (mp3parse, bpf);
+      mp3parse->skip--;
+    }
   }
 
   return flow;
+}
+
+static GstFlowReturn
+gst_mp3parse_chain (GstPad * pad, GstBuffer * buf)
+{
+  GstMPEGAudioParse *mp3parse;
+  GstClockTime timestamp;
+
+  mp3parse = GST_MP3PARSE (GST_PAD_PARENT (pad));
+
+  GST_LOG_OBJECT (mp3parse, "buffer of %d bytes", GST_BUFFER_SIZE (buf));
+
+  timestamp = GST_BUFFER_TIMESTAMP (buf);
+
+  mp3parse->discont |= GST_BUFFER_IS_DISCONT (buf);
+
+  /* If we don't yet have a next timestamp, save it and the incoming offset
+   * so we can apply it to the right outgoing buffer */
+  if (GST_CLOCK_TIME_IS_VALID (timestamp)) {
+    gint64 avail = gst_adapter_available (mp3parse->adapter);
+
+    mp3parse->pending_ts = timestamp;
+    mp3parse->pending_offset = mp3parse->tracked_offset + avail;
+
+    /* If we have no data pending and the next timestamp is
+     * invalid we can use the upstream timestamp for the next frame.
+     *
+     * This will give us a timestamp if we're resyncing and upstream
+     * gave us -1 as offset. */
+    if (avail == 0 && !GST_CLOCK_TIME_IS_VALID (mp3parse->next_ts))
+      mp3parse->next_ts = timestamp;
+
+    GST_LOG_OBJECT (mp3parse, "Have pending ts %" GST_TIME_FORMAT
+        " to apply in %lld bytes (@ off %lld)",
+        GST_TIME_ARGS (mp3parse->pending_ts), avail, mp3parse->pending_offset);
+  }
+
+  /* Update the cur_offset we'll apply to outgoing buffers */
+  if (mp3parse->cur_offset == -1 && GST_BUFFER_OFFSET (buf) != -1)
+    mp3parse->cur_offset = GST_BUFFER_OFFSET (buf);
+
+  /* And add the data to the pool */
+  gst_adapter_push (mp3parse->adapter, buf);
 
-header_error:
-  GST_ELEMENT_ERROR (mp3parse, STREAM, DECODE,
-      ("Invalid MP3 header found"), (NULL));
-  return GST_FLOW_ERROR;
+  return gst_mp3parse_handle_data (mp3parse, FALSE);
 }
 
 static gboolean