Bug 212457 – [EME] Get CDMProxy and friends ready for more CDMs

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

[Xabier Rodríguez Calvar]

[EME] Get CDMProxy and friends ready for more CDMs

[Xabier Rodríguez Calvar]

Created attachment 400433 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400442 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400452 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400460 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400467 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400478 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400485 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400590 [details]
Patch

[Philippe Normand]

Comment on attachment 400590 [details]
Patch

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

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:159
> -    static constexpr Seconds MaxKeyWaitTimeSeconds = 5_s;
> +    static constexpr Seconds MaxKeyWaitTimeSeconds = 7_s;

What's the reason for this increase?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3560
> -    m_cdmInstance->setProxy(adoptRef(*new CDMProxyClearKey));
> +    m_cdmInstance->setProxy(CDMProxyFactory::createCDMProxyForKeySystem(m_cdmInstance->keySystem()));

Could this be done directly during the CDMInstanceProxy construction?

[Charlie Turner]

Comment on attachment 400485 [details]
Patch

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

Overall I think this might need splitting further, bugs with their tests first, then OpenCDM with the scaffolding needed for it.

> Source/WebCore/ChangeLog:33
> +        No new tests, just a rework covered by the existing ones.

There are bug fixes mixed into reworks here. For example, the session tracking bugs you found should be testable, since you noticed them during a manual test not in the test suite? What tests make sure the weakptr tracking code in this patch won't break in the future? Can we test that? If not an explanation here would be helpful.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:118
> +bool KeyHandle::takeValue(KeyHandleValueVariant&& value)

takeValueIfDifferent

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:127
> +bool KeyHandle::takeData(const RefPtr<KeyHandle>& other, bool shouldCheckID)

This looks like a copy ctor in disguise.

shouldCheckID is also always false in this patch, so that should be left for a later patch when it's used. For example, with no ChangeLog information, it's not at all obvious why we copy ids sometimes but not in others.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:149
> +void KeyStore::setEqualIDMergeStrategy(EqualIDMergeStrategy strategy)

This pattern boilerplate is not achieving anything? I think this patch needs splitting up further, and things like Variant's and strategy patterns can be included in a patch where they are used and reward with clarity.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:223
> +        didStoreChange = keyWithMatchingKeyID->takeData(key);

It doesn't make sense for the "key store" to have changed when you "take data" from a key. The "did change" logic should be kept in the store, not in the keys themselves.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:271
> +void KeyStore::replaceEqualIDMergeStrategy(RefPtr<KeyHandle>& destination, const RefPtr<KeyHandle>& origin)

This "replaces" data based on creation times, not IDs. It's misnamed and confusing why creation times are even of interest.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:275
> +        destination->takeData(origin);

Here the return value is unused.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:335
> +RefPtr<KeyHandle> CDMProxy::tryWaitForKeyHandle(const KeyIDType& keyID) const

Why do move away from Optional<> and back to NULLs?

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:373
> +RefPtr<KeyHandle> CDMProxy::getOrWaitForKeyHandle(const KeyIDType& keyID) const

Optional<> seems better

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:386
> +    if (keyHandle)

if (auto keyHandle = ...)

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:446
> +    purgeNullSessions();

You need to clearly explain this in the ChangeLog, the session lifetime issues you found, etc. Also, a test should be provided to check it's working.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:44
> +    Vector<uint8_t>

Only 1 type in a Variant?

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:57
> +    bool takeData(const RefPtr<KeyHandle>&, bool shouldCheckID = false);

As I note in the call-sites, this seems wrong. It's used inconsistently and takes responsibility of key store changes, which it should not know about.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:107
> +    WallTime m_creationTime { WallTime::now() };

Please explain the need for that in the ChangeLog.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:159
> +    static constexpr Seconds MaxKeyWaitTimeSeconds = 7_s;

What caused that? 5 seemed like an eternity.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:200
> +    // registered CDMProxyFactory objects is queried for the first time.

Better to explain why it's needed than what it is.

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:465
> +    CDMInstanceSessionClearKey* newSession = new CDMInstanceSessionClearKey(*this);

auto*

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:132
> +    auto keyDataValue = WTF::get<Vector<uint8_t>>(keyData.value());

I don't know the details of Variant's implementation, but I'm scared this a copy. auto&?

[Xabier Rodríguez Calvar]

I'll try to honor all things I don't mention explicitly here.

(In reply to Charlie Turner from comment #10)
> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:127
> > +bool KeyHandle::takeData(const RefPtr<KeyHandle>& other, bool shouldCheckID)
> 
> This looks like a copy ctor in disguise.

It is, but I need to know if actually changes anything to perform the merges if it actually changes anything.
  
> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:223
> > +        didStoreChange = keyWithMatchingKeyID->takeData(key);
> 
> It doesn't make sense for the "key store" to have changed when you "take
> data" from a key. The "did change" logic should be kept in the store, not in
> the keys themselves.

Well, I think it does, maybe the name could be better (I accept suggestions) but as I said above, you need to know if anything changes when you

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:271
> > +void KeyStore::replaceEqualIDMergeStrategy(RefPtr<KeyHandle>& destination, const RefPtr<KeyHandle>& origin)
> 
> This "replaces" data based on creation times, not IDs. It's misnamed and
> confusing why creation times are even of interest.
> 
> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:275
> > +        destination->takeData(origin);
> 
> Here the return value is unused.

We don't need to return anything here but I do need it in the other call.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:335
> > +RefPtr<KeyHandle> CDMProxy::tryWaitForKeyHandle(const KeyIDType& keyID) const
> 
> Why do move away from Optional<> and back to NULLs?
> 
> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:373
> > +RefPtr<KeyHandle> CDMProxy::getOrWaitForKeyHandle(const KeyIDType& keyID) const
> 
> Optional<> seems better

We're not returning the value here but the entire handle (it is needed in the OpenCDM code) and the KeyStore holds RefPtr<KeyHandle>. For me it looks easier and efficient to return a const RefPtr<KeyHandle>& in the KeyStore. In the case of the CDMProxy, I assume we're not returning just the the const & because of threading issues and enforcing a ref count bump. If looks more strightforward to just keep returning the pointer. I could change it here to return an Optional<Ref<KeyHandle>> here but I think it is better to keep the const RefPtr& in the KeyStore.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:57
> > +    bool takeData(const RefPtr<KeyHandle>&, bool shouldCheckID = false);
> 
> As I note in the call-sites, this seems wrong. It's used inconsistently and
> takes responsibility of key store changes, which it should not know about.

I think it is used as it is needed, I don't think it is inconsistent. There are many functions that don't use return values in certain cases but are used in others. Which would be your proposal here?

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:159
> > +    static constexpr Seconds MaxKeyWaitTimeSeconds = 7_s;
> 
> What caused that? 5 seemed like an eternity.

YouTube TV tests have some tests that delay the update to 6s but I will move this to the another patch.
 
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:132
> > +    auto keyDataValue = WTF::get<Vector<uint8_t>>(keyData.value());
> 
> I don't know the details of Variant's implementation, but I'm scared this a
> copy. auto&?

template<typename _Type,typename ... _Types>
constexpr _Type& get(Variant<_Types...>&);

template<typename _Type,typename ... _Types>
constexpr _Type const& get(Variant<_Types...> const&);

It's a & but I agree to add auto&

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #11)
> I'll try to honor all things I don't mention explicitly here.
> 
> (In reply to Charlie Turner from comment #10)
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:127
> > > +bool KeyHandle::takeData(const RefPtr<KeyHandle>& other, bool shouldCheckID)
> > 
> > This looks like a copy ctor in disguise.
> 
> It is, but I need to know if actually changes anything to perform the merges
> if it actually changes anything.

This is not the responsibility of a key. Instead of having the key tell you it changed, how about modifying the key store to check in the only place it is needed,

KeyStore::add(...)
  if (keyWithMatchingKeyIDIndex != WTF::notFound) {
    auto& keyWithMatchingKeyID = m_keys[keyWithMatchingKeyIDIndex];
    RELEASE_ASSERT(keyWithMatchingKeyID->value() == key->value(), "Can this really happen?");
    bool didStoreChange = false;
    // we know the values aren't different, so this is just checking status
    // and creation time.
    if (keyWithMatchingKeyID != key) {
         keyWithMatchingKeyID = key;
         didStoreChange = true;
    }

Then, the other use of takeData() is just destination = origin; and we don't need to add "ad-hoc" methods to the KeyHandle class. Key's should not need to know anything about the structures they're used in, since this would break encapsulation.

>   
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:223
> > > +        didStoreChange = keyWithMatchingKeyID->takeData(key);
> > 
> > It doesn't make sense for the "key store" to have changed when you "take
> > data" from a key. The "did change" logic should be kept in the store, not in
> > the keys themselves.
> 
> Well, I think it does, maybe the name could be better (I accept suggestions)
> but as I said above, you need to know if anything changes when you

Think you prematurely stopped your thoughts here, but I maintain that the Key knowing about the structure it lives in breaks encapsulation.

> 
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:271
> > > +void KeyStore::replaceEqualIDMergeStrategy(RefPtr<KeyHandle>& destination, const RefPtr<KeyHandle>& origin)
> > 
> > This "replaces" data based on creation times, not IDs. It's misnamed and
> > confusing why creation times are even of interest.
> > 
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:275
> > > +        destination->takeData(origin);
> > 
> > Here the return value is unused.
> 
> We don't need to return anything here but I do need it in the other call.

I saw that :), my point was that this is often a smell the API is wrong (not always! but here imo yes)

> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:335
> > > +RefPtr<KeyHandle> CDMProxy::tryWaitForKeyHandle(const KeyIDType& keyID) const
> > 
> > Why do move away from Optional<> and back to NULLs?
> > 
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:373
> > > +RefPtr<KeyHandle> CDMProxy::getOrWaitForKeyHandle(const KeyIDType& keyID) const
> > 
> > Optional<> seems better
> 
> We're not returning the value here but the entire handle (it is needed in
> the OpenCDM code) and the KeyStore holds RefPtr<KeyHandle>.

For the same reason CDMProxy::getOrWaitForKeyValue returns an Optional<>, CDMProxy::getOrWaitForKey should, no? By changing this, you introduce inconsistency, some call-sites need to worry about NULLs, others are type-safe using Optional<>. Both APIs explicitly require the caller to worry the key might not be available. In modern C++, the absence of a value is best modelled with Optional<> than NULL.

> For me it looks
> easier and efficient to return a const RefPtr<KeyHandle>& in the KeyStore.

I don't see anything easier or more efficient, can you elaborate?

> In the case of the CDMProxy, I assume we're not returning just the the const
> & because of threading issues and enforcing a ref count bump.

Didn't follow you.

> If looks more
> strightforward to just keep returning the pointer.

I'd prefer to keep the type safety even if it seems more complex (it often does, but means we never have to worry about NULL values, which cause a lot of bugs)

> I could change it here to
> return an Optional<Ref<KeyHandle>> here but I think it is better to keep the
> const RefPtr& in the KeyStore.
> 
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.h:57
> > > +    bool takeData(const RefPtr<KeyHandle>&, bool shouldCheckID = false);
> > 
> > As I note in the call-sites, this seems wrong. It's used inconsistently and
> > takes responsibility of key store changes, which it should not know about.
> 
> I think it is used as it is needed, I don't think it is inconsistent. There
> are many functions that don't use return values in certain cases but are
> used in others. Which would be your proposal here?

I hope the above answers this.

> 
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.h:159
> > > +    static constexpr Seconds MaxKeyWaitTimeSeconds = 7_s;
> > 
> > What caused that? 5 seemed like an eternity.
> 
> YouTube TV tests have some tests that delay the update to 6s but I will move
> this to the another patch.

Ah, I remember now. Thanks. Agree this is a separate problem.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #12)
> > > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:335
> > > > +RefPtr<KeyHandle> CDMProxy::tryWaitForKeyHandle(const KeyIDType& keyID) const
> > > 
> > > Why do move away from Optional<> and back to NULLs?
> > > 
> > > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:373
> > > > +RefPtr<KeyHandle> CDMProxy::getOrWaitForKeyHandle(const KeyIDType& keyID) const
> > > 
> > > Optional<> seems better
> > 
> > We're not returning the value here but the entire handle (it is needed in
> > the OpenCDM code) and the KeyStore holds RefPtr<KeyHandle>.
> 
> For the same reason CDMProxy::getOrWaitForKeyValue returns an Optional<>,
> CDMProxy::getOrWaitForKey should, no? By changing this, you introduce
> inconsistency, some call-sites need to worry about NULLs, others are
> type-safe using Optional<>. Both APIs explicitly require the caller to worry
> the key might not be available. In modern C++, the absence of a value is
> best modelled with Optional<> than NULL.
> 
> > For me it looks
> > easier and efficient to return a const RefPtr<KeyHandle>& in the KeyStore.
> 
> I don't see anything easier or more efficient, can you elaborate?
> 
> > In the case of the CDMProxy, I assume we're not returning just the the const
> > & because of threading issues and enforcing a ref count bump.
> 
> Didn't follow you.
> 
> > If looks more
> > strightforward to just keep returning the pointer.
> 
> I'd prefer to keep the type safety even if it seems more complex (it often
> does, but means we never have to worry about NULL values, which cause a lot
> of bugs)
> 
> > I could change it here to
> > return an Optional<Ref<KeyHandle>> here but I think it is better to keep the
> > const RefPtr& in the KeyStore.

I think it is more efficient cause you don't need to create neither the Optional nor the Ref nor copy the RefPtr (and bump the ref count) if you just return the const RefPtr& that is inside the KeyStore. I just don't see the point. And with the PtrPtr you can also use the -> operator, you don't need to call value everytime the value() of the Optional or extract it.

Anyway, I don't a very strong opinion on this so if you really think your advantages outrange mine, I'll change it.

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #13)
> (In reply to Charlie Turner from comment #12)
> > > > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:335
> > > > > +RefPtr<KeyHandle> CDMProxy::tryWaitForKeyHandle(const KeyIDType& keyID) const
> > > > 
> > > > Why do move away from Optional<> and back to NULLs?
> > > > 
> > > > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:373
> > > > > +RefPtr<KeyHandle> CDMProxy::getOrWaitForKeyHandle(const KeyIDType& keyID) const
> > > > 
> > > > Optional<> seems better
> > > 
> > > We're not returning the value here but the entire handle (it is needed in
> > > the OpenCDM code) and the KeyStore holds RefPtr<KeyHandle>.
> > 
> > For the same reason CDMProxy::getOrWaitForKeyValue returns an Optional<>,
> > CDMProxy::getOrWaitForKey should, no? By changing this, you introduce
> > inconsistency, some call-sites need to worry about NULLs, others are
> > type-safe using Optional<>. Both APIs explicitly require the caller to worry
> > the key might not be available. In modern C++, the absence of a value is
> > best modelled with Optional<> than NULL.
> > 
> > > For me it looks
> > > easier and efficient to return a const RefPtr<KeyHandle>& in the KeyStore.
> > 
> > I don't see anything easier or more efficient, can you elaborate?
> > 
> > > In the case of the CDMProxy, I assume we're not returning just the the const
> > > & because of threading issues and enforcing a ref count bump.
> > 
> > Didn't follow you.
> > 
> > > If looks more
> > > strightforward to just keep returning the pointer.
> > 
> > I'd prefer to keep the type safety even if it seems more complex (it often
> > does, but means we never have to worry about NULL values, which cause a lot
> > of bugs)
> > 
> > > I could change it here to
> > > return an Optional<Ref<KeyHandle>> here but I think it is better to keep the
> > > const RefPtr& in the KeyStore.
> 
> I think it is more efficient cause you don't need to create neither the
> Optional nor the Ref nor copy the RefPtr (and bump the ref count) if you
> just return the const RefPtr& that is inside the KeyStore.

Let me know if you measure any performance regression using Optional<>. The ref bump is needed in the decryptors anyway, otherwise the keys could be destroyed from under their feet if they share the same ref as the key store.

> I just don't see
> the point. And with the PtrPtr you can also use the -> operator, you don't
> need to call value everytime the value() of the Optional or extract it.

This sounds like an argument against using Optional<> at all. The point is type-safety. It forces user of the API to handle properly cases when the object does not exist. RefPtr does not model that.

[Xabier Rodríguez Calvar]

I'll post other patches taking into consideration the comments on this.