From 7e5a1b0460ce2c340c8a108d6a89f5ab724f186a Mon Sep 17 00:00:00 2001 From: =?utf8?q?Tim-Philipp=20M=C3=BCller?= Date: Sun, 19 Nov 2006 13:08:30 +0000 Subject: [PATCH] gst/mpegstream/: Fix flow value combination; this fixes playbin/totem locking up if a VobSub file is specified as sub... Original commit message from CVS: * gst/mpegstream/gstdvddemux.c: (gst_dvd_demux_base_init), (gst_dvd_demux_class_init), (gst_dvd_demux_combine_flows), (gst_dvd_demux_send_subbuffer): * gst/mpegstream/gstmpegdemux.c: (gst_mpeg_demux_class_init), (gst_mpeg_demux_init_stream), (gst_mpeg_demux_parse_packet), (gst_mpeg_demux_parse_pes), (gst_mpeg_demux_combine_flows), (gst_mpeg_demux_send_subbuffer): * gst/mpegstream/gstmpegdemux.h: * gst/mpegstream/gstmpegparse.c: (gst_mpeg_parse_process_event), (gst_mpeg_parse_chain): Fix flow value combination; this fixes playbin/totem locking up if a VobSub file is specified as subtitle file (#334322). Flow value combination should only happen once we are fairly sure we've got all pads that are available for now. Since there isn't a well-specified time when this is the case in MPEG, we'll just assume this is the case once there has been a certain number of packets for each stream we've found so far. --- ChangeLog | 20 ++++++++ gst/mpegstream/gstdvddemux.c | 83 +++++++++++++++++++++++++++----- gst/mpegstream/gstmpegdemux.c | 108 +++++++++++++++++++++++++++++++++++------- gst/mpegstream/gstmpegdemux.h | 22 +++++---- gst/mpegstream/gstmpegparse.c | 25 +++++----- 5 files changed, 208 insertions(+), 50 deletions(-) diff --git a/ChangeLog b/ChangeLog index a4e556b..0738f66 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,23 @@ +2006-11-19 Tim-Philipp Müller + + * gst/mpegstream/gstdvddemux.c: (gst_dvd_demux_base_init), + (gst_dvd_demux_class_init), (gst_dvd_demux_combine_flows), + (gst_dvd_demux_send_subbuffer): + * gst/mpegstream/gstmpegdemux.c: (gst_mpeg_demux_class_init), + (gst_mpeg_demux_init_stream), (gst_mpeg_demux_parse_packet), + (gst_mpeg_demux_parse_pes), (gst_mpeg_demux_combine_flows), + (gst_mpeg_demux_send_subbuffer): + * gst/mpegstream/gstmpegdemux.h: + * gst/mpegstream/gstmpegparse.c: (gst_mpeg_parse_process_event), + (gst_mpeg_parse_chain): + Fix flow value combination; this fixes playbin/totem locking up if + a VobSub file is specified as subtitle file (#334322). Flow value + combination should only happen once we are fairly sure we've got all + pads that are available for now. Since there isn't a well-specified + time when this is the case in MPEG, we'll just assume this is the + case once there has been a certain number of packets for each + stream we've found so far. + 2006-11-15 Wim Taymans Patch by: Jan Arne Petersen diff --git a/gst/mpegstream/gstdvddemux.c b/gst/mpegstream/gstdvddemux.c index 9bc4acc..76f0aca 100644 --- a/gst/mpegstream/gstdvddemux.c +++ b/gst/mpegstream/gstdvddemux.c @@ -139,8 +139,6 @@ static void gst_dvd_demux_base_init (gpointer klass); static void gst_dvd_demux_init (GstDVDDemux * dvd_demux, GstDVDDemuxClass * klass); -static GstFlowReturn gst_dvd_demux_send_buffer (GstMPEGParse * mpeg_parse, - GstBuffer * buffer, GstClockTime time); static gboolean gst_dvd_demux_process_event (GstMPEGParse * mpeg_parse, GstEvent * event); @@ -167,6 +165,9 @@ static GstFlowReturn gst_dvd_demux_send_subbuffer GstMPEGStream * outstream, GstBuffer * buffer, GstClockTime timestamp, guint offset, guint size); +static GstFlowReturn gst_dvd_demux_combine_flows (GstMPEGDemux * mpegdemux, + GstMPEGStream * stream, GstFlowReturn flow); + static void gst_dvd_demux_set_cur_audio (GstDVDDemux * dvd_demux, gint stream_nr); static void gst_dvd_demux_set_cur_subpicture @@ -192,7 +193,7 @@ gst_dvd_demux_base_init (gpointer klass) GstMPEGDemuxClass *demux_class = GST_MPEG_DEMUX_CLASS (klass); GstDVDDemuxClass *dvd_demux_class = GST_DVD_DEMUX_CLASS (klass); - mpeg_parse_class->send_buffer = gst_dvd_demux_send_buffer; + mpeg_parse_class->send_buffer = NULL; mpeg_parse_class->process_event = gst_dvd_demux_process_event; /* sink pad */ @@ -245,6 +246,7 @@ gst_dvd_demux_class_init (GstDVDDemuxClass * klass) mpeg_demux_class->get_audio_stream = gst_dvd_demux_get_audio_stream; mpeg_demux_class->get_video_stream = gst_dvd_demux_get_video_stream; mpeg_demux_class->send_subbuffer = gst_dvd_demux_send_subbuffer; + mpeg_demux_class->combine_flows = gst_dvd_demux_combine_flows; mpeg_demux_class->process_private = gst_dvd_demux_process_private; mpeg_demux_class->synchronise_pads = gst_dvd_demux_synchronise_pads; mpeg_demux_class->sync_stream_to_time = gst_dvd_demux_sync_stream_to_time; @@ -288,15 +290,6 @@ gst_dvd_demux_init (GstDVDDemux * dvd_demux, GstDVDDemuxClass * klass) dvd_demux->langcodes = NULL; } - -static GstFlowReturn -gst_dvd_demux_send_buffer (GstMPEGParse * mpeg_parse, GstBuffer * buffer, - GstClockTime time) -{ - gst_buffer_unref (buffer); - return GST_FLOW_OK; -} - static gboolean gst_dvd_demux_process_event (GstMPEGParse * mpeg_parse, GstEvent * event) { @@ -918,6 +911,60 @@ gst_dvd_demux_process_private (GstMPEGDemux * mpeg_demux, return ret; } +/* random magic value */ +#define MIN_BUFS_FOR_NO_MORE_PADS 100 + +static GstFlowReturn +gst_dvd_demux_combine_flows (GstMPEGDemux * mpegdemux, GstMPEGStream * stream, + GstFlowReturn flow) +{ + GstDVDDemux *demux = (GstDVDDemux *) mpegdemux; + gint i; + + /* store the value */ + stream->last_flow = flow; + + /* if it's success we can return the value right away */ + if (GST_FLOW_IS_SUCCESS (flow)) + goto done; + + /* any other error that is not-linked can be returned right + * away */ + if (flow != GST_FLOW_NOT_LINKED) { + GST_DEBUG_OBJECT (demux, "flow %s on pad %" GST_PTR_FORMAT, + gst_flow_get_name (flow), stream->pad); + goto done; + } + + /* let parent class check for anything non-not-linked for its streams */ + flow = + GST_MPEG_DEMUX_CLASS (parent_class)->combine_flows (mpegdemux, stream, + flow); + if (flow != GST_FLOW_NOT_LINKED) + goto done; + + /* only return NOT_LINKED if all other pads returned NOT_LINKED */ + for (i = 0; i < GST_DVD_DEMUX_NUM_SUBPICTURE_STREAMS; i++) { + if (demux->subpicture_stream[i] != NULL) { + flow = demux->subpicture_stream[i]->last_flow; + /* some other return value (must be SUCCESS but we can return + * other values as well) */ + if (flow != GST_FLOW_NOT_LINKED) + goto done; + if (demux->subpicture_stream[i]->buffers_sent < MIN_BUFS_FOR_NO_MORE_PADS) { + flow = GST_FLOW_OK; + goto done; + } + } + } + /* if we get here, all other pads were unlinked and we return + * NOT_LINKED then */ + GST_DEBUG_OBJECT (demux, "all pads combined have not-linked flow"); + +done: + return flow; +} + static GstFlowReturn gst_dvd_demux_send_subbuffer (GstMPEGDemux * mpeg_demux, GstMPEGStream * outstream, GstBuffer * buffer, @@ -987,10 +1034,22 @@ gst_dvd_demux_send_subbuffer (GstMPEGDemux * mpeg_demux, gst_buffer_set_caps (outbuf, outstream->caps); ret = gst_pad_push (outpad, outbuf); + + /* this was one of the current_foo pads, which is shadowing one of the + * foo_%02d pads, so since we keep track of the last flow value in the + * stream structure we need to combine the OK/NOT_LINKED flows here + * (because both pads share the same stream structure) */ + if ((ret == GST_FLOW_NOT_LINKED && outstream->last_flow == GST_FLOW_OK) || + (ret == GST_FLOW_OK && outstream->last_flow == GST_FLOW_NOT_LINKED)) { + outstream->last_flow = GST_FLOW_OK; + ret = GST_FLOW_OK; + } } gst_buffer_unref (buffer); + ret = DEMUX_CLASS (dvd_demux)->combine_flows (mpeg_demux, outstream, ret); + return ret; } diff --git a/gst/mpegstream/gstmpegdemux.c b/gst/mpegstream/gstmpegdemux.c index 2abba38..f3b41ed 100644 --- a/gst/mpegstream/gstmpegdemux.c +++ b/gst/mpegstream/gstmpegdemux.c @@ -92,9 +92,6 @@ GST_BOILERPLATE_FULL (GstMPEGDemux, gst_mpeg_demux, GstMPEGParse, static void gst_mpeg_demux_class_init (GstMPEGDemuxClass * klass); -static GstFlowReturn gst_mpeg_demux_send_buffer (GstMPEGParse * mpeg_parse, - GstBuffer * buffer, GstClockTime time); - static GstPad *gst_mpeg_demux_new_output_pad (GstMPEGDemux * mpeg_demux, const gchar * name, GstPadTemplate * temp); static void gst_mpeg_demux_init_stream (GstMPEGDemux * mpeg_demux, @@ -120,6 +117,8 @@ static GstFlowReturn gst_mpeg_demux_parse_pes (GstMPEGParse * mpeg_parse, static GstFlowReturn gst_mpeg_demux_send_subbuffer (GstMPEGDemux * mpeg_demux, GstMPEGStream * outstream, GstBuffer * buffer, GstClockTime timestamp, guint offset, guint size); +static GstFlowReturn gst_mpeg_demux_combine_flows (GstMPEGDemux * mpeg_demux, + GstMPEGStream * stream, GstFlowReturn flow); static GstFlowReturn gst_mpeg_demux_process_private (GstMPEGDemux * mpeg_demux, GstBuffer * buffer, guint stream_nr, GstClockTime timestamp, guint headerlen, guint datalen); @@ -190,7 +189,7 @@ gst_mpeg_demux_class_init (GstMPEGDemuxClass * klass) mpeg_parse_class->parse_syshead = gst_mpeg_demux_parse_syshead; mpeg_parse_class->parse_packet = gst_mpeg_demux_parse_packet; mpeg_parse_class->parse_pes = gst_mpeg_demux_parse_pes; - mpeg_parse_class->send_buffer = gst_mpeg_demux_send_buffer; + mpeg_parse_class->send_buffer = NULL; klass->new_output_pad = gst_mpeg_demux_new_output_pad; klass->init_stream = gst_mpeg_demux_init_stream; @@ -198,6 +197,7 @@ gst_mpeg_demux_class_init (GstMPEGDemuxClass * klass) klass->get_audio_stream = gst_mpeg_demux_get_audio_stream; klass->get_private_stream = gst_mpeg_demux_get_private_stream; klass->send_subbuffer = gst_mpeg_demux_send_subbuffer; + klass->combine_flows = gst_mpeg_demux_combine_flows; klass->process_private = gst_mpeg_demux_process_private; klass->synchronise_pads = gst_mpeg_demux_synchronise_pads; klass->sync_stream_to_time = gst_mpeg_demux_sync_stream_to_time; @@ -229,14 +229,6 @@ gst_mpeg_demux_init (GstMPEGDemux * mpeg_demux, GstMPEGDemuxClass * klass) mpeg_demux->last_pts = -1; } -static GstFlowReturn -gst_mpeg_demux_send_buffer (GstMPEGParse * mpeg_parse, GstBuffer * buffer, - GstClockTime time) -{ - gst_buffer_unref (buffer); - return GST_FLOW_OK; -} - static gint _demux_get_writer_id (GstIndex * index, GstPad * pad) { @@ -291,6 +283,9 @@ gst_mpeg_demux_init_stream (GstMPEGDemux * mpeg_demux, str->cur_ts = 0; str->scr_offs = 0; + + str->last_flow = GST_FLOW_OK; + str->buffers_sent = 0; } static GstMPEGStream * @@ -712,12 +707,12 @@ done: if (id == 0xBD) { /* Private stream 1. */ GST_DEBUG_OBJECT (mpeg_demux, "we have a private 1 packet"); - CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 0, timestamp, + ret = CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 0, timestamp, headerlen, datalen); } else if (id == 0xBF) { /* Private stream 2. */ GST_DEBUG_OBJECT (mpeg_demux, "we have a private 2 packet"); - CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 1, timestamp, + ret = CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 1, timestamp, headerlen, datalen); } else if (id >= 0xC0 && id <= 0xDF) { /* Audio. */ @@ -855,13 +850,13 @@ gst_mpeg_demux_parse_pes (GstMPEGParse * mpeg_parse, GstBuffer * buffer) if (id == 0xBD) { /* Private stream 1. */ GST_DEBUG_OBJECT (mpeg_demux, "we have a private 1 packet"); - CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 0, timestamp, - headerlen, datalen); + ret = CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 0, + timestamp, headerlen, datalen); } else if (id == 0xBF) { /* Private stream 2. */ GST_DEBUG_OBJECT (mpeg_demux, "we have a private 2 packet"); - CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 1, timestamp, - headerlen, datalen); + ret = CLASS (mpeg_demux)->process_private (mpeg_demux, buffer, 1, + timestamp, headerlen, datalen); } else if (id >= 0xC0 && id <= 0xDF) { /* Audio. */ GST_DEBUG_OBJECT (mpeg_demux, "we have an audio packet"); @@ -886,6 +881,78 @@ gst_mpeg_demux_parse_pes (GstMPEGParse * mpeg_parse, GstBuffer * buffer) return ret; } +/* random magic value */ +#define MIN_BUFS_FOR_NO_MORE_PADS 100 + +static GstFlowReturn +gst_mpeg_demux_combine_flows (GstMPEGDemux * demux, GstMPEGStream * stream, + GstFlowReturn flow) +{ + gint i; + + /* store the value */ + stream->last_flow = flow; + + /* if it's success we can return the value right away */ + if (GST_FLOW_IS_SUCCESS (flow)) + goto done; + + /* any other error that is not-linked can be returned right + * away */ + if (flow != GST_FLOW_NOT_LINKED) { + GST_DEBUG_OBJECT (demux, "flow %s on pad %" GST_PTR_FORMAT, + gst_flow_get_name (flow), stream->pad); + goto done; + } + + /* only return NOT_LINKED if all other pads returned NOT_LINKED */ + for (i = 0; i < GST_MPEG_DEMUX_NUM_VIDEO_STREAMS; i++) { + if (demux->video_stream[i] != NULL) { + flow = demux->video_stream[i]->last_flow; + /* some other return value (must be SUCCESS but we can return + * other values as well) */ + if (flow != GST_FLOW_NOT_LINKED) + goto done; + if (demux->video_stream[i]->buffers_sent < MIN_BUFS_FOR_NO_MORE_PADS) { + flow = GST_FLOW_OK; + goto done; + } + } + } + for (i = 0; i < GST_MPEG_DEMUX_NUM_AUDIO_STREAMS; i++) { + if (demux->audio_stream[i] != NULL) { + flow = demux->audio_stream[i]->last_flow; + /* some other return value (must be SUCCESS but we can return + * other values as well) */ + if (flow != GST_FLOW_NOT_LINKED) + goto done; + if (demux->audio_stream[i]->buffers_sent < MIN_BUFS_FOR_NO_MORE_PADS) { + flow = GST_FLOW_OK; + goto done; + } + } + } + for (i = 0; i < GST_MPEG_DEMUX_NUM_PRIVATE_STREAMS; i++) { + if (demux->private_stream[i] != NULL) { + flow = demux->private_stream[i]->last_flow; + /* some other return value (must be SUCCESS but we can return + * other values as well) */ + if (flow != GST_FLOW_NOT_LINKED) + goto done; + if (demux->private_stream[i]->buffers_sent < MIN_BUFS_FOR_NO_MORE_PADS) { + flow = GST_FLOW_OK; + goto done; + } + } + } + /* if we get here, all other pads were unlinked and we return + * NOT_LINKED then */ + GST_DEBUG_OBJECT (demux, "all pads combined have not-linked flow"); + +done: + return flow; +} + static GstFlowReturn gst_mpeg_demux_send_subbuffer (GstMPEGDemux * mpeg_demux, GstMPEGStream * outstream, GstBuffer * buffer, @@ -927,6 +994,9 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux * mpeg_demux, GST_BUFFER_TIMESTAMP (outbuf) = timestamp; GST_BUFFER_OFFSET (outbuf) = GST_BUFFER_OFFSET (buffer) + offset; ret = gst_pad_push (outstream->pad, outbuf); + GST_LOG ("flow on %s: %s", GST_PAD_NAME (outstream->pad), + gst_flow_get_name (ret)); + ++outstream->buffers_sent; if (GST_CLOCK_TIME_IS_VALID (mpeg_demux->max_gap) && GST_CLOCK_TIME_IS_VALID (mpeg_parse->current_ts) && @@ -936,6 +1006,8 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux * mpeg_demux, mpeg_parse->current_ts - mpeg_demux->max_gap_tolerance); } + ret = CLASS (mpeg_demux)->combine_flows (mpeg_demux, outstream, ret); + return ret; } diff --git a/gst/mpegstream/gstmpegdemux.h b/gst/mpegstream/gstmpegdemux.h index c98c145..cdc77ee 100644 --- a/gst/mpegstream/gstmpegdemux.h +++ b/gst/mpegstream/gstmpegdemux.h @@ -88,14 +88,16 @@ typedef struct _GstMPEGDemuxClass GstMPEGDemuxClass; /* Information associated to a single MPEG stream. */ struct _GstMPEGStream { - gint type; - gint number; - GstPad *pad; - GstCaps *caps; - gint index_id; - gint size_bound; - GstClockTime cur_ts; - GstClockTimeDiff scr_offs; + gint type; + gint number; + GstPad *pad; + GstCaps *caps; + gint index_id; + gint size_bound; + GstClockTime cur_ts; + GstClockTimeDiff scr_offs; + GstFlowReturn last_flow; + guint buffers_sent; }; /* Extended structure to hold additional information for video @@ -183,6 +185,10 @@ struct _GstMPEGDemuxClass { guint size); + GstFlowReturn (*combine_flows) (GstMPEGDemux *mpeg_demux, + GstMPEGStream *stream, + GstFlowReturn flow); + GstFlowReturn (*process_private) (GstMPEGDemux *mpeg_demux, GstBuffer *buffer, guint stream_nr, diff --git a/gst/mpegstream/gstmpegparse.c b/gst/mpegstream/gstmpegparse.c index e992dc7..31292f6 100644 --- a/gst/mpegstream/gstmpegparse.c +++ b/gst/mpegstream/gstmpegparse.c @@ -407,6 +407,9 @@ gst_mpeg_parse_process_event (GstMPEGParse * mpeg_parse, GstEvent * event) gst_mpeg_packetize_flush_cache (mpeg_parse->packetize); break; } + case GST_EVENT_EOS: + GST_DEBUG_OBJECT (mpeg_parse, "EOS"); + /* fall through to default handler */ default: if (CLASS (mpeg_parse)->send_event) { ret = CLASS (mpeg_parse)->send_event (mpeg_parse, event); @@ -684,8 +687,8 @@ gst_mpeg_parse_event (GstPad * pad, GstEvent * event) static GstFlowReturn gst_mpeg_parse_chain (GstPad * pad, GstBuffer * buffer) { - GstMPEGParse *mpeg_parse = GST_MPEG_PARSE (gst_pad_get_parent (pad)); - GstFlowReturn result = GST_FLOW_ERROR; + GstMPEGParse *mpeg_parse = GST_MPEG_PARSE (GST_PAD_PARENT (pad)); + GstFlowReturn result; guint id; gboolean mpeg2; GstClockTime time; @@ -704,10 +707,10 @@ gst_mpeg_parse_chain (GstPad * pad, GstBuffer * buffer) if (result == GST_FLOW_RESEND) { /* there was not enough data in packetizer cache */ result = GST_FLOW_OK; - goto done; + break; } if (result != GST_FLOW_OK) - goto done; + break; id = GST_MPEG_PACKETIZE_ID (mpeg_parse->packetize); mpeg2 = GST_MPEG_PACKETIZE_IS_MPEG2 (mpeg_parse->packetize); @@ -749,7 +752,7 @@ gst_mpeg_parse_chain (GstPad * pad, GstBuffer * buffer) GST_DEBUG_OBJECT (mpeg_parse, "waiting for SCR"); gst_buffer_unref (buffer); result = GST_FLOW_OK; - goto done; + break; } /* Update the byte count. */ @@ -768,7 +771,7 @@ gst_mpeg_parse_chain (GstPad * pad, GstBuffer * buffer) GST_ELEMENT_ERROR (mpeg_parse, CORE, NEGOTIATION, (NULL), (NULL)); gst_buffer_unref (buffer); result = GST_FLOW_ERROR; - goto done; + break; } } @@ -819,13 +822,11 @@ gst_mpeg_parse_chain (GstPad * pad, GstBuffer * buffer) ", total since SCR: %" G_GINT64_FORMAT ", br: %" G_GINT64_FORMAT ", next SCR: %" G_GINT64_FORMAT, size, bss, br, mpeg_parse->next_scr); } - } while (!GST_FLOW_IS_FATAL (result)); - -done: - gst_object_unref (mpeg_parse); + } while (GST_FLOW_IS_SUCCESS (result)); - if (result == GST_FLOW_NOT_LINKED) - result = GST_FLOW_OK; + if (result != GST_FLOW_OK) { + GST_DEBUG_OBJECT (mpeg_parse, "flow: %s", gst_flow_get_name (result)); + } return result; } -- 2.7.4