Bug 205763 – [Media in GPU process] Add remote MIME type cache

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

[Eric Carlson]

Add remote a MIME type cache to decrease the number of times we need to use IPC to answer questions about supported types.

[Radar WebKit Bug Importer]

<rdar://problem/58312152>

[Eric Carlson]

Created attachment 386746 [details]
Patch

[Jer Noble]

Comment on attachment 386746 [details]
Patch

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

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:91
> +void MIMETypeCache::setSupportedTypes(const Vector<String>& newTypes)

This change restores the broken naming convention and behavior you fixed in your last patch. set() should mean “set”; it doesn’t here. If a client calls add(), then set(), the expectation is that the second call replaces values added by the first. That doesn’t work here. 

Clients who would like to optimize for the case where the cache is already initialized can just do: `if(cache.isEmpty()) cache.addSupportedTypes();`.

[Eric Carlson]

Created attachment 386775 [details]
Patch

[Eric Carlson]

Created attachment 386778 [details]
Patch

[Eric Carlson]

Created attachment 386781 [details]
Patch

[Eric Carlson]

Created attachment 386789 [details]
Patch

[Dean Jackson]

Comment on attachment 386789 [details]
Patch

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

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:68
> +    MediaPlayerEnums::SupportsType result = MediaPlayerEnums::SupportsType::IsNotSupported;

Can this be auto?

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:84
> +    do {
> +        if (!isAvailable() || mimeType.isEmpty())
> +            break;
> +
> +        auto contentType = ContentType { mimeType };
> +        auto containerType = contentType.containerType();
> +        if (!supportsContainerType(containerType))
> +            break;
> +
> +        result = MediaPlayerEnums::SupportsType::MayBeSupported;
> +        if (contentType.codecs().isEmpty())
> +            break;
> +
> +        if (canDecodeExtendedType(contentType))
> +            result = MediaPlayerEnums::SupportsType::IsSupported;
> +    } while (0);

This confused me!

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:95
> +    m_supportedTypes = HashSet<String, ASCIICaseInsensitiveHash>();

Use { } rather than ()?

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:117
> +static HashMap<MediaPlayerEnums::MediaEngineIdentifier, std::unique_ptr<RemoteMediaPlayerMIMETypeCache>, WTF::IntHash<MediaPlayerEnums::MediaEngineIdentifier>, WTF::StrongEnumHashTraits<MediaPlayerEnums::MediaEngineIdentifier>>& mimeCaches()
>  {
> -// FIXME: Use parameters.mediaMIMETypes.
> +    static NeverDestroyed<HashMap<MediaPlayerEnums::MediaEngineIdentifier, std::unique_ptr<RemoteMediaPlayerMIMETypeCache>, WTF::IntHash<MediaPlayerEnums::MediaEngineIdentifier>, WTF::StrongEnumHashTraits<MediaPlayerEnums::MediaEngineIdentifier>>> caches;

Could we typedef/using this thing?

[Eric Carlson]

Comment on attachment 386789 [details]
Patch

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

>> Source/WebCore/platform/graphics/MIMETypeCache.cpp:68
>> +    MediaPlayerEnums::SupportsType result = MediaPlayerEnums::SupportsType::IsNotSupported;
> 
> Can this be auto?

Yes, fixed.

>> Source/WebCore/platform/graphics/MIMETypeCache.cpp:84
>> +    } while (0);
> 
> This confused me!

Sorry about that :-)

>> Source/WebCore/platform/graphics/MIMETypeCache.cpp:95
>> +    m_supportedTypes = HashSet<String, ASCIICaseInsensitiveHash>();
> 
> Use { } rather than ()?

Good point, changed.

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:117
>> +    static NeverDestroyed<HashMap<MediaPlayerEnums::MediaEngineIdentifier, std::unique_ptr<RemoteMediaPlayerMIMETypeCache>, WTF::IntHash<MediaPlayerEnums::MediaEngineIdentifier>, WTF::StrongEnumHashTraits<MediaPlayerEnums::MediaEngineIdentifier>>> caches;
> 
> Could we typedef/using this thing?

Yes, fixed.

[Eric Carlson]

Created attachment 386804 [details]
Patch for landing

[Eric Carlson]

Created attachment 386807 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 386807 [details]
Patch for landing

Clearing flags on attachment: 386807

Committed r254048: <https://trac.webkit.org/changeset/254048>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.