Bug 147309 – [Cocoa] canPlayType('audio/mpeg; codecs="mp3"') returns ""

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

[Sajid Anwar]

I've reported part of this bug as <rdar://22000718>.

The Objective-C platform media player/source calls [AVFoundation isPlayableExtendedMIMEType:] to determine if a MIME type + codec string is supported. I've peeked inside with a debugger and found that it obtains the list of supported video/audio types and codecs from /System/Library/Frameworks/MediaToolbox.framework/Resources/MediaValidator.plist . Inside this plist is a list of supported "AudioCodecs", which contains strings like "aac " and ".mp3". However, "mp3" is nowhere to be found, despite the fact that virtually all examples, tutorials, and specifications online use canPlayType() to check the 'audio/mpeg; codecs="mp3"' MIME type.

Currently, canPlayType('audio/mpeg; codecs="mp3"') returns "maybe", but you can edit the plist and add the "mp3" entry, and then canPlayType will return "probably", which is expected since AV Foundation can obviously play MP3 audio.

On top of that, as I reported in the radar, there appears to be a bug in the implementation of [AVURLAsset isPlayableExtendedMIMEType:] that returns NO on the ".mp3" codec despite its existence in the plist, meaning even canPlayType('audio/mpeg; codecs=".mp3"') returns "maybe".

Should WebKit special-case the "mp3" audio codec to return "probably"? That might not be an elegant solution, but unless Apple adds the "mp3" entry to the "AudioCodecs" section of the plist, the call will always return "maybe" instead of "probably".

A little more information about the AV Foundation bug can be found at the radar, which I've duplicated on Open Radar: https://openradar.appspot.com/22000718

[Alexey Proskuryakov]

Thank you for the report!

WebKit would need a workaround even once <rdar://22000718> is fixed, because it would be good to have this working on earlier OS X versions.

[Sajid Anwar]

Good point with the earlier versions of WebKit. What would the approach be? Replacing the ends of both MediaPlayerPrivateAVFoundationObjC::supportsType and MediaPlayerPrivateMediaSourceAVFObjC::supportsType with something like this?

    NSString *typeString = [NSString stringWithFormat:@"%@; codecs=\"%@\"", (NSString *)parameters.type, (NSString *)parameters.codecs];
    
    if ([typeString isEqualToString:@"audio/mpeg; codecs=\"mp3\""])
        return MediaPlayer::IsSupported;

    return [AVURLAsset isPlayableExtendedMIMEType:typeString] ? MediaPlayer::IsSupported : MediaPlayer::MayBeSupported;

The assumption here is that this kind of special-casing is harmless if AV Foundation has always supported the MP3 codec (likely).

[Alex Buznik]

Hello.
Currently in Safari 14 (15610.1.28.1.7, 15610), canPlayType('audio/mpeg; codecs="mp3"') returns "".
This is going to break a lot of streaming sites.

[Eric Carlson]

Created attachment 409050 [details]
Patch

[Darin Adler]

Comment on attachment 409050 [details]
Patch

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

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:145
> +    if (type.containerType().convertToASCIILowercase() == "audio/mpeg") {

Can be more efficient to use equalLettersIgnoringASCIICase or equalIgnoringASCIICase in a case like this.

> Source/WebCore/platform/graphics/MIMETypeCache.h:60
> +    Optional<bool> overrideExtendedType(const ContentType&);

This should be a static member function; it is a rule, not something  based on the contents of the cache itself. Or it could be a free function not mentioned in the header at all.

Sam has pointed out that Optional<bool> is confusing and often reminds us not to use it. We don’t *really* need it here. Have false mean don’t override, since we’re not using false at this time.

The name of this function has the form we try to avoid where it sounds like it might be the caller telling the cache to override, but really we are asking if we should override. So it could be more like shouldOverrideExtendedType?

[Eric Carlson]

Comment on attachment 409050 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/platform/graphics/MIMETypeCache.cpp:145
>> +    if (type.containerType().convertToASCIILowercase() == "audio/mpeg") {
> 
> Can be more efficient to use equalLettersIgnoringASCIICase or equalIgnoringASCIICase in a case like this.

Good idea, changed.

>> Source/WebCore/platform/graphics/MIMETypeCache.h:60
>> +    Optional<bool> overrideExtendedType(const ContentType&);
> 
> This should be a static member function; it is a rule, not something  based on the contents of the cache itself. Or it could be a free function not mentioned in the header at all.
> 
> Sam has pointed out that Optional<bool> is confusing and often reminds us not to use it. We don’t *really* need it here. Have false mean don’t override, since we’re not using false at this time.
> 
> The name of this function has the form we try to avoid where it sounds like it might be the caller telling the cache to override, but really we are asking if we should override. So it could be more like shouldOverrideExtendedType?

I made it a member function because of the ASSERT, which I think is important because we should never override an extended type unless the content type is supported.

I had it return an Optional<bool> so we could have it return false for types in the future, but we clearly don't need that yet so I'll change the name to shouldOverrideExtendedType and have it return a simple bool.

[Eric Carlson]

Created attachment 409057 [details]
Patch for landing

[Darin Adler]

Comment on attachment 409057 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:83
> +        if (auto override = shouldOverrideExtendedType(contentType)) {

Don’t need the local variable here any more now that we are doing the simplified version.

[Eric Carlson]

Created attachment 409064 [details]
Patch for landing

[Eric Carlson]

Comment on attachment 409057 [details]
Patch for landing

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

>> Source/WebCore/platform/graphics/MIMETypeCache.cpp:83
>> +        if (auto override = shouldOverrideExtendedType(contentType)) {
> 
> Don’t need the local variable here any more now that we are doing the simplified version.

Good point, changed!

[Eric Carlson]

The inspector/cpu-profiler/threads.html crash is unrelated.

[EWS]

Committed r267210: <https://trac.webkit.org/changeset/267210>

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

[Radar WebKit Bug Importer]

<rdar://problem/69101113>