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?
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 :)
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 :)
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.
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).
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...
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 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.
(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.
(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!)
+1
The check could be done in MediaPlayerPrivateGStreamerMSE::isAvailable()
(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.
(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.
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 :)
Closing, as per previous comment. Feel free to reopen if needed.