Bug 189725 – [EME] Introduce the concept of CDMInstanceSession.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[EME] Introduce the concept of CDMInstanceSession.

[Jer Noble]

Created attachment 350076 [details]
Patch

[EWS Watchlist]

Attachment 350076 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:465:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:522:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 350076 [details]
Patch

Looks like we need to revert the part of the patch which moves CDMInstanceClearKey::keys() to CDMInstanceSession::keys() (or more likely, make the former aggregate the latter).

[Jer Noble]

Created attachment 350127 [details]
Patch

[EWS Watchlist]

Attachment 350127 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:465:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:522:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/44606289>

[Eric Carlson]

Comment on attachment 350127 [details]
Patch

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

r=me once the bots are happy

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:72
> +    virtual void updateLicense(const String& sessionId, LicenseType, const SharedBuffer& response, LicenseUpdateCallback) = 0;

Nit: LicenseUpdateCallback&&

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:83
> +    virtual void loadSession(LicenseType, const String& sessionId, const String& origin, LoadSessionCallback) = 0;

Ditto.

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:86
> +    virtual void closeSession(const String& sessionId, CloseSessionCallback) = 0;

Nit: CloseSessionCallback&&

> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:89
> +    virtual void removeSessionData(const String& sessionId, LicenseType, RemoveSessionDataCallback) = 0;

Nit: RemoveSessionDataCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:497
> +    static const NeverDestroyed<String> s_keySystem = MAKE_STATIC_STRING_IMPL("org.w3.clearkey");

Nit: you use the brace-initialization style to CDMInstanceFairPlayStreamingAVFObjC::keySystem, both should use the same.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:510
> +    Vector<CDMInstanceSessionClearKey::Key> allKeys { };

Nit: allKeys.reserveInitialCapacity( ... )

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:537
> +void CDMInstanceSessionClearKey::updateLicense(const String& sessionId, LicenseType, const SharedBuffer& response, LicenseUpdateCallback callback)

LicenseUpdateCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:638
> +void CDMInstanceSessionClearKey::loadSession(LicenseType, const String& sessionId, const String&, LoadSessionCallback callback)

Ditto.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:671
> +void CDMInstanceSessionClearKey::closeSession(const String&, CloseSessionCallback callback)

CloseSessionCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:682
> +void CDMInstanceSessionClearKey::removeSessionData(const String& sessionId, LicenseType, RemoveSessionDataCallback callback)

RemoveSessionDataCallback&&

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:108
> +    void requestLicense(LicenseType, const AtomicString& initDataType, Ref<SharedBuffer>&& initData, LicenseCallback) final;
> +    void updateLicense(const String&, LicenseType, const SharedBuffer&, LicenseUpdateCallback) final;
> +    void loadSession(LicenseType, const String&, const String&, LoadSessionCallback) final;
> +    void closeSession(const String&, CloseSessionCallback) final;
> +    void removeSessionData(const String&, LicenseType, RemoveSessionDataCallback) final;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h:112
> +    void requestLicense(LicenseType, const AtomicString& initDataType, Ref<SharedBuffer>&& initData, LicenseCallback) final;
> +    void updateLicense(const String&, LicenseType, const SharedBuffer&, LicenseUpdateCallback) final;
> +    void loadSession(LicenseType, const String&, const String&, LoadSessionCallback) final;
> +    void closeSession(const String&, CloseSessionCallback) final;
> +    void removeSessionData(const String&, LicenseType, RemoveSessionDataCallback) final;

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.h:123
> +    void didProvideRequest(AVContentKeyRequest *);
> +    void didProvideRenewingRequest(AVContentKeyRequest *);
> +    void didProvidePersistableRequest(AVContentKeyRequest *);
> +    void didFailToProvideRequest(AVContentKeyRequest *, NSError *);
> +    void requestDidSucceed(AVContentKeyRequest *);
> +    bool shouldRetryRequestForReason(AVContentKeyRequest *, NSString *);
> +    void sessionIdentifierChanged(NSData *);

Nit: our pedantic style guide wants no space between type and * in .cpp code.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:247
> +    static NeverDestroyed<String> s_keySystem { "com.apple.fps"_s };

Initialization style.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:402
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::updateLicense(const String&, LicenseType, const SharedBuffer& responseData, LicenseUpdateCallback callback)

LicenseUpdateCallback&&

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:448
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::loadSession(LicenseType licenseType, const String& sessionId, const String& origin, LoadSessionCallback callback)

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:487
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::closeSession(const String&, CloseSessionCallback callback)

CloseSessionCallback&&

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:505
> +void CDMInstanceSessionFairPlayStreamingAVFObjC::removeSessionData(const String& sessionId, LicenseType licenseType, RemoveSessionDataCallback callback)

RemoveSessionDataCallback&&

> Source/WebCore/testing/MockCDMFactory.cpp:274
> +    static const NeverDestroyed<String> s_keySystem = MAKE_STATIC_STRING_IMPL("org.webkit.mock");

Initialization style.

> Source/WebCore/testing/MockCDMFactory.cpp:315
> +void MockCDMInstanceSession::updateLicense(const String& sessionID, LicenseType, const SharedBuffer& response, LicenseUpdateCallback callback)

LicenseUpdateCallback&&

> Source/WebCore/testing/MockCDMFactory.cpp:349
> +void MockCDMInstanceSession::loadSession(LicenseType, const String&, const String&, LoadSessionCallback callback)

Ditto.

> Source/WebCore/testing/MockCDMFactory.cpp:365
> +void MockCDMInstanceSession::closeSession(const String& sessionID, CloseSessionCallback callback)

Ditto.

> Source/WebCore/testing/MockCDMFactory.cpp:377
> +void MockCDMInstanceSession::removeSessionData(const String& id, LicenseType, RemoveSessionDataCallback callback)

Ditto.

> Source/WebCore/testing/MockCDMFactory.h:155
>      void requestLicense(LicenseType, const AtomicString& initDataType, Ref<SharedBuffer>&& initData, LicenseCallback) final;
>      void updateLicense(const String&, LicenseType, const SharedBuffer&, LicenseUpdateCallback) final;
>      void loadSession(LicenseType, const String&, const String&, LoadSessionCallback) final;

Ditto.

> Source/WebCore/testing/MockCDMFactory.h:157
>      void removeSessionData(const String&, LicenseType, RemoveSessionDataCallback) final;

Ditto.

[Xabier Rodríguez Calvar]

Comment on attachment 350127 [details]
Patch

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

Wow Jer, big patch! Well thought, nice arch changes. It is going to break some things downstream for us when this gets merged into our branches, but as I keep telling my customers, it is not good working downstream for a long time because these things can happen!

> Source/WebCore/ChangeLog:10
> +        Currently, the same CDMInstance owned by a MediaKeys object is passed to every MediaKeySession created by that
> +        MediaKeys, and since the CDMInstance has only a single CDMInstanceClient, subsequent MediaKeySessions prevent
> +        previous ones from getting updates.

I bet you have (or can have) a nice test to prevent regressions on this. If there is already, I think you should list it here, update expectations, etc. If not, I think we should have one.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:51
> +Ref<MediaKeySession> MediaKeySession::create(ScriptExecutionContext& context, WeakPtr<MediaKeys>&& keys, MediaKeySessionType sessionType, bool useDistinctiveIdentifier, Ref<CDM>&& implementation, Ref<CDMInstanceSession>&& instance)

Since you're making a quite substantial name change here, the variables/parameters should named either session or instanceSession, but not instance since they are no CDMInstances anymore. I think you can also consider yourself "ditto-ed" for all the other appearances of parameters and variables where this happens.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:113
> -    Ref<CDMInstance> m_instance;
> +    Ref<CDMInstanceSession> m_instance;

Ditto.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:120
> +    WeakPtrFactory<CDMInstanceSessionClient> m_cdmInstanceClientWeakPtrFactory;

session -> m_cdmInstanceSessionClientWeakPtrFactory.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:67
> +    auto sessionInstance = m_instance->createSession();

sessionInstance -> instanceSession or session

> Source/WebCore/platform/encryptedmedia/CDMInstance.h:39
> +#include <wtf/WeakPtr.h>

This seems to be unused here or am I missing anything?

>> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:72
>> +    virtual void updateLicense(const String& sessionId, LicenseType, const SharedBuffer& response, LicenseUpdateCallback) = 0;
> 
> Nit: LicenseUpdateCallback&&

Yes, and I guess Eric meant this not only here but in other places, like requestLicense, etc.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:61
> +    bool supportsInitDataType(const AtomicString&) const final;

All these changes seem to be unrelated to the current patch though I think it is nice to make them. I think mentioning that some additional clean up and improvement was done in the process.

[Jer Noble]

(In reply to Eric Carlson from comment #7)
> Comment on attachment 350127 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350127&action=review
> 
> r=me once the bots are happy
> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:510
> > +    Vector<CDMInstanceSessionClearKey::Key> allKeys { };
> 
> Nit: allKeys.reserveInitialCapacity( ... )

This one is going to be difficult, because the data structure this vector is derived from is a HashSet of Vectors. We'd need to walk the list of values twice, summing as we go, just to calculate the initial capacity.

[Jer Noble]

(In reply to Xabier Rodríguez Calvar from comment #8)
> Comment on attachment 350127 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350127&action=review
> 
> Wow Jer, big patch! Well thought, nice arch changes. It is going to break
> some things downstream for us when this gets merged into our branches, but
> as I keep telling my customers, it is not good working downstream for a long
> time because these things can happen!

Hopefully this doesn't cause too much disruption. The easiest short term integration would be to make your CDMInstance derived class also derive from CDMInstanceSession and have createSession() just return this.  Long term, this should help with state management of different MediaKeySessions.

> > Source/WebCore/ChangeLog:10
> > +        Currently, the same CDMInstance owned by a MediaKeys object is passed to every MediaKeySession created by that
> > +        MediaKeys, and since the CDMInstance has only a single CDMInstanceClient, subsequent MediaKeySessions prevent
> > +        previous ones from getting updates.
> 
> I bet you have (or can have) a nice test to prevent regressions on this. If
> there is already, I think you should list it here, update expectations, etc.
> If not, I think we should have one.

I do, but because it's got a FairPlay server requirement, I can't make it public. I'll update the changelog to reflect that such a test exists though.

> > Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:51
> > +Ref<MediaKeySession> MediaKeySession::create(ScriptExecutionContext& context, WeakPtr<MediaKeys>&& keys, MediaKeySessionType sessionType, bool useDistinctiveIdentifier, Ref<CDM>&& implementation, Ref<CDMInstanceSession>&& instance)
> 
> Since you're making a quite substantial name change here, the
> variables/parameters should named either session or instanceSession, but not
> instance since they are no CDMInstances anymore. I think you can also
> consider yourself "ditto-ed" for all the other appearances of parameters and
> variables where this happens.

Will do.

> > Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:113
> > -    Ref<CDMInstance> m_instance;
> > +    Ref<CDMInstanceSession> m_instance;
> 
> Ditto.
> 
> > Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:120
> > +    WeakPtrFactory<CDMInstanceSessionClient> m_cdmInstanceClientWeakPtrFactory;
> 
> session -> m_cdmInstanceSessionClientWeakPtrFactory.
> 
> > Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:67
> > +    auto sessionInstance = m_instance->createSession();
> 
> sessionInstance -> instanceSession or session
> 
> > Source/WebCore/platform/encryptedmedia/CDMInstance.h:39
> > +#include <wtf/WeakPtr.h>
> 
> This seems to be unused here or am I missing anything?

Yes, I'll remove this.

> >> Source/WebCore/platform/encryptedmedia/CDMInstanceSession.h:72
> >> +    virtual void updateLicense(const String& sessionId, LicenseType, const SharedBuffer& response, LicenseUpdateCallback) = 0;
> > 
> > Nit: LicenseUpdateCallback&&
> 
> Yes, and I guess Eric meant this not only here but in other places, like
> requestLicense, etc.
> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:61
> > +    bool supportsInitDataType(const AtomicString&) const final;
> 
> All these changes seem to be unrelated to the current patch though I think
> it is nice to make them. I think mentioning that some additional clean up
> and improvement was done in the process.

Will do.

[Jer Noble]

Created attachment 350222 [details]
Patch for landing

[EWS Watchlist]

Attachment 350222 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:567:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:465:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:522:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 350222 [details]
Patch for landing

Xabier, I'm setting this to cq?. When you're available to handle any downstream breakages, would you mind cq+ing this patch?.

[Xabier Rodríguez Calvar]

(In reply to Jer Noble from comment #13)
> Comment on attachment 350222 [details]
> Patch for landing
> 
> Xabier, I'm setting this to cq?. When you're available to handle any
> downstream breakages, would you mind cq+ing this patch?.

Oh, thanks for being so considerate but you don't need to wait. We don't integrate things so often so I was foreseeing the problem of somebody requiring some more expert knowledge than just fixing some small conflicts during merge at some point in the near future but not immediate. I was just thinking out loud and I think we can't and shouldn't condition upstream development with downstream constraints. Upstream is upstream and leads the way.

About the cq+, I'll try to remember doing it close to the moment you wake up so that it has landed but you can attend any unforeseen breakage should it happen (fingers crossed!)

[WebKit Commit Bot]

Comment on attachment 350222 [details]
Patch for landing

Clearing flags on attachment: 350222

Committed r236317: <https://trac.webkit.org/changeset/236317>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(In reply to Jer Noble from comment #10)
> (In reply to Xabier Rodríguez Calvar from comment #8)
> > Comment on attachment 350127 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350127&action=review
> > 
> > Wow Jer, big patch! Well thought, nice arch changes. It is going to break
> > some things downstream for us when this gets merged into our branches, but
> > as I keep telling my customers, it is not good working downstream for a long
> > time because these things can happen!
> 
> Hopefully this doesn't cause too much disruption. The easiest short term
> integration would be to make your CDMInstance derived class also derive from
> CDMInstanceSession and have createSession() just return this.  Long term,
> this should help with state management of different MediaKeySessions.
> 

This doesn't work because both CDMInstance and CDMInstanceSession are RefCounted and both classes use WTF_MAKE_FAST_ALLOCATED so the default constructor for the class inheriting both becomes ambiguous.

[Jer Noble]

(In reply to Philippe Normand from comment #17)
> (In reply to Jer Noble from comment #10)
> > (In reply to Xabier Rodríguez Calvar from comment #8)
> > > Comment on attachment 350127 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=350127&action=review
> > > 
> > > Wow Jer, big patch! Well thought, nice arch changes. It is going to break
> > > some things downstream for us when this gets merged into our branches, but
> > > as I keep telling my customers, it is not good working downstream for a long
> > > time because these things can happen!
> > 
> > Hopefully this doesn't cause too much disruption. The easiest short term
> > integration would be to make your CDMInstance derived class also derive from
> > CDMInstanceSession and have createSession() just return this.  Long term,
> > this should help with state management of different MediaKeySessions.
> > 
> 
> This doesn't work because both CDMInstance and CDMInstanceSession are
> RefCounted and both classes use WTF_MAKE_FAST_ALLOCATED so the default
> constructor for the class inheriting both becomes ambiguous.

Oof, that's unfortunate. We could pull a "ref() { refCDMInstance(); } virtual refCDMInstance()" trick, and force subclasses to implement reference counting.

[Jer Noble]

I'm also happy to roll this patch out while we come up with a better solution that works for all ports.

[Philippe Normand]

(In reply to Jer Noble from comment #19)
> I'm also happy to roll this patch out while we come up with a better
> solution that works for all ports.

That's alright. Don't bother. I'll try to properly re-design my implementation :)