asfdemux: Ignore parsing errors from broken packets

We should instead be counting the number of errors and exiting if
they're too numerous. This makes a number of broken ASF files playable.

https://bugzilla.gnome.org/show_bug.cgi?id=678543

Conflicts:

	gst/asfdemux/asfpacket.c
	gst/asfdemux/gstasfdemux.c

diff --git a/gst/asfdemux/asfpacket.c b/gst/asfdemux/asfpacket.c
index ca6a6e1..1e64efc 100644 (file)
--- a/gst/asfdemux/asfpacket.c
+++ b/gst/asfdemux/asfpacket.c
@@ -493,14 +493,14 @@ gst_asf_demux_parse_payload (GstASFDemux * demux, AsfPacket * packet,
   return TRUE;
 }
 
-gboolean
+GstAsfDemuxParsePacketError
 gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
 {
   AsfPacket packet = { 0, };
   GstMapInfo map;
   const guint8 *data;
   gboolean has_multiple_payloads;
-  gboolean ret = TRUE;
+  GstAsfDemuxParsePacketError ret = GST_ASF_DEMUX_PARSE_PACKET_ERROR_NONE;
   guint8 ec_flags, flags1;
   guint size;
 
@@ -510,8 +510,10 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
   GST_LOG_OBJECT (demux, "Buffer size: %u", size);
 
   /* need at least two payload flag bytes, send time, and duration */
-  if (G_UNLIKELY (size < 2 + 4 + 2))
-    goto short_packet;
+  if (G_UNLIKELY (size < 2 + 4 + 2)) {
+    GST_WARNING_OBJECT (demux, "Packet size is < 8");
+    return GST_ASF_DEMUX_PARSE_PACKET_ERROR_RECOVERABLE;
+  }
 
   packet.buf = buf;
   /* evidently transient */
@@ -534,8 +536,10 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
     GST_LOG_OBJECT (demux, "packet has error correction (%u bytes)", ec_len);
 
     /* still need at least two payload flag bytes, send time, and duration */
-    if (size <= (1 + ec_len) + 2 + 4 + 2)
-      goto short_packet;
+    if (size <= (1 + ec_len) + 2 + 4 + 2) {
+      GST_WARNING_OBJECT (demux, "Packet size is < 8 with Error Correction");
+      return GST_ASF_DEMUX_PARSE_PACKET_ERROR_FATAL;
+    }
 
     data += 1 + ec_len;
     size -= 1 + ec_len;
@@ -556,8 +560,10 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
 
   packet.padding = asf_packet_read_varlen_int (flags1, 3, &data, &size);
 
-  if (G_UNLIKELY (size < 6))
-    goto short_packet;
+  if (G_UNLIKELY (size < 6)) {
+    GST_WARNING_OBJECT (demux, "Packet size is < 6");
+    return GST_ASF_DEMUX_PARSE_PACKET_ERROR_FATAL;
+  }
 
   packet.send_time = GST_READ_UINT32_LE (data) * GST_MSECOND;
   packet.duration = GST_READ_UINT16_LE (data + 4) * GST_MSECOND;
@@ -575,8 +581,10 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
   GST_LOG_OBJECT (demux, "duration         : %" GST_TIME_FORMAT,
       GST_TIME_ARGS (packet.duration));
 
-  if (G_UNLIKELY (packet.padding == (guint) - 1 || size < packet.padding))
-    goto short_packet;
+  if (G_UNLIKELY (packet.padding == (guint) - 1 || size < packet.padding)) {
+    GST_WARNING_OBJECT (demux, "No padding, or padding bigger than buffer");
+    return GST_ASF_DEMUX_PARSE_PACKET_ERROR_RECOVERABLE;
+  }
 
   size -= packet.padding;
 
@@ -588,7 +596,8 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
         "adjusting available data size");
     if (size < demux->packet_size - packet.length) {
       /* the buffer is smaller than the implicit padding */
-      goto short_packet;
+      GST_WARNING_OBJECT (demux, "Buffer is smaller than the implicit padding");
+      return GST_ASF_DEMUX_PARSE_PACKET_ERROR_RECOVERABLE;
     } else {
       /* subtract the implicit padding */
       size -= (demux->packet_size - packet.length);
@@ -598,8 +607,10 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
   if (has_multiple_payloads) {
     guint i, num, lentype;
 
-    if (G_UNLIKELY (size < 1))
-      goto short_packet;
+    if (G_UNLIKELY (size < 1)) {
+      GST_WARNING_OBJECT (demux, "No room more in buffer");
+      return GST_ASF_DEMUX_PARSE_PACKET_ERROR_RECOVERABLE;
+    }
 
     num = (GST_READ_UINT8 (data) & 0x3F) >> 0;
     lentype = (GST_READ_UINT8 (data) & 0xC0) >> 6;
@@ -613,26 +624,22 @@ gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf)
       GST_LOG_OBJECT (demux, "Parsing payload %u/%u, size left: %u", i + 1, num,
           size);
 
-      ret = gst_asf_demux_parse_payload (demux, &packet, lentype, &data, &size);
-
-      if (G_UNLIKELY (!ret)) {
+      if (G_UNLIKELY (!gst_asf_demux_parse_payload (demux, &packet, lentype,
+                  &data, &size))) {
         GST_WARNING_OBJECT (demux, "Failed to parse payload %u/%u", i + 1, num);
+        ret = GST_ASF_DEMUX_PARSE_PACKET_ERROR_FATAL;
         break;
       }
     }
   } else {
     GST_LOG_OBJECT (demux, "Parsing single payload");
-    ret = gst_asf_demux_parse_payload (demux, &packet, -1, &data, &size);
+    if (G_UNLIKELY (!gst_asf_demux_parse_payload (demux, &packet, -1, &data,
+                &size))) {
+      GST_WARNING_OBJECT (demux, "Failed to parse payload");
+      ret = GST_ASF_DEMUX_PARSE_PACKET_ERROR_RECOVERABLE;
+    }
   }
 
   gst_buffer_unmap (buf, &map);
   return ret;
-
-/* ERRORS */
-short_packet:
-  {
-    gst_buffer_unmap (buf, &map);
-    GST_WARNING_OBJECT (demux, "Short packet!");
-    return FALSE;
-  }
 }

diff --git a/gst/asfdemux/asfpacket.h b/gst/asfdemux/asfpacket.h
index a1722c8..99655f7 100644 (file)
--- a/gst/asfdemux/asfpacket.h
+++ b/gst/asfdemux/asfpacket.h
@@ -57,7 +57,13 @@ typedef struct {
   guint8        prop_flags;        /* payload length types                 */
 } AsfPacket;
 
-gboolean   gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf);
+typedef enum {
+  GST_ASF_DEMUX_PARSE_PACKET_ERROR_NONE,
+  GST_ASF_DEMUX_PARSE_PACKET_ERROR_RECOVERABLE,
+  GST_ASF_DEMUX_PARSE_PACKET_ERROR_FATAL
+} GstAsfDemuxParsePacketError;
+
+GstAsfDemuxParsePacketError gst_asf_demux_parse_packet (GstASFDemux * demux, GstBuffer * buf);
 
 #define gst_asf_payload_is_complete(payload) \
     ((payload)->buf_filled >= (payload)->mo_size)

diff --git a/gst/asfdemux/gstasfdemux.c b/gst/asfdemux/gstasfdemux.c
index 613d34e..8043f37 100644 (file)
--- a/gst/asfdemux/gstasfdemux.c
+++ b/gst/asfdemux/gstasfdemux.c
@@ -1621,9 +1621,9 @@ gst_asf_demux_loop (GstASFDemux * demux)
   }
 
   if (G_LIKELY (demux->speed_packets == 1)) {
-    /* FIXME: maybe we should just skip broken packets and error out only
-     * after a few broken packets in a row? */
-    if (G_UNLIKELY (!gst_asf_demux_parse_packet (demux, buf))) {
+    GstAsfDemuxParsePacketError err;
+    err = gst_asf_demux_parse_packet (demux, buf);
+    if (G_UNLIKELY (err != GST_ASF_DEMUX_PARSE_PACKET_ERROR_NONE)) {
       /* when we don't know when the data object ends, we should check
        * for a chained asf */
       if (demux->num_packets == 0) {
@@ -1635,7 +1635,13 @@ gst_asf_demux_loop (GstASFDemux * demux)
           return;
         }
       }
-      goto parse_error;
+      /* FIXME: We should tally up fatal errors and error out only
+       * after a few broken packets in a row? */
+
+      GST_INFO_OBJECT (demux, "Ignoring recoverable parse error");
+      gst_buffer_unref (buf);
+      ++demux->packet;
+      return;
     }
 
     flow = gst_asf_demux_push_complete_payloads (demux, FALSE);
@@ -1646,13 +1652,13 @@ gst_asf_demux_loop (GstASFDemux * demux)
     guint n;
     for (n = 0; n < demux->speed_packets; n++) {
       GstBuffer *sub;
+      GstAsfDemuxParsePacketError err;
 
       sub =
-          gst_buffer_copy_region (buf, GST_BUFFER_COPY_NONE,
+          gst_buffer_copy_region (buf, GST_BUFFER_COPY_ALL,
           n * demux->packet_size, demux->packet_size);
-      /* FIXME: maybe we should just skip broken packets and error out only
-       * after a few broken packets in a row? */
-      if (G_UNLIKELY (!gst_asf_demux_parse_packet (demux, sub))) {
+      err = gst_asf_demux_parse_packet (demux, sub);
+      if (G_UNLIKELY (err != GST_ASF_DEMUX_PARSE_PACKET_ERROR_NONE)) {
         /* when we don't know when the data object ends, we should check
          * for a chained asf */
         if (demux->num_packets == 0) {
@@ -1665,12 +1671,17 @@ gst_asf_demux_loop (GstASFDemux * demux)
             return;
           }
         }
-        goto parse_error;
+        /* FIXME: We should tally up fatal errors and error out only
+         * after a few broken packets in a row? */
+
+        GST_INFO_OBJECT (demux, "Ignoring recoverable parse error");
+        flow = GST_FLOW_OK;
       }
 
       gst_buffer_unref (sub);
 
-      flow = gst_asf_demux_push_complete_payloads (demux, FALSE);
+      if (err == GST_ASF_DEMUX_PARSE_PACKET_ERROR_NONE)
+        flow = gst_asf_demux_push_complete_payloads (demux, FALSE);
 
       ++demux->packet;
 
@@ -1762,6 +1773,8 @@ read_failed:
     flow = GST_FLOW_EOS;
     goto pause;
   }
+#if 0
+  /* See FIXMEs above */
 parse_error:
   {
     gst_buffer_unref (buf);
@@ -1771,6 +1784,7 @@ parse_error:
     flow = GST_FLOW_ERROR;
     goto pause;
   }
+#endif
 }
 
 #define GST_ASF_DEMUX_CHECK_HEADER_YES       0
@@ -1855,6 +1869,7 @@ gst_asf_demux_chain (GstPad * pad, GstObject * parent, GstBuffer * buf)
 
       while (gst_adapter_available (demux->adapter) >= data_size) {
         GstBuffer *buf;
+        GstAsfDemuxParsePacketError err;
 
         /* we don't know the length of the stream
          * check for a chained asf everytime */
@@ -1875,15 +1890,16 @@ gst_asf_demux_chain (GstPad * pad, GstObject * parent, GstBuffer * buf)
 
         buf = gst_adapter_take_buffer (demux->adapter, data_size);
 
-        /* FIXME: maybe we should just skip broken packets and error out only
+        /* FIXME: We should tally up fatal errors and error out only
          * after a few broken packets in a row? */
-        if (G_UNLIKELY (!gst_asf_demux_parse_packet (demux, buf))) {
-          GST_WARNING_OBJECT (demux, "Parse error");
-        }
+        err = gst_asf_demux_parse_packet (demux, buf);
 
         gst_buffer_unref (buf);
 
-        ret = gst_asf_demux_push_complete_payloads (demux, FALSE);
+        if (G_LIKELY (err == GST_ASF_DEMUX_PARSE_PACKET_ERROR_NONE))
+          ret = gst_asf_demux_push_complete_payloads (demux, FALSE);
+        else
+          GST_WARNING_OBJECT (demux, "Parse error");
 
         if (demux->packet >= 0)
           ++demux->packet;