This avoids tons of innecessary context switches, is simpler than the previous approach and much faster. This fixes stutter while loading YouTube videos.
Created attachment 350470 [details] Patch
This patch may have a trivial merge conflict in absence of this one: https://bugs.webkit.org/show_bug.cgi?id=189868
I'll insert one whitespace line to avoid the conflict.
Created attachment 350471 [details] Patch
Created attachment 350518 [details] Patch
(Changed one instance of GST_DEBUG to GST_TRACE to avoid spam in the logs)
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
Created attachment 350633 [details] Patch
(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.
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).
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.
Created attachment 350647 [details] Patch
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.
Created attachment 350649 [details] Patch
Comment on attachment 350649 [details] Patch Clearing flags on attachment: 350649 Committed r236409: <https://trac.webkit.org/changeset/236409>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 351285 [details] Patch
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.