Created attachment 291860 [details] [review]
patch to fix the issue.

Comment on attachment 291860 [details] [review]
patch to fix the issue.

This is not correct as it would send a serialized event from the application thread.

Instead the best would probably be to send EOS from the EOS of the other track

If send EOS until other track reach EOS. How audio sink finish preroll if need PAUSE during wait other track reach EOS?

Ah, I see what you mean. Hmm... I think input-selector should block when it receives an EOS event for an unselected track. And when this track is then later selected, the EOS event will be sent directly.

Yes, my patch is send the sticky event when the activated track reach EOS. If you can't accept my patch. How to send the EOS event? Who will trick the send?

As I said, your patch is wrong because it will send the EOS event from the application thread or whoever switches the pad.

What will need to happen is what I outlined in comment 4. The sinkpads of input-selector need to block on EOS (and handle flushing and everything correctly), and when you switch to one of those sinkpads it would be unblocked and then directly forward the EOS event.

Add one semaphore to block EOS event for unactive track. Post the semaphore when the track actived. Is it right?

Yes, a GCond would be best... there might even be one already that you could reuse for this.

I still have concern. How to handle select long track -> EOS track (send out the holded EOS event) -> long track -> EOS track (how to handle this?).

You won't handle that at all. The input-selector is EOS once a currently active track went EOS, and it won't be possible to switch to any other track (EOS or not) afterwards.

Making that work would require flushing the downstream of input-selector, but which would have to be done in a way that ensures no data is flushed away. And that's far from trivial.

Got it, will generate patch later.

I tried block EOS event, seems it will cause block whole pipeline. Our test clip is one audio track send EOS at the begin without any valid data.

Is this inside playbin? I think playbin (actually decodebin) is also supposed to aggregate all EOS first and only then forward them all. It currently does not do that.

What is the backtrace of all threads and can you share your patch?

Created attachment 292742 [details] [review]
patch for block method.

Bug #738285 has an initial patch to send EOS in decodebin after all pads are drained. Related to that is also bug #738064 and bug #740045

I think it should be possible to reuse the existing condition variable for that. It is also used to block unselected sinkpads until they're active (or have fallen behind too much compared to the active one... but EOS means it can't fall behind anymore as it's too far ahead already).


How does it deadlock for you, what's the backtrace of all threads?

Existing condition variable used for element and will be signaled all the time when playbacking. Can it be used for pad EOS blocking?
I will check why pipeline blocked. Will back after I got the reason.

You could check all the conditions after it stopped waiting, and if they are not met you would just wait again.

Ok. The root cause of block is haven't release lock when blocked on EOS.
Will upload patch after more test. Thanks.

Created attachment 292788 [details] [review]
Updated patch. Use block method. Please review.

Review of attachment 292788 [details] [review]:

::: plugins/elements/gstinputselector.c
@@ +562,3 @@
         forward = (active_selpad->eos && !active_selpad->eos_sent);
         active_selpad->eos_sent = TRUE;
+        gst_input_selector_eos_wait (sel, selpad);

You probably should go out here immediately if self->flushing is TRUE

@@ +563,3 @@
         active_selpad->eos_sent = TRUE;
+        gst_input_selector_eos_wait (sel, selpad);
+        forward = TRUE;

This looks wrong, you first set forward conditionally and then unconditionally to TRUE :)

I'm not sure all this logic is correct either. See the comment in the code a few lines above. I think if what was described in that comment happened, we shouldn't wait but forward the EOS event immediately instead of waiting.

@@ +567,2 @@
       }
       GST_DEBUG_OBJECT (pad, "received EOS");

I think if we're forwarding the EOS event here, we should set eos_block to FALSE and wake up any other pads that might currently be waiting.


Also instead of eos_block... isn't this more a boolean that describes if the whole selector is EOS now? So maybe just call it self->eos or self->eos_sent or self->is_eos or something like that.

Thanks for your review.
I will check more and back to here later.

Below code in function gst_selector_pad_event().

  GST_INPUT_SELECTOR_LOCK (sel);
  prev_active_sinkpad =
      sel->active_sinkpad ? gst_object_ref (sel->active_sinkpad) : NULL;
  active_sinkpad = gst_input_selector_activate_sinkpad (sel, pad);
  GST_INPUT_SELECTOR_UNLOCK (sel);

  if (prev_active_sinkpad != active_sinkpad) {
    if (prev_active_sinkpad)
      g_object_notify (G_OBJECT (prev_active_sinkpad), "active");
    g_object_notify (G_OBJECT (active_sinkpad), "active");
    g_object_notify (G_OBJECT (sel), "active-pad");
  }
  if (prev_active_sinkpad)
    gst_object_unref (prev_active_sinkpad);

  GST_INPUT_SELECTOR_LOCK (sel);

  /* only forward if we are dealing with the active sinkpad */
  forward = (pad == active_sinkpad);

I want get active_sinkpad again before decide forward. So active pad EOS must forward as function gst_input_selector_set_active_pad() is also called after locked.

So below code is useless as if active_selpad->eos is true, active_selpad->eos_sent must be true. And active_selpad->eos_sent = TRUE isn't reasonable.

        GstSelectorPad *active_selpad;

        /* If the active sinkpad is in EOS state but EOS
         * was not sent downstream this means that the pad
         * got EOS before it was set as active pad and that
         * the previously active pad got EOS after it was
         * active
         */
        active_selpad = GST_SELECTOR_PAD (active_sinkpad);
        forward = (active_selpad->eos && !active_selpad->eos_sent);
        active_selpad->eos_sent = TRUE;


And will add self->eos to indicate selector into EOS state and don't wait and discard all received buffer.

Is it ok? If OK, will submit patch later.

Yes, you're right that this code does not make any sense at all anymore. If forward==TRUE then pad==active_sinkpad at that point and then eos=eos_sent=TRUE.

This must be a leftover from some older refactoring.

Created attachment 293223 [details] [review]
update the patch

The patch should work with https://bugzilla.gnome.org/show_bug.cgi?id=741893

Review of attachment 293223 [details] [review]:

::: plugins/elements/gstinputselector.c
@@ +1179,3 @@
+      res = GST_FLOW_OK;
+    else
+      res = GST_FLOW_NOT_LINKED;

Should be GST_FLOW_EOS in both cases

::: plugins/elements/gstinputselector.h
@@ +79,3 @@
   gboolean blocked;
+  gboolean eos;
+  gboolean eos_blocked;

Can't you use a single variable for both? Just eos?

Created attachment 293313 [details] [review]
update the patch based on review. the patch is based on latest code.

Created attachment 293315 [details] [review]
update the patch based on review. the patch is based on latest code. update typo.

Thanks, looks mostly good now. I did some minor changes on top of it and merged it :)

commit bc1ec4ec5f4879747c0535a163e704afa7f33654
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Dec 24 13:46:28 2014 +0100

    inputselector: Use the same waiting function for EOS and non-EOS waiting

commit a1c183417f0a93f0669f8c561a38fe39fdafd453
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Dec 24 13:44:09 2014 +0100

    inputselector: Wake up all waitings pads directly if we forward the EOS event
    
    Otherwise they might wait a bit longer unnecessarily.
    
    Also do some minor cleanup.

commit 11e4b744f5e7b686785cfce9081193c5e5ff6a5d
Author: Song Bing <b06498@freescale.com>
Date:   Wed Dec 24 10:13:51 2014 +0800

    inputselector: Block when receiving an EOS event on a deactivated pad
    
    ... and only unblock when either a) the pad becomes active and the event
    should be forwarded or b) the active pad went EOS itself.
    
    Otherwise it can happen that we switch from a longer track that is not EOS yet
    to a shorter track that already is EOS, but the shorter track won't have any
    possibility to send its EOS event downstream anymore.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740949