Bug 233230 - [GStreamer] MediaPlayerPrivateGStreamer mishandles failure to create WebKitTextCombiner
Summary: [GStreamer] MediaPlayerPrivateGStreamer mishandles failure to create WebKitTe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Other Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
: 237383 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-11-16 19:08 PST by openaudible
Modified: 2022-03-02 17:04 PST (History)
19 users (show)

See Also:


Attachments
Patch (1.98 KB, patch)
2021-12-23 09:38 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2021-12-23 09:44 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (3.41 KB, patch)
2022-02-09 06:12 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description openaudible 2021-11-16 19:08:10 PST
Webkit on ubuntu 21.10 is failing due to a missing gstreamer plugin called subenc, which provides WebVTT captioning. 

It is available in an package called: gstreamer1.0-plugins-bad 

The "fix" is to sudo apt install gstreamer1.0-plugins-bad

According to GStreamer, Bad Plug-ins is a set of plug-ins that aren't up to par compared to the rest. They might be close to being good quality, but they're missing something - be it a good code review, some documentation, a set of tests, a real live maintainer, or some actual wide use.

So when Ubuntu 21.10 doesn't include the bad libraries, and webkit assumes they are going to be available... there is a problem. I don't know whose problem exactly.

The code that fails is calling webkitTextCombinerNew() and asserting that the object returned is not null.
Error message is: "WebKit wasn't able to find a WebVTT encoder. Not continuing without platform support for subtitles"
And library quits on ASSERT failure.

A bug fix in webkit would be to not fail if the webVTT plugin isn't available.. or use a different plugin that is "not bad"? Or improve the error message to make it clear a GStreamer plugin that provides WebVTT is not installed. 


Code that fails is webkitTextCombinerNew() in Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp
GstElement* webkitTextCombinerNew()
{
    // The combiner relies on webvttenc, fail early if it's not there.
    if (!isGStreamerPluginAvailable("subenc")) {
        WTFLogAlways("WebKit wasn't able to find a WebVTT encoder. Not continuing without platform support for subtitles.");
        return nullptr;
    }

    return GST_ELEMENT(g_object_new(WEBKIT_TYPE_TEXT_COMBINER, nullptr));
}

The plugin details for subenc are:
https://gstreamer.freedesktop.org/documentation/subenc/index.html?gi-language=c

Was googling and didn't find anyone discussing this issue or the simple apt install "fix". It was non-trivial to figure out. 
Reproduce with fresh ubuntu 21.10 follow along with https://github.com/openaudible/openaudible/issues/757 
I am mostly posting this bug report as a way to let the search engines guide users to the solution... So feel free to close as somewhat resolved..
Comment 1 Michael Catanzaro 2021-11-17 07:13:16 PST
(In reply to openaudible from comment #0)
> The code that fails is calling webkitTextCombinerNew() and asserting that
> the object returned is not null.
> Error message is: "WebKit wasn't able to find a WebVTT encoder. Not
> continuing without platform support for subtitles"

So far so good from WebKit's perspective. Printing an error message when the optional package is missing is the right thing to do.

CC: Berto because it's worth checking the Debian packaging to ensure it has a Recommends: to ensure this isn't broken by default. In Fedora I have Recommends: on both gst-plugins-good and gst-plugins-bad. I see you have a Requires: on gst-plugins-good in Debian (a little aggressive, but OK), and a Recommends: ${gst:Recommends}. I wonder what that ${gst:Recommends} evaluates to. gst-plugins-good and -bad are both optional in case users want to uninstall them, but important to have by default.

> And library quits on ASSERT failure.

That would be a WebKit bug: it shouldn't crash. But you didn't provide any info that would tell us where the problem is. Please provide a backtrace taken with gdb following the instructions at https://blogs.gnome.org/mcatanzaro/2021/09/18/creating-quality-backtraces-for-crash-reports/. Thanks.

> A bug fix in webkit would be to not fail if the webVTT plugin isn't
> available.. or use a different plugin that is "not bad"? Or improve the
> error message to make it clear a GStreamer plugin that provides WebVTT is
> not installed. 

From an upstream perspective, we just need to make it not crash. I think the error message is sufficiently detailed that somebody who has intentionally chosen not to install recommended packages should be able to figure out that a GStreamer element is missing.
Comment 2 openaudible 2021-11-17 09:34:54 PST
I think webkitTextCombinerNew() is called just once in the code base.

Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2760

Current code is:

    GstElement* textCombiner = webkitTextCombinerNew();
    ASSERT(textCombiner);
    g_object_set(m_pipeline.get(), "text-stream-combiner", textCombiner, nullptr);

Crash would be in the ASSERT(textCombiner);

You might be able to change ASSERT(textCombiner); to if (textCombiner) 

The older 2.4.0 code is quite a bit different, but also just uses ASSERT... 

This does seem like a dependency issue-- when installing webkit, one of the dependences it now needs to have is the bad plugin.. I don't entirely know who made a change that ultimately broke things.

I think just having some google results when searching for the error message will be a big help.

Thanks and good luck.
Comment 3 Michael Catanzaro 2021-11-17 09:48:23 PST
You're not hitting that ASSERT because that's a debug assert. It's disabled in release builds.

> And library quits on ASSERT failure.

How do you know this is happening? Does WebKit print something indicating that an assertion failed?

(In reply to openaudible from comment #2)
> You might be able to change ASSERT(textCombiner); to if (textCombiner) 

Probably, yes. I looked up GstPlayBin3:text-stream-combiner and it is not nullable, so that is indeed a WebKit bug. We still want to see a backtrace for your crash, though, to figure out what goes wrong next. None of that should trigger a crash, and it's not obvious where the crash occurs.
Comment 4 openaudible 2021-11-17 13:40:33 PST
I am not sure about much. I do know installing the missing library fixes it. I don’t know the exact code used on a new Ubuntu 21.10. 

If it wasn’t an assert, it would be a null pointer dereference, I assume. 

If you know how to get an html page to create an element that calls the newTextCombiner, it should fail on a fresh Ubuntu 21.10. You might also be able to reproduce by removing the bad gs library. Or temporarily return null and see if tests pass. It is possible it is already fixed in latest code but breaks in whatever source is used for 21.10. 

Apologize I don’t have better notes or skills. Was not able to build from source with debugging enabled. So do not have a full stack trace.  (A docker build would be nice for non c programmers!)

I just know it started to affect a few users of my app that uses eclipse swt browser that uses WebKit.
Comment 5 Michael Catanzaro 2021-11-17 15:25:08 PST
(In reply to openaudible from comment #4)
> If it wasn’t an assert, it would be a null pointer dereference, I assume. 

We'll find out for sure when you post a backtrace.

> If you know how to get an html page to create an element that calls the newTextCombiner,

I don't tbh, I have no idea what text combiner does either. Subtitles? Once you post a backtrace, I'll take a look again and we'll see what to do.

> Was not able to build from source with debugging enabled.

Oh no, don't try building it yourself! That's an unreasonable amount of effort to expect for a simple bug report. I do expect you to install the debug symbols provided by Ubuntu and generate a backtrace, though, since otherwise we'd rather go look at crash reports that do have backtraces instead. See the section "Debuginfo Installation" in my blog post. Thanks.
Comment 6 Michael Catanzaro 2021-11-17 15:27:07 PST
There are instructions for Ubuntu here: https://wiki.ubuntu.com/DebuggingProgramCrash
Comment 7 Alberto Garcia 2021-11-17 16:19:55 PST
(In reply to Michael Catanzaro from comment #1)
> CC: Berto because it's worth checking the Debian packaging to ensure it has
> a Recommends: to ensure this isn't broken by default. In Fedora I have
> Recommends: on both gst-plugins-good and gst-plugins-bad. I see you have a
> Requires: on gst-plugins-good in Debian (a little aggressive, but OK), and a
> Recommends: ${gst:Recommends}. I wonder what that ${gst:Recommends}
> evaluates to. gst-plugins-good and -bad are both optional in case users want
> to uninstall them, but important to have by default.

In Debian we depend on -base and -good, and recommend -gl, -bad and -libav

I don't remember the exact discussion at the moment, but I think I
depended on -good in order to have an audio sink available, otherwise
we were hitting this assertion:

https://salsa.debian.org/webkit-team/webkit/-/blob/debian/2.32.1-1_deb10u1/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp#L1322

The recommended packages are necessary in order to play videos
(YouTube etc).
Comment 8 openaudible 2021-11-17 18:13:05 PST
Here is the stack trace. I've never used apport-retrace. 

#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=139724797575232) at pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=139724797575232) at pthread_kill.c:80
#2  __GI___pthread_kill (threadid=139724797575232, signo=signo@entry=6) at pthread_kill.c:91
#3  0x00007f143d884476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007f143d86a7b7 in __GI_abort () at abort.c:79
#5  0x00007f143e09d260 in CRASH_WITH_INFO(...) () at WTF/Headers/wtf/Assertions.h:750
#6  WebCore::makeGStreamerElement () at ../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:508
#7  0x00007f14401e736e in WebCore::MediaPlayerPrivateGStreamer::createVideoSink () at ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3428
#8  0x00007f14401efef1 in WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin () at ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2741
#9  0x00007f14401f0aeb in WebCore::MediaPlayerPrivateGStreamer::load () at ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:313
#10 0x00007f143fb3c91e in WebCore::MediaPlayerPrivateInterface::load () at ../Source/WebCore/platform/graphics/MediaPlayerPrivate.h:44
#11 WebCore::MediaPlayer::loadWithNextMediaEngine () at ../Source/WebCore/platform/graphics/MediaPlayer.cpp:595
#12 0x00007f143fb3cf11 in WebCore::MediaPlayer::load () at ../Source/WebCore/platform/graphics/MediaPlayer.cpp:474
#13 0x00007f143f5ffafb in WebCore::HTMLMediaElement::loadResource () at ../Source/WebCore/html/HTMLMediaElement.cpp:1576
#14 0x00007f143f600ab5 in WebCore::HTMLMediaElement::loadNextSourceChild () at ../Source/WebCore/html/HTMLMediaElement.cpp:1449
#15 0x00007f143f3ff72a in WTF::Function<void ()>::operator()() const () at WTF/Headers/wtf/Function.h:82
#16 WebCore::EventLoopFunctionDispatchTask::execute () at ../Source/WebCore/dom/EventLoop.cpp:159
#17 WebCore::EventLoop::run () at ../Source/WebCore/dom/EventLoop.cpp:123
#18 0x00007f143f4897d1 in WebCore::WindowEventLoop::didReachTimeToRun () at ../Source/WebCore/dom/WindowEventLoop.cpp:120
#19 0x00007f143fa96de9 in WebCore::ThreadTimers::sharedTimerFiredInternal () at ../Source/WebCore/platform/ThreadTimers.cpp:127
#20 WebCore::ThreadTimers::sharedTimerFiredInternal () at ../Source/WebCore/platform/ThreadTimers.cpp:99
#21 0x00007f143d076415 in operator() () at ../Source/WTF/wtf/glib/RunLoopGLib.cpp:177

If I'm reading the right source, I should get a friendly message saying "GStreamer element %s not found. Please install it"... ? But don't see it.
Comment 9 Xabier Rodríguez Calvar 2021-11-17 23:20:28 PST
(In reply to openaudible from comment #8)
> Here is the stack trace. I've never used apport-retrace. 
> 
> #0  __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=139724797575232) at pthread_kill.c:44
> #1  __pthread_kill_internal (signo=6, threadid=139724797575232) at
> pthread_kill.c:80
> #2  __GI___pthread_kill (threadid=139724797575232, signo=signo@entry=6) at
> pthread_kill.c:91
> #3  0x00007f143d884476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4  0x00007f143d86a7b7 in __GI_abort () at abort.c:79
> #5  0x00007f143e09d260 in CRASH_WITH_INFO(...) () at
> WTF/Headers/wtf/Assertions.h:750
> #6  WebCore::makeGStreamerElement () at
> ../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:508
> #7  0x00007f14401e736e in
> WebCore::MediaPlayerPrivateGStreamer::createVideoSink () at
> ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.
> cpp:3428
> #8  0x00007f14401efef1 in
> WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin () at
> ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.
> cpp:2741
> #9  0x00007f14401f0aeb in WebCore::MediaPlayerPrivateGStreamer::load () at
> ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.
> cpp:313
> #10 0x00007f143fb3c91e in WebCore::MediaPlayerPrivateInterface::load () at
> ../Source/WebCore/platform/graphics/MediaPlayerPrivate.h:44
> #11 WebCore::MediaPlayer::loadWithNextMediaEngine () at
> ../Source/WebCore/platform/graphics/MediaPlayer.cpp:595
> #12 0x00007f143fb3cf11 in WebCore::MediaPlayer::load () at
> ../Source/WebCore/platform/graphics/MediaPlayer.cpp:474
> #13 0x00007f143f5ffafb in WebCore::HTMLMediaElement::loadResource () at
> ../Source/WebCore/html/HTMLMediaElement.cpp:1576
> #14 0x00007f143f600ab5 in WebCore::HTMLMediaElement::loadNextSourceChild ()
> at ../Source/WebCore/html/HTMLMediaElement.cpp:1449
> #15 0x00007f143f3ff72a in WTF::Function<void ()>::operator()() const () at
> WTF/Headers/wtf/Function.h:82
> #16 WebCore::EventLoopFunctionDispatchTask::execute () at
> ../Source/WebCore/dom/EventLoop.cpp:159
> #17 WebCore::EventLoop::run () at ../Source/WebCore/dom/EventLoop.cpp:123
> #18 0x00007f143f4897d1 in WebCore::WindowEventLoop::didReachTimeToRun () at
> ../Source/WebCore/dom/WindowEventLoop.cpp:120
> #19 0x00007f143fa96de9 in WebCore::ThreadTimers::sharedTimerFiredInternal ()
> at ../Source/WebCore/platform/ThreadTimers.cpp:127
> #20 WebCore::ThreadTimers::sharedTimerFiredInternal () at
> ../Source/WebCore/platform/ThreadTimers.cpp:99
> #21 0x00007f143d076415 in operator() () at
> ../Source/WTF/wtf/glib/RunLoopGLib.cpp:177
> 
> If I'm reading the right source, I should get a friendly message saying
> "GStreamer element %s not found. Please install it"... ? But don't see it.

This should be stored in the journal or wherever logs go, but it's crashing in a RELEASE_ASSERT because WebKit can't function for that operation without the required GStreamer plugin.
Comment 10 Adrian Perez 2021-11-18 03:24:51 PST
(In reply to openaudible from comment #8)
> Here is the stack trace. I've never used apport-retrace. 
> 
> #0  __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=139724797575232) at pthread_kill.c:44
> #1  __pthread_kill_internal (signo=6, threadid=139724797575232) at
> pthread_kill.c:80
> #2  __GI___pthread_kill (threadid=139724797575232, signo=signo@entry=6) at
> pthread_kill.c:91
> #3  0x00007f143d884476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4  0x00007f143d86a7b7 in __GI_abort () at abort.c:79
> #5  0x00007f143e09d260 in CRASH_WITH_INFO(...) () at
> WTF/Headers/wtf/Assertions.h:750
> #6  WebCore::makeGStreamerElement () at
> ../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:508
> #7  0x00007f14401e736e in
> WebCore::MediaPlayerPrivateGStreamer::createVideoSink () at
> ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.
> cpp:3428
> #8  0x00007f14401efef1 in
> WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin () at
> ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.
> cpp:2741
> #9  0x00007f14401f0aeb in WebCore::MediaPlayerPrivateGStreamer::load () at
> ../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.
> cpp:313
> #10 0x00007f143fb3c91e in WebCore::MediaPlayerPrivateInterface::load () at
> ../Source/WebCore/platform/graphics/MediaPlayerPrivate.h:44
> #11 WebCore::MediaPlayer::loadWithNextMediaEngine () at
> ../Source/WebCore/platform/graphics/MediaPlayer.cpp:595
> #12 0x00007f143fb3cf11 in WebCore::MediaPlayer::load () at
> ../Source/WebCore/platform/graphics/MediaPlayer.cpp:474
> #13 0x00007f143f5ffafb in WebCore::HTMLMediaElement::loadResource () at
> ../Source/WebCore/html/HTMLMediaElement.cpp:1576
> #14 0x00007f143f600ab5 in WebCore::HTMLMediaElement::loadNextSourceChild ()
> at ../Source/WebCore/html/HTMLMediaElement.cpp:1449
> #15 0x00007f143f3ff72a in WTF::Function<void ()>::operator()() const () at
> WTF/Headers/wtf/Function.h:82
> #16 WebCore::EventLoopFunctionDispatchTask::execute () at
> ../Source/WebCore/dom/EventLoop.cpp:159
> #17 WebCore::EventLoop::run () at ../Source/WebCore/dom/EventLoop.cpp:123
> #18 0x00007f143f4897d1 in WebCore::WindowEventLoop::didReachTimeToRun () at
> ../Source/WebCore/dom/WindowEventLoop.cpp:120
> #19 0x00007f143fa96de9 in WebCore::ThreadTimers::sharedTimerFiredInternal ()
> at ../Source/WebCore/platform/ThreadTimers.cpp:127
> #20 WebCore::ThreadTimers::sharedTimerFiredInternal () at
> ../Source/WebCore/platform/ThreadTimers.cpp:99
> #21 0x00007f143d076415 in operator() () at
> ../Source/WTF/wtf/glib/RunLoopGLib.cpp:177
> 
> If I'm reading the right source, I should get a friendly message saying
> "GStreamer element %s not found. Please install it"... ? But don't see it.

That comes from the makeGStreamerElement() helper function (source in
Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp) indeed,
and as Michael mentioned it typically goes to the journal. As

From your backtrace and knowing that you are running Ubuntu 21.10, that
means it's WebKitGTK 2.34.0 or 2.34.1, which means frame #7 is doing this:

    if (!m_player->isVideoPlayer()) {
        m_videoSink = makeGStreamerElement("fakevideosink", nullptr);
        return m_videoSink.get();
    }

This element comes from libgstdebugutilsbad.so, which is part of the
gstreamer1.0-plugins-bad Ubuntu package. It looks to me like a packaging
issue, and that distro packages should depend on -bad, not only recommend
it.
Comment 11 openaudible 2021-11-18 07:34:55 PST
Great. I don't know what/where the journal is.. but a message stderr/stdout would be nice in WTFCrashWithInfo.

I do think there is just a dependency issue going on. Make bad plugins required? That missing plug-in "might" be causing other bugs I didn't uncover. 

Thanks for looking into this and your hard work. I don't envy all those bugs in your tracker. 

Cheers
Comment 12 Michael Catanzaro 2021-11-18 08:53:35 PST
(In reply to Adrian Perez from comment #10)
> This element comes from libgstdebugutilsbad.so, which is part of the
> gstreamer1.0-plugins-bad Ubuntu package. It looks to me like a packaging
> issue, and that distro packages should depend on -bad, not only recommend
> it.

Well that's for Calvaris and the other WebKit/GStreamer developers to decide, certainly not for me. I have no strong opinion either way. I'm fine with changing Fedora's packaging to use Requires instead of Recommends if crashing is the desired behavior when the plugins are missing. But look at this:

    // The combiner relies on webvttenc, fail early if it's not there.
    if (!isGStreamerPluginAvailable("subenc")) {
        WTFLogAlways("WebKit wasn't able to find a WebVTT encoder. Not continuing without platform support for subtitles.");
        return nullptr;
    }

The existence of this check sort of implies the plugin is optional, and expected behavior if missing is to fail gracefully without crashing, right? Whichever strategy is adopted to handle missing GStreamer elements, the code should be consistent. I could be wrong, but I thought the strategy was that we can crash if base elements are missing, since WebKit cannot build without those, but -good or -bad elements use runtime checks since those could be easily forgotten by mistake. If the strategy is to always crash, that's fine too and I'll adjust our packaging.
Comment 13 Michael Catanzaro 2021-11-18 08:54:26 PST
(In reply to openaudible from comment #8)
> Here is the stack trace.

That helps, thanks. That's a good stacktrace.
Comment 14 Adrian Perez 2021-11-18 10:51:09 PST
(In reply to Michael Catanzaro from comment #12)
> (In reply to Adrian Perez from comment #10)
> > This element comes from libgstdebugutilsbad.so, which is part of the
> > gstreamer1.0-plugins-bad Ubuntu package. It looks to me like a packaging
> > issue, and that distro packages should depend on -bad, not only recommend
> > it.
> 
> Well that's for Calvaris and the other WebKit/GStreamer developers to
> decide, certainly not for me. I have no strong opinion either way. I'm fine
> with changing Fedora's packaging to use Requires instead of Recommends if
> crashing is the desired behavior when the plugins are missing. But look at
> this:
> 
>     // The combiner relies on webvttenc, fail early if it's not there.
>     if (!isGStreamerPluginAvailable("subenc")) {
>         WTFLogAlways("WebKit wasn't able to find a WebVTT encoder. Not
> continuing without platform support for subtitles.");
>         return nullptr;
>     }
> 
> The existence of this check sort of implies the plugin is optional, and
> expected behavior if missing is to fail gracefully without crashing, right?
> Whichever strategy is adopted to handle missing GStreamer elements, the code
> should be consistent. I could be wrong, but I thought the strategy was that
> we can crash if base elements are missing, since WebKit cannot build without
> those, but -good or -bad elements use runtime checks since those could be
> easily forgotten by mistake. If the strategy is to always crash, that's fine
> too and I'll adjust our packaging.

I think that I did not manage to make my point clear earlier..

 * WebVTT (subtitles) support is optional, and as you correctly point out
   there is a runtime check on the “subenc“ element.

 * OTOH gst-plugins-bad is *NOT* optional because media elements which do
   not have video (I guess they are audio-only) will use “fakevideosink“
   unconditionally. Line 3428 in MediaPlayerPrivateGStreamer.cpp (frame #7)
   from the Very Good Indeed™ stacktrace is using makeGStreamerElement(),
   which will assert that the element was created.
Comment 15 Michael Catanzaro 2021-11-18 13:12:48 PST
(In reply to Adrian Perez from comment #14)
>  * WebVTT (subtitles) support is optional, and as you correctly point out
>    there is a runtime check on the “subenc“ element.
> 
>  * OTOH gst-plugins-bad is *NOT* optional because media elements which do
>    not have video (I guess they are audio-only) will use “fakevideosink“
>    unconditionally. Line 3428 in MediaPlayerPrivateGStreamer.cpp (frame #7)
>    from the Very Good Indeed™ stacktrace is using makeGStreamerElement(),
>    which will assert that the element was created.

What I don't understand is whether this crash is the desired behavior or not. Clearly both Berto and myself assumed otherwise, or we would have had Requires in our packaging. If this is the desired end result, then we can say I was wrong about it being optional, close the WebKit bug here, and go add our downstream Requires. But it seems inconsistent with how we handle other missing GStreamer elements, yes?

Anyway... yeah, it's a good stacktrace. :P The place where the crash happens is not even close to where OP thought it was happening. That's why we need stacktraces.
Comment 16 Philippe Normand 2021-12-23 09:38:59 PST
Created attachment 447881 [details]
Patch
Comment 17 Philippe Normand 2021-12-23 09:44:08 PST
Created attachment 447882 [details]
Patch
Comment 18 Michael Catanzaro 2021-12-23 11:58:53 PST
Can you confirm that we should add Requires for gst-plugins-bad to downstream packaging, in addition to both -good and -base?
Comment 19 Philippe Normand 2021-12-23 12:34:49 PST
(In reply to Michael Catanzaro from comment #18)
> Can you confirm that we should add Requires for gst-plugins-bad to
> downstream packaging, in addition to both -good and -base?

Yes, due mostly to fakevideosink, I'm afraid (and some day for GstWebRTC, unless it moves elsewhere).
Comment 20 EWS 2021-12-23 12:57:43 PST
Committed r287410 (245545@main): <https://commits.webkit.org/245545@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447882 [details].
Comment 21 Radar WebKit Bug Importer 2021-12-23 12:58:17 PST
<rdar://problem/86863068>
Comment 22 Michael Catanzaro 2022-02-08 14:08:45 PST
Due to some downstream complaints about increased install media size, I'll ask one more time just to be perfectly sure we have this correct:

Option 1: if GStreamer elements are missing, WebKit will print a warning, fail to play media, and not crash. This corresponds to a Recommends dependency downstream.

Option 2: if GStreamer elements are missing, WebKit will gracefully crash. This corresponds to a Requires dependency downstream.

I've implemented Option 2 downstream per feedback in this bug, but that doesn't seem entirely consistent with WebKit's actual behavior. Do you still agree that Option 2 (Requires) is what we are aiming for?

In retrospect, Option 1 seemed nicer to me, but of course that's up to GStreamer port maintainers to decide, not up to me.
Comment 23 Adam Williamson 2022-02-08 14:45:30 PST
Just to chime in with a few more notes from the downstream issue here - https://bugzilla.redhat.com/show_bug.cgi?id=2031213 : the actual use case here is the Fedora (and RHEL) installer. In this environment yelp (GNOME's help browser) is installed so that we can display the installer's own help pages to the user. yelp uses webkit for rendering.

There is no general-purpose web browser in the environment. There are no video or audio files for webkit to play. There are only help pages to be displayed in yelp.

So it's a bit sad that we now have a dozen entirely useless libraries related to audio and video playback pulled into the installer environment, which needs to be as streamlined as possible.

We do have a mechanism by which we can override the dependencies and rip the packages back out when build the installer, but this is somewhat fragile if the package names change. We'll have to do that if webkit isn't going to fail more gracefully in this kind of case, I guess, but it'd be nice not to have to.
Comment 24 Xabier Rodríguez Calvar 2022-02-08 22:10:01 PST
(In reply to Adam Williamson from comment #23)
> Just to chime in with a few more notes from the downstream issue here -
> https://bugzilla.redhat.com/show_bug.cgi?id=2031213 : the actual use case
> here is the Fedora (and RHEL) installer. In this environment yelp (GNOME's
> help browser) is installed so that we can display the installer's own help
> pages to the user. yelp uses webkit for rendering.
> 
> There is no general-purpose web browser in the environment. There are no
> video or audio files for webkit to play. There are only help pages to be
> displayed in yelp.
> 
> So it's a bit sad that we now have a dozen entirely useless libraries
> related to audio and video playback pulled into the installer environment,
> which needs to be as streamlined as possible.
> 
> We do have a mechanism by which we can override the dependencies and rip the
> packages back out when build the installer, but this is somewhat fragile if
> the package names change. We'll have to do that if webkit isn't going to
> fail more gracefully in this kind of case, I guess, but it'd be nice not to
> have to.

If it's in the installer, I guess you can build WebKit without the VIDEO option active maybe even as a static library.
Comment 25 Adam Williamson 2022-02-09 00:23:12 PST
Unfortunately that's not an option - we don't build the installer components separately, the image is built out of the regular distro packages.

If there's nothing else that can be done we can strip the packages out in the 'cleanup' phase of the installer build, it's just slightly sucky :/
Comment 26 Philippe Normand 2022-02-09 06:09:58 PST
incoming patch to gracefully handle missing fakevideosink
Comment 27 Philippe Normand 2022-02-09 06:12:11 PST
Created attachment 451364 [details]
Patch
Comment 28 Michael Catanzaro 2022-02-09 06:17:45 PST
(In reply to Michael Catanzaro from comment #22)
> Option 1: if GStreamer elements are missing, WebKit will print a warning,
> fail to play media, and not crash. This corresponds to a Recommends
> dependency downstream.

I'll let Calvaris or another multimedia reviewer approve this. But it looks good to me.

For avoidance of doubt, this implements Option 1.
Comment 29 Adam Williamson 2022-02-09 09:13:37 PST
That's awesome, thanks very much!
Comment 30 EWS 2022-02-09 10:14:08 PST
Committed r289480 (247022@main): <https://commits.webkit.org/247022@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451364 [details].
Comment 31 Michael Catanzaro 2022-03-02 17:04:01 PST
*** Bug 237383 has been marked as a duplicate of this bug. ***