Bug 208001 - [GStreamer] Fix race in TextCombinerGStreamer
Summary: [GStreamer] Fix race in TextCombinerGStreamer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-20 04:34 PST by Alicia Boya García
Modified: 2020-02-21 04:31 PST (History)
10 users (show)

See Also:


Attachments
Patch (9.67 KB, patch)
2020-02-20 04:35 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch for landing (10.45 KB, patch)
2020-02-20 10:58 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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