Bug 219961 – [Cocoa] WebM format reader doesn't work with a url in a <source> element

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

The WebM format reader doesn't work if the source url is in a <source> element

[Radar WebKit Bug Importer]

<rdar://problem/72399014>

[Eric Carlson]

Created attachment 416374 [details]
Patch

[Eric Carlson]

Created attachment 416375 [details]
Patch

[Eric Carlson]

Created attachment 416376 [details]
Patch

[Eric Carlson]

Created attachment 416417 [details]
Patch

[Andy Estes]

Comment on attachment 416417 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:484
> +    if (!os_variant_allows_internal_security_policies("com.apple.WebKit"))

Since the value won't change at runtime, can you break this out into a helper function that caches the result of os_variant_allows_internal_security_policies() in a static bool? This was my mistake from earlier, but maybe you can fix it while you're here :)

[Andy Estes]

Comment on attachment 416417 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:447
> +bool SourceBufferParserWebM::m_formatReaderEnabled = false;

Since globals are initialized to 0 by default, no need for the ` = false` here.

[Eric Carlson]

Created attachment 416419 [details]
Patch

[Eric Carlson]

Created attachment 416420 [details]
Patch for landing

[Eric Carlson]

Created attachment 416425 [details]
Patch for landing

[Eric Carlson]

Created attachment 416428 [details]
Patch for landing

[Eric Carlson]

Created attachment 416443 [details]
Final patch for landing

[EWS]

Committed r270939: <https://trac.webkit.org/changeset/270939>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416443 [details].

[Darin Adler]

Comment on attachment 416443 [details]
Final patch for landing

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

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:483
> +#endif
> +
> +#if USE(APPLE_INTERNAL_SDK)

This should be elif so we don’t compile multiple return statements.

[Eric Carlson]

Reopening to attach new patch.

[Eric Carlson]

Created attachment 416465 [details]
Address post-review comments.

[EWS]

Committed r270952: <https://trac.webkit.org/changeset/270952>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416465 [details].

[Darin Adler]

Comment on attachment 416419 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/AVAssetMIMETypeCache.mm:105
> +    static auto cache = makeNeverDestroyed([this] {
> +        UNUSED_PARAM(this);

Why do we capture "this"?

[Eric Carlson]

Comment on attachment 416419 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/AVAssetMIMETypeCache.mm:105
>> +        UNUSED_PARAM(this);
> 
> Why do we capture "this"?

That was left over from a earlier version of the patch, but it was removed before landing.

[Ryan Haddad]

Reverted r270939 and r270952 for reason:

Caused layout test timeouts on internal bots

Committed r271038: <https://trac.webkit.org/changeset/271038>

[Ryan Haddad]

Reverted r270939 and r270952 for reason:

Caused layout test timeouts on internal bots

Committed r271038: <https://trac.webkit.org/changeset/271038>

[Ryan Haddad]

Details in rdar://72484508

[Eric Carlson]

Created attachment 417051 [details]
Patch

[Eric Carlson]

Created attachment 417058 [details]
Patch

[Eric Carlson]

Created attachment 417060 [details]
Patch

[Eric Carlson]

Created attachment 417061 [details]
Patch

[Eric Carlson]

Created attachment 417062 [details]
Patch

[EWS]

Committed r271194: <https://trac.webkit.org/changeset/271194>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417062 [details].