From: Bastien Nocera Date: Thu, 21 Jun 2012 14:13:57 +0000 (+0100) Subject: asfdemux: Ignore parsing errors from broken packets X-Git-Tag: 1.19.3~505^2~734 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=53cfef3e0f143d8687fd39d9d3bfba936b3680a1;p=platform%2Fupstream%2Fgstreamer.git 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 --- 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 --- 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 --- 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;