GNOME Bugzilla – Bug 638168
textoverlay: don't return wrong-state when stopping while in text chain
Last modified: 2012-05-24 13:56:29 UTC
If the text chain function is called and waits for the current text to disappear, and if a state change from PAUSED to READY happens meanwhile (such as when subtitles streams are switched in Totem), the text pad will be set to flushing, and textoverlay will return wrong-state, which will cause the video to freeze. This patch fixes this, but I don't like it, it feels wrong somehow, but I don't quite know why. So I'm putting it there for comments.
Created attachment 177124 [details] [review] textoverlay: don't return wrong-state when stopping while in text chain
Yes, I don't think this is the correct thing to do either. Could you provide a small testcase to reproduce this behaviour? Changing subtitle streams shouldn't really set anything to flushing... and if it does, could you check *who* does it?
The flushing was caused by _pad_blocked_cb removing the subtitle related elements, the lines following /* Now the interesting parts are done: subtitle overlaying! */. I'll check that I can still get it to happen.
So, test case: - Remove the tiger plugin (set its rank to NONE), so katedec + textoverlay get used. - Get the Stephen Fry "GNU birthday" video - Play it in Totem - Select a subtitles language - Select the second - Wait a bit When selecting a language while one is currently selected, the code will go through the wrong-state return path. Waiting a bit will cause the pipeline to stop (after queues dry out I think). The Stephen Fry video is good at showing this as it's got subtitles all the time, but it "should" happen with any video that's got at least two text tracks.
Sorry, forgot the URL: http://www.gnu.org/fry/happy-birthday-to-gnu-download.html
http://stallman.org/fry/Stephen_Fry-Happy_Birthday_GNU-hq_600px_780kbit.ogv even.
Sebastian, do you have any more ideas about this ? AFAICT, the pipeline bits are destroyed and recreated because other elements might be needed in their stead (eg, if the two subtitle streams have different formats, they might need different elements). I had another quick look at this, but I've no real idea what to do here.
For the record, this case can also happen when seeking. Video and text pads are set to flushing, the current text is popped, and the lock signal broadcast. This wakes up the text chain, which founds both pads flushing and returns wrong-state. This causes the (playbin2) queue upstream to panic and stop.
Created attachment 213764 [details] [review] patch
Ok, I have done some investigations on this issue and here goes my findings: From my tests, using the patch from comment #2 makes the playback stops with "internal error" in gstqueue.c at: if (eos && (ret == GST_FLOW_NOT_LINKED || ret < GST_FLOW_UNEXPECTED)) { GST_ELEMENT_ERROR (queue, STREAM, FAILED, (_("Internal data flow error.")), ("streaming task paused, reason %s (%d)", gst_flow_get_name (ret), ret)); What happens under the hood: - video starts playing, default subtitle selected and everything working - user selects a subtitle with ID - playbin2 calls set_text_stream(ID), which then sends a custom flush event to subtitle_overlay - subtitle_overlay then calls _pad_blocked_cb leading to the removal of the current text_overlay (pango) and the setup of a new one - while setting the new overlay, subtitle_overlay sends a newsegment to the subtitle pad (subparse). Currently this newsegment event was being sent in BYTES format and being ignored by subparse, so the patch at comment #9 makes sure the events are sent in TIME format. To make everything work, subparse should send the newsegment event to text_overlay (pango), which would then read the new buffer, etc. But gst_sub_parse_chain is never called with a new buffer (same for gst_subtitle_overlay_subtitle_sink_chain). So the patch on comment #9 includes: - Attempt to fix the issue with BYTES vs TIME format by doing the conversion before sending the events - Set subparser->need_segment to true on the NEWSEGMENT handling to make sure the event is relayed on gst_sub_parse_chain - Also includes the reverse patch from comment #2 to fix the "internal error" issue We still need to figure out why the new subtitle buffer is never parsed (gst_sub_parse_chain never called). https://bugzilla.gnome.org/show_bug.cgi?id=600648 may be related to this issue.
Thanks for looking into that. The pad may not be linked (so the _get_peer may return NULL), you may want to default to a 0,-1,0 segment in this case. Also res &= may be better as res = res && to avoid calling subsequent conversions if the first fails, as they likely will all do at once. May not matter much though. _generate_update_newsegment_event might also want to skip conversion if the format is already TIME, as your other code already does. -1 could be changed to GST_CLOCK_TIME_NONE. The "Subtitle segment format %s not TIME, converting" message seems to be output on checking self->subtitle_segment.format, I think it should be format. The patch from comment 2 was not applied, so no need to revert it in your patch. Also can you make the new patch as: git format-patch -1 (assuming this patch is the last one you committed to the branch you're on). Thanks
Review of attachment 213764 [details] [review]: Looks good, other than Vincent's comments: ::: ext/pango/gsttextoverlay.c @@ +1996,2 @@ GST_OBJECT_UNLOCK (overlay); + ret = GST_FLOW_WRONG_STATE; This is just reverting Vincent's patch, right? Which is not in GStreamer GIT ::: gst/playback/gstsubtitleoverlay.c @@ +580,3 @@ + /* always push newsegment with format TIME */ + tformat = GST_FORMAT_TIME; + res = gst_pad_query_convert (peer, segment->format, segment->accum, &tformat, &taccum); Only do conversions if not in TIME format already. Conversion of -1 values is not necessary either. And the peer could be NULL. @@ +2146,3 @@ + + tformat = GST_FORMAT_TIME; + res = gst_pad_query_convert (peer, format, start, &tformat, &tstart); Same comments as above ::: gst/subparse/gstsubparse.c @@ +1643,3 @@ + /* make sure the newsegment is pushed to the srcpad in gst_sub_parse_chain */ + self->need_segment = TRUE; Is subparse creating a new event from the segment or passing through the upstream event if in TIME format? If it creates a new one, it really shouldn't do that for several reasons
Created attachment 213815 [details] [review] gstsuboverlay: Convert NewSegment events to always be in the TIME format.
Created attachment 213816 [details] [review] gstsubparse: Make sure newsegment events not in TIME format are relayed.
Thanks for the review. I updated and split the patches (attached on comment #13 and comment #14).
It looks like the 'subqueue' element is still with its srcresult set to wrong-state when the first buffer from the newly selected subtitle arrives, causing the whole new subtitle branch to stop. Shouldn't a flush-start/stop pair happen to to clean the state of 'subqueue'? Or was the subtitle specific flush supposed to trigger some cleanup?
Flush-stop should clear the wrong-state on the queue elements The subtitle specific flush is supposed to flush the current subtitle from the video. This is e.g. necessary if you currently display a subtitle that will be displayed for the next two minutes to allow switching to the newly selected subtitle stream before the duration of the previous buffer finished.
Review of attachment 213816 [details] [review]: Looks good but as discussed on IRC, subparse should also passthrough newsegment events in TIME format (from the chain function) instead of creation a new event. This is necessary to preserve the sequence number and the marker hack used by subtitleoverlay
Review of attachment 213815 [details] [review]: Looks good
Created attachment 213880 [details] [review] gstplaysink: Properly reset chain when receiving a custom flush event to avoid wrong-state Another issue was that while trying to change subtitles the gstplaysink queue sometimes would get stuck in wrong-state. This patch attempts to fix it by setting up a new event and chain functions to the gstplaysink text sink and resetting the flow to FLOW_OK if we got a custom flush event from gsttextoverlay (happens when changing streams).
Review of attachment 213816 [details] [review]: This patch is not required.
The remaining issue it the time it takes to switch the subtitle, mostly because the buffers of the newly selected stream have already been pushed by the decoder and dropped because their pad at the input-selector wasn't active. When you switch, you need to wait until the next subtitle buffer arrives, and this new buffer might be lots of seconds ahead as the decoders are a few buffers ahead of the currently rendered text, a few buffers can be a lot of time for sparse streams.
Review of attachment 213880 [details] [review]: ::: gst/playback/gstplaysink.c @@ +1863,3 @@ + playsink->text_flush = FALSE; + GST_PLAY_SINK_UNLOCK (playsink); + gst_pad_send_event (pad, gst_event_new_flush_stop ()); This might cause problems if the flush-stop event reaches the other side of the subtitleoverlay, due to resetting the segments and other state. This doesn't happen here? Also should this really be gst_pad_send_event() and not gst_pad_push_event()?
(In reply to comment #23) > Review of attachment 213880 [details] [review]: > > ::: gst/playback/gstplaysink.c > @@ +1863,3 @@ > + playsink->text_flush = FALSE; > + GST_PLAY_SINK_UNLOCK (playsink); > + gst_pad_send_event (pad, gst_event_new_flush_stop ()); > > This might cause problems if the flush-stop event reaches the other side of the > subtitleoverlay, due to resetting the segments and other state. This doesn't > happen here? I haven't seen any side effects so far, but it might be good to be careful here and drop it before it leaves the subtitleoverlay. > > Also should this really be gst_pad_send_event() and not gst_pad_push_event()? gst_pad_send_event looks correct to me, it is a sink pad and the event is going downstream.
Created attachment 214004 [details] [review] gstsuboverlay: Lets not propagate the flush-stop used to reset the queue when changing subtitles.
(In reply to comment #23) > Review of attachment 213880 [details] [review]: > > ::: gst/playback/gstplaysink.c > @@ +1863,3 @@ > + playsink->text_flush = FALSE; > + GST_PLAY_SINK_UNLOCK (playsink); > + gst_pad_send_event (pad, gst_event_new_flush_stop ()); > > This might cause problems if the flush-stop event reaches the other side of the > subtitleoverlay, due to resetting the segments and other state. This doesn't > happen here? I added a patch at comment #25 that attempts to fix this.
Created attachment 214010 [details] [review] gstplaysink: Re-sync queue after sending flush-stop.
(In reply to comment #26) > (In reply to comment #23) > > Review of attachment 213880 [details] [review] [details]: > > > > ::: gst/playback/gstplaysink.c > > @@ +1863,3 @@ > > + playsink->text_flush = FALSE; > > + GST_PLAY_SINK_UNLOCK (playsink); > > + gst_pad_send_event (pad, gst_event_new_flush_stop ()); > > > > This might cause problems if the flush-stop event reaches the other side of the > > subtitleoverlay, due to resetting the segments and other state. This doesn't > > happen here? > I added a patch at comment #25 that attempts to fix this. Updated patch at comment #27 instead.
Created attachment 214023 [details] [review] playsink: do not store more than a second of subtitles Use a shorter queue for subtitles to avoid switches for subtitles taking longer than they already take.
This last patch I attached at comment 29 reduces the delay when switching subtitles, but it still takes too long. For example, switching the subtitles at video from Comment 6 at 6s makes the subtitles disappear and only appear again at 14s, that's a lot. What happens from input-selector: 0) Subtitle-0 is active, pushing buffers (there is a buffer already at textoverlay, another stored at subqueue and another one blocked while waiting to enter the subqueue, so input-selector is always 3 buffers behind) 1) Subtitle-0 buffer pushed with ts=11s 2) Switch to subtitle-1 (video on screen is still at 6s) 3) Subtitles disappear at screen 4) input-selector closes the current segment by pushing a newsegment with start=14s * 5) subtitle-1 pushes a buffer with ts=11s (gets dropped at textoverlay) 6) subtitle-1 pushes a buffer with ts=14s (rendered, finally!) * The stream closes at 14s when switching because oggdemux sends some kind of synchronization newsegment and input selector keeps that event. So far I don't have an idea of how to make it switch faster, comments are welcome.
Review of attachment 214010 [details] [review]: ::: gst/playback/gstplaysink.c @@ +633,3 @@ + + _playsink_reset_segment_event_marker_id = + g_quark_from_static_string ("gst-playsink-reset-segment-event-marker"); Please do this in class_init, only has to be done once @@ +1895,3 @@ + if (format != GST_FORMAT_TIME) { + GST_ERROR_OBJECT (pad, "Newsegment event in non-time format: %s", + gst_format_get_name (format)); You should not ignore non-TIME segments here but just handle all formats equally @@ +1933,3 @@ + + /* make queue drop all cached data. + * This event will be dropped inside subtitleoverlay. */ I think it would be cleaner to send the event from outside subtitleoverlay and also drop them from outside subtitleoverlay. This way we now have a quite weird, asymmetric interaction between playsink and subtitleoverlay. Letting it go through subtitleoverlay has no negative side effects because we update the segments afterwards again. (also, isn't subtitleoverlay flushing itself anyway?)
(In reply to comment #30) > This last patch I attached at comment 29 reduces the delay when switching > subtitles, but it still takes too long. > > For example, switching the subtitles at video from Comment 6 at 6s makes the > subtitles disappear and only appear again at 14s, that's a lot. > > What happens from input-selector: > > 0) Subtitle-0 is active, pushing buffers (there is a buffer already at > textoverlay, another stored at subqueue and another one blocked while waiting > to enter the subqueue, so input-selector is always 3 buffers behind) > 1) Subtitle-0 buffer pushed with ts=11s > 2) Switch to subtitle-1 (video on screen is still at 6s) > 3) Subtitles disappear at screen > 4) input-selector closes the current segment by pushing a newsegment with > start=14s * > 5) subtitle-1 pushes a buffer with ts=11s (gets dropped at textoverlay) > 6) subtitle-1 pushes a buffer with ts=14s (rendered, finally!) > > > * The stream closes at 14s when switching because oggdemux sends some kind of > synchronization newsegment and input selector keeps that event. I don't see an apparent solution for 0.10 (but for 0.11). What's the filler/update segment that is sent by oggdemux? Does it really advance the running time to 14s already? The segments could be adjusted a bit in subtitleoverlay maybe, changing them for the elements inside the accept new text buffers that happen at the current video time (and before the segment start as given by oggdemux). But this would become even more hackish then
> > * The stream closes at 14s when switching because oggdemux sends some kind of > > synchronization newsegment and input selector keeps that event. Is this the stream syncing code in gst_ogg_demux_sync_streams ? AFAIK, this should push segment updates to discontinuous streams as continuous streams progress, so should not generate timestamps larger than wherever A/V currently is.
Created attachment 214114 [details] [review] gstplaysink: Re-sync queue after sending flush-stop. (In reply to comment #31) > Review of attachment 214010 [details] [review]: > > ::: gst/playback/gstplaysink.c > @@ +633,3 @@ > + > + _playsink_reset_segment_event_marker_id = > + g_quark_from_static_string ("gst-playsink-reset-segment-event-marker"); > > Please do this in class_init, only has to be done once Done > @@ +1895,3 @@ > + if (format != GST_FORMAT_TIME) { > + GST_ERROR_OBJECT (pad, "Newsegment event in non-time format: %s", > + gst_format_get_name (format)); > > You should not ignore non-TIME segments here but just handle all formats > equally Done > @@ +1933,3 @@ > + > + /* make queue drop all cached data. > + * This event will be dropped inside subtitleoverlay. */ > > I think it would be cleaner to send the event from outside subtitleoverlay and > also drop them from outside subtitleoverlay. This way we now have a quite > weird, asymmetric interaction between playsink and subtitleoverlay. Letting it > go through subtitleoverlay has no negative side effects because we update the > segments afterwards again. (also, isn't subtitleoverlay flushing itself > anyway?) Done. The event is being handled completely inside playsink now and the events marked will never leave the playsink:tbin.
Created attachment 214142 [details] [review] playbin2: fix subtitle only seeks when switching to external subs Sending a non-flushing seek might not be enough for switching to an external sub that has already been used because the flushes are needed to reset the state of its decodebin's queue. For example, if the subtitle is short enough, the queue might get and EOS and keep its 'unexpected' return state. If the user switches to another subtitle and back to the external one, the buffers won't get past the queue. This patch fixes this by adding the flush flag to the seek and preventing that this flush leaves the suburidecodebin.
Created attachment 214151 [details] [review] gstplaysink: Re-sync queue after sending flush-stop. A new updated patch replacing the old one and fixing a bug where the newsegment events were never sent due to the segment being reset on flush-stop.
(In reply to comment #33) > > > * The stream closes at 14s when switching because oggdemux sends some kind of > > > synchronization newsegment and input selector keeps that event. > > Is this the stream syncing code in gst_ogg_demux_sync_streams ? > AFAIK, this should push segment updates to discontinuous streams as continuous > streams progress, so should not generate timestamps larger than wherever A/V > currently is. ...where A/V is at the demuxer currently. Due to queueing this could be different to where it is further downstream, and as subtitle streams are sparse the filler newsegment event can run ahead of the A/V data and reach inputselector earlier. Might be possible prevent this running ahead inside multiqueue though, it already syncs the newsegment events but probably not good enough yet.
Review of attachment 214142 [details] [review]: Looks good and makes sense, only that most elements that implement seeking don't handle the sequence numbers correctly (yet, and this should be really fixed!) :( As an alternative you could drop all flush events until you received the first non-update newsegment event... but that might not be very reliable either.
Review of attachment 214151 [details] [review]: Looks good but: ::: gst/playback/gstplaysink.c @@ +226,1 @@ gboolean text_flush; This depends on the previous patch in attachment #213880 [details]? Could you merge the two patches?
Created attachment 214180 [details] [review] gstplaysink: Properly reset chain when receiving a custom flush event. (In reply to comment #39) > Review of attachment 214151 [details] [review]: > > Looks good but: > > ::: gst/playback/gstplaysink.c > @@ +226,1 @@ > gboolean text_flush; > > This depends on the previous patch in attachment #213880 [details]? Could you merge the > two patches? Thanks for the review. Attaching the new merged patch.
Comment on attachment 214142 [details] [review] playbin2: fix subtitle only seeks when switching to external subs Actually this should be fine, basesrc handles it correctly and there's nothing else used for the suburi (unless you do unsupported things like taking subtitles from some random container file).
commit b425a9bb59159537f3b92b8e3b9273890494134e commit ce4b69bf22155b18ae3795e1c100debaef298718 commit 4b8fc402e269c083f4ff33609572c0f13630aef9 commit d63ee44a7e1481d6520ce8e31782fdb379eb84fb Pushed the patches we had, now there is only the delay when switching remaining.
Created attachment 214508 [details] [review] gstplaybin2: Properly change subtitles. Modify playbin to indicate when a stream change start/stop, so that playsink can properly send a flush stop and ignore the WRONG_STATE coming from textoverlay.
Created attachment 214509 [details] [review] gstinputselector: Properly sync when changing streams. Update input selector so that on "sync-stream" mode it makes sure no buffer is ignored on stream changes.
The patches from comment #43 and comment #44 should make stream changes work properly. Ps.: The patch from comment #43 also includes a reversal of patch from comment #1 and won't work otherwise.
Created attachment 214513 [details] [review] gstplaybin2: Properly change subtitles. Actually upstream does not have patch from comment #1 applied, so attaching a new patch without the commit reversal. In any case the patch from comment #1 does not work with the new patches and must not be applied if the new patches are.
Review of attachment 214509 [details] [review]: ::: plugins/elements/gstinputselector.c @@ +522,3 @@ */ + active_selpad = GST_SELECTOR_PAD (active_sinkpad); + forward = (active_selpad->eos && !active_selpad->eos_sent); Why do you get the pad again here? As you hold the lock over the complete switch statement the active pad couldn't have changed here @@ -576,3 @@ - if (pad != active_sinkpad) - goto not_active; I don't think this makes sense, passthrough allocations should only be done on the activated pad. @@ +693,2 @@ else + clock_time = GST_CLOCK_TIME_NONE; You should put the change the uses the clock time for syncing into another commit, separated from the queueing commit. Also the syncing to the clock should be optional and the option to sync to the running time should be kept as it's generally useful too @@ +775,3 @@ + /* the buffer is still valid if its duration is valid and the + * timestamp + duration is >= time, or if its duration is invalid + * and the timestamp is >= time */ What about buffers without valid timestamp? @@ +832,3 @@ + GstBuffer *old_buffer; + + old_buffer = g_queue_pop_tail (selpad->buffers); Pushing of all the old buffers should only happen for the first call to the chain function after a stream switch. The size of the queue in the element should also be configurable and have a maximum in sizes and time. @@ +1311,3 @@ /* no stop time given, get the latest running_time on the active pad to * close and open the new segment */ + stop_time = gst_selector_pad_get_running_time (old); You get the stop time from the element's running time but the start time from the clock. This could even cause start_time to be larger than stop_time
(In reply to comment #47) > Review of attachment 214509 [details] [review]: > > ::: plugins/elements/gstinputselector.c > @@ +522,3 @@ > */ > + active_selpad = GST_SELECTOR_PAD (active_sinkpad); > + forward = (active_selpad->eos && !active_selpad->eos_sent); > > Why do you get the pad again here? As you hold the lock over the complete > switch statement the active pad couldn't have changed here I forgot to change it when changing the event to always lock over the switch statement, will change. > @@ -576,3 @@ > > - if (pad != active_sinkpad) > - goto not_active; > > I don't think this makes sense, passthrough allocations should only be done on > the activated pad. The issue is that we cannot return NOT_LINKED in buffer_alloc if we did not push anything to the active pad yet as it was doing, will update here. > @@ +693,2 @@ > else > + clock_time = GST_CLOCK_TIME_NONE; > > You should put the change the uses the clock time for syncing into another > commit, separated from the queueing commit. Also the syncing to the clock > should be optional and the option to sync to the running time should be kept as > it's generally useful too So with the patch we only use the clock in "sync-streams" mode and without using it it wouldn't work, should I really keep the old behaviour that does not work as expected? I don't think we should and also no split should be required in the patch if that is the case > @@ +775,3 @@ > + /* the buffer is still valid if its duration is valid and the > + * timestamp + duration is >= time, or if its duration is invalid > + * and the timestamp is >= time */ > > What about buffers without valid timestamp? buffers with invalid timestamps should be ignored always, I will update. > @@ +832,3 @@ > + GstBuffer *old_buffer; > + > + old_buffer = g_queue_pop_tail (selpad->buffers); > > Pushing of all the old buffers should only happen for the first call to the > chain function after a stream switch. This should be true already, will double check. > The size of the queue in the element > should also be configurable and have a maximum in sizes and time. Will add it. > @@ +1311,3 @@ > /* no stop time given, get the latest running_time on the active pad to > * close and open the new segment */ > + stop_time = gst_selector_pad_get_running_time (old); > > You get the stop time from the element's running time but the start time from > the clock. This could even cause start_time to be larger than stop_time The stop time is used for the CLOSED newsegment and the start time for the UPDATE newsegment, so I don't think there is any issue here.
Created attachment 214630 [details] [review] gstinputselector: Properly sync when changing streams. Here goes another round. I fixed some issues with the patch and also made some improvements. Comments bellow. (In reply to comment #48) > (In reply to comment #47) > > Review of attachment 214509 [details] [review] [details]: > > > > ::: plugins/elements/gstinputselector.c > > @@ +522,3 @@ > > */ > > + active_selpad = GST_SELECTOR_PAD (active_sinkpad); > > + forward = (active_selpad->eos && !active_selpad->eos_sent); > > > > Why do you get the pad again here? As you hold the lock over the complete > > switch statement the active pad couldn't have changed here > I forgot to change it when changing the event to always lock over the switch > statement, will change. Actually, after a new check active_selpad was never set before in this method, only active_sinkpad, that is why we are retrieving it there. > > @@ -576,3 @@ > > > > - if (pad != active_sinkpad) > > - goto not_active; > > > > I don't think this makes sense, passthrough allocations should only be done on > > the activated pad. > The issue is that we cannot return NOT_LINKED in buffer_alloc if we did not > push anything to the active pad yet as it was doing, will update here. Fixed > > @@ +693,2 @@ > > else > > + clock_time = GST_CLOCK_TIME_NONE; > > > > You should put the change the uses the clock time for syncing into another > > commit, separated from the queueing commit. Also the syncing to the clock > > should be optional and the option to sync to the running time should be kept as > > it's generally useful too > So with the patch we only use the clock in "sync-streams" mode and without > using it it wouldn't work, should I really keep the old behaviour that does not > work as expected? I don't think we should and also no split should be required > in the patch if that is the case Made some changes on how this works now. Also kept full-compatibility with the old non "sync-streams" behaviour. > > @@ +775,3 @@ > > + /* the buffer is still valid if its duration is valid and the > > + * timestamp + duration is >= time, or if its duration is invalid > > + * and the timestamp is >= time */ > > > > What about buffers without valid timestamp? > buffers with invalid timestamps should be ignored always, I will update. Fixed. > > @@ +832,3 @@ > > + GstBuffer *old_buffer; > > + > > + old_buffer = g_queue_pop_tail (selpad->buffers); > > > > Pushing of all the old buffers should only happen for the first call to the > > chain function after a stream switch. > This should be true already, will double check. Actually it was not, but now it is :). > > The size of the queue in the element > > should also be configurable and have a maximum in sizes and time. > Will add it. Not yet. Do we really need this even with the cached buffers being discarded when old? I don't think we should as this may break the synchronization, but if so what should we use as defaults here? > > @@ +1311,3 @@ > > /* no stop time given, get the latest running_time on the active pad to > > * close and open the new segment */ > > + stop_time = gst_selector_pad_get_running_time (old); > > > > You get the stop time from the element's running time but the start time from > > the clock. This could even cause start_time to be larger than stop_time > The stop time is used for the CLOSED newsegment and the start time for the > UPDATE newsegment, so I don't think there is any issue here. No change here anymore for non "sync-streams" mode, using a different method when in "sync-streams" mode. The patch is a big improvement over the last one, as it is less intrusive, keeping full compatibility with old behaviour for non "sync-streams" mode, and it guarantees that the proper buffer newsegments are sent when sending cached buffers.
Created attachment 214633 [details] [review] gstinputselector: Properly sync when changing streams. The same patch as the one from comment #49 but with a fix to a wrong placed break statement in gst_input_selector_cleanup_old_cached_buffers :)
Review of attachment 214633 [details] [review]: The sync mode should be separate from the queueing because both are useful without the other too. The queue size should have a maximum to keep memory usage lower in bad cases and it will still improve stream switching latency then because we at least don't start at the very end of the queue. Also might be necessary to keep key frames in mind here, after dropping a buffer you must not push anything before a keyframe probably. ::: plugins/elements/gstinputselector.c @@ +381,3 @@ +{ + GstSelectorPadCachedBuffer *cached_buffer = + g_new (GstSelectorPadCachedBuffer, 1); Use g_slice_new() and g_slice_free() here... less memory fragmentation @@ +720,3 @@ + clock = gst_element_get_clock (GST_ELEMENT_CAST (sel)); + if (clock) + clock_time = gst_clock_get_time (clock); The old mode that doesn't use the clock but only the segment information and buffer timestamps should be preserved. It's useful in a lot of situations. Also did you check that buffers are not too late now (by waiting for the clock they should all be too late, unless we're running a live pipeline). @@ +808,3 @@ + if (GST_BUFFER_DURATION_IS_VALID (cached_buffer->buffer)) + running_time = GST_BUFFER_TIMESTAMP (cached_buffer->buffer) + + GST_BUFFER_DURATION (cached_buffer->buffer); This is not the running time. You need to convert with gst_segment_to_running_time() here @@ +859,3 @@ + /* If we have no valid timestamp we can't sync this buffer */ + if (!GST_BUFFER_TIMESTAMP_IS_VALID (buf)) + goto ignore; You should only ignore the syncing here, nothing else (should still go into the queue, pushed downstream, etc)
Created attachment 214676 [details] [review] gstinputselector: Properly sync when changing streams. New patch with comments addressed and working with both subtitles and audio stream switches.
Created attachment 214694 [details] [review] gstinputselector: Properly sync when changing streams. I was reviewing my own patch from comment #52, and found an infinite loop if there are cached buffers with invalid timestamps when cleaning up old cached buffers. This patch is the same as the patch from comment #52 but with the fix applied.
Created attachment 214697 [details] [review] gstinputselector: Properly sync when changing streams. ... and a fix to a possible leak with patch from comment #53
Segfaulting when using gst-plugins-base/tests/examples/playback on the mewmew sample clip http://matroska.free.fr/samples/mewmew/downloads/mewmew-vorbis-ssa.mkv : Program received signal SIGSEGV, Segmentation fault.
+ Trace 230258
Thread 140737038296832 (LWP 26419)
Created attachment 214765 [details] [review] gstinputselector: Properly sync when changing streams. (In reply to comment #55) > Segfaulting when using gst-plugins-base/tests/examples/playback on the mewmew > sample clip > http://matroska.free.fr/samples/mewmew/downloads/mewmew-vorbis-ssa.mkv : > > Program received signal SIGSEGV, Segmentation fault. > Sorry, this segfault was related to invalid buffers, the latest changes. Fixed now.
Created attachment 214835 [details] [review] gstinputselector: Properly sync when changing streams. This patch modifies the last one to only sync using the clock, no stream time fallback anymore as it wasn't reliable to use stream time for sync, some elements would return wrong time (like matroskademux). It requires the (to be attached) patches to playbin and assrender to be applied in order to work. With all 3 patches applied, changing audio and subtrack streams should be instantaneous. Tested with the following videos: For subtitles change: - http://docs.gstreamer.com/media/sintel_trailer-480p.ogv with the suburi http://docs.gstreamer.com/media/sintel_trailer_gr.srt - http://matroska.free.fr/samples/mewmew/downloads/mewmew-vorbis-ssa.mkv - http://www.gnu.org/fry/happy-birthday-to-gnu-download.html For audio change: - http://docs.gstreamer.com/media/sintel_cropped_multilingual.webm
Created attachment 214836 [details] [review] gstplaybin2: Also send flush events when changing audio
Created attachment 214837 [details] [review] gstassrender: Refactoring.
(In reply to comment #59) > Created an attachment (id=214837) [details] [review] > gstassrender: Refactoring. The changes here are based on pango/textoverlay and are tested.
Closing this one, the original bug is done and remaining changes are improvements handled in bug #676739.