Bug 145362 – [Mac] short-circuit MIME type lookup when possible

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

HTMLMediaElement::canPlayType can contribute significantly to page load time, so hard code some MIME types to allow us to accept and reject some types without calling into AVFoundation.avoid

[Eric Carlson]

Created attachment 253667 [details]
Proposed patch.

[Eric Carlson]

Created attachment 253687 [details]
Updated patch.

[Build Bot]

Comment on attachment 253687 [details]
Updated patch.

Attachment 253687 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6477736507867136

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253689 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253687 [details]
Updated patch.

Attachment 253687 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4799937598455808

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253690 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Eric Carlson]

Created attachment 253693 [details]
Updated patch.

[Build Bot]

Comment on attachment 253693 [details]
Updated patch.

Attachment 253693 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4570420988608512

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253694 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253693 [details]
Updated patch.

Attachment 253693 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4938701549338624

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253695 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Eric Carlson]

Created attachment 253712 [details]
Updated patch.

[Eric Carlson]

Created attachment 253715 [details]
Updated patch.

[Build Bot]

Comment on attachment 253715 [details]
Updated patch.

Attachment 253715 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6715202028437504

New failing tests:
http/tests/media/clearkey/clear-key-hls-aes128.html

[Build Bot]

Created attachment 253722 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253715 [details]
Updated patch.

Attachment 253715 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4578849861926912

New failing tests:
http/tests/media/clearkey/clear-key-hls-aes128.html

[Build Bot]

Created attachment 253724 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Eric Carlson]

Created attachment 253734 [details]
Updated patch.

[Dean Jackson]

Comment on attachment 253734 [details]
Updated patch.

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1566
> +    if (equalIgnoringCase(type, "video/h264"))
> +        return true;

I assume this is some weird variant we don't support?

[Eric Carlson]

(In reply to comment #19)
> Comment on attachment 253734 [details]
> Updated patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253734&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1566
> > +    if (equalIgnoringCase(type, "video/h264"))
> > +        return true;
> 
> I assume this is some weird variant we don't support?

"video/h264" is meaningless as H.264 is a codec but a MIME type is meant to describe the container.

[WebKit Commit Bot]

Comment on attachment 253734 [details]
Updated patch.

Clearing flags on attachment: 253734

Committed r184910: <http://trac.webkit.org/changeset/184910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 253734 [details]
Updated patch.

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

I have some suggestions for future improvements to this code.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1548
> +static bool unsupportedMIMEType(const String& type)

A function that returns a boolean should not be named “unsupported MIME type”. I suggest adding the word “is”.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1559
> +    if (equalIgnoringCase(type, "video/x-flv"))

I don’t understand why this function is ignoring case, but other functions in the file require that the type already is lowercase, using plain old HashSet rather than a HashSet that does case folding. We should be consistent.

Also, full case folding is wasteful. equalIgnoringASCIICase is considerably more efficient.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1571
> +static HashSet<String>& staticMimeTypeCache()

I’m not sure the word “cache” in the name of this function is helpful. Also not wild about spelling MIME as Mime in some places but not others.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1578
> +    static NeverDestroyed<HashSet<String>> cache;
> +    static bool typeListInitialized = false;
> +
> +    if (typeListInitialized)
> +        return cache;
> +    typeListInitialized = true;

This idiom is unnecessarily complex. If we are going to use a HashSet for things like this (and I am not sure we should for such a small list of strings that does not need to be updated over time, we should do something more efficient in the future), I suggest something more like this:

    static HashSet<String> createStaticMIMETypesSet()
    {
        // code to initialize the set
    }

    static const HashSet<String>& staticMIMETypes()
    {
        static NeverDestroyed<HashSet<String>> types = staticMIMETypes();
        return types;
    }

Note also that the return value should be const&, not just &.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1609
> +    cache.get().add("application/vnd.apple.mpegurl");
> +    cache.get().add("application/x-mpegurl");
> +    cache.get().add("audio/3gpp");
> +    cache.get().add("audio/aac");
> +    cache.get().add("audio/aacp");
> +    cache.get().add("audio/aiff");
> +    cache.get().add("audio/basic");
> +    cache.get().add("audio/mp3");
> +    cache.get().add("audio/mp4");
> +    cache.get().add("audio/mpeg");
> +    cache.get().add("audio/mpeg3");
> +    cache.get().add("audio/mpegurl");
> +    cache.get().add("audio/mpg");
> +    cache.get().add("audio/wav");
> +    cache.get().add("audio/wave");
> +    cache.get().add("audio/x-aac");
> +    cache.get().add("audio/x-aiff");
> +    cache.get().add("audio/x-m4a");
> +    cache.get().add("audio/x-mpegurl");
> +    cache.get().add("audio/x-wav");
> +    cache.get().add("video/3gpp");
> +    cache.get().add("video/3gpp2");
> +    cache.get().add("video/mp4");
> +    cache.get().add("video/mpeg");
> +    cache.get().add("video/mpeg2");
> +    cache.get().add("video/mpg");
> +    cache.get().add("video/quicktime");
> +    cache.get().add("video/x-m4v");
> +    cache.get().add("video/x-mpeg");
> +    cache.get().add("video/x-mpg");

Eric, this will create a really huge function; each call to add gets inlined separately. A loop be better.

[Darin Adler]

Comment on attachment 253734 [details]
Updated patch.

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1578
>> +    typeListInitialized = true;
> 
> This idiom is unnecessarily complex. If we are going to use a HashSet for things like this (and I am not sure we should for such a small list of strings that does not need to be updated over time, we should do something more efficient in the future), I suggest something more like this:
> 
>     static HashSet<String> createStaticMIMETypesSet()
>     {
>         // code to initialize the set
>     }
> 
>     static const HashSet<String>& staticMIMETypes()
>     {
>         static NeverDestroyed<HashSet<String>> types = staticMIMETypes();
>         return types;
>     }
> 
> Note also that the return value should be const&, not just &.

Oops, got this wrong. I meant:

    static const HashSet<String>& staticMIMETypes()
    {
        static NeverDestroyed<HashSet<String>> types = createStaticMIMETypesSet();
        return types;
    }

Another way to write this is with a lambda. Something like this.

    static const HashSet<String>& staticMIMETypes()
    {
        static NeverDestroyed<HashSet<String>> types = [] () {
            HashSet<String> types;
            // code to initialize the set
            return types;
        }();
        return types;
    }

[Eric Carlson]

Created attachment 253798 [details]
Patch

Address Darin's post-review comments.

[Jer Noble]

Comment on attachment 253798 [details]
Patch

Woah, look at those crazy initializer lambdas!

[Eric Carlson]

Committed r184916 https://trac.webkit.org/r184916 to address Darin's comments.

[Darin Adler]

Comment on attachment 253798 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1550
> +    String lowerCaseType = type.convertToASCIILowercase();

I don’t think this is needed. The calling function already works only on lowercase.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1578
> +        static const char* typeNames[] = {

Should add one more const here.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1729
> -        if (!mimeType.isEmpty() && !staticMimeTypeCache().contains(mimeType) && !avfMimeTypeCache().contains(mimeType))
> +        if (!mimeType.isEmpty() && !staticMIMETypeList().contains(mimeType) && !avfMIMETypes().contains(mimeType))

These checks are case sensitive because these are HashSet<String>. Because of that, all the code above that ignores ASCII case doesn’t make sense. If this works, then the above code would work without the “ignoring ASCII case”.