Bug 190469 - [MSE][GStreamer] Missing opusparse gstreamer element crashes WebKitWebProcess
Summary: [MSE][GStreamer] Missing opusparse gstreamer element crashes WebKitWebProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-11 07:47 PDT by Mart Raudsepp
Modified: 2019-03-05 07:06 PST (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mart Raudsepp 2018-10-11 07:47:16 PDT
In Gentoo we don't ship opusparse element, because when opusdec/openenc got moved to gst-plugins-base, opusparse was left into gst-plugins-bad and when packaging this change, I was told by GStreamer upstream that nothing probably should need it.
However now with MSE enabling epiphany (due to youtube), this crashes webkit-gtk hard, due to this code in Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:

        GstElement* opusparse = gst_element_factory_make("opusparse", parserName.get());
        RELEASE_ASSERT(opusparse);

with relevant gdb bits as:

(gdb) bt
#0  0x00007f34b6246e2e in WTFCrash () at /usr/src/debug/net-libs/webkit-gtk-2.22.2/webkitgtk-2.22.2/Source/WTF/wtf/Assertions.cpp:267
#1  0x00007f34bbc71f0c in createOptionalParserForFormat () at /usr/src/debug/net-libs/webkit-gtk-2.22.2/webkitgtk-2.22.2/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:882
#2  WebCore::AppendPipeline::connectDemuxerSrcPadToAppsinkFromAnyThread () at /usr/src/debug/net-libs/webkit-gtk-2.22.2/webkitgtk-2.22.2/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:956
#3  0x00007f34aa8aa0de in ffi_call_unix64 () from /usr/lib64/libffi.so.6
(gdb) frame 1
#1  0x00007f34bbc71f0c in createOptionalParserForFormat () at /usr/src/debug/net-libs/webkit-gtk-2.22.2/webkitgtk-2.22.2/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:882
882	        RELEASE_ASSERT(opusparse);


It seems counter-intuitive to release assert SIGSEGV upon no opusparse found when
1) It is an element in gst-plugins-bad
2) There is no documentation in webkit-gtk to tell that this is a hard requirement for runtime MSE or it crashes (and spins 2 minutes in coredumpd service)
3) It crashes in createOptionalParserForFormat method, emphasis on "Optional" - sounds like it isn't optional then, other than if no opus audio MSE sources happen to be encountered..

Additionally it behaves the same without vorbisparse, but that's less likely to be missing, at least.


Are you sure it is really necessary for proper functionality? Either way, could it be made to at least not RELEASE_ASSERT and bring the whole process down?
Comment 1 Adrian Perez 2018-10-11 08:25:18 PDT
Last week I was bitten by this as well. I agree that we should handle
this situation more gracefully, but some rework of the MSE playback
code may be needed to do so. Would it be acceptable for distros to
have a runtime dependency on gst-plugins-bad (or the package that
includes opusparse) in the meantime?

At any rate, let's keep this bug around to track implementation of
the changes needed to handle this situation better :)
Comment 2 Mart Raudsepp 2018-10-11 08:47:50 PDT
In Gentoo we split different ext/ and sys/ plugins into separate packages, as we are source-based distribution and can't just spread it out into package sets easily at binary level. We already have a gst-plugins-opus, that ships opusdec/opusenv. So I'll need to ponder what to do here - add some complicated logic to build both gst-plugins-base and gst-plugins-bad ext/opus in the same package, or add a temoprary gst-plugins-opusparse package for this. Before I do either of that, I was hoping to get to know why exactly opusparse is really needed there - does it really need parsing before feeding it to the decoder? It sounded like opusparse doesn't do anything important for most cases, so isn't really needed to be in the pipeline - but MSE is kind of special, and maybe it's really needed indeed. But that's my question now :)
Comment 3 Enrique Ocaña 2018-10-11 08:53:15 PDT
It's used to compute sample durations (not present in webm/opus). Without that info, the WebCore MSE algorithms to manage buffered data can't work properly.
Comment 4 Mart Raudsepp 2018-10-11 09:16:21 PDT
OK, so I'll package it and make at least epiphany runtime depend on it (and probably webkit-gtk itself later on). Hopefully opusparse is getting some attention now then, for it to move to gst-plugins-base too with 1.16.. Though I don't see any updates to it in git. If it is a hard requirement for webkit-gtk MSE now, I suggest giving it some attention and getting it to enough quality for gst-plugins-base :)
As we'll want to support MSE properly, I don't care too much about clean handling of it missing now, as long as the dependency is made explicit somewhere (list of dependencies somewhere at least).
Comment 5 Philippe Normand 2018-10-11 10:03:02 PDT
opusparse being part of gst-plugins-bad doesn't make it unfit for packaging... Many people still get confused by "bad" for a plugin-set shipping unstable libraries and plugins...
Comment 6 Michael Catanzaro 2018-10-12 12:55:16 PDT
Alicia, this is the bug I was telling you about this morning.

I'd say we should not crash if GStreamer plugins are missing. Instead, we should print a command line warning (probably with g_warning) and gracefully fail to play the video.
Comment 7 Alicia Boya García 2018-10-12 22:37:59 PDT
(In reply to Michael Catanzaro from comment #6)
> Alicia, this is the bug I was telling you about this morning.
> 
> I'd say we should not crash if GStreamer plugins are missing. Instead, we
> should print a command line warning (probably with g_warning) and gracefully
> fail to play the video.

In the current master this is a critical rather than a release assert, but still... The assertion is there for a reason.

You need durations and many WebM files don't provide them so we need opusparse to get them from the Opus packets. Without durations MSE algorithms don't work.

Yes, you could omit the assertion and carry on, we could even use a default because YT always uses 20 ms... but then people would inadvertently rely on that and we would have files that work for some users but fail for other users with no obvious reason.

I'd rather fail fast on every case so that packagers actually notice the problem and install the missing dependencies.
Comment 8 Michael Catanzaro 2018-10-13 08:21:12 PDT
(In reply to Alicia Boya García from comment #7)
> In the current master this is a critical rather than a release assert, but
> still...

That's even worse, since criticals are programming errors, so if you don't want it to fail gracefully, I would change it back to a release assert (which can be used to indicate a packaging issue rather than a programming error).

> I'd rather fail fast on every case so that packagers actually notice the
> problem and install the missing dependencies.

I guess it's OK, but then we need to announce the new dependency. We should also fail the build if it's missing at build time: not strictly necessary, since it's a runtime dependency, but otherwise packagers are not going to notice. I would had assumed the desired behavior would have been to just not play media if required codecs are missing, but whatever.
Comment 9 Adrian Perez 2018-10-13 13:00:27 PDT
(In reply to Michael Catanzaro from comment #8)
> (In reply to Alicia Boya García from comment #7)
> > In the current master this is a critical rather than a release assert, but
> > still...
> 
> That's even worse, since criticals are programming errors, so if you don't
> want it to fail gracefully, I would change it back to a release assert
> (which can be used to indicate a packaging issue rather than a programming
> error).

Now that I understand what the element is used for (thanks Alicia for
commenting on that), I would go with:

  if (G_UNLIKELY(!element)) {
      WTFLogAlways("Needed opusparse GStreamer element is not available.");
      abort();
  } 

It looks like a much better to have a descriptive message (which could be
even be more detailed than my example above) instead of an assertion which
may leave people puzzled wondering why something is triggering the assertion.

> > I'd rather fail fast on every case so that packagers actually notice the
> > problem and install the missing dependencies.
> 
> I guess it's OK, but then we need to announce the new dependency. We should
> also fail the build if it's missing at build time: not strictly necessary,
> since it's a runtime dependency, but otherwise packagers are not going to
> notice. I would had assumed the desired behavior would have been to just not
> play media if required codecs are missing, but whatever.

Yes, let's list the dependency and announce it properly.

With regard to checking at build time, it sounds like a can of works: one
cannot ask GStreamer to check whether an element is installed (it needs
running some code, which is a no-no for cross-compilation), and randomly
checking a few places on disk will result in false negatives. This is one
more reason why I believe it is important to provide a meaningful error
message designed to be consumed by humans before aborting the WebProcess
(and not just an assertion failure message, which most likely only us
developers will get a sense of!)
Comment 10 Alicia Boya García 2018-10-13 13:43:50 PDT
+1
Comment 11 Philippe Normand 2018-10-15 02:02:04 PDT
The check could be done in MediaPlayerPrivateGStreamerMSE::isAvailable()
Comment 12 Philippe Normand 2018-10-15 03:18:30 PDT
(In reply to Philippe Normand from comment #11)
> The check could be done in MediaPlayerPrivateGStreamerMSE::isAvailable()

I mean, MediaPlayerPrivateGStreamerMSE::supportsType() :)
Also why is the MSE implementation not checking the presence of demuxers? I don't understand why this code needed a specific implementation.
Comment 13 Alicia Boya García 2018-10-15 07:11:26 PDT
(In reply to Philippe Normand from comment #12)
> (In reply to Philippe Normand from comment #11)
> > The check could be done in MediaPlayerPrivateGStreamerMSE::isAvailable()
> 
> I mean, MediaPlayerPrivateGStreamerMSE::supportsType() :)
> Also why is the MSE implementation not checking the presence of demuxers? I
> don't understand why this code needed a specific implementation.

It was assumed that the user had opusparse installed because WebKit already relied in gst-plugins-bad for other elements... turns out this assumption is incorrect.
Comment 14 Philippe Normand 2019-02-20 07:11:20 PST
With the new GStreamerRegistryScanner opus, vorbis and h264 are now advertised as supported at runtime only if a decoder was found and a parser:

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp#L149

So I think the initial issue reported here should no longer happen :)
Comment 15 Philippe Normand 2019-03-05 07:06:37 PST
Closing, as per previous comment. Feel free to reopen if needed.