Bug 185242 – [MSE][GStreamer] Delete properly the stream from the WebKitMediaSource

• Component: Media
• Status: RESOLVED FIXED

[Yacine Bandou]

When the sourceBuffer is removed from mediasource, the appropriate stream is not properly deleted from WebKitMediaSource.
The Appsrc and the Parser didn't removed from the WebKitMediaSource bin element.

[Enrique Ocaña]

Is this SourceBuffer removal happening at any time point different from the player destruction? Specifically, do you have any use case where the player keeps being used after a single SourceBuffer is removed? This would mean that actual removal of the appsrc and parser elements on demand should be taken into account (currently isn't, as you pointed out).

Otherwise, the WebKitMediaSrc bin destruction should automatically trigger the destruction of the appsrc and parser elements. I don't remember the implications now, but probably the implementation is like this (removal deferred to bin destruction) in order to avoid transient states caused by the on-demand removal, which might complicate WebKitMediaSrc management without need in a moment where its final destruction is about to happen.

[Yacine Bandou]

(In reply to Enrique Ocaña from comment #1)
> Is this SourceBuffer removal happening at any time point different from the
> player destruction? Specifically, do you have any use case where the player
> keeps being used after a single SourceBuffer is removed? This would mean
> that actual removal of the appsrc and parser elements on demand should be
> taken into account (currently isn't, as you pointed out).
> 
> Otherwise, the WebKitMediaSrc bin destruction should automatically trigger
> the destruction of the appsrc and parser elements. I don't remember the
> implications now, but probably the implementation is like this (removal
> deferred to bin destruction) in order to avoid transient states caused by
> the on-demand removal, which might complicate WebKitMediaSrc management
> without need in a moment where its final destruction is about to happen.

Currently when there is an error in playback as no decryption key in the decryptor the implementation calls  HTMLMediaElement::noneSupported() wich 
detaches the MediaSource from MediaElement then it removes all its SourceBuffers.

When we remove SourceBuffer the function webKitMediaSrcFreeStream(m_webKitMediaSrc.get(), stream)  is called via PlaybackPipeline::removeSourceBuffer(sourceBufferPrivate).

Then the PlayerPrivate catch "source-setup" signal and calls MediaPlayerPrivateGStreamerMSE::sourceSetup wich calls setPrivateAndOpen via MediaSourceGStreamer::open.

The function setPrivateAndOpen tries to call the instance of MediaElement wich is already detached from MediaSource, thus causing a crash.  

When we properly delete the Stream by removing the appropriate appsrc and parser from the bin, the pipeline never send the "source-setup" signal.

Otherwise, I think the function webKitMediaSrcFreeStream should delete the Stream correctly without having this crash.

[Yacine Bandou]

Created attachment 339498 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 339498 [details]
Patch

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

I see this patch is ok though I prefer Enrique to give the final go.

> Source/WebCore/ChangeLog:10
> +        The Appsrc and the Parser didn't removed from the WebKitMediaSource bin element.

This sentence doesn't make too much sense to me.

> Source/WebCore/ChangeLog:12
> +        This patch avoid the regression of r231089, see https://bugs.webkit.org/show_bug.cgi?id=185071

avoids.

[Enrique Ocaña]

Comment on attachment 339498 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:543
> +            source->priv->numberOfAudioStreams--;

These changes in numberOf{Audio,Video,Text}Streams must be protected by GST_OBJECT_{LOCK,UNLOCK}(webKitMediaSrc) (webKitMediaSrc is "source" in webKitMediaSrcFreeStream()). See PlaybackPipeline::attachTrack() and ::reattachTrack().

In order to avoid taking both the webKitMediaSrc and stream locks at the same time, split the code in two pieces, duplicating the "if (stream->type != WebCore::Invalid) {" block and its inner switch statement. The first "if" block would be protected by GST_OBJECT{LOCK,UNLOCK}(webKitMediaSrc) and would just change numberOf{Audio,Video,Text}Streams as quickly as possible. The second "if" block would be the original one, which locks on streamLock.

[Yacine Bandou]

Created attachment 339534 [details]
Patch

[Enrique Ocaña]

Comment on attachment 339534 [details]
Patch

Looks good. Let's wait for Xabier to +r it.

[WebKit Commit Bot]

Comment on attachment 339534 [details]
Patch

Clearing flags on attachment: 339534

Committed r231351: <https://trac.webkit.org/changeset/231351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39974810>

[Alicia Boya García]

Comment on attachment 339534 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:516
> +        gst_element_set_state(stream->appsrc, GST_STATE_NULL);

This will deadlock whenever the streaming thread is waiting for some action that requires WebKit intervention. Note that:

a) Setting a source element state to NULL deactivates its pad and therefore attempts to tear down its streaming thread.

b) Tearing down a streaming thread is a stream-synchronized operation. The streaming thread is shared with other elements downstream. This means that we need to wait until no buffer or (stream synchronized) event is traveling through this stream.

c) A traveling buffer may be blocked waiting for something. For instance, usually the stream ends at the sinkpad of a queue element (the srcpad of the queue element starts a separate streaming thread, so for threading purposes that's separate stream). If the queue is full and won't gain any space until WebKit's main thread performs some action you've got yourself in a catch 22, deadlock.

This is what is causing YTTV 18. MediaElementEvents to deadlock after this patch. Note that even though the test finishes when WebKit reports playback has ended, several seconds worth of frames are still queued in the playback pipeline because of a change of duration to a shorter value. The pipeline is set to PAUSED when the shorter duration is reached, so those extra frames are not played, but still try to continue travelling through the pipeline. When the sink element receives a frame in this PAUSED state it has to wait for a state change to PLAYING before doing anything with it (see gst_base_sink_wait_preroll()). This blocks the streaming thread containing the sink. Streaming threads upstream still continue pushing buffers for some time until the queue becomes full, then get blocked as well.

Summarizing: in order to tear down the source element you need to tear down its streaming thread. In order to tear down the streaming thread you need to wait for the current flowing buffer to complete its journey in this streaming thread, but that won't happen until the queue at the end of this stream has available space and therefore stops blocking the thread. The queue won't have available space until the sink is set to PLAYING or the pipeline is flushed. That won't ever happen (moreso when the main thread is waiting), therefore you've got a deadlock.

[Yacine Bandou]

(In reply to Alicia Boya García from comment #11)
> Comment on attachment 339534 [details]
> Patch
> 
> 
> Summarizing: in order to tear down the source element you need to tear down
> its streaming thread. In order to tear down the streaming thread you need to
> wait for the current flowing buffer to complete its journey in this
> streaming thread, but that won't happen until the queue at the end of this
> stream has available space and therefore stops blocking the thread. The
> queue won't have available space until the sink is set to PLAYING or the
> pipeline is flushed. That won't ever happen (moreso when the main thread is
> waiting), therefore you've got a deadlock.

I agree with you, when I studied in detail what happen with encrypted-media WPT tests, I saw what you said.
For that, I tried to fix it with the patch 185592, but it is a bad solution.

After a detailed investigation, I found that, this patch doesn't fix the crash, it just replaces the crash by a blocking.

Here is the real root cause of the crash. (-> : calls)

1.When an error occurs in playback pipeline (no decipher key), we receive an error message in MediaPlayerPrivateGStreamer::handleMessage -> MediaPlayerPrivateGStreamer::loadingFailed (MediaPlayer::FormatError).

2.The function loadingFailed -> HTMLMediaElement::mediaPlayerNetworkStateChanged (MediaPlayer::FormatError) -> setNetworkState -> mediaLoadingFailed -> noneSupported go to the point 3
                              |
                               -> HTMLMediaElement::mediaPlayerReadyStateChanged(MediaPlayer::HaveNothing) -> setReadyState -> updatePlayState() go to the point 5

3.nonSupported -> detachMediaSource -> MediaSource::detachFromElement -> removeSourceBuffer -> MediaSourceGStreamer::removeSourceBuffer -> .. ->PlaybackPipeline::removeSourceBuffer

4.PlaybackPipeline::removeSourceBuffer -> webKitMediaSrcFreeStream . 

5.HTMLMediaElement::updatePlayState -> potentiallyPlaying -> stoppedDueToErrors ( This function returns false because (m_readyState >= HAVE_METADATA && m_error) is false, m_readyState equal to HaveNothing )

6.HTMLMediaElement::updatePlayState -> MediaPlayer::play -> MediaPlayerPrivateGStreamer::play -> changePipelineState (set the pipeline to playing state)

7.WebkitMediaSourceGStreamer sends "source-setup" signal when its state change from Ready to Paused, the signal catched in MediaPlayerPrivateGStreamer::sourceSetupCallback

8.MediaPlayerPrivateGStreamer::sourceSetupCallback -> MediaPlayerPrivateGStreamerMSE::sourceSetup -> MediaSourceGStreamer::open -> MediaSource::setPrivateAndOpen (crash in this function because the mediaSource is detached from mediaElement in the point 3, thus the variable  m_mediaElement is null).


In the point 5 the function "HTMLMediaElement::stoppedDueToErrors" returns false, because m_readyState equal to HaveNothing and it is not upper than HAVE_METADATA.
I think we don't have to set the ReadyState to HaveNothing when an error occurs in pipeline, we should just set NetworkState to error.


Here is the my roadmap:
1. I'll push a new patch to remove a part of this patch (185242) which caused the blocking.
2. I'll push an other new patch to fix the crash, this patch just set NetworkState without ReadyState when an error occurs in pipeline.
3. I'll close the bug 185592 as invalid bug.
4. I'll update the patch 185593 to depend on the two new patches.

[Alicia Boya García]

> 1. I'll push a new patch to remove a part of this patch (185242) which caused the blocking.

Is there a reason to keep some parts of this patch? Which ones?

[Alicia Boya García]

(Notice that, as explained in https://bugs.webkit.org/show_bug.cgi?id=185592#c13, you can't just remove the appsrc and assume the stream is gone).

[Yacine Bandou]

(In reply to Alicia Boya García from comment #13)
> > 1. I'll push a new patch to remove a part of this patch (185242) which caused the blocking.
> 
> Is there a reason to keep some parts of this patch? Which ones?

I keep the part which update the "source->priv->numberOfXXXXStreams"

 527    GST_OBJECT_LOCK(source);
 528    switch (stream->type) {
 529    case WebCore::Audio:
 530        source->priv->numberOfAudioStreams--;
 531        break;
 532    case WebCore::Video:
 533        source->priv->numberOfVideoStreams--;
 534        break;
 535    case WebCore::Text:
 536        source->priv->numberOfTextStreams--;
 537        break;
 538    default:
 539        break;
 540    }
 541    GST_OBJECT_UNLOCK(source);