[GStreamer] Introduce workaround for race condition in appsink #3965

ntrrgc · 2022-09-02T20:02:05Z

`8404805`

[GStreamer] Introduce workaround for race condition in appsink

Reviewed by Philippe Normand.

This patch introduces a WebKit-side workaround for this GStreamer bug:
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/2413
appsink: Fix race condition on caps handling

That race condition causes occasional crashes on MSE flushes following
quality changes.

The workaround consists in registering an element that is a subclass of
appsink with a probe that pushes a redundant caps event if a buffer is
received after a flush. This ensures appsink processes the caps so that
the GstSample later pulled from appsink contains the right caps.

The workaround is only installed if the version of gst-plugins-base is
known to have this bug.

Canonical link: https://commits.webkit.org/255431@main

9e01e06

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
✅ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-02T20:03:40Z

EWS run on previous version of this PR (hash 3877f3f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

ntrrgc · 2022-09-02T20:07:37Z

The style failure is intentional, as it's a consequence of GObject class definition.

ERROR: Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:833:  webkit_app_sink_with_workaround_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:459:  webkit_app_sink_with_workaround_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

philn · 2022-09-04T13:28:01Z

The style failure is intentional, as it's a consequence of GObject class definition.

ERROR: Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:833:  webkit_app_sink_with_workaround_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:459:  webkit_app_sink_with_workaround_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

Usually every new GObject sub-class we add in WebCore needs to be allow-listed in the style-checker...
Besides, could this have been done in a new code unit (cpp, h files)? The Common stuff is becoming a bit, ... big.

Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp

ntrrgc · 2022-10-05T12:11:24Z

Usually every new GObject sub-class we add in WebCore needs to be allow-listed in the style-checker...
Besides, could this have been done in a new code unit (cpp, h files)? The Common stuff is becoming a bit, ... big.

I can do this. My concern with adding too many .cpp files is build times. Currently Source/platform/graphics/gstreamer is not using unified builds for some reason... which is a separate issue I think should be solved.

webkit-early-warning-system · 2022-10-07T12:26:16Z

EWS run on previous version of this PR (hash b7ca045)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
✅ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
❌ 🛠 🧪 merge	✅ 🛠 watch	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-commit-queue · 2022-10-07T17:44:44Z

No reviewer information in commit message, blocking PR #3965

webkit-early-warning-system · 2022-10-10T07:44:00Z

EWS run on current version of this PR (hash 9e01e06)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
✅ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

Reviewed by Philippe Normand. This patch introduces a WebKit-side workaround for this GStreamer bug: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/2413 appsink: Fix race condition on caps handling That race condition causes occasional crashes on MSE flushes following quality changes. The workaround consists in registering an element that is a subclass of appsink with a probe that pushes a redundant caps event if a buffer is received after a flush. This ensures appsink processes the caps so that the GstSample later pulled from appsink contains the right caps. The workaround is only installed if the version of gst-plugins-base is known to have this bug. Canonical link: https://commits.webkit.org/255431@main

webkit-commit-queue · 2022-10-12T14:17:09Z

Committed 255431@main (8404805): https://commits.webkit.org/255431@main

Reviewed commits have been landed. Closing PR #3965 and removing active labels.

https://bugs.webkit.org/show_bug.cgi?id=257035 Reviewed by Xabier Rodriguez-Calvar. Before this patch, there were two classes having a m_hasAllTracks field: MediaSourcePrivateGStreamer and MediaPlayerPrivateGStreamerMSE. This redundancy lead to non-synchronization, and in consequence this was making flushes not occur on MSE, as SourceBufferPrivateGStreamer::flush() checked for the one in MediaPlayerPrivateGStreamerMSE which no code ever set to true. A visible consequence of this was video was being repeated on quality changes, due to the lack of flush of old frames. This patch fixes the issue by keeping the `hasAllTracks` state in one single place, in MediaSourcePrivateGStreamer. Note: The triggering of MSE flushes exposed several bugs on the handling of flushes in other parts of the code. It's important these are addressed to avoid regressions in behavior when incorporating this patch: (1) WebKit#3802 [GStreamer] MediaPlayerPrivateGStreamer: Abort stale tasks on flushes Without this patch, MSE flushes can deadlock. (2) https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/2413 appsink: Fix race condition on caps handling appsink used to have a race condition on the handling of caps after a flush that can cause crashes. A fix is present in GStreamer 1.20.3+. A workaround in the WebKit side is possible and desirable to avoid headaches in Linux distros, and has been uploaded as: WebKit#3965 [GStreamer] Introduce workaround for race condition in appsink (3) https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3471 basesink: Support position queries after non-resetting flushes basesink doesn't answer position queries between a FLUSH_STOP with reset_time=FALSE and a SEGMENT event until GStreamer 1.24.0, which incorporates that patch. For backwards-compatibility -- and present-compatibility for that matter since GStreamer 1.24.0 has not been released yet, a workaround has been Proposed for WebKit: WebKit#14082 [MSE][GStreamer] Workaround basesink's lack of support for position queries between a non-resetting flush and a pre-roll * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp: (WebCore::MediaPlayerPrivateGStreamerMSE::sourceSetup): * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h: (WebCore::MediaPlayerPrivateGStreamerMSE::hasAllTracks const): Deleted. * Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.cpp: (WebCore::MediaSourcePrivateGStreamer::addSourceBuffer): * Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.h: * Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp: (WebCore::SourceBufferPrivateGStreamer::flush): Canonical link: https://commits.webkit.org/264510@main

https://bugs.webkit.org/show_bug.cgi?id=257035 Reviewed by Xabier Rodriguez-Calvar. Before this patch, there were two classes having a m_hasAllTracks field: MediaSourcePrivateGStreamer and MediaPlayerPrivateGStreamerMSE. This redundancy lead to non-synchronization, and in consequence this was making flushes not occur on MSE, as SourceBufferPrivateGStreamer::flush() checked for the one in MediaPlayerPrivateGStreamerMSE which no code ever set to true. A visible consequence of this was video was being repeated on quality changes, due to the lack of flush of old frames. This patch fixes the issue by keeping the `hasAllTracks` state in one single place, in MediaSourcePrivateGStreamer. Note: The triggering of MSE flushes exposed several bugs on the handling of flushes in other parts of the code. It's important these are addressed to avoid regressions in behavior when incorporating this patch: (1) WebKit/WebKit#3802 [GStreamer] MediaPlayerPrivateGStreamer: Abort stale tasks on flushes Without this patch, MSE flushes can deadlock. (2) https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/2413 appsink: Fix race condition on caps handling appsink used to have a race condition on the handling of caps after a flush that can cause crashes. A fix is present in GStreamer 1.20.3+. A workaround in the WebKit side is possible and desirable to avoid headaches in Linux distros, and has been uploaded as: WebKit/WebKit#3965 [GStreamer] Introduce workaround for race condition in appsink (3) https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3471 basesink: Support position queries after non-resetting flushes basesink doesn't answer position queries between a FLUSH_STOP with reset_time=FALSE and a SEGMENT event until GStreamer 1.24.0, which incorporates that patch. For backwards-compatibility -- and present-compatibility for that matter since GStreamer 1.24.0 has not been released yet, a workaround has been Proposed for WebKit: WebKit/WebKit#14082 [MSE][GStreamer] Workaround basesink's lack of support for position queries between a non-resetting flush and a pre-roll * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp: (WebCore::MediaPlayerPrivateGStreamerMSE::sourceSetup): * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h: (WebCore::MediaPlayerPrivateGStreamerMSE::hasAllTracks const): Deleted. * Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.cpp: (WebCore::MediaSourcePrivateGStreamer::addSourceBuffer): * Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.h: * Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp: (WebCore::SourceBufferPrivateGStreamer::flush): Canonical link: https://commits.webkit.org/264510@main

ntrrgc requested a review from philn as a code owner September 2, 2022 20:02

ntrrgc self-assigned this Sep 2, 2022

ntrrgc mentioned this pull request Sep 2, 2022

[MSE][GStreamer] Actually do flushes, unify m_hasAllTracks #3966

Merged

ceyusa reviewed Sep 5, 2022

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp Outdated Show resolved Hide resolved

ceyusa reviewed Sep 5, 2022

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp Outdated Show resolved Hide resolved

ntrrgc force-pushed the eng/20220902-appsinkworkaround branch from 3877f3f to b7ca045 Compare October 7, 2022 12:25

ntrrgc requested a review from JonWBedard as a code owner October 7, 2022 12:25

philn approved these changes Oct 7, 2022

View reviewed changes

ntrrgc added the merge-queue Applied to send a pull request to merge-queue label Oct 7, 2022

webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Oct 7, 2022

ntrrgc removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2022

ntrrgc force-pushed the eng/20220902-appsinkworkaround branch from b7ca045 to 9e01e06 Compare October 10, 2022 07:43

ntrrgc added the merge-queue Applied to send a pull request to merge-queue label Oct 12, 2022

webkit-early-warning-system force-pushed the eng/20220902-appsinkworkaround branch from 9e01e06 to 8404805 Compare October 12, 2022 14:17

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 12, 2022

webkit-early-warning-system merged commit 8404805 into WebKit:main Oct 12, 2022

ntrrgc deleted the eng/20220902-appsinkworkaround branch October 19, 2022 09:40

ntrrgc mentioned this pull request May 24, 2023

[MSE][GStreamer] Workaround basesink's lack of support for position queries between a non-resetting flush and a pre-roll #14082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GStreamer] Introduce workaround for race condition in appsink #3965

[GStreamer] Introduce workaround for race condition in appsink #3965

ntrrgc commented Sep 2, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 2, 2022 •

edited

ntrrgc commented Sep 2, 2022

philn commented Sep 4, 2022

ntrrgc commented Oct 5, 2022

webkit-early-warning-system commented Oct 7, 2022 •

edited

webkit-commit-queue commented Oct 7, 2022

webkit-early-warning-system commented Oct 10, 2022 •

edited

webkit-commit-queue commented Oct 12, 2022

[GStreamer] Introduce workaround for race condition in appsink #3965

[GStreamer] Introduce workaround for race condition in appsink #3965

Conversation

ntrrgc commented Sep 2, 2022 • edited by webkit-early-warning-system

8404805

webkit-early-warning-system commented Sep 2, 2022 • edited

ntrrgc commented Sep 2, 2022

philn commented Sep 4, 2022

ntrrgc commented Oct 5, 2022

webkit-early-warning-system commented Oct 7, 2022 • edited

webkit-commit-queue commented Oct 7, 2022

webkit-early-warning-system commented Oct 10, 2022 • edited

webkit-commit-queue commented Oct 12, 2022

ntrrgc commented Sep 2, 2022 •

edited by webkit-early-warning-system

`8404805`

webkit-early-warning-system commented Sep 2, 2022 •

edited

webkit-early-warning-system commented Oct 7, 2022 •

edited

webkit-early-warning-system commented Oct 10, 2022 •

edited