Bug 185592 – [MSE][GStreamer] Before deleting the stream from WebKitMediaSource we should unlink its elements and flush the appsrc.

• Component: WPE WebKit
• Status: RESOLVED INVALID

[Yacine Bandou]

Before delete the stream from WebKitMediaSource we need to unlink the elements appsrc and parser and flush 
the appsrc before setting its state to NULL, otherwise we'll block in appsrc when we go from paused state to ready.

[Yacine Bandou]

Created attachment 340280 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 340280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340280&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:535
> +        GRefPtr<GstPad> sinkPad;
> +        GRefPtr<GstPad> sourcePad;
> +        if (stream->parser) {
> +            sourcePad = adoptGRef(gst_element_get_static_pad(stream->appsrc, "src"));
> +            if (gst_pad_is_linked(sourcePad.get())) {
> +                sinkPad = adoptGRef(gst_element_get_static_pad(stream->parser, "sink"));
> +                gst_pad_unlink(sourcePad.get(), sinkPad.get());
> +            }
> +            sourcePad = adoptGRef(gst_element_get_static_pad(stream->parser, "src"));
> +            if (gst_pad_is_linked(sourcePad.get())) {
> +                sinkPad = adoptGRef(gst_pad_get_peer(sourcePad.get()));
> +                gst_pad_unlink(sourcePad.get(), sinkPad.get());
> +            }
> +        } else {
> +            sourcePad = adoptGRef(gst_element_get_static_pad(stream->appsrc, "src"));
> +            if (gst_pad_is_linked(sourcePad.get())) {
> +                sinkPad = adoptGRef(gst_pad_get_peer(sourcePad.get()));
> +                gst_pad_unlink(sourcePad.get(), sinkPad.get());
> +            }
> +        }

About this piece of code, I think you can begin with the src pad of the source, check if it is linked, get the peer pad of the src, which would be the "sink" of the next element, then you can unlink and gst_pad_get_parent_element of the "sink" pad to begin the operation again until there is no "sink" pad, meaning that you reached the appsink.

[Xabier Rodríguez Calvar]

Enrique, Alicia, what do you think of this?

[Alicia Boya García]

(In reply to Yacine Bandou from comment #0)
> Before delete the stream from WebKitMediaSource we need to unlink the
> elements appsrc and parser and flush 
> the appsrc before setting its state to NULL, otherwise we'll block in appsrc
> when we go from paused state to ready.

Is the problem you are mentioning a deadlock or just an avoidable wait?

[Yacine Bandou]

(In reply to Alicia Boya García from comment #4)
> (In reply to Yacine Bandou from comment #0)
> > Before delete the stream from WebKitMediaSource we need to unlink the
> > elements appsrc and parser and flush 
> > the appsrc before setting its state to NULL, otherwise we'll block in appsrc
> > when we go from paused state to ready.
> 
> Is the problem you are mentioning a deadlock or just an avoidable wait?

I think it is just an avoidable wait in "gst_pad_stop_task" of basesrc (appsrc).
Because I didn't see a deadlock in the threads callstack.

[Yacine Bandou]

Created attachment 340305 [details]
thread call stack without the patch

Here is the thread call stack.

[Alicia Boya García]

(In reply to Yacine Bandou from comment #5)
> (In reply to Alicia Boya García from comment #4)
> > (In reply to Yacine Bandou from comment #0)
> > > Before delete the stream from WebKitMediaSource we need to unlink the
> > > elements appsrc and parser and flush 
> > > the appsrc before setting its state to NULL, otherwise we'll block in appsrc
> > > when we go from paused state to ready.
> > 
> > Is the problem you are mentioning a deadlock or just an avoidable wait?
> 
> I think it is just an avoidable wait in "gst_pad_stop_task" of basesrc
> (appsrc).

So, have you noticed any measurable improvement with the patch?

[Yacine Bandou]

(In reply to Alicia Boya García from comment #7)
> (In reply to Yacine Bandou from comment #5)
> > (In reply to Alicia Boya García from comment #4)
> > > (In reply to Yacine Bandou from comment #0)
> > > > Before delete the stream from WebKitMediaSource we need to unlink the
> > > > elements appsrc and parser and flush 
> > > > the appsrc before setting its state to NULL, otherwise we'll block in appsrc
> > > > when we go from paused state to ready.
> > > 
> > > Is the problem you are mentioning a deadlock or just an avoidable wait?
> > 
> > I think it is just an avoidable wait in "gst_pad_stop_task" of basesrc
> > (appsrc).
> 
> So, have you noticed any measurable improvement with the patch?

When we properly unlink and flush the appsrc (basesrc), we don't stay block when we go from the paused state to ready in appsrc.

Do you have an other way to avoid this blocking ? I attached the thread call stack

[Alicia Boya García]

(In reply to Yacine Bandou from comment #8)
> 
> When we properly unlink and flush the appsrc (basesrc), we don't stay block
> when we go from the paused state to ready in appsrc.
> 
> Do you have an other way to avoid this blocking ? I attached the thread call
> stack

A small blocking wait is expected, as the main thread needs to wait for the worker thread to finish before freeing the memory and resources it was using. The code performing the wait is in gst_base_src_stop().

This happens whenever the pad gets "deactivated" (gst_pad_set_active() is called with active=FALSE), which triggers the gstbasesrc pad (de)activation function, gst_base_src_activate_mode(). This happens whenever the source element state is downgraded to READY (you can see this in gst_element_change_state_func(), look for `case GST_STATE_CHANGE_PAUSED_TO_READY:`).

This is all expected behavior in GStreamer and for the most part, threaded code in general.

Unlinking the pads during playback or flushing does not alter this behavior.

[Alicia Boya García]

Comment on attachment 340280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340280&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:540
> +            return;

This is the reason your patch removes the wait for the background thread. Flushing an unlinked element is an error, therefore gst_element_send_event() returns != 0 and the function returns here, not reaching `gst_element_set_state(stream->appsrc, GST_STATE_NULL);` which is a few lines below.

With the patch the pipeline state is not changed to NULL, therefore the streaming thread is not finished -- and incidentally, no wait for its termination occurs.

[Yacine Bandou]

(In reply to Alicia Boya García from comment #9)
> (In reply to Yacine Bandou from comment #8)
> > 
> > When we properly unlink and flush the appsrc (basesrc), we don't stay block
> > when we go from the paused state to ready in appsrc.
> > 
> > Do you have an other way to avoid this blocking ? I attached the thread call
> > stack
> 
> A small blocking wait is expected, as the main thread needs to wait for the
> worker thread to finish before freeing the memory and resources it was
> using. The code performing the wait is in gst_base_src_stop().
> 
> This happens whenever the pad gets "deactivated" (gst_pad_set_active() is
> called with active=FALSE), which triggers the gstbasesrc pad (de)activation
> function, gst_base_src_activate_mode(). This happens whenever the source
> element state is downgraded to READY (you can see this in
> gst_element_change_state_func(), look for `case
> GST_STATE_CHANGE_PAUSED_TO_READY:`).
> 
> This is all expected behavior in GStreamer and for the most part, threaded
> code in general.
> 
> Unlinking the pads during playback or flushing does not alter this behavior.


The problem, it is not a small blocking but it is infinite waiting as a deadlock.

But in the callstack I don't see a deadlock.

[Yacine Bandou]

(In reply to Alicia Boya García from comment #10)
> Comment on attachment 340280 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340280&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:540
> > +            return;
> 
> This is the reason your patch removes the wait for the background thread.
> Flushing an unlinked element is an error, therefore gst_element_send_event()
> returns != 0 and the function returns here, not reaching
> `gst_element_set_state(stream->appsrc, GST_STATE_NULL);` which is a few
> lines below.
> 
> With the patch the pipeline state is not changed to NULL, therefore the
> streaming thread is not finished -- and incidentally, no wait for its
> termination occurs.

I agree, that's exactly what's happening.
It is not a good solution ;)

I'll try to see how to fix this block, if you have any suggestions don't hesitate

[Alicia Boya García]

Unlinking elements is not necessary, the links are removed automatically when they are removed from the bin.

Flushing *before* unlinking will clear the stream and allow the source element to reach NULL state. That will avoid the deadlock.

But on the other hand, what is the motivation for this? Note that you are removing the appsrc and parser, but all other elements downstream (including decoders and sinks) remain. Even if you removed the ghostpads from WebKitMediaSrc, uridecodebin does not listen for that, so it won't help either. uridecodebin is just not designed to handle appearing and reappearing streams.

[Yacine Bandou]

(In reply to Alicia Boya García from comment #13)

> But on the other hand, what is the motivation for this? Note that you are
> removing the appsrc and parser, but all other elements downstream (including
> decoders and sinks) remain. Even if you removed the ghostpads from
> WebKitMediaSrc, uridecodebin does not listen for that, so it won't help
> either. uridecodebin is just not designed to handle appearing and
> reappearing streams.

Yes, for this reason, I'll not remove the appsrc from bin this is not necessary as we explained in 185242.

I close this bug as invalid.