Bug 209290 - [GStreamer] Fail gracefully in the absence of a WebVTT encoder.
Summary: [GStreamer] Fail gracefully in the absence of a WebVTT encoder.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-19 10:14 PDT by Charlie Turner
Modified: 2020-03-24 08:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2020-03-19 10:15 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2020-03-20 05:29 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2020-03-20 06:25 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2020-03-19 10:14:41 PDT
[GStreamer] Fail gracefully in the absence of a WebVTT encoder.
Comment 1 Charlie Turner 2020-03-19 10:15:06 PDT
Created attachment 393990 [details]
Patch
Comment 2 Charlie Turner 2020-03-19 10:25:17 PDT
Comment on attachment 393990 [details]
Patch

This is not quite right yet... Taking off the review.
Comment 3 Charlie Turner 2020-03-20 05:29:29 PDT
Created attachment 394077 [details]
Patch
Comment 4 Philippe Normand 2020-03-20 05:34:32 PDT
Comment on attachment 394077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394077&action=review

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:261
> +        GRefPtr<GstElementFactory> elementFactory = gst_element_factory_find("fdkaacdec");

This is transfer-full. Shouldn't we adopt the ref?

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:267
> +                GRefPtr<GstElementFactory> avAACDecoderFactory = gst_element_factory_find(elementNames[i]);

Ditto
Comment 5 Charlie Turner 2020-03-20 06:25:54 PDT
Created attachment 394079 [details]
Patch

Fix reference leak, thanks for the review!
Comment 6 EWS 2020-03-23 04:04:03 PDT
Committed r258832: <https://trac.webkit.org/changeset/258832>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394079 [details].
Comment 7 Radar WebKit Bug Importer 2020-03-23 04:05:13 PDT
<rdar://problem/60767735>
Comment 8 Milan Crha 2020-03-24 01:43:56 PDT
Could you claim that:
> WTFLogAlways("WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec");
only once per run or anything like that, please? It's boring to see it again and again, on every WebView creation (tried this with WebKitWebView from WebKitGTK+, a new WebView means a new warning on the console).
Comment 9 Milan Crha 2020-03-24 02:34:05 PDT
I just noticed, the "warning" also shows whenever the inspector is show. I can hide it and show it and it prints the "warning" again.
Comment 10 Charlie Turner 2020-03-24 04:32:54 PDT
(In reply to Milan Crha from comment #8)
> Could you claim that:
> > WTFLogAlways("WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec");
> only once per run or anything like that, please?

Due to the multi-process design of WebKit, GStreamer is initialized for each WebView you create. Accounting for whether other *processes* have issued a message is more trouble than its worth in this (special) case.

> It's boring to see it again and again.

The point is to nag you about a potentially broken system configuration and have you install the FDK AAC Codec Library. However, https://ffmpeg.org/pipermail/ffmpeg-devel/2019-July/247063.html, the upstream report for issues found in FFmpeg's implementation, has seen no follow-up, so the validity of the original checks are also questionable. For these reasons I will simply remove the warning, but leave the rank adjustments for users who have installed the alternative AAC decoder.
Comment 11 Milan Crha 2020-03-24 05:57:08 PDT
(In reply to Charlie Turner from comment #10)
> The point is to nag you about a potentially broken system configuration and
> have you install the FDK AAC Codec Library.

Right, I understood that it is vital for some use cases (to help diagnose a problem), but not for all. In my case, I do not expect WebKitGTK+ to do any of that, it's used to render emails, which are controlled a lot. It can use GStreamer for audio/video attachments, but it doesn't mean I want to see it all the time. I have also unit tests and it's quite bad to see the warning after every single test run, like below. It's nothing pleasant.

When something breaks for the users of the project I work on, there are instructions on how to enable additional debugging, which enables such prints. That makes it possible to claim, but not to bother the users. WebKit has something like WEBKIT_DEBUG environment variable, right? Maybe it should consider it.

The output from one of my unit test runs:

/paste/singleline-html2html: [rsvg_internals/src/svg.rs:84] &self.load_options.base_url = None
WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/singleline-html2plain: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/singleline-plain2html: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/singleline-plain2plain: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-html2html: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-html2plain: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-div-html2html: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-div-html2plain: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-p-html2html: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-p-html2plain: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-plain2html: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/multiline-plain2plain: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
OK
/paste/quoted-singleline-html2html: WARNING: You might have broken LC support in your AAC decoders, consider installing fdkaacdec
Comment 12 Charlie Turner 2020-03-24 06:29:36 PDT
(In reply to Milan Crha from comment #11)
> (In reply to Charlie Turner from comment #10)
> > The point is to nag you about a potentially broken system configuration and
> > have you install the FDK AAC Codec Library.
> 
> Right, I understood that it is vital for some use cases (to help diagnose a
> problem), but not for all.

Sorry for the noise. I decided to remove the warning in https://bugs.webkit.org/show_bug.cgi?id=209472 mostly because it's not clear exactly what is "broken", so its a bad warning anyway. The upstream FFmpeg bug should be followed up to find out more and I'll stop bothering users about it :)
Comment 13 Milan Crha 2020-03-24 08:07:37 PDT
Okay, thanks a lot.