Bug 189871 – [MSE][GStreamer] Pull demuxed samples in batches

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alicia Boya García]

This avoids tons of innecessary context switches, is simpler than the
previous approach and much faster.

This fixes stutter while loading YouTube videos.

[Alicia Boya García]

Created attachment 350470 [details]
Patch

[Alicia Boya García]

This patch may have a trivial merge conflict in absence of this one: https://bugs.webkit.org/show_bug.cgi?id=189868

[Alicia Boya García]

I'll insert one whitespace line to avoid the conflict.

[Alicia Boya García]

Created attachment 350471 [details]
Patch

[Alicia Boya García]

Created attachment 350518 [details]
Patch

[Alicia Boya García]

(Changed one instance of GST_DEBUG to GST_TRACE to avoid spam in the logs)

[Xabier Rodríguez Calvar]

Comment on attachment 350518 [details]
Patch

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

I'd require some cosmetic changes and I'd appreciate Enrique's input in such a delicate matter.

> Source/WebCore/ChangeLog:9
> +        This avoids tons of innecessary context switches, is simpler than the
> +        previous approach and much faster.

This could be a bit more verbose.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:641
> +        // All in all, return OK, even if it's not the proper thing to do. We don't want to break the demuxer.

This comment line does not seem to be needed since we don't manage flow anymore.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:645
> +    if (!gst_sample_get_buffer(sample.get())) {

UNLIKELY

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:670
> +    if (mediaSample->decodeTime() == MediaTime::zeroTime()
> +        && mediaSample->presentationTime() > MediaTime::zeroTime()
> +        && mediaSample->presentationTime() <= MediaTime(1, 10)) {

You can put these in a single line.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:756
> +    if (batchedSampleCount > 0)
> +        checkEndOfAppend();

Isn't this check a bit useless since it looks like we are getting always at least one sample?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:100
> +    void consumeAppSinkAvailableSamples();

GStreamer element is appsink, so we capitalize only the first letter, meaning that this method should be named consumeAppsinkAvailableSamples. It can be a controversial, but the decission was made (by me) some time ago already :)

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:121
> +    // The main thread must set it to false before actually pulling samples.

The syntax of this sentence does not make sense in my head. Do you mean "written" in both cases?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:124
> +    std::atomic_flag m_busAlreadyNotifiedOfAvailableSamples;

m_didBusAlreadyNotifyOfAvailableSamples

[Alicia Boya García]

Created attachment 350633 [details]
Patch

[Alicia Boya García]

(In reply to Alicia Boya García from comment #8)
> Created attachment 350633 [details]
> Patch

This was wrongly uploaded by webkit-patch, it's meant for a different bug (which is indeed correctly specified in the patch...) It seems it does not handle `-g` very well for commits other than HEAD.

[Alicia Boya García]

Comment on attachment 350518 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:641
>> +        // All in all, return OK, even if it's not the proper thing to do. We don't want to break the demuxer.
> 
> This comment line does not seem to be needed since we don't manage flow anymore.

Indeed.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:756
>> +        checkEndOfAppend();
> 
> Isn't this check a bit useless since it looks like we are getting always at least one sample?

Usually, but not always, if the moons conspire against you, the threads could end up excuting like this:

ST: receives frame, test_and_set() -> (not previously set) push notification
ST: receives frame, test_and_set() -> (already set) do nothing
MT: checks GstBus message, sees the notification
MT: clear()
ST: receives frame, test_and_set() -> (not previously set) push notification
MT: pull all 3 frames
MT: checks GstBus message, sees the notification
MT: clear()
MT: nothing to pull, everything had already been pulled.

This locking strategy allows one useless pull in the worst case, but no more than that (in order to cause another pull, they would need to push a new frame, hence making the first pull not useless anymore). In exchange for that, it has minimal latency (frames arrive as quickly as they are demuxed and the MT can react) and with minimal locking (the streaming thread and the main thread don't need to wait on each other).

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:121
>> +    // The main thread must set it to false before actually pulling samples.
> 
> The syntax of this sentence does not make sense in my head. Do you mean "written" in both cases?

Yes, it's a typo.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:124
>> +    std::atomic_flag m_busAlreadyNotifiedOfAvailableSamples;
> 
> m_didBusAlreadyNotifyOfAvailableSamples

No, the bus is asynchronous (at least in this case). This flag is set when the notification has been sent (from the streaming thread); it's cleared when it has arrived (to the main thread).

[Enrique Ocaña]

Comment on attachment 350518 [details]
Patch

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

After having reviewed it, I find the patch is technically right in general (and indeed a clever way to optimize appends). Thank you for submitting it.

>> Source/WebCore/ChangeLog:9
>> +        previous approach and much faster.
> 
> This could be a bit more verbose.

The truth is that having something like the text below in the description would have helped me to understand better the patch:

Now, only the notifications of "new samples available" (appsink-new-sample bus messages) travel from the streaming thread to the main thread through the bus and the main thread is the responsible of pulling the samples from appsink. Before, the samples were pulled from appsink in the non-main thread and traveled to the main thread through the bus one by one.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:756
>> +        checkEndOfAppend();
> 
> Isn't this check a bit useless since it looks like we are getting always at least one sample?

I agree with having checkEndOfAppend() here. Appart from not carrying samples through the bus anymore, most of the efficiency of this new batched way of doing things comes from the latency between the detection of new samples in the streaming thread (and bus notification posting) and the reception of the bus notification in the main thread. During that latency, some new samples have accumulated in appsink.

Therefore, this case could be possible:

1. The streaming thread detects a new sample and posts the notification in the bus.
2. The main thread clears the m_busAlreadyNotifiedOfAvailableSamples flag (allows the streaming thread to begin processing again, see line 312) but still hasn't called consumeAppSinkAvailableSamples().
3. The streaming thread continues, detects another new sample and posts the notification again in the bus.
4. The main thread starts now to process the call to consumeAppSinkAvailableSamples() from the first notification. This processing pulls 2 samples (the one from the first notification and the one from the second).
5. After some time, the second notification is received in the main thread, consumeAppSinkAvailableSamples() is called again but this time it finds no available samples. The first call had pulled them and no new samples have been detected from the streaming thread since it posted second notification.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-862
> -    // avoid deadlocks when changing to GST_STATE_NULL having a non empty appsink.

I'm not sure if the warning about the importance of pulling the sample out keeps being true. I think we can ignore it by now and get rid of this block of code as you suggest, but in case we detect stalls in the future during AppendPipeline destruction we would have to revisit this warning again and add code to the destructor to try to consume the pending samples (in the main thread) before setting the pipeline to GST_STATE_NULL.

[Alicia Boya García]

Created attachment 350647 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 350647 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:644
> +    if (G_UNLIKELY(!gst_sample_get_buffer(sample.get()))) {

Just UNLIKELY should be enough.

[Alicia Boya García]

Created attachment 350649 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350649 [details]
Patch

Clearing flags on attachment: 350649

Committed r236409: <https://trac.webkit.org/changeset/236409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alicia Boya García]

Reopening to attach new patch.

[Alicia Boya García]

Created attachment 351285 [details]
Patch

[Alicia Boya García]

Comment on attachment 351285 [details]
Patch

This would belong to https://bugs.webkit.org/show_bug.cgi?id=190036, and Philippe has just landed a similar patch.