Bug 152890 – [WinCairo] Support more video formats.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

Use MediaFoundation api to detect all supported video/audio formats.

Created attachment 268541 [details]
Patch

[Alex Christensen]

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.

Thanks for reviewing, will update the patch :)

Created attachment 268569 [details]
Patch

[Alex Christensen]

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.

Committed r194787: <http://trac.webkit.org/changeset/194787>.