GNOME Bugzilla – Bug 750761
inputselector: Handle different duration track selection
Last modified: 2015-06-23 13:31:28 UTC
Based on below ticket comment 10, Gstreamer can't handle long track -> short track (reach EOS) -> long track. https://bugzilla.gnome.org/show_bug.cgi?id=740949 But below test case has issue: long track->short track (reach EOS)->long track->seek. The issue is video sink can't received caps event after seek, so can't render video rightly. Do we need enable long track->short track->long track? or just send sticky event when seek if input selector already send EOS?
Whenever pads are switched in input-selector, it should send all sticky events. Independent of EOS or not. You mean that for a switch from an EOS stream to a non-EOS one it doesn't do that currently?
(In reply to Sebastian Dröge (slomo) from comment #1) > Whenever pads are switched in input-selector, it should send all sticky > events. Independent of EOS or not. > Sticky event is send in chain(), so switch to EOS track haven't chance to send sticky event. > You mean that for a switch from an EOS stream to a non-EOS one it doesn't do > that currently? Yes. pad can't send buffer or event after send EOS event. So can't send buffer when switch from an EOS stream to a non-EOS one. Seek will reset pad, so pad can send buffer after seek. but no sticky event send to video sink.
Ok, so yes. It should do the same sticky event sending then before outputting any new buffers.
So, video will freeze when switch from an EOS stream to a non-EOS one, but video will ok after seek. Can send one flush event to source pad when switch from an EOS stream to a non-EOS one?
Why does it freeze? It shouldn't, and sending flushes is not a good idea here
Because the EOS event went through before?
(In reply to Sebastian Dröge (slomo) from comment #6) > Because the EOS event went through before? Yes, inputselector's src pad is in EOS state. the pad will discard all pushed data and event, so video sink can't received buffer.
Perhaps the EOS flag should be cleared on STREAM_START as well?
Sounds reasonable, let's just do that before 1.6? :)
Yes, I will try it tomorrow.
By the way, when 1.6 will be released?
Another concern is all sink pad of input-selector will free run after switch to EOS track. So after switch form EOS track to non-EOS track, the output of input-selector will far behind media clock, which means long time video freeze. Will it happen?
Created attachment 305125 [details] [review] pad: Clear EOS flag after received STREAM_START event
Created attachment 305126 [details] [review] Support track switch from EOS track to non-EOS one.
I changed input-selector sync-mode to clock, so data buffer can blocked in input-selector when switch to EOS track. Is it ok?
Created attachment 305127 [details] [review] Support track switch from EOS track to non-EOS one.
(In reply to kevin from comment #15) > I changed input-selector sync-mode to clock, so data buffer can blocked in > input-selector when switch to EOS track. Is it ok? Or should switch sync-mode to clock when active pad reach EOS?
Review of attachment 305125 [details] [review]: ::: gst/gstpad.c @@ +4829,3 @@ + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_EOS); + remove_event_by_type (pad, GST_EVENT_EOS); + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_EOS); Why remove the flag twice? :) Please update the patch to only do it once, and also add the GST_LOG_OBJECT() from the other places here And shouldn't the code in push_event/send_event already have done this?
Review of attachment 305127 [details] [review]: ::: plugins/elements/gstinputselector.c @@ +114,2 @@ #define DEFAULT_SYNC_STREAMS TRUE +#define DEFAULT_SYNC_MODE GST_INPUT_SELECTOR_SYNC_MODE_CLOCK Don't change the default, it doesn't generally make sense to use the clock sync mode @@ +478,3 @@ case GST_EVENT_FLUSH_STOP: + /* Shouldn't use gst_selector_pad_reset () to avoid reset events_pending + * as still need send sticky event when switch to one EOS track and seek FLUSH_STOP should reset everything, EOS and SEGMENT sticky events (of this pad!) should go away at least. Others not so much Can you make gst_selector_pad_reset() take a boolean parameter for switching between these?
(In reply to kevin from comment #17) > (In reply to kevin from comment #15) > > I changed input-selector sync-mode to clock, so data buffer can blocked in > > input-selector when switch to EOS track. Is it ok? > > Or should switch sync-mode to clock when active pad reach EOS? Not sure what to do about that really, using clock sync mode is not going to work well either in general. We could block all inputs that wanted to go after EOS based on the running time and the segment I guess.
(In reply to Sebastian Dröge (slomo) from comment #18) > Review of attachment 305125 [details] [review] [review]: > > ::: gst/gstpad.c > @@ +4829,3 @@ > + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_EOS); > + remove_event_by_type (pad, GST_EVENT_EOS); > + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_EOS); > > Why remove the flag twice? :) Please update the patch to only do it once, > and also add the GST_LOG_OBJECT() from the other places here > Ok. > And shouldn't the code in push_event/send_event already have done this? store_sticky_event () will check the EOS flag and return without push_event.
(In reply to Sebastian Dröge (slomo) from comment #19) > Review of attachment 305127 [details] [review] [review]: > > ::: plugins/elements/gstinputselector.c > @@ +114,2 @@ > #define DEFAULT_SYNC_STREAMS TRUE > +#define DEFAULT_SYNC_MODE GST_INPUT_SELECTOR_SYNC_MODE_CLOCK > > Don't change the default, it doesn't generally make sense to use the clock > sync mode > Ok, still need consider how to handle this. we shouldn't block all input buffer when EOS, the reason is upstream will buffer too many data in streaming case. > @@ +478,3 @@ > case GST_EVENT_FLUSH_STOP: > + /* Shouldn't use gst_selector_pad_reset () to avoid reset > events_pending > + * as still need send sticky event when switch to one EOS track and > seek > > FLUSH_STOP should reset everything, EOS and SEGMENT sticky events (of this > pad!) should go away at least. Others not so much > Remove EOS and SEGMENT will do in pad. here just don't set events_pending to FALSE, so chain () can send the sticky event when seek in the case: long track->short track (reach EOS) -> seek to the point that the track have media data. > Can you make gst_selector_pad_reset() take a boolean parameter for switching > between these? Ok.
(In reply to kevin from comment #21) > (In reply to Sebastian Dröge (slomo) from comment #18) > > Review of attachment 305125 [details] [review] [review] [review]: > > > > ::: gst/gstpad.c > > @@ +4829,3 @@ > > + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_EOS); > > + remove_event_by_type (pad, GST_EVENT_EOS); > > + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_EOS); > > > > Why remove the flag twice? :) Please update the patch to only do it once, > > and also add the GST_LOG_OBJECT() from the other places here > > > Ok. > > And shouldn't the code in push_event/send_event already have done this? > store_sticky_event () will check the EOS flag and return without push_event. Good point! I'll push this patch if you change the other part :)
(In reply to kevin from comment #22) > (In reply to Sebastian Dröge (slomo) from comment #19) > > Review of attachment 305127 [details] [review] [review] [review]: > > > > ::: plugins/elements/gstinputselector.c > > @@ +114,2 @@ > > #define DEFAULT_SYNC_STREAMS TRUE > > +#define DEFAULT_SYNC_MODE GST_INPUT_SELECTOR_SYNC_MODE_CLOCK > > > > Don't change the default, it doesn't generally make sense to use the clock > > sync mode > > > Ok, still need consider how to handle this. we shouldn't block all input > buffer when EOS, the reason is upstream will buffer too many data in > streaming case. Well, what else would you do? The clock is not even necessarily running anymore after EOS was received by the sink.
(In reply to Sebastian Dröge (slomo) from comment #24) > (In reply to kevin from comment #22) > > (In reply to Sebastian Dröge (slomo) from comment #19) > > > Review of attachment 305127 [details] [review] [review] [review] [review]: > > > > > > ::: plugins/elements/gstinputselector.c > > > @@ +114,2 @@ > > > #define DEFAULT_SYNC_STREAMS TRUE > > > +#define DEFAULT_SYNC_MODE GST_INPUT_SELECTOR_SYNC_MODE_CLOCK > > > > > > Don't change the default, it doesn't generally make sense to use the clock > > > sync mode > > > > > Ok, still need consider how to handle this. we shouldn't block all input > > buffer when EOS, the reason is upstream will buffer too many data in > > streaming case. > > Well, what else would you do? The clock is not even necessarily running > anymore after EOS was received by the sink. Sink shouldn't receive EOS until all track reach EOS as streamsynchronizer will send GAP event to sink.
Created attachment 305185 [details] [review] Update patch based on review comments.
Created attachment 305267 [details] [review] Support track switch from EOS track to non-EOS one. sync-mode is active stream. All sink pad will be blocked if active pad reach EOS. So no any issue when switch EOS track to non-EOS one.
Comment on attachment 305185 [details] [review] Update patch based on review comments. This breaks some unit tests at least in core, I didn't try running the others.
Created attachment 305307 [details] [review] Update patch to pass unit test.
Comment on attachment 305307 [details] [review] Update patch to pass unit test. 66%: Checks: 3, Failures: 1, Errors: 0 elements/funnel.c:230:F:funnel simple:test_funnel_eos:0: Assertion 'num_eos == 3' failed Running suite(s): funnel 0:00:00.002299098 8389 0x1e4cb20 WARN funnel gstfunnel.c:256:void gst_funnel_release_pad(GstElement *, GstPad *):<funnel0> Failure pushing EOS 66%: Checks: 3, Failures: 1, Errors: 0 elements/funnel.c:230:F:funnel simple:test_funnel_eos:0: Assertion 'num_eos == 3' failed Makefile:3584: recipe for target 'elements/funnel.check' failed
Created attachment 305408 [details] [review] Fixed unit test fail. shouldn't remove EOS event in gst_pad_push_event_unchecked (). or below use case will lost EOS event. link pad and send EOS event. EOS event will trig push sticky event. link pad will change STREAM-START event to need resend. so EOS event will lost. Update the patch to pass unit test.
Created attachment 305464 [details] [review] Clear EOS flag after received STREAM_START event
Created attachment 305466 [details] [review] Unblock EOS wait when track switching.
Still has issue in input selector to process EOS event. below test case can reproduce issue: non-EOS track->EOS->track->non-EOS track->EOS track. The reason is input selector only block EOS event once. No EOS event after send output it. How to handle it?
Why can't you block EOS a second time just the same way it happened for the first time?
Created attachment 305526 [details] [review] Unblock EOS wait when track switching
Created attachment 305635 [details] [review] Support track switch from EOS track to non-EOS one
With those patch, player can switch from EOS to non-EOS on the fly. Notes: Changed input-selector sync-mode to clock. User experience is good. If sync with active stream, switch track from EOS to non-EOS will get bad user experience.
Review of attachment 305464 [details] [review]: ::: gst/gstpad.c @@ -5310,3 @@ - case GST_EVENT_RECONFIGURE: - if (GST_PAD_IS_SRC (pad)) - GST_OBJECT_FLAG_SET (pad, GST_PAD_FLAG_NEED_RECONFIGURE); Why do you move this code, this changes behaviour (see the GST_PAD_IS_FLUSHING() below)
Review of attachment 305635 [details] [review]: ::: plugins/elements/gstinputselector.c @@ +114,2 @@ #define DEFAULT_SYNC_STREAMS TRUE +#define DEFAULT_SYNC_MODE GST_INPUT_SELECTOR_SYNC_MODE_CLOCK As I told you, the clock mode does not really make sense generally and also will break things here We need to do something like blocking all non-EOS input buffers that are after the position of the EOS.
Created attachment 305892 [details] [review] Clear EOS flag after received STREAM_START event Update based on review.
Created attachment 305894 [details] [review] Support track switch from EOS track to non-EOS one. Update based on review. input-selector will block all sink pad after active track received EOS.
Review of attachment 305894 [details] [review]: ::: plugins/elements/gstinputselector.c @@ +459,3 @@ +static gboolean +gst_input_selector_eos_wait (GstInputSelector * self, GstSelectorPad * pad) +{ So this basically blocks EOS until the pad becomes the active pad, right? How does that make things work correctly, and how is it different to before? What would now still happen is that the active EOS is forwarded, and all other streams are blocked on EOS until they become active. But if the active stream is EOS, all buffers of the inactive streams are just dropped until they themselves go EOS. So while we now can switch from an EOS to a non-EOS stream, we will most likely still have dropped a lot of data instead of waiting at the current time. @@ +473,3 @@ + } + + gst_pad_push_event (self->srcpad, gst_event_new_eos ()); You should ideally forward exactly the EOS event that arrived here instead of a new one :) @@ -1016,3 @@ } - if (sel->eos) { I think sel->eos is completely unused now and could go away
(In reply to Sebastian Dröge (slomo) from comment #43) > Review of attachment 305894 [details] [review] [review]: > > ::: plugins/elements/gstinputselector.c > @@ +459,3 @@ > +static gboolean > +gst_input_selector_eos_wait (GstInputSelector * self, GstSelectorPad * pad) > +{ > > So this basically blocks EOS until the pad becomes the active pad, right? > How does that make things work correctly, and how is it different to before? > No, eos_wait () will block until flush or state change. So can switch track between EOS track and non-EOS track on the fly. So can switch track: EOS->non-EOS->EOS->non-EOS. Old code can't support this. > > What would now still happen is that the active EOS is forwarded, and all > other streams are blocked on EOS until they become active. But if the active > stream is EOS, all buffers of the inactive streams are just dropped until > they themselves go EOS. So while we now can switch from an EOS to a non-EOS > stream, we will most likely still have dropped a lot of data instead of > waiting at the current time. > All inactive track will blocked when active reach EOS. All inactive track is waiting active track advancing, but active no any data. > @@ +473,3 @@ > + } > + > + gst_pad_push_event (self->srcpad, gst_event_new_eos ()); > > You should ideally forward exactly the EOS event that arrived here instead > of a new one :) > No, eos_wait () may send EOS event many times. it will send eos event every time when switch track to EOS track. > @@ -1016,3 @@ > } > > - if (sel->eos) { > > I think sel->eos is completely unused now and could go away Still need it to indicate state change. eos_wait () need unblock. Maybe we can change the parameter name.
(In reply to kevin from comment #44) > (In reply to Sebastian Dröge (slomo) from comment #43) > > Review of attachment 305894 [details] [review] [review] [review]: > > > > ::: plugins/elements/gstinputselector.c > > @@ +459,3 @@ > > +static gboolean > > +gst_input_selector_eos_wait (GstInputSelector * self, GstSelectorPad * pad) > > +{ > > > > So this basically blocks EOS until the pad becomes the active pad, right? > > How does that make things work correctly, and how is it different to before? > > > No, eos_wait () will block until flush or state change. So can switch track > between EOS track and non-EOS track on the fly. So can switch track: > EOS->non-EOS->EOS->non-EOS. Old code can't support this. Ah right, but this will break gapless playback. It should also unblock if all streams are EOS. > > What would now still happen is that the active EOS is forwarded, and all > > other streams are blocked on EOS until they become active. But if the active > > stream is EOS, all buffers of the inactive streams are just dropped until > > they themselves go EOS. So while we now can switch from an EOS to a non-EOS > > stream, we will most likely still have dropped a lot of data instead of > > waiting at the current time. > > > All inactive track will blocked when active reach EOS. All inactive track is > waiting active track advancing, but active no any data. Where are they waiting in the code? I didn't see any waiting in the chain function related to this > > @@ +473,3 @@ > > + } > > + > > + gst_pad_push_event (self->srcpad, gst_event_new_eos ()); > > > > You should ideally forward exactly the EOS event that arrived here instead > > of a new one :) > > > No, eos_wait () may send EOS event many times. it will send eos event every > time when switch track to EOS track. You can send the same EOS event multiple times by getting a new reference before gst_pad_push_event()
(In reply to Sebastian Dröge (slomo) from comment #45) > (In reply to kevin from comment #44) > > (In reply to Sebastian Dröge (slomo) from comment #43) > > > Review of attachment 305894 [details] [review] [review] [review] [review]: > > > > > > ::: plugins/elements/gstinputselector.c > > > @@ +459,3 @@ > > > +static gboolean > > > +gst_input_selector_eos_wait (GstInputSelector * self, GstSelectorPad * pad) > > > +{ > > > > > > So this basically blocks EOS until the pad becomes the active pad, right? > > > How does that make things work correctly, and how is it different to before? > > > > > No, eos_wait () will block until flush or state change. So can switch track > > between EOS track and non-EOS track on the fly. So can switch track: > > EOS->non-EOS->EOS->non-EOS. Old code can't support this. > > Ah right, but this will break gapless playback. It should also unblock if > all streams are EOS. Ok, I will check it. > > > > > What would now still happen is that the active EOS is forwarded, and all > > > other streams are blocked on EOS until they become active. But if the active > > > stream is EOS, all buffers of the inactive streams are just dropped until > > > they themselves go EOS. So while we now can switch from an EOS to a non-EOS > > > stream, we will most likely still have dropped a lot of data instead of > > > waiting at the current time. > > > > > All inactive track will blocked when active reach EOS. All inactive track is > > waiting active track advancing, but active no any data. > > Where are they waiting in the code? I didn't see any waiting in the chain > function related to this gst_input_selector_wait_running_time () will be blocked. > > > > @@ +473,3 @@ > > > + } > > > + > > > + gst_pad_push_event (self->srcpad, gst_event_new_eos ()); > > > > > > You should ideally forward exactly the EOS event that arrived here instead > > > of a new one :) > > > > > No, eos_wait () may send EOS event many times. it will send eos event every > > time when switch track to EOS track. > > You can send the same EOS event multiple times by getting a new reference > before gst_pad_push_event() Ok, will update later.
Sounds good, thanks :)
Created attachment 305917 [details] [review] Support track switch from EOS track to non-EOS one. Update based on review. not much test as I haven't test environment on hand now.
Thanks, all merged in time for 1.5.2 :)