Bug 208001

Summary: [GStreamer] Fix race in TextCombinerGStreamer
Product: WebKit Reporter: Alicia Boya García <aboya>
Component: WebKitGTKAssignee: Alicia Boya García <aboya>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, clopez, commit-queue, ews-watchlist, gustavo, menard, pnormand, vjaquez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=208041
Attachments:
Description Flags
Patch
none
Patch for landing none

Alicia Boya García
Reported 2020-02-20 04:34:40 PST
TextCombinerGStreamer uses the CAPS event to determine whether adding a webvttenc between the text track pad and the funnel element used to be able to display several subtitles at the same time. The way this was done previously had a race though: all text track pads were preemptively linked directly to the funnel, only adding the webvttenc element later in the middle when receiving the CAPS event. When two or more text tracks were present, it wasn't infrequent that one track had its CAPS event processed (causing the webvttenc element to be added) and propagated (fixating the funnel caps) before another track attempted caps negotiation. Because the pads were connected to the funnel preemptively, and because without the webvttenc element the caps of the text pad don't match the funnel's, this causes a caps mismatch error, stopping playback completely. The CAPS event is therefore never sent. To avoid this race, we must avoid linking elements until we get the CAPS events, when we actually know where we should link them to, therefore avoiding early caps negotiation errors.
Attachments
Patch (9.67 KB, patch)
2020-02-20 04:35 PST, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Patch for landing (10.45 KB, patch)
2020-02-20 10:58 PST, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alicia Boya García
Comment 1 2020-02-20 04:35:39 PST
Created attachment 391277 [details] Patch
Xabier Rodríguez Calvar
Comment 2 2020-02-20 07:43:00 PST
Comment on attachment 391277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391277&action=review > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:69 > + GstPad* funnelPad; This seems to be never released. Please gst_object_unref during dispose. Actually I think it would be a wonderful idea to turn the Finalize into a Dispose (conceptually more correct, I think) and add the unref there. > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:146 > + /* Caps are plain text, we want a WebVTT encoder between the ghostpad and the funnel. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:148 > + /* Setup a WebVTT encoder. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:158 > + /* Switch the ghostpad to target the WebVTT encoder. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:165 > + /* Connect the WebVTT encoder to the funnel. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:173 > + /* Caps are not plain text, we assume it's WebVTT. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:175 > + /* Remove the WebVTT encoder if present. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:184 > + /* Link the pad to the funnel. */ // > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:188 > + } /* else: pipeline is already correct. */ //
Alicia Boya García
Comment 3 2020-02-20 10:58:44 PST
Created attachment 391309 [details] Patch for landing
WebKit Commit Bot
Comment 4 2020-02-20 13:09:43 PST
The commit-queue encountered the following flaky tests while processing attachment 391309 [details]: editing/spelling/spellcheck-input-search-crash.html bug 207995 (authors: arv@chromium.org, g.czajkowski@samsung.com, mark.lam@apple.com, and morrita@google.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 5 2020-02-20 13:09:52 PST
The commit-queue encountered the following flaky tests while processing attachment 391309 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 6 2020-02-20 14:31:23 PST
Comment on attachment 391309 [details] Patch for landing Clearing flags on attachment: 391309 Committed r257090: <https://trac.webkit.org/changeset/257090>
WebKit Commit Bot
Comment 7 2020-02-20 14:31:25 PST
All reviewed patches have been landed. Closing bug.
Carlos Alberto Lopez Perez
Comment 8 2020-02-21 04:19:14 PST
(In reply to WebKit Commit Bot from comment #6) > Comment on attachment 391309 [details] > Patch for landing > > Clearing flags on attachment: 391309 > > Committed r257090: <https://trac.webkit.org/changeset/257090> This has broken the build on the GTK minimum dependencies bots: Debian 10: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/30091 Ubuntu 18.04: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/29626 gst_clear_object and gst_clear_tag_list require GStreamer 1.16 Meanwhile both Debian 10 and Ubuntu 18.04 ship GStreamer 1.14
Philippe Normand
Comment 9 2020-02-21 04:20:47 PST
(In reply to Carlos Alberto Lopez Perez from comment #8) > (In reply to WebKit Commit Bot from comment #6) > > Comment on attachment 391309 [details] > > Patch for landing > > > > Clearing flags on attachment: 391309 > > > > Committed r257090: <https://trac.webkit.org/changeset/257090> > > This has broken the build on the GTK minimum dependencies bots: > > Debian 10: > https://build.webkit.org/builders/GTK%20Linux%2064- > bit%20Release%20Debian%20Stable%20%28Build%29/builds/30091 > Ubuntu 18.04: > https://build.webkit.org/builders/GTK%20Linux%2064- > bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/29626 > > gst_clear_object and gst_clear_tag_list require GStreamer 1.16 > > Meanwhile both Debian 10 and Ubuntu 18.04 ship GStreamer 1.14 See also https://bugs.webkit.org/show_bug.cgi?id=208041
Note You need to log in before you can comment on or make changes to this bug.