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

Description Alicia Boya García 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.
Comment 1 Alicia Boya García 2020-02-20 04:35:39 PST 
Created attachment 391277 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 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. */

//
Comment 3 Alicia Boya García 2020-02-20 10:58:44 PST 
Created attachment 391309 [details]
Patch for landing
Comment 4 WebKit Commit Bot 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.
Comment 5 WebKit Commit Bot 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.
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2020-02-20 14:31:25 PST 
All reviewed patches have been landed.  Closing bug.
Comment 8 Carlos Alberto Lopez Perez 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
Comment 9 Philippe Normand 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