Add a FairPlay Streaming based CDM for Modern EME
<rdar://problem/35445033>
Created attachment 326480 [details] Patch
Comment on attachment 326480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326480&action=review > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:88 > + SuccessValue setStorageDirectory(const String&) override; Nit: final. > Source/WebCore/platform/graphics/ISOBox.h:54 > +class ISOBox { > +public: > + virtual ~ISOBox() = default; > + > + using PeekResult = std::optional<std::pair<FourCC, uint64_t>>; > + static PeekResult peekBox(JSC::DataView&, unsigned offset); > + bool read(JSC::DataView&, unsigned& offset); > + > + uint64_t size() const { return m_size; } > + const FourCC& boxType() const { return m_boxType; } > + const Vector<uint8_t>& extendedType() const { return m_extendedType; } > + > +protected: > + virtual bool parse(JSC::DataView&, unsigned& offset); > + > + uint64_t m_size { 0 }; > + FourCC m_boxType { 0 }; > + Vector<uint8_t> m_extendedType; > +}; WebCore already has an "ISOBox" class in platform/graphics/ISOVTTCue.h. It looks like this has some advantages so uses of it should be replaced with this, but until that happens we shouldn't have two classes with the same name. > Source/WebCore/platform/graphics/ISOBox.h:92 > +class ISOSchemeTypeBox : public ISOFullBox { > +public: > + static FourCC boxTypeName() { return 'schm'; } > + > + FourCC schemeType() const { return m_schemeType; } > + uint32_t schemeVersion() const { return m_schemeVersion; } > + > +protected: > + bool parse(JSC::DataView&, unsigned& offset) override; > + > + FourCC m_schemeType { 0 }; > + uint32_t m_schemeVersion { 0 }; > +}; I don't think we want every box type to be in the same file, so the non-generic classes should be split out to (an)other file(s). > Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:62 > + RefPtr<InspectorValue> value; > + if (!InspectorValue::parseJSON(json, value)) > + return { }; Sigh, I am afraid this is a layering violation. Can a CDM have an ExecState (eg get one from a ScriptExecutionContext) so it can use JSONParse instead?
Created attachment 326552 [details] Patch
Created attachment 326554 [details] Patch
Comment on attachment 326554 [details] Patch Attachment 326554 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5174273 New failing tests: platform/mac/media/encrypted-media/fps-requestMediaKeySystemAccess.html platform/mac/media/encrypted-media/fps-createMediaKeys.html
Created attachment 326562 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 326554 [details] Patch Attachment 326554 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5174310 New failing tests: platform/mac/media/encrypted-media/fps-requestMediaKeySystemAccess.html platform/mac/media/encrypted-media/fps-createMediaKeys.html svg/wicd/test-rightsizing-a.xhtml
Created attachment 326564 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 326554 [details] Patch Attachment 326554 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5174309 New failing tests: platform/mac/media/encrypted-media/fps-requestMediaKeySystemAccess.html platform/mac/media/encrypted-media/fps-createMediaKeys.html
Created attachment 326566 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 326554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326554&action=review > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:192 > + if (persistentStateAllowed == m_persistentStateAllowed) Nit: why bother with this check? > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:276 > + RetainPtr<NSData> appIdentifier = m_serverCertificate ? m_serverCertificate->createNSData() : nullptr; Shouldn’t this adoptNS(), or is makeStreamingContentKeyRequestDataForApp: expected to release it?
(In reply to Eric Carlson from comment #12) > Comment on attachment 326554 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326554&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:192 > > + if (persistentStateAllowed == m_persistentStateAllowed) > > Nit: why bother with this check? Yeah, no need. > > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:276 > > + RetainPtr<NSData> appIdentifier = m_serverCertificate ? m_serverCertificate->createNSData() : nullptr; > > Shouldn’t this adoptNS(), or is makeStreamingContentKeyRequestDataForApp: > expected to release it? createNSData() returns a RetainPtr<NSData>, so no need to adopt it.
Created attachment 326590 [details] Patch
Created attachment 326591 [details] Patch
Attachment 326591 [details] did not pass style-queue: ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/mac/TestExpectations:1746: More specific entry for platform/mac/media/encrypted-media/fps-createMediaKeys.html on line LayoutTests/platform/mac/TestExpectations:1746 overrides line LayoutTests/platform/mac/TestExpectations:1745. [test/expectations] [5] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 326595 [details] Patch
Created attachment 326601 [details] Patch
Comment on attachment 326601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326601&action=review > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:88 > + SuccessValue setStorageDirectory(const String&) override; Nit: final > Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:170 > + factories.append(&CDMFactoryClearKey::singleton()); Nit: is there any reason to not add the Clear Key factory in CDMFactory::registeredFactories instead of requiring every port to add it? > Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:229 > + && !WTF::anyOf(configuration.audioCapabilities, [](auto& capability) { > + return CDMInstanceFairPlayStreamingAVFObjC::mimeTypeIsPlayable(capability.contentType); > + })) > + return false; Do you want to only reject when none of the configurations is supported, or should it fail if any is unsupported? > Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.cpp:235 > + && !WTF::anyOf(configuration.videoCapabilities, [](auto& capability) { > + return CDMInstanceFairPlayStreamingAVFObjC::mimeTypeIsPlayable(capability.contentType); > + })) > + return false; Ditto. > Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h:40 > + std::unique_ptr<CDMPrivate> createCDM(const String&) override; > + bool supportsKeySystem(const String&) override; Nit: "override" => "final" > Source/WebCore/platform/graphics/avfoundation/CDMFairPlayStreaming.h:66 > + bool supportsInitDataType(const AtomicString&) const override; > + bool supportsConfiguration(const CDMKeySystemConfiguration&) const override; > + bool supportsConfigurationWithRestrictions(const CDMKeySystemConfiguration&, const CDMRestrictions&) const override; > + bool supportsSessionTypeWithConfiguration(CDMSessionType&, const CDMKeySystemConfiguration&) const override; > + bool supportsRobustness(const String&) const override; > + CDMRequirement distinctiveIdentifiersRequirement(const CDMKeySystemConfiguration&, const CDMRestrictions&) const override; > + CDMRequirement persistentStateRequirement(const CDMKeySystemConfiguration&, const CDMRestrictions&) const override; > + bool distinctiveIdentifiersAreUniquePerOriginAndClearable(const CDMKeySystemConfiguration&) const override; > + RefPtr<CDMInstance> createInstance() override; > + void loadAndInitialize() override; > + bool supportsServerCertificates() const override; > + bool supportsSessions() const override; > + bool supportsInitData(const AtomicString&, const SharedBuffer&) const override; > + RefPtr<SharedBuffer> sanitizeResponse(const SharedBuffer&) const override; > + std::optional<String> sanitizeSessionId(const String&) const override; Ditto. > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:273 > + RetainPtr<NSData> appIdentifier = m_serverCertificate ? m_serverCertificate->createNSData() : nullptr; Nit: auto appIdentifier = ...
Created attachment 326623 [details] Patch for landing
Comment on attachment 326623 [details] Patch for landing Clearing flags on attachment: 326623 Committed r224707: <https://trac.webkit.org/changeset/224707>
All reviewed patches have been landed. Closing bug.
This change broke the Windows debug build: WebCore.lib(UnifiedSource322.obj) : error LNK2019: unresolved external symbol "public: class WTF::String __thiscall WebCore::FourCC::toString(void)const " (?toString@FourCC@WebCore@@QBE?AVString@WTF@@XZ) referenced in function "protected: virtual bool __thiscall WebCore::ISOWebVTTCue::parse(class JSC::DataView &,unsigned int &)" (?parse@ISOWebVTTCue@WebCore@@MAE_NAAVDataView@JSC@@AAI@Z) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj] https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/5464
(In reply to Ryan Haddad from comment #23) > This change broke the Windows debug build: > > WebCore.lib(UnifiedSource322.obj) : error LNK2019: unresolved external > symbol "public: class WTF::String __thiscall > WebCore::FourCC::toString(void)const " > (?toString@FourCC@WebCore@@QBE?AVString@WTF@@XZ) referenced in function > "protected: virtual bool __thiscall WebCore::ISOWebVTTCue::parse(class > JSC::DataView &,unsigned int &)" > (?parse@ISOWebVTTCue@WebCore@@MAE_NAAVDataView@JSC@@AAI@Z) > [C:\cygwin\home\buildbot\slave\win- > debug\build\WebKitBuild\Debug\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj] > > https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/ > 5464 Looks like FourCC.cpp isn't being built by the Windows build.
Reopening to attach new patch.
Created attachment 326654 [details] Build fix
Comment on attachment 326654 [details] Build fix Clearing flags on attachment: 326654 Committed r224721: <https://trac.webkit.org/changeset/224721>
This also broke the GTK+ debug build for the same reason. Could FourCC just be added to the unified sources like all other iso files?
Is this supposed to be hidden behind ENABLE(ENCRYPTED_MEDIA)? There's a big warning spam: [474/1984] Building CXX object Source/...Core.dir/html/track/WebVTTParser.cpp.o In file included from ../../Source/WebCore/html/track/WebVTTParser.cpp:40:0: ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.h:45:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'vtcc'; } ^~~~~~ [640/1984] Building CXX object Source/...unified-sources/UnifiedSource345.cpp.o In file included from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:1:0: ../../Source/WebCore/platform/graphics/iso/ISOBox.cpp:78:28: warning: multi-character character constant [-Wmultichar] if (m_boxType.value == 'uuid') { ^~~~~~ In file included from ../../Source/WebCore/platform/graphics/iso/ISOOriginalFormatBox.cpp:27:0, from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:2: ../../Source/WebCore/platform/graphics/iso/ISOOriginalFormatBox.h:34:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'frma'; } ^~~~~~ In file included from ../../Source/WebCore/platform/graphics/iso/ISOProtectionSchemeInfoBox.cpp:27:0, from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:3: ../../Source/WebCore/platform/graphics/iso/ISOProtectionSchemeInfoBox.h:37:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'sinf'; } ^~~~~~ In file included from ../../Source/WebCore/platform/graphics/iso/ISOProtectionSchemeInfoBox.cpp:29:0, from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:3: ../../Source/WebCore/platform/graphics/iso/ISOSchemeInformationBox.h:34:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'schi'; } ^~~~~~ In file included from ../../Source/WebCore/platform/graphics/iso/ISOProtectionSchemeInfoBox.cpp:30:0, from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:3: ../../Source/WebCore/platform/graphics/iso/ISOSchemeTypeBox.h:34:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'schm'; } ^~~~~~ In file included from ../../Source/WebCore/platform/graphics/iso/ISOSchemeInformationBox.cpp:29:0, from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:4: ../../Source/WebCore/platform/graphics/iso/ISOTrackEncryptionBox.h:34:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'tenc'; } ^~~~~~ In file included from ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.cpp:27:0, from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:7: ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.h:45:42: warning: multi-character character constant [-Wmultichar] static FourCC boxTypeName() { return 'vtcc'; } ^~~~~~ In file included from DerivedSources/WebCore/unified-sources/UnifiedSource345.cpp:7:0: ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.cpp:76:39: warning: multi-character character constant [-Wmultichar] static FourCC vttIdBoxType() { return 'iden'; } ^~~~~~ ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.cpp:77:45: warning: multi-character character constant [-Wmultichar] static FourCC vttSettingsBoxType() { return 'sttg'; } ^~~~~~ ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.cpp:78:44: warning: multi-character character constant [-Wmultichar] static FourCC vttPayloadBoxType() { return 'payl'; } ^~~~~~ ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.cpp:79:48: warning: multi-character character constant [-Wmultichar] static FourCC vttCurrentTimeBoxType() { return 'ctim'; } ^~~~~~ ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.cpp:80:48: warning: multi-character character constant [-Wmultichar] static FourCC vttCueSourceIDBoxType() { return 'vsid'; } ^~~~~~
I tried using the warning hammer: #if COMPILER(GCC) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmultichar" #endif static FourCC boxTypeName() { return 'vtcc'; } #if COMPILER(GCC) #pragma GCC diagnostic pop #endif #if COMPILER(GCC) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmultichar" #endif static FourCC vttIdBoxType() { return 'iden'; } static FourCC vttSettingsBoxType() { return 'sttg'; } static FourCC vttPayloadBoxType() { return 'payl'; } static FourCC vttCurrentTimeBoxType() { return 'ctim'; } static FourCC vttCueSourceIDBoxType() { return 'vsid'; } #if COMPILER(GCC) #pragma GCC diagnostic pop #endif But, for whatever reason, this warning cannot be disabled. Maybe for the best; GCC says the value of a multichar constant is implementation-defined, so maybe we shouldn't be using them at all.
(In reply to Michael Catanzaro from comment #31) > But, for whatever reason, this warning cannot be disabled. Of course it can be disabled by building with -Wno-multichar. But the pragmas don't work for some reason.
platform/mac/media/encrypted-media/fps-createMediaKeys.html is failing and platform/mac/media/encrypted-media/fps-requestMediaKeySystemAccess.html is timing out on High Sierra WK1 bots: https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/r224759%20(918)/results.html Are these tests expected to work there?
(In reply to Carlos Garcia Campos from comment #29) > This also broke the GTK+ debug build for the same reason. Could FourCC just > be added to the unified sources like all other iso files? Jer, I'm going to do a rollout on this one... for several reasons: * GTK debug bot is still broken * Shouldn't use multichar constants * FairPlay code should (probably?) not be built when ENCRYPTED_MEDIA is turned off * FairPlay code should (probably?) only be built on Apple platforms * Failing tests noted by Ryan Sorry if this is inconvenient.
(In reply to Michael Catanzaro from comment #34) > (In reply to Carlos Garcia Campos from comment #29) > > This also broke the GTK+ debug build for the same reason. Could FourCC just > > be added to the unified sources like all other iso files? > > Jer, I'm going to do a rollout on this one... for several reasons: > > * GTK debug bot is still broken > * Shouldn't use multichar constants > * FairPlay code should (probably?) not be built when ENCRYPTED_MEDIA is > turned off > * FairPlay code should (probably?) only be built on Apple platforms > * Failing tests noted by Ryan > > Sorry if this is inconvenient. For what it is worth, the tests were fixed in https://trac.webkit.org/changeset/224833.
(In reply to Michael Catanzaro from comment #34) > (In reply to Carlos Garcia Campos from comment #29) > > This also broke the GTK+ debug build for the same reason. Could FourCC just > > be added to the unified sources like all other iso files? > > Jer, I'm going to do a rollout on this one... for several reasons: Please don't do a rollout. > * GTK debug bot is still broken > * Shouldn't use multichar constants This isn't true. OSType multi-character constants are used all over macOS and iOS. > * FairPlay code should (probably?) not be built when ENCRYPTED_MEDIA is > turned off > * FairPlay code should (probably?) only be built on Apple platforms > * Failing tests noted by Ryan These are fixed by <https://bugs.webkit.org/show_bug.cgi?id=179544>
(In reply to Jer Noble from comment #36) > Please don't do a rollout. OK. > > * Shouldn't use multichar constants > > This isn't true. OSType multi-character constants are used all over macOS > and iOS. Since the value of the constants is not standardized, I don't think they should be used in cross-platform code. The warning is there for a reason. I didn't even know it was possible until today. Anyway, maybe it doesn't matter, if this code is not needed on other platforms: > > * FairPlay code should (probably?) not be built when ENCRYPTED_MEDIA is > > turned off > > * FairPlay code should (probably?) only be built on Apple platforms Any thoughts on this?
(In reply to Michael Catanzaro from comment #30) > Is this supposed to be hidden behind ENABLE(ENCRYPTED_MEDIA)? > > There's a big warning spam: > > [474/1984] Building CXX object > Source/...Core.dir/html/track/WebVTTParser.cpp.o > In file included from ../../Source/WebCore/html/track/WebVTTParser.cpp:40:0: > ../../Source/WebCore/platform/graphics/iso/ISOVTTCue.h:45:42: warning: > multi-character character constant [-Wmultichar] > static FourCC boxTypeName() { return 'vtcc'; } > ^~~~~~ WebVTTParser has nothing to do with ENCRYPTED_MEDIA, and neither does FourCC, so they shouldn't be hidden behind an ENABLE(ENCRYPTED_MEDIA) flag. It looks like the GCC pragma support is buggy: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57241 & https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431. The other option for GCC builds would be to disable this warning entirely. But OSTypes are defined as 32-bit integers constructed from multi-character constants. And they are used in many places both in macOS and iOS APIs and also in parsing ISO BMFF media data. Multi-character constants are a perfectly valid way of defining them. Doing a string -> integer conversion for every access or a string comparison for every equality check is an enormous overhead with no benefit.
(In reply to Michael Catanzaro from comment #37) > (In reply to Jer Noble from comment #36) > > Please don't do a rollout. > > OK. > > > > * Shouldn't use multichar constants > > > > This isn't true. OSType multi-character constants are used all over macOS > > and iOS. > > Since the value of the constants is not standardized, I don't think they > should be used in cross-platform code. The warning is there for a reason. I > didn't even know it was possible until today. > > Anyway, maybe it doesn't matter, if this code is not needed on other > platforms: > > > > * FairPlay code should (probably?) not be built when ENCRYPTED_MEDIA is > > > turned off > > > * FairPlay code should (probably?) only be built on Apple platforms > > Any thoughts on this? This has nothing to do with FairPlay code; it has to do with parsing WebVTT atoms out of an ISO BMFF byte stream. And that is not platform-specific. That said, we could probably modify the constructor of FourCC to take a static constant string, and add some compile-time checks that the string was exactly four ASCII characters long.
(In reply to Jer Noble from comment #38) > It looks like the GCC pragma support is buggy: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57241 & > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431. The other option for > GCC builds would be to disable this warning entirely. Good find... it's a really annoying bug. This isn't the first time I've noticed it. > But OSTypes are defined as 32-bit integers constructed from multi-character > constants. And they are used in many places both in macOS and iOS APIs and > also in parsing ISO BMFF media data. Multi-character constants are a > perfectly valid way of defining them. Doing a string -> integer conversion > for every access or a string comparison for every equality check is an > enormous overhead with no benefit. The problem is the value of the multi-character constant is implementation-defined, and in practice the results are not consistent across platforms, see https://stackoverflow.com/a/6945217/1120203. So it looks like there is no way to safely use them in cross-platform code. They were not previously used anywhere else in (cross-platform) WebKit, and it seems like a good idea to not change that now. (In reply to Jer Noble from comment #39) > That said, we could probably modify the constructor of FourCC to take a > static constant string, and add some compile-time checks that the string was > exactly four ASCII characters long. Yes. Or alternatively you could just compute the desired integer value for the initializations, with a comment to indicate the four characters in use.
Committed r224860: <https://trac.webkit.org/changeset/224860>
(In reply to Michael Catanzaro from comment #40) > Yes. Or alternatively you could just compute the desired integer value for > the initializations, with a comment to indicate the four characters in use. See: bug #179706.
(In reply to Michael Catanzaro from comment #41) > Committed r224860: <https://trac.webkit.org/changeset/224860> This broke the macOS build: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20%28Build%29/builds/6652
Reverted r224860 for reason: This change broke the macOS and iOS builds. Committed r224865: <https://trac.webkit.org/changeset/224865>
Hmmm, I did not realize that the Sources.txt was processed by XCode as well.
Created attachment 326959 [details] Patch
Waiting for EWS.... (In reply to Michael Catanzaro from comment #45) > Hmmm, I did not realize that the Sources.txt was processed by XCode as well. Looks like files cannot be added via XCode anymore, at least not generally. They have to go into a Sources.txt instead, same as for CMake builds. And then if that causes the creation of a new unified source file, the unified source file needs to be added to the XCode project. Lovely. Well, it's worth it for faster builds....
Committed r224867: <https://trac.webkit.org/changeset/224867>