Bug 152890

Summary: [WinCairo] Support more video formats.
Product: WebKit Reporter: peavo
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch achristensen: review+, achristensen: commit-queue-

Description peavo 2016-01-08 06:24:31 PST
Use MediaFoundation api to detect all supported video/audio formats.
Comment 1 peavo 2016-01-08 06:28:33 PST
Created attachment 268541 [details]
Patch
Comment 2 Alex Christensen 2016-01-08 09:59:21 PST
Comment on attachment 268541 [details]
Patch

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

Functionality looks good, there are just a few optimizations and reorganizations we need to do.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:124
> +    static HashSet<String> cachedTypes;

Use NeverDestroyed instead of static.  Also, you should probably make this in a separate function like gstreamer's mimeTypeCache(), except have it just return a reference, not a whole HashSet.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:134
> +    HRESULT hr = MFGetSupportedMimeTypesPtr()(&propVarMimeTypeArray);

This will crash on WindowsXP.  I don't think that's a problem, because we don't support XP any more.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:156
> +    HashSet<String> types;
> +    getSupportedTypes(types);

We should't copy the whole HashSet every time we want to check if one type is supported.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:158
> +    for (HashSet<String>::iterator it = types.begin(); it != types.end(); ++it) {

This is what HashSet.contains is for.  No need to iterate.
Comment 3 peavo 2016-01-08 10:16:35 PST
Thanks for reviewing, will update the patch :)
Comment 4 peavo 2016-01-08 12:05:12 PST
Created attachment 268569 [details]
Patch
Comment 5 Alex Christensen 2016-01-08 12:08:53 PST
Comment on attachment 268569 [details]
Patch

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

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:142
> +        for (int i = 0; i < mimeTypeArray.cElems; i++)

cElems is an unsigned int.  Please use unsigned for i.
Comment 6 peavo 2016-01-08 13:44:39 PST
Committed r194787: <http://trac.webkit.org/changeset/194787>.