Summary: | [Cocoa] canPlayType('audio/mpeg; codecs="mp3"') returns "" | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sajid Anwar <sajidanwar94> | ||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sajidanwar94, sergio, shu, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.10 | ||||||||||
URL: | http://jsbin.com/vubukodepo/edit?js,output | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=216692 | ||||||||||
Attachments: |
|
Description
Sajid Anwar
2015-07-26 12:12:58 PDT
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. 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). 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. Created attachment 409050 [details]
Patch
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? 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. Created attachment 409057 [details]
Patch for landing
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. Created attachment 409064 [details]
Patch for landing
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! The inspector/cpu-profiler/threads.html crash is unrelated. Committed r267210: <https://trac.webkit.org/changeset/267210> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409064 [details]. |