Bug 209290 – [GStreamer] Fail gracefully in the absence of a WebVTT encoder.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Charlie Turner]

[GStreamer] Fail gracefully in the absence of a WebVTT encoder.

[Charlie Turner]

Created attachment 393990 [details]
Patch

[Charlie Turner]

Comment on attachment 393990 [details]
Patch

This is not quite right yet... Taking off the review.

[Charlie Turner]

Created attachment 394077 [details]
Patch

[Philippe Normand]

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

[Charlie Turner]

Created attachment 394079 [details]
Patch

Fix reference leak, thanks for the review!

[EWS]

Committed r258832: <https://trac.webkit.org/changeset/258832>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394079 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60767735>

[Milan Crha]

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).

[Milan Crha]

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.

[Charlie Turner]

(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.

[Milan Crha]

(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

[Charlie Turner]

(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 :)

[Milan Crha]

Okay, thanks a lot.