Bug 179499 – Add a FairPlay Streaming based CDM for Modern EME

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

[Jer Noble]

Add a FairPlay Streaming based CDM for Modern EME

[Jer Noble]

<rdar://problem/35445033>

[Jer Noble]

Created attachment 326480 [details]
Patch

[Eric Carlson]

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?

[Jer Noble]

Created attachment 326552 [details]
Patch

[Jer Noble]

Created attachment 326554 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Eric Carlson]

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?

[Jer Noble]

(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.

[Jer Noble]

Created attachment 326590 [details]
Patch

[Jer Noble]

Created attachment 326591 [details]
Patch

[Build Bot]

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.

[Jer Noble]

Created attachment 326595 [details]
Patch

[Jer Noble]

Created attachment 326601 [details]
Patch

[Eric Carlson]

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 = ...

[Jer Noble]

Created attachment 326623 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 326623 [details]
Patch for landing

Clearing flags on attachment: 326623

Committed r224707: <https://trac.webkit.org/changeset/224707>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Jer Noble]

(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.

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 326654 [details]
Build fix

[WebKit Commit Bot]

Comment on attachment 326654 [details]
Build fix

Clearing flags on attachment: 326654

Committed r224721: <https://trac.webkit.org/changeset/224721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

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?

[Michael Catanzaro]

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'; }
                                                ^~~~~~

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Ryan Haddad]

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?

[Michael Catanzaro]

(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.

[Ryan Haddad]

(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.

[Jer Noble]

(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>

[Michael Catanzaro]

(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?

[Jer Noble]

(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.

[Jer Noble]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Committed r224860: <https://trac.webkit.org/changeset/224860>

[Jer Noble]

(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.

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r224860 for reason:

This change broke the macOS and iOS builds.

Committed r224865: <https://trac.webkit.org/changeset/224865>

[Michael Catanzaro]

Hmmm, I did not realize that the Sources.txt was processed by XCode as well.

[Michael Catanzaro]

Created attachment 326959 [details]
Patch

[Michael Catanzaro]

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....

[Michael Catanzaro]

Committed r224867: <https://trac.webkit.org/changeset/224867>