Bug 135972 – [GStreamer] unrelated codecs required to play videos

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Michael Catanzaro]

WebKit can't play videos on youtube.com (or much anywhere else) in Fedora, which only distributes gst-plugins-good and about half of gst-plugins-bad -- therefore we expect playing MP4 videos to not work, but WebM videos to play fine. For example, [1] is available in WebM so it should work, and it plays fine in Firefox, but it does not work in Epiphany. Attempting to play it in Epiphany brings up an unwanted PackageKit dialog that tries to find an appropriate gstreamer plugin, but fails since it does not exist in Fedora. I think this problem will exist in all Red Hat and SUSE distros, and I bet you'll be able to reproduce on a Debian distro by removing gst-plugins-libav, which I think is the one that handles H.264.

If I remove "video/mp4" from mimeTypeCache() in MediaPlayerPrivateGStreamer.cpp, the video will then play fine. Maybe this list should not be hardcoded, but mapped to the gstreamer plugins that are actually installed.

[1] https://www.youtube.com/watch?v=LvCW2BTYkeQ

[Philippe Normand]

This would basically mean reverting http://trac.webkit.org/changeset/117207

[Michael Catanzaro]

I think so. Thanks for working on this!

[Philippe Normand]

I'm not really enthusiastic about reverting that and reintroducing all these hacks in the code...

[Michael Catanzaro]

Well ideally gstreamer would give us a list of mime types, but that seems unlikely judging from the comments I've found at [1].  Probably mapping between capabilities and mime types is what we have to do. :/  (And a hardcoded list of guesses is not really so much less hacky.)

Anything else we can do to make the videos play? We could hardcode a hack for just MP4 and MP3, I suppose. Or we could have separate hardcoded lists of mime types for the codecs provided by the different gst-plugins packages, and try to detect which packages are installed. These don't seem better to me....

[1] https://bugs.kde.org/show_bug.cgi?id=267319#c25

[Sebastian Dröge (slomo)]

We could make the mime type list more dynamic relatively easily. Not by reverting that commit though because the original code was as wrong as the current one on that regard.

Instead of looking at typefinders we should use the GstElementFactory API to check:
- for container formats if a demuxer exists
- for codecs if a decoder exists

Remaining question is: What does e.g. "video/mp4" mean? An MP4 container stream... with which codecs? MPEG-4 Part 2? H264? H263? AAC? MP3? ...?

Is there somewhere a definition of what all these mimetypes actually mean in the context of the web, or is there a way to get more details?
I remember that you usually ask if the browser supports e.g. 'video/mp4; codecs="mp4v.20.8"', is there an extensive list of what is allowed there?

[Sebastian Dröge (slomo)]

Relevant part of the spec:
http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#mime-types

So considering this, I think the best we could do here would be if we could directly override media.canPlayType() with our own implementation instead of just passing a list of mimetypes we might support to the upper layers.

Inside the media.canPlayType() implementation we could then check directly against the available GStreamer plugins and handle missing mime type details accordingly.

For example for
- video/mp4 we could check if: a) a MP4 demuxer is available, b) at least one video or audio decoder for H264, MPEG-4 Part 2, MP3 or AAC is available. Here we would return "maybe"
- video/mp4,codec=mp4v.20.8" check if: a) a MP4 demuxer is available, b) a MPEG-4 Part 2 video decoder is available. Here we can probably return "probably" instead of "maybe" then


Opinions?

[Sebastian Dröge (slomo)]

Note that checking these things against the GStreamer registry is fast if that looks like a possible problem.

[Sebastian Dröge (slomo)]

So we could just reimplement MediaPlayerPrivateGStreamer::supportsType() in a way that always checks against the available element factories, maybe with caching of results if it becomes a bottleneck.

[Michael Catanzaro]

Since I don't understand slomo's proposed fix :), I am thinking as a very quick workaround to just check if the relevant gstreamer plugin exists on the filesystem and add video/mp4 to the list of supported MIME types only if so. I don't normally like stupid workarounds, but it's hard to understate how bad "cannot play any videos" is; it makes it impossible to promote WebKitGTK+ in Red Hat land.

[Michael Catanzaro]

Created attachment 258010 [details]
hack patch, makes YouTube work in Fedora

Obviously not intended for upstream, but this is what I will use in Fedora until this bug can be fixed properly.

[Michael Catanzaro]

Example code from Sebastian, for a non-terrible solution to this (I'm posting it here so that I don't lose it):

GList *decoder_factories, *h264_decoders;
GstCaps *caps;

decoder_factories = gst_element_factory_list_get_elements (GST_ELEMENT_FACTORY_TYPE_DECODER |
        GST_ELEMENT_FACTORY_TYPE_MEDIA_VIDEO,
        GST_RANK_MARGINAL);

// Repeat this for all caps you care about

caps = gst_caps_new_empty_simple ("video/x-h264");
h264_decoders = gst_element_factory_list_filter (decoder_factories, caps, GST_PAD_SINK, FALSE);
gst_caps_unref (caps);

if (h264_decoders != NULL) {
  // found h264 decoder
}

gst_plugin_feature_list_free (h264_decoders);

// end repeat

gst_plugin_feature_list_free (decoder_factories);

[Philippe Normand]

You'll also need to somehow clear the mimeType cache whenever a new plugin is installed so the new decoder factories are also checked and the cache correctly represents the list of supported media types.

[Mario Sanchez Prada]

For the record, I'm pasting below the patch that I just applied downstream yesterday that is serving us well for the time being in our particular and controlled scenario.

Notice that I'm not asking for review over it, as I understand that solution is not valid for upstream, just sharing in case it's useful to someone else. That said, if someone wants to take over on making a real patch, or guiding me on what exactly would be a good enough way to do it, I'm happy to help. 

--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
@@ -1690,6 +1690,25 @@ void MediaPlayerPrivateGStreamer::loadin
     m_readyTimerHandler.cancel();
 }
 
+static bool gstRegistryHasDecoder(const String& decoder)
+{
+    GList *decoder_factories, *gst_decoders;
+    GstCaps *caps;
+    bool retval;
+
+    decoder_factories = gst_element_factory_list_get_elements (GST_ELEMENT_FACTORY_TYPE_DECODER | GST_ELEMENT_FACTORY_TYPE_MEDIA_VIDEO, GST_RANK_MARGINAL);
+    caps = gst_caps_new_empty_simple (decoder.utf8().data());
+    gst_decoders = gst_element_factory_list_filter (decoder_factories, caps, GST_PAD_SINK, FALSE);
+    gst_caps_unref (caps);
+
+    retval = gst_decoders != NULL;
+
+    gst_plugin_feature_list_free (gst_decoders);
+    gst_plugin_feature_list_free (decoder_factories);
+
+    return retval;
+}
+
 static HashSet<String> mimeTypeCache()
 {
     initializeGStreamerAndRegisterWebKitElements();
@@ -1768,7 +1787,6 @@ static HashSet<String> mimeTypeCache()
         "video/flv",
         "video/mj2",
         "video/mp2t",
-        "video/mp4",
         "video/mpeg",
         "video/mpegts",
         "video/ogg",
@@ -1795,6 +1813,15 @@ static HashSet<String> mimeTypeCache()
     for (unsigned i = 0; i < (sizeof(mimeTypes) / sizeof(*mimeTypes)); ++i)
         cache.add(String(mimeTypes[i]));
 
+    if (gstRegistryHasDecoder(String::fromUTF8("video/x-h264")))
+        cache.add(String::fromUTF8("video/mp4"));
+
     typeListInitialized = true;
     return cache;
 }

[Michael Catanzaro]

I think what we want to do is have mimeTypeCache be initially empty, and add each MIME type based on the result of a call to gstRegistryHasDecoder. The problem is, the MIME types ("video/mp4") are different than the values we must pass to gstRegistryHasDecoder ("video/x-h264") -- and there's not even going to be a 1-1 correspondence here, e.g. in Fedora we will be shipping OpenH264, which handles H.264 as used in WebRTC, but cannot be used to play MP4 videos. We also probably don't want to claim to support MP4 if, for example, we don't have support for AAC, since we'd get bug reports that videos play without sound, which users will assume to be our fault. (Red Hat is planning to add MP4 support to OpenH264, but I'm not aware of any plans to license AAC, so this will become a practical problem in the near future.)

So we need someone with serious multimedia experience to think about this. :)

[Xabier Rodríguez Calvar]

(In reply to comment #6)
> Relevant part of the spec:
> http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#mime-types
> 
> So considering this, I think the best we could do here would be if we could
> directly override media.canPlayType() with our own implementation instead of
> just passing a list of mimetypes we might support to the upper layers.
> 
> Inside the media.canPlayType() implementation we could then check directly
> against the available GStreamer plugins and handle missing mime type details
> accordingly.
> 
> For example for
> - video/mp4 we could check if: a) a MP4 demuxer is available, b) at least
> one video or audio decoder for H264, MPEG-4 Part 2, MP3 or AAC is available.
> Here we would return "maybe"
> - video/mp4,codec=mp4v.20.8" check if: a) a MP4 demuxer is available, b) a
> MPEG-4 Part 2 video decoder is available. Here we can probably return
> "probably" instead of "maybe" then
> 
> Opinions?

I think this is the way to go.

[Philippe Normand]

Created attachment 278929 [details]
patch

[Philippe Normand]

Created attachment 278930 [details]
patch

[Philippe Normand]

Comment on attachment 278930 [details]
patch

I'll try a variant without the function.

[Philippe Normand]

Created attachment 278931 [details]
patch

Ah well, a function isn't easy to avoid here :)

[Darin Adler]

Comment on attachment 278931 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1675
> +            set.add(String::fromUTF8("video/quicktime"));

This is an inefficient idiom for WebKit.

Each call to HashSet::add is a large amount of inlined code, so it’s not good to have a function that calls it more than a couple times. Instead it’s important to use a loop or non-inlined helper function. That’s why I had changed this into a loop before, so there’s only a single call to add.

To convert a C literal into a string, the thing to use is either ASCIILiteral or the default constructor, each of which will do the correct thing for all ASCII characters. There is no reason to use String::fromUTF8 unless there are non-ASCII characters involved. It’s less efficient. The benefit of using ASCIILiteral is that the code won’t copy all the characters. Another possibility is to use AtomicString; it’s good for long-lived strings that might also appear as HTML attribute values since the string will be re-used whenever the same string occurs.

[Darin Adler]

When I say “use AtomicString”, I don’t mean actually mean we should change the type of the HashSet value to AtomicString, just that we would construct an AtomicString use it as a plain string. This puts it into the AtomicString hash table, which has a cost in memory and slight performance cost since the table is larger, but guarantees we will share the string with any future AtomicString with the same value, which is the associated benefit.

[Sebastian Dröge (slomo)]

Comment on attachment 278931 [details]
patch

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

Generally looks good

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1675
>> +            set.add(String::fromUTF8("video/quicktime"));
> 
> This is an inefficient idiom for WebKit.
> 
> Each call to HashSet::add is a large amount of inlined code, so it’s not good to have a function that calls it more than a couple times. Instead it’s important to use a loop or non-inlined helper function. That’s why I had changed this into a loop before, so there’s only a single call to add.
> 
> To convert a C literal into a string, the thing to use is either ASCIILiteral or the default constructor, each of which will do the correct thing for all ASCII characters. There is no reason to use String::fromUTF8 unless there are non-ASCII characters involved. It’s less efficient. The benefit of using ASCIILiteral is that the code won’t copy all the characters. Another possibility is to use AtomicString; it’s good for long-lived strings that might also appear as HTML attribute values since the string will be re-used whenever the same string occurs.

You could make this some kind of table for the common cases at least (struct { caps; vector<mimetypes> }), but not for all (especially the container-codec relationships). Or add a non-inlineable wrapper function around HashSet::add() so the code is only included once

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1700
> +            set.add(String::fromUTF8("video/x-m4v"));

This seems weird. Is video/mp4 and video/x-m4v really always h264? Shouldn't this be more like video/mp4,codec=mp4v.20.8 or so? And you also need qtdemux to be able to play video/mp4, that is application/x-quicktime

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1709
> +        if (gstRegistryHasElementForMediaType(videoDecoderFactories, "video/mpeg, mpegversion=(int)1"))

mpegversion=2 should also be here, they're almost the same codec. Also systemstream=false if this is about the video codec and not the container

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1710
> +            set.add(String::fromUTF8("video/mpeg"));

Is this about the container format or the video codec?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1723
> +                set.add(String::fromUTF8("video/ogg"));

What about Ogg/Opus?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1770
> +            set.add(String::fromUTF8("audio/x-wavpack-correction"));

We currently don't really handle correction files

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1790
> +            if (gstRegistryHasElementForMediaType(videoDecoderFactories, "video/x-vp8") || gstRegistryHasElementForMediaType(videoDecoderFactories, "video/x-vp9"))

vp10?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1793
> +            if (vorbisSupported)

|| opusSupported?

[Michael Catanzaro]

I will just say: it would be really nice if this functionality could live somewhere in GStreamer. It's inevitably going to grow stale in WebKit when new content types are added in GStreamer.

[Sebastian Dröge (slomo)]

Are these mimetypes actually defined in a meaningful way somewhere? But yes, a mimetype<->caps mapping would be useful to have somewhere.

[Mario Sanchez Prada]

This is great. Thanks Philip for taking this over!

[Philippe Normand]

Comment on attachment 278931 [details]
patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1675
>>> +            set.add(String::fromUTF8("video/quicktime"));
>> 
>> This is an inefficient idiom for WebKit.
>> 
>> Each call to HashSet::add is a large amount of inlined code, so it’s not good to have a function that calls it more than a couple times. Instead it’s important to use a loop or non-inlined helper function. That’s why I had changed this into a loop before, so there’s only a single call to add.
>> 
>> To convert a C literal into a string, the thing to use is either ASCIILiteral or the default constructor, each of which will do the correct thing for all ASCII characters. There is no reason to use String::fromUTF8 unless there are non-ASCII characters involved. It’s less efficient. The benefit of using ASCIILiteral is that the code won’t copy all the characters. Another possibility is to use AtomicString; it’s good for long-lived strings that might also appear as HTML attribute values since the string will be re-used whenever the same string occurs.
> 
> You could make this some kind of table for the common cases at least (struct { caps; vector<mimetypes> }), but not for all (especially the container-codec relationships). Or add a non-inlineable wrapper function around HashSet::add() so the code is only included once

Ok I think I managed to simplify this part of the code, thanks for the suggestions :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1700
>> +            set.add(String::fromUTF8("video/x-m4v"));
> 
> This seems weird. Is video/mp4 and video/x-m4v really always h264? Shouldn't this be more like video/mp4,codec=mp4v.20.8 or so? And you also need qtdemux to be able to play video/mp4, that is application/x-quicktime

m4v seems to be an Apple-specific variant of mp4 with DRM added on top. We can't add the codec field in the mime-type, this field is checked separately.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1709
>> +        if (gstRegistryHasElementForMediaType(videoDecoderFactories, "video/mpeg, mpegversion=(int)1"))
> 
> mpegversion=2 should also be here, they're almost the same codec. Also systemstream=false if this is about the video codec and not the container

Ok

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1710
>> +            set.add(String::fromUTF8("video/mpeg"));
> 
> Is this about the container format or the video codec?

The codec

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1723
>> +                set.add(String::fromUTF8("video/ogg"));
> 
> What about Ogg/Opus?

I'm not sure what the mime-type should be.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1770
>> +            set.add(String::fromUTF8("audio/x-wavpack-correction"));
> 
> We currently don't really handle correction files

Ok I'll remove this then

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1790
>> +            if (gstRegistryHasElementForMediaType(videoDecoderFactories, "video/x-vp8") || gstRegistryHasElementForMediaType(videoDecoderFactories, "video/x-vp9"))
> 
> vp10?

Why not :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1793
>> +            if (vorbisSupported)
> 
> || opusSupported?

Yep

[Philippe Normand]

Created attachment 279237 [details]
patch

[WebKit Commit Bot]

Attachment 279237 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1675:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1676:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1677:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1687:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1688:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1693:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1695:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1698:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1699:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1701:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1703:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1705:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1706:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1707:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1708:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1709:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 16 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 279238 [details]
patch

[Xabier Rodríguez Calvar]

Comment on attachment 279238 [details]
patch

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

> Source/WebCore/ChangeLog:24
> +        * platform/graphics/gstreamer/GStreamerUtilities.cpp:
> +        (WebCore::gstRegistryHasElementForMediaType):
> +        * platform/graphics/gstreamer/GStreamerUtilities.h:
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +        (WebCore::mimeTypeSet):
> +        (WebCore::MediaPlayerPrivateGStreamer::getSupportedTypes):
> +        (WebCore::MediaPlayerPrivateGStreamer::supportsType):
> +        (WebCore::mimeTypeCache): Deleted.

Nit: I would be a bit more verbose here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:189
> +bool gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString)

Useless complain: this function should be included in GStreamer, it could be more efficient.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1711
> +            {AudioDecoder, "audio/midi", {"audio/midi", "audio/riff-midi"}},
> +            {AudioDecoder, "audio/x-sbc", { }},
> +            {AudioDecoder, "audio/x-sid", { }},
> +            {AudioDecoder, "audio/x-flac", {"audio/x-flac", "audio/flac"}},
> +            {AudioDecoder, "audio/x-wav", {"audio/x-wav", "audio/wav"}},
> +            {AudioDecoder, "audio/x-wavpack", {"audio/x-wavpack"}},
> +            {AudioDecoder, "audio/x-speex", {"audio/speex", "audio/x-speex"}},
> +            {AudioDecoder, "audio/x-ac3", { }},
> +            {AudioDecoder, "audio/x-eac3", {"audio/x-ac3"}},
> +            {AudioDecoder, "audio/x-dts", { }},
> +            {VideoDecoder, "video/x-h264, profile=(string)high", {"video/mp4", "video/x-m4v"}},
> +            {VideoDecoder, "video/x-msvideocodec", {"video/x-msvideo"}},
> +            {VideoDecoder, "video/x-h263", { }},
> +            {VideoDecoder, "video/mpegts", { }},
> +            {VideoDecoder, "video/mpeg, mpegversion=(int){1,2}, systemstream=(boolean)false", {"video/mpeg"}},
> +            {VideoDecoder, "video/x-dirac", { }},
> +            {VideoDecoder, "video/x-flash-video", {"video/flv", "video/x-flv"}},
> +            {Demuxer, "video/quicktime", { }},
> +            {Demuxer, "video/quicktime, variant=(string)3gpp", {"video/3gpp"}},
> +            {Demuxer, "application/x-3gp", { }},
> +            {Demuxer, "video/x-ms-asf", { }},
> +            {Demuxer, "audio/x-aiff", { }},
> +            {Demuxer, "application/x-pn-realaudio", { }},
> +            {Demuxer, "application/vnd.rn-realmedia", { }},
> +            {Demuxer, "audio/x-wav", {"audio/x-wav", "audio/wav"}},
> +            {Demuxer, "application/x-hls", {"application/vnd.apple.mpegurl", "application/x-mpegurl"}}

Nit: I haven't checked in other parts of WK, but I would write these as:

{ Demuxer, "application/x-hls", { "application/vnd.apple.mpegurl", "application/x-mpegurl" } }

If I am contradicting any other coding style, feel free to ignore me.

[Xabier Rodríguez Calvar]

(In reply to comment #30)
> Comment on attachment 279238 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279238&action=review

Btw, all these comments are just nits, no need to wait for any green light from me or anything.

[Philippe Normand]

Comment on attachment 279238 [details]
patch

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

>> Source/WebCore/ChangeLog:24
>> +        (WebCore::mimeTypeCache): Deleted.
> 
> Nit: I would be a bit more verbose here.

Okey dokey!

>> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:189
>> +bool gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString)
> 
> Useless complain: this function should be included in GStreamer, it could be more efficient.

More efficient?
I don't know, to be honest. It's a bit weird to pass a GList* parameter like this usually in GStreamer no? I did it like this so that the factories are initialized/freed once only, in contrast with the first version of this patch.
For now I think it's fine to have this code in WebKit instead of GStreamer :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1711
>> +            {Demuxer, "application/x-hls", {"application/vnd.apple.mpegurl", "application/x-mpegurl"}}
> 
> Nit: I haven't checked in other parts of WK, but I would write these as:
> 
> { Demuxer, "application/x-hls", { "application/vnd.apple.mpegurl", "application/x-mpegurl" } }
> 
> If I am contradicting any other coding style, feel free to ignore me.

The style bot didn't complain so I think I'll leave this as it is ;)

[Philippe Normand]

Created attachment 279379 [details]
patch

[Philippe Normand]

Committed r201163: <http://trac.webkit.org/changeset/201163>