Bug 145362 - [Mac] short-circuit MIME type lookup when possible
Summary: [Mac] short-circuit MIME type lookup when possible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-24 12:50 PDT by Eric Carlson
Modified: 2015-06-29 02:53 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch. (6.96 KB, patch)
2015-05-24 13:03 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (8.00 KB, patch)
2015-05-25 11:04 PDT, Eric Carlson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (445.83 KB, application/zip)
2015-05-25 11:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (432.12 KB, application/zip)
2015-05-25 11:48 PDT, Build Bot
no flags Details
Updated patch. (7.98 KB, patch)
2015-05-25 13:54 PDT, Eric Carlson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (346.70 KB, application/zip)
2015-05-25 14:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (266.64 KB, application/zip)
2015-05-25 14:46 PDT, Build Bot
no flags Details
Updated patch. (6.03 KB, patch)
2015-05-26 11:15 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (5.74 KB, patch)
2015-05-26 12:18 PDT, Eric Carlson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (590.30 KB, application/zip)
2015-05-26 13:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (735.57 KB, application/zip)
2015-05-26 13:15 PDT, Build Bot
no flags Details
Updated patch. (5.77 KB, patch)
2015-05-26 13:54 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2015-05-27 13:24 PDT, Eric Carlson
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2015-05-24 12:50:26 PDT
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
Comment 1 Eric Carlson 2015-05-24 13:03:47 PDT
Created attachment 253667 [details]
Proposed patch.
Comment 2 Eric Carlson 2015-05-25 11:04:43 PDT
Created attachment 253687 [details]
Updated patch.
Comment 3 Build Bot 2015-05-25 11:36:31 PDT
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.
Comment 4 Build Bot 2015-05-25 11:36:34 PDT
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
Comment 5 Build Bot 2015-05-25 11:48:17 PDT
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.
Comment 6 Build Bot 2015-05-25 11:48:20 PDT
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
Comment 7 Eric Carlson 2015-05-25 13:54:45 PDT
Created attachment 253693 [details]
Updated patch.
Comment 8 Build Bot 2015-05-25 14:12:07 PDT
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.
Comment 9 Build Bot 2015-05-25 14:12:09 PDT
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
Comment 10 Build Bot 2015-05-25 14:46:23 PDT
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.
Comment 11 Build Bot 2015-05-25 14:46:26 PDT
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
Comment 12 Eric Carlson 2015-05-26 11:15:04 PDT
Created attachment 253712 [details]
Updated patch.
Comment 13 Eric Carlson 2015-05-26 12:18:57 PDT
Created attachment 253715 [details]
Updated patch.
Comment 14 Build Bot 2015-05-26 13:07:31 PDT
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
Comment 15 Build Bot 2015-05-26 13:07:34 PDT
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
Comment 16 Build Bot 2015-05-26 13:15:53 PDT
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
Comment 17 Build Bot 2015-05-26 13:15:55 PDT
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
Comment 18 Eric Carlson 2015-05-26 13:54:33 PDT
Created attachment 253734 [details]
Updated patch.
Comment 19 Dean Jackson 2015-05-27 10:41:48 PDT
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?
Comment 20 Eric Carlson 2015-05-27 10:48:01 PDT
(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.
Comment 21 WebKit Commit Bot 2015-05-27 11:37:45 PDT
Comment on attachment 253734 [details]
Updated patch.

Clearing flags on attachment: 253734

Committed r184910: <http://trac.webkit.org/changeset/184910>
Comment 22 WebKit Commit Bot 2015-05-27 11:37:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Darin Adler 2015-05-27 11:48:20 PDT
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.
Comment 24 Darin Adler 2015-05-27 11:50:49 PDT
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;
    }
Comment 25 Eric Carlson 2015-05-27 13:24:04 PDT
Created attachment 253798 [details]
Patch

Address Darin's post-review comments.
Comment 26 Jer Noble 2015-05-27 13:27:26 PDT
Comment on attachment 253798 [details]
Patch

Woah, look at those crazy initializer lambdas!
Comment 27 Eric Carlson 2015-05-27 13:34:20 PDT
Committed r184916 https://trac.webkit.org/r184916 to address Darin's comments.
Comment 28 Darin Adler 2015-05-27 13:37:21 PDT
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”.