Bug 206730

Created attachment 388662 [details] Patch

Created attachment 388665 [details] Patch

Comment on attachment 388665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388665&action=review Since there are some missing files in the patch (and I have to call it a day and a working week), I'll continue on Tuesday (taking Monday off). Anyway, some comments: > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:506 > + keyIDVec.reserveInitialCapacity(in.keyIDNumBytes); Since you're appending be passing an array with size and you aren't appending anything else, I'd say this is not needed since the first thing the append is going to do is reserving the size that you specify. > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:523 > + // uin8_t* bufferToDecrypt, size_t bufferNumBytes); ? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:696 > + keysChanged = keysChanged || m_keyStore.add(decodedKey); Short-circuit evaluation is biting you here. I think you should either reverse both arguments or accumulate some other way. > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:137 > + const uint8_t* keyID; I think it is better to capitalize as keyId, I see Id instead of ID more used outside our code. Do you mind checking it patch-wide, please? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:138 > + size_t keyIDNumBytes; keyIdSize? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:141 > + size_t ivNumBytes; ivSize? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:144 > + size_t encryptedBufferNumBytes; encryptedBufferSize? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:147 > + const uint8_t* subSamplesBuffer; subsamplesBuffer > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:148 > + size_t subSamplesBufferNumBytes; subsamplesBufferSize? Mind that the word is subsample, so we should capitalize it like that IMHO. > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149 > + size_t numSubSamples; numberOfSubsamples? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:164 > + // resuse some Crypto APIs in WebCore? reuse > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:168 > + bool CENCSetCounterVector(const CENCDecryptContext&); > + bool CENCSetDecryptionKey(const CENCDecryptContext&); > + bool CENCDecryptFullSample(CENCDecryptContext&); > + bool CENCDecryptSubSampled(CENCDecryptContext&); I would call these methods cenc* > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:176 > + I don't think we need this extra line > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1797 > + m_handledProtectionEvents.add(GST_EVENT_SEQNUM(event.get())); /me dreams of getting rid of this at some point. > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:117 > + WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self)); Mein Gott, we really need to have a look at the style of this file at some point... > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:142 > + RefPtr<GstMappedBuffer> mappedSubSamplesBuffer; This should be moved to just above the if > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:143 > + CDMProxyClearKey::CENCDecryptContext ctx; context > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:202 > + auto removeProtectionMetaOnReturn = makeScopeExit([buffer, protectionMeta] { > + gst_buffer_remove_meta(buffer, reinterpret_cast<GstMeta*>(protectionMeta)); > + }); Beautiful! > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:277 > + GRefPtr<GstContext> context = adoptGRef(gst_element_get_context(GST_ELEMENT(self), "drm-cdm-instance")); We might want to rename this context name to drm-cdm-proxy > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:286 > + const GValue* value = gst_structure_get_value(gst_context_get_structure(context.get()), "cdm-instance"); cdm-proxy > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:290 > + GST_DEBUG_OBJECT(self, "received a new CDM proxy instance %p, refcount %u", proxy, proxy->refCount()); Do we really need the refcount? > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:342 > // We should be handling protection events in this class in > // addition to out-of-band data. In regular playback, after a > // preferred system ID context is set, any future protection > - // events will not be handled by the demuxer, so the must be > + // events will not be handled by the demuxer, so they must be > // handled in here. I think this descryption does not belong here cause protection events are not OOB, are they? I think this should be maybe before the switch. Another concern of mine is that we are reacting to all OOB events, I guess we should at least filter by structure name, right? Which event are we reacting to here? > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:-350 > - if (gst_structure_has_name(gst_query_get_structure(query), "any-decryptor-waiting-for-key")) { Good! > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:386 > + priv->cdmProxy = value ? reinterpret_cast<CDMProxy*>(g_value_get_pointer(value)) : nullptr; Wouldn't it be nice to create a boxed type of a RefPtr<CDMProxy>, something like: GType WEBKIT_G_TYPE_REF_PTR_CDM_PROXY = g_boxed_type_register_static ("RefPtrCDMProxy", (GBoxedCopyFunc) WTF::refIfNotNull<CDMProxy>, (GBoxedFreeFunc) WTF::derefIfNotNull<CDMProxy>); Then we could use WEBKIT_G_TYPE_REF_PTR_CDM_PROXY (or maybe something more adecuate to WebKit style) when we declare the content of the GstStructure and this way we would be able to maintain references inside the structures and not just loose pointers. I guess we might have as well something like: template<typename T> webkitGValueSet(GValue* value, const RefPtr<T>&); webkitGValueTake(GValue* value, RefPtr<T>&&); // We could even forget this and overload the Set method for both const & and &&. RefPtr<T> webkitGValueGet(GValue* value); // In this case I don't think we need to mimic the get/dup GValue API as we're dealing in smart pointers. I might be missing anything but I think it can work nicely. If you think it is too much for now or you are not in the mood of implementing this so cool stuff, we can give it a try later.

Created attachment 388765 [details] Patch Add missing CDMProxy files that were left behind in the working tree

Created attachment 388843 [details] Patch Remove dependency on CDMClearKey.h from Apple CDM, since I haven't added CDMProxy to the Apple build system, and I don't believe they need ClearKey support? Or at least it is falsely advertised as supported? If it is I'll do the extra steps to add the CDMProxy to the X-Files.

(In reply to Xabier Rodríguez Calvar from comment #3) > Comment on attachment 388665 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388665&action=review > > Since there are some missing files in the patch (and I have to call it a day > and a working week), I'll continue on Tuesday (taking Monday off). Anyway, > some comments: > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:506 > > + keyIDVec.reserveInitialCapacity(in.keyIDNumBytes); > > Since you're appending be passing an array with size and you aren't > appending anything else, I'd say this is not needed since the first thing > the append is going to do is reserving the size that you specify. Done. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:523 > > + // uin8_t* bufferToDecrypt, size_t bufferNumBytes); > > ? Removed. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:696 > > + keysChanged = keysChanged || m_keyStore.add(decodedKey); > > Short-circuit evaluation is biting you here. I think you should either > reverse both arguments or accumulate some other way. The problem being I wouldn't add any keys after a key was added that did change the key store... Oops. Changed to if(m_keyStore.add(decodedKey) keysChanged=true. I need to get some more "exotic" ClearKey tests in the tree, I think we only have one multikey test and its skipped for other reasons... Anyway, future work :-) > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:137 > > + const uint8_t* keyID; > > I think it is better to capitalize as keyId, I see Id instead of ID more > used outside our code. Do you mind checking it patch-wide, please? That's a really big change for no clear reason apart from your personal taste, I'll leave it as is since ID and Id are used in interchangeable in the WebKit codebase and I don't see any room for confusion here. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:138 > > + size_t keyIDNumBytes; > > keyIdSize? I prefer having the unit in my variables names, Size gives no information, i.e., Size of what? Having Bytes in the name can help when using lower level functions operating on the byte level. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:141 > > + size_t ivNumBytes; > > ivSize? Ditto. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:144 > > + size_t encryptedBufferNumBytes; > > encryptedBufferSize? Ditto. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:147 > > + const uint8_t* subSamplesBuffer; > > subsamplesBuffer Done. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:148 > > + size_t subSamplesBufferNumBytes; > > subsamplesBufferSize? Mind that the word is subsample, so we should > capitalize it like that IMHO. This was following the existing naming convention in the GStreamer decryptors, which you initially reviewed! I don't think it makes any difference aside from work changing all the names in my patch, but I did it anyway because I do agree it "looks better". It's shame we're not consistent on these things because it takes a lot of time to change these inconsistent nits, especially when 3 years later you might decide the other way is better again. I'm not attacking you btw, my aesthetics change over time too; because of this I believe such nits should not be enforced in reviews. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149 > > + size_t numSubSamples; > > numberOfSubsamples? Done. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:164 > > + // resuse some Crypto APIs in WebCore? > > reuse Done. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:168 > > + bool CENCSetCounterVector(const CENCDecryptContext&); > > + bool CENCSetDecryptionKey(const CENCDecryptContext&); > > + bool CENCDecryptFullSample(CENCDecryptContext&); > > + bool CENCDecryptSubSampled(CENCDecryptContext&); > > I would call these methods cenc* Done. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:176 > > + > > I don't think we need this extra line Done. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1797 > > + m_handledProtectionEvents.add(GST_EVENT_SEQNUM(event.get())); > > /me dreams of getting rid of this at some point. Agreed, future work though :-) > > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:117 > > + WebKitMediaClearKeyDecryptPrivate* priv = WEBKIT_MEDIA_CK_DECRYPT_GET_PRIVATE(WEBKIT_MEDIA_CK_DECRYPT(self)); > > Mein Gott, we really need to have a look at the style of this file at some > point... Agreed :-) > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:142 > > + RefPtr<GstMappedBuffer> mappedSubSamplesBuffer; > > This should be moved to just above the if Which if and why? > > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:143 > > + CDMProxyClearKey::CENCDecryptContext ctx; > > context Done. > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:277 > > + GRefPtr<GstContext> context = adoptGRef(gst_element_get_context(GST_ELEMENT(self), "drm-cdm-instance")); > > We might want to rename this context name to drm-cdm-proxy Done. > > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:286 > > + const GValue* value = gst_structure_get_value(gst_context_get_structure(context.get()), "cdm-instance"); > > cdm-proxy Done. > > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:290 > > + GST_DEBUG_OBJECT(self, "received a new CDM proxy instance %p, refcount %u", proxy, proxy->refCount()); > > Do we really need the refcount? It's been a very useful debug output in the past, may as well keep it. > > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:342 > > // We should be handling protection events in this class in > > // addition to out-of-band data. In regular playback, after a > > // preferred system ID context is set, any future protection > > - // events will not be handled by the demuxer, so the must be > > + // events will not be handled by the demuxer, so they must be > > // handled in here. > > I think this descryption does not belong here cause protection events are > not OOB, are they? The comment says "in addition to out-of-band data", so it appears the comment is suggesting we add things to sink event handler. > I think this should be maybe before the switch. Aye, done. > > Another concern of mine is that we are reacting to all OOB events, I guess > we should at least filter by structure name, right? > > Which event are we reacting to here? We only send one, attempt-to-decrypt, so there's no worries reacting to more events. If we ever added another one I'd agree with you. > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:386 > > + priv->cdmProxy = value ? reinterpret_cast<CDMProxy*>(g_value_get_pointer(value)) : nullptr; > > Wouldn't it be nice to create a boxed type of a RefPtr<CDMProxy>, something > like: > > GType WEBKIT_G_TYPE_REF_PTR_CDM_PROXY = g_boxed_type_register_static > ("RefPtrCDMProxy", (GBoxedCopyFunc) WTF::refIfNotNull<CDMProxy>, > (GBoxedFreeFunc) WTF::derefIfNotNull<CDMProxy>); > > Then we could use WEBKIT_G_TYPE_REF_PTR_CDM_PROXY (or maybe something more > adecuate to WebKit style) when we declare the content of the GstStructure > and this way we would be able to maintain references inside the structures > and not just loose pointers. I guess we might have as well something like: > > template<typename T> > webkitGValueSet(GValue* value, const RefPtr<T>&); > webkitGValueTake(GValue* value, RefPtr<T>&&); // We could even forget this > and overload the Set method for both const & and &&. > RefPtr<T> webkitGValueGet(GValue* value); // In this case I don't think we > need to mimic the get/dup GValue API as we're dealing in smart pointers. > > > I might be missing anything but I think it can work nicely. > > If you think it is too much for now or you are not in the mood of > implementing this so cool stuff, we can give it a try later. That does seem like a lovely improvement, I'm not familiar with that API but I'll take a look and if its a simple addition I'll bundle it in a future rev of this patch. Thanks for the review!

Comment on attachment 388665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388665&action=review That was answering your comments, I'll continue the review or the new patch. >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:137 >>> + const uint8_t* keyID; >> >> I think it is better to capitalize as keyId, I see Id instead of ID more used outside our code. Do you mind checking it patch-wide, please? > > That's a really big change for no clear reason apart from your personal taste, I'll leave it as is since ID and Id are used in interchangeable in the WebKit codebase and I don't see any room for confusion here. Forget this, as I'll explain below. >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:138 >>> + size_t keyIDNumBytes; >> >> keyIdSize? > > I prefer having the unit in my variables names, Size gives no information, i.e., Size of what? Having Bytes in the name can help when using lower level functions operating on the byte level. According to the style you should use complete words and num is no complete word. In order to make it shorter I thought "size" was better and I still think there is no ambiguity here as key ids are bytes. Please keep size or SizeInBytes if you want as there are other parts of WebKit with that scheme. >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:141 >>> + size_t ivNumBytes; >> >> ivSize? > > Ditto. Ditto >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:144 >>> + size_t encryptedBufferNumBytes; >> >> encryptedBufferSize? > > Ditto. Ditto. >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:148 >>> + size_t subSamplesBufferNumBytes; >> >> subsamplesBufferSize? Mind that the word is subsample, so we should capitalize it like that IMHO. > > This was following the existing naming convention in the GStreamer decryptors, which you initially reviewed! I don't think it makes any difference aside from work changing all the names in my patch, but I did it anyway because I do agree it "looks better". It's shame we're not consistent on these things because it takes a lot of time to change these inconsistent nits, especially when 3 years later you might decide the other way is better again. I'm not attacking you btw, my aesthetics change over time too; because of this I believe such nits should not be enforced in reviews. I don't enforce the nits, at least all of them. And yes, my aesthetics change with time and so does my proficiency in English. In this case, when I saw the code, I decided to search the diccionary and found subsample, which makes it a full word leading to the suggestion of changing it. In the case of ID, I searched the diccionary and found id and didn't read any longer. Now I checked again and saw that the id is something and ID with our intended meaning is spelled as ID, therefore the current scheme you followed is perfect. I guess I should check better my linguistic suggestions or consult them with people like you, who definitely know more about English than me. >>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:142 >>> + RefPtr<GstMappedBuffer> mappedSubSamplesBuffer; >> >> This should be moved to just above the if > > Which if and why? I meant "if (!subSampleCount)" and because of the rule of having variable declarations as closest to the point of use. This is not a big deal anyway. >>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:342 >>> // handled in here. >> >> I think this descryption does not belong here cause protection events are not OOB, are they? I think this should be maybe before the switch. >> >> Another concern of mine is that we are reacting to all OOB events, I guess we should at least filter by structure name, right? >> >> Which event are we reacting to here? > > The comment says "in addition to out-of-band data", so it appears the comment is suggesting we add things to sink event handler. Ok, but then please ASSERT on the structure name even if we don't actively check it in release. I think it's a good failsafe.

Comment on attachment 388843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388843&action=review I didn't finish. It's a dense patch. Step by step :) > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:40 > +namespace { Why do you need this? > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:81 > + LOG(EME, "EME - CDMProxy - key store now has %u keys", numKeys()); > + std::for_each(m_keys.begin(), m_keys.end(), > + [](const Key& key) { > + LOG(EME, "\tEME - CDMProxy - Key ID: %s", key.idAsString().ascii().data()); > + }); Maybe we can flag this as #ifndef NDEBUG > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:155 > + // data are small in number, so probably its fine. it's > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:163 > + { Not needed. > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:237 > +bool CDMProxy::getOrWaitForKey(const Vector<char>& keyID, Vector<char>& keyValueOut) const I would recommend returning a WTF::Optional<Vector<char>> to avoid the bool + output parameter. > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:264 > + bool nobodyWaitingForKey = !m_numDecryptorsWaitingForKey; wasNobodyWaitingForKey? > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:46 > + static Ref<Key> create(KeyStatus status, Vector<char> keyID, Vector<char> keyValue) Any reason you want to copy the vector here? From what I see, where you call it you call this, it is perfectly fine to pass a && > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:53 > + Vector<char> id() const { return m_id; } > + Vector<char> value() const { return m_value; } Wouldn't it be a good idea to make this return const Vector<char>& ? > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:61 > + void ref() { m_references++; } > + void deref() { m_references--; } > + unsigned refCount() const { return m_references; } Why can't you just make this WebKit refcounted and avoid all the error prone manual reference counting? If the intention is to make the object lighter, can't you still store a Ref inside the list at the store? > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:115 > + std::list<Key> m_keys; Why not a Vector? > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:169 > + void trackSession(RefPtr<CDMInstanceSession> session) { m_sessions.push_back(session); } && and WTFMove? > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:179 > + std::list<RefPtr<CDMInstanceSession>> m_sessions; Why not Vector? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149 > + const uint8_t* subSamplesBuffer; > + size_t subSamplesBufferNumBytes; > + size_t numSubsamples; Weren't these fixed? > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:142 > + RefPtr<GstMappedBuffer> mappedSubSamplesBuffer; We could have mappedSubsamplesBuffer.

(In reply to Xabier Rodríguez Calvar from comment #7) > > >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:138 > >>> + size_t keyIDNumBytes; > >> > >> keyIdSize? > > > > I prefer having the unit in my variables names, Size gives no information, i.e., Size of what? Having Bytes in the name can help when using lower level functions operating on the byte level. > > According to the style you should use complete words and num is no complete > word. In order to make it shorter I thought "size" was better and I still > think there is no ambiguity here as key ids are bytes. Please keep size or > SizeInBytes if you want as there are other parts of WebKit with that scheme. Done. > > >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:141 > >>> + size_t ivNumBytes; > >> > >> ivSize? > > > > Ditto. > > Ditto Done. > > >>> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:144 > >>> + size_t encryptedBufferNumBytes; > >> > >> encryptedBufferSize? > > > > Ditto. > > Ditto. Done. > >>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:342 > >>> // handled in here. > >> > >> I think this descryption does not belong here cause protection events are not OOB, are they? I think this should be maybe before the switch. > >> > >> Another concern of mine is that we are reacting to all OOB events, I guess we should at least filter by structure name, right? > >> > >> Which event are we reacting to here? > > > > The comment says "in addition to out-of-band data", so it appears the comment is suggesting we add things to sink event handler. > > Ok, but then please ASSERT on the structure name even if we don't actively > check it in release. I think it's a good failsafe. Done.

Comment on attachment 388843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388843&action=review Another step! > Source/WebCore/ChangeLog:14 > + thread-safety asserts. I took the liberty of renaming to CDMProxy, I think you forgot to update the Changelog since including CDMProxy.* in the patch cause there is no reference to the files in it. I also miss a higher level explanation above other than the minor things attached to functions, etc. > Source/WebCore/platform/GStreamer.cmake:163 > + "${WEBCORE_DIR}/platform/encryptedmedia/cdmproxy" I really think CDMProxy belongs in the parent directory, no need to create a custom one. > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:138 > +Vector<char> KeyStore::keyValue(const Vector<char>& keyID) const What about returning a const Vector<char>& and maybe save some copy? We can always copy it later by receiving it in a non reference. > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:281 > +void CDMInstanceProxy::requestKeyStore(CompletionHandler<void(const KeyStore&)>&& callback) > +{ > + // NOTE: We can immediately resolve this callback in the case of > + // ClearKey, but for a CDM like Widevine, which asynchronously > + // tells you about key statuses, this callback would need to be > + // stored somewhere and later resolved. > + > + auto locker = holdLock(m_keysMutex); > + callback(m_keyStore); > +} I foresee a problem here for CDMs other than ClearKey, as this method is virtual. I guess at some point we'll have to either make the lock protected or maybe split the method in two, a non-virtual public one that holds the lock and calls a virtual protected impl. Anyway, this is for the future and maybe you have a more clever way of solving this.

(In reply to Xabier Rodríguez Calvar from comment #8) > Comment on attachment 388843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388843&action=review > > I didn't finish. It's a dense patch. Step by step :) > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:40 > > +namespace { > > Why do you need this? https://web.archive.org/web/20181115023158/http://www.comeaucomputing.com/techtalk/#nostatic > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:81 > > + LOG(EME, "EME - CDMProxy - key store now has %u keys", numKeys()); > > + std::for_each(m_keys.begin(), m_keys.end(), > > + [](const Key& key) { > > + LOG(EME, "\tEME - CDMProxy - Key ID: %s", key.idAsString().ascii().data()); > > + }); > > Maybe we can flag this as #ifndef NDEBUG The installed key IDs are important to know in release and debug. > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:155 > > + // data are small in number, so probably its fine. > > it's Done. > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:163 > > + { > > Not needed. Done. > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:237 > > +bool CDMProxy::getOrWaitForKey(const Vector<char>& keyID, Vector<char>& keyValueOut) const > > I would recommend returning a WTF::Optional<Vector<char>> to avoid the bool > + output parameter. Nice, done. > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:264 > > + bool nobodyWaitingForKey = !m_numDecryptorsWaitingForKey; > > wasNobodyWaitingForKey? if (nobodyWaitingForKey) reads more naturally than if (wasNobodyWaitingForKey). > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:46 > > + static Ref<Key> create(KeyStatus status, Vector<char> keyID, Vector<char> keyValue) > > Any reason you want to copy the vector here? From what I see, where you call > it you call this, it is perfectly fine to pass a && It seems I went for that design initially, since the body of this method does cast to rvalues. I've fixed it, but note these are 16-byte vectors, so in theory they will be small-size optimised by the Vector implementation and copied very efficiently. Moving or not moving such values doesn't seem like it will be dangerous, but I've not run the experiments... > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:53 > > + Vector<char> id() const { return m_id; } > > + Vector<char> value() const { return m_value; } > > Wouldn't it be a good idea to make this return const Vector<char>& ? Done > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:61 > > + void ref() { m_references++; } > > + void deref() { m_references--; } > > + unsigned refCount() const { return m_references; } > > Why can't you just make this WebKit refcounted and avoid all the error prone > manual reference counting? > > If the intention is to make the object lighter, can't you still store a Ref > inside the list at the store? The reason I hand-rolled it is due to its use in KeyStore::remove. Because multiple sessions can add a key, I must be careful not to remove from the "global key store" a key that is referenced still by a session. Ref<> is for RAII semantics on a type, I don't want the Key to be destroyed when the last ref is dropped, rather I want "this is the last Key ref" to translate to "OK, now we can remove it from the key list". I agree this code raises eye-brows, can you offer a specific alternative using Ref<> that achieves what I described? I can't see it. > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:115 > > + std::list<Key> m_keys; > > Why not a Vector? Vector<> is typically not the right data-structure to support heavy inserts/deletes, which the key store is going to be exposed to. > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:169 > > + void trackSession(RefPtr<CDMInstanceSession> session) { m_sessions.push_back(session); } > > && and WTFMove? That doesn't match the caller expectation, RefPtr<CDMInstanceSession> newSession = adoptRef(new CDMInstanceSessionClearKey(*this)); trackSession(newSession); // moving would trash the return value return newSession; > > > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:179 > > + std::list<RefPtr<CDMInstanceSession>> m_sessions; > > Why not Vector? Same reason as the key store. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149 > > + const uint8_t* subSamplesBuffer; > > + size_t subSamplesBufferNumBytes; > > + size_t numSubsamples; > > Weren't these fixed? The seemed to have escaped me. Done. > > > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:142 > > + RefPtr<GstMappedBuffer> mappedSubSamplesBuffer; > > We could have mappedSubsamplesBuffer. Done.

Comment on attachment 388843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388843&action=review >> Source/WebCore/ChangeLog:14 >> + thread-safety asserts. I took the liberty of renaming to CDMProxy, > > I think you forgot to update the Changelog since including CDMProxy.* in the patch cause there is no reference to the files in it. > > I also miss a higher level explanation above other than the minor things attached to functions, etc. Ah yes, generate-Changelog doesn't take unstaged files into account, I think I generated the log when my tree was in that state. I'll fix that and add more explanation. >> Source/WebCore/platform/GStreamer.cmake:163 >> + "${WEBCORE_DIR}/platform/encryptedmedia/cdmproxy" > > I really think CDMProxy belongs in the parent directory, no need to create a custom one. Done. >> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:138 >> +Vector<char> KeyStore::keyValue(const Vector<char>& keyID) const > > What about returning a const Vector<char>& and maybe save some copy? We can always copy it later by receiving it in a non reference. Done. >> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:281 >> +} > > I foresee a problem here for CDMs other than ClearKey, as this method is virtual. I guess at some point we'll have to either make the lock protected or maybe split the method in two, a non-virtual public one that holds the lock and calls a virtual protected impl. Anyway, this is for the future and maybe you have a more clever way of solving this. That's true. I integrated such a CDM yet, so there will likely be some rejigging along the lines you outline.

Created attachment 389135 [details] Patch Addresses all previous comments

Comment on attachment 388843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388843&action=review Not done yet :/ >>> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:81 >>> + }); >> >> Maybe we can flag this as #ifndef NDEBUG > > The installed key IDs are important to know in release and debug. Sure, I mixed this with disabling logs. I meant: #if !LOG_DISABLED || !RELEASE_LOG_DISABLED >>> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.cpp:264 >>> + bool nobodyWaitingForKey = !m_numDecryptorsWaitingForKey; >> >> wasNobodyWaitingForKey? > > if (nobodyWaitingForKey) reads more naturally than if (wasNobodyWaitingForKey). I disagree, I think if (isNobodyWaitingForKey) reads better, besides, it respects the style about bool. >>> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:61 >>> + unsigned refCount() const { return m_references; } >> >> Why can't you just make this WebKit refcounted and avoid all the error prone manual reference counting? >> >> If the intention is to make the object lighter, can't you still store a Ref inside the list at the store? > > The reason I hand-rolled it is due to its use in KeyStore::remove. Because multiple sessions can add a key, I must be careful not to remove from the "global key store" a key that is referenced still by a session. Ref<> is for RAII semantics on a type, I don't want the Key to be destroyed when the last ref is dropped, rather I want "this is the last Key ref" to translate to "OK, now we can remove it from the key list". I agree this code raises eye-brows, can you offer a specific alternative using Ref<> that achieves what I described? I can't see it. Thanks for the explanation, it is what I was trying to understand. What remains confusing is that you're mixing a couple of things that are different in my head. On one side it is object life managed by a smart pointer (you use Ref<Key> around) and then use the same life mechanism to count how many sessions are using that key, you know what I'm sayin'? It raises my eye brows a lot. I wouldn't mix the object ref counting to count how many sessions have a reference to that key. I would do it some other way. Now, if you still think it is the best way, I request you add a big and nice comment explaining all this. > Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:95 > + bool add(Ref<Key>&); I would use move semantics here. >>> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:115 >>> + std::list<Key> m_keys; >> >> Why not a Vector? > > Vector<> is typically not the right data-structure to support heavy inserts/deletes, which the key store is going to be exposed to. Do key updates happen so frequently to consider this an issue? Am I missing anything? If I am wront I can buy this, unless you decide to honor my comments about not mixing session references with life references, then you could keep a Ref<Key> and would not need to rewrite the Vector, but just the object. And anyway, considering we should be using WebKit types whenever possible, I think it would make sense to use DoublyLinkedList instead, right? >>> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:169 >>> + void trackSession(RefPtr<CDMInstanceSession> session) { m_sessions.push_back(session); } >> >> && and WTFMove? > > That doesn't match the caller expectation, > > RefPtr<CDMInstanceSession> newSession = adoptRef(new CDMInstanceSessionClearKey(*this)); > trackSession(newSession); // moving would trash the return value > return newSession; I missed that. Considering we can't move, it might be interesting to either WTFMove considering that the parameter should have the reference increased or maybe declaring it as const & and let the push_back "do the ref". >>> Source/WebCore/platform/encryptedmedia/cdmproxy/CDMProxy.h:179 >>> + std::list<RefPtr<CDMInstanceSession>> m_sessions; >> >> Why not Vector? > > Same reason as the key store. Same questions then (not the related to life management, of course).

Comment on attachment 389135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389135&action=review Review still WIP > Source/WebCore/ChangeLog:10 > + CDMInstance is a main-thread only class, it's purpose is to provide its > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:82 > + LOG(EME, "EME - CDMProxy - key store now has %u keys", numKeys()); > + std::for_each(m_keys.begin(), m_keys.end(), > + [](const Key& key) { > + LOG(EME, "\tEME - CDMProxy - Key ID: %s", key.idAsString().ascii().data()); > + }); Please, flag for log disabled as we don't need to traverse anything if we are not going to do void. > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:99 > +bool KeyStore::add(Ref<Key>& key) I don't know if I am missing anything but I think this would look better with either a && or a const &, preferrably a && and moving when adding or assigning. > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:199 > +Optional<Vector<char>> CDMProxy::tryWaitForKey(const Vector<char>& keyID) const Beautiful! > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:207 > + auto stopWaitingForKeyOnReturn = makeScopeExit([this] { > + stoppedWaitingForKey(); > + }); ScopeExit FTW! > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:263 > + bool nobodyWaitingForKey = !m_numDecryptorsWaitingForKey; WK style, please prepend "is". I really think it reads better, if you strongly object as a native speaker, leave it. I trust you. > Source/WebCore/platform/encryptedmedia/CDMProxy.h:61 > + void ref() { m_references++; } > + void deref() { m_references--; } > + unsigned refCount() const { return m_references; } Asked on the other patch. > Source/WebCore/platform/encryptedmedia/CDMProxy.h:95 > + bool add(Ref<Key>&); As in the other patch, && ? > Source/WebCore/platform/encryptedmedia/CDMProxy.h:115 > + std::list<Key> m_keys; OT = please, refer to other patch to answer > Source/WebCore/platform/encryptedmedia/CDMProxy.h:136 > + Optional<Vector<char>> tryWaitForKey(const Vector<char>& keyID) const; > + Optional<Vector<char>> getOrWaitForKey(const Vector<char>& keyID) const; 😍 > Source/WebCore/platform/encryptedmedia/CDMProxy.h:156 > + // Media player query methods - main thread only . > Source/WebCore/platform/encryptedmedia/CDMProxy.h:158 > + virtual bool isWaitingForKey() const { ASSERT(isMainThread()); return m_numDecryptorsWaitingForKey > 0; } I wonder if in order to achieve most robustness, considering that it is important that we run this on the main thread, we implement the template method design patter here by asserting on the superclass and calling the virtual impl. I'm happy if you leave it like this, as well. > Source/WebCore/platform/encryptedmedia/CDMProxy.h:161 > + // Proxy methods - must be thread-safe .

Comment on attachment 389135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389135&action=review >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:136 >> + Vector<char> keyValue(const Vector<char>& keyID) const; >> + bool keyAvailable(const Vector<char>& keyID) const; >> + bool keyAvailableUnlocked(const Vector<char>& keyID) const; >> + Optional<Vector<char>> tryWaitForKey(const Vector<char>& keyID) const; >> + Optional<Vector<char>> getOrWaitForKey(const Vector<char>& keyID) const; > > 😍 Now that I think of this, wouldn't be more logical to use uint8_t instead of char here? I think it conceptually represents much better one byte. > Source/WebCore/platform/encryptedmedia/CDMProxy.h:169 > + void trackSession(RefPtr<CDMInstanceSession> session) { m_sessions.push_back(session); } const RefPtr & > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:504 > + // FIXME: Unnecessary copy, can we avoid this while still exposing > + // a non-GStreamer-specific DecryptInput API? These buffers are > + // small (16 bytes), so not a huge deal, I guess. ============================================================================================== > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:124 > + UNUSED_PARAM(m_cdmInstance); Is this really needed? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149 > + // FIXME: GStreamer-specific data layout. > + const uint8_t* subsamplesBuffer; > + size_t subsamplesBufferSizeInBytes; > + size_t numSubsamples; If we want to get rid of the specific data layout, we can parse it before assigning it here and maybe have a Vector<size_t> that is a series of clear/decrypted/clear/decrypted... Or maybe even a Vector<std::pair<size_t, size_t>> representing sequences of clear/decrypted sizes. That std::pair could even become a struct ClearDecryptedChunkSizesInBytes for example. If you feel like implementing it now, good. If not, I'm open to leave a comment of how we could implement this in the future, with my suggestion or something you can come up with. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1780 > + GST_ERROR("can't block the main thread waiting for a CDM instance"); GST_ERROR_OBJECT > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1789 > + GST_TRACE("found %zu protection events, %zu decryptors available", protectionSystemEvents.events().size(), protectionSystemEvents.availableSystems().size()); _OBJECT

Comment on attachment 389135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389135&action=review >> Source/WebCore/ChangeLog:10 >> + CDMInstance is a main-thread only class, it's purpose is to provide > > its Done. >> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:82 >> + }); > > Please, flag for log disabled as we don't need to traverse anything if we are not going to do void. Done. >> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:99 >> +bool KeyStore::add(Ref<Key>& key) > > I don't know if I am missing anything but I think this would look better with either a && or a const &, preferrably a && and moving when adding or assigning. Done. Made this take the key by && >> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:263 >> + bool nobodyWaitingForKey = !m_numDecryptorsWaitingForKey; > > WK style, please prepend "is". I really think it reads better, if you strongly object as a native speaker, leave it. I trust you. I agree, missed this the first time you mentioned it. Done. >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:61 >> + unsigned refCount() const { return m_references; } > > Asked on the other patch. Sorry, missed this one to, it's a good point. This does confuse two things. 1/ Ref<Key>, i.e., making sure each session's key store has a valid reference to the keys it knows about. 2/ Removing a key when we should per the EME spec. That is when it has been released or all sessions have removed it explicitly. I think the mistake I've made here is using very general ref-counting names for point 2/, when perhaps I could make them more specific and understandable. It still seems to be I need some alternative ref-counting strategy for point 2/ which Ref<> is by design not going to help me with. I renamed this in the next patch, WDYT? >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:95 >> + bool add(Ref<Key>&); > > As in the other patch, && ? Done. >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:115 >> + std::list<Key> m_keys; > > OT = please, refer to other patch to answer Reviewed and agreed, changed to Vector, I've also switched to managed the Key's as RefPtr as it came out more consistent in the various methods. >>> Source/WebCore/platform/encryptedmedia/CDMProxy.h:136 >>> + Optional<Vector<char>> getOrWaitForKey(const Vector<char>& keyID) const; >> >> 😍 > > Now that I think of this, wouldn't be more logical to use uint8_t instead of char here? I think it conceptually represents much better one byte. Agreed & done. Also needed to make this change to use the VectorHash<> helper trait. >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:156 >> + // Media player query methods - main thread only > > . Done. >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:158 >> + virtual bool isWaitingForKey() const { ASSERT(isMainThread()); return m_numDecryptorsWaitingForKey > 0; } > > I wonder if in order to achieve most robustness, considering that it is important that we run this on the main thread, we implement the template method design patter here by asserting on the superclass and calling the virtual impl. > > I'm happy if you leave it like this, as well. Left as is. >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:161 >> + // Proxy methods - must be thread-safe > > . Done. >> Source/WebCore/platform/encryptedmedia/CDMProxy.h:169 >> + void trackSession(RefPtr<CDMInstanceSession> session) { m_sessions.push_back(session); } > > const RefPtr & Done. >> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:124 >> + UNUSED_PARAM(m_cdmInstance); > > Is this really needed? It's avoiding a boring warning. If you mean the m_cdmInstance field, I think yes because it's important to know that proxies have a handle to instances architecturally. This will be useful for CDMs that async tell us about new keys from the proxy side and we will want to tell the instance side about it. I know you hate these names, hope it makes sense... >> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:149 >> + size_t numSubsamples; > > If we want to get rid of the specific data layout, we can parse it before assigning it here and maybe have a Vector<size_t> that is a series of clear/decrypted/clear/decrypted... Or maybe even a Vector<std::pair<size_t, size_t>> representing sequences of clear/decrypted sizes. That std::pair could even become a struct ClearDecryptedChunkSizesInBytes for example. > > If you feel like implementing it now, good. If not, I'm open to leave a comment of how we could implement this in the future, with my suggestion or something you can come up with. Great idea. I added a comment and think it's best for a follow up patch. This was is getting a bit large. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1780 >> + GST_ERROR("can't block the main thread waiting for a CDM instance"); > > GST_ERROR_OBJECT Done. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1789 >> + GST_TRACE("found %zu protection events, %zu decryptors available", protectionSystemEvents.events().size(), protectionSystemEvents.availableSystems().size()); > > _OBJECT Done.

Created attachment 389384 [details] Patch The ChangeLog needs regenerating, but I will do that after an r+ since the churn is painful to manage in the ChangeLog :-). Will also fix the Apple build when its ready to land.

Comment on attachment 389384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389384&action=review WIP, will finish tomorrow. > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:115 > + RELEASE_ASSERT(keyWithMatchingKeyID->value() == key->value(), "Can this really happen?"); This made me smile as I was really wondering the same thing when I read the code the first time. I don't really know if we need a RELEASE_ASSERT here is ok here. For me, it would be more user friendly to fail playback rather than crashing and we could keep this for debug build with an ASSERT. > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:161 > + RELEASE_ASSERT(false && "key must exist to call this method"); > + UNREACHABLE(); Ditto. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1777 > +bool MediaPlayerPrivateGStreamer::blockWaitingForCDMAttachment(GstMessage* message) I think a bit better name for this would be fireInitDataEventsAndBlockForCDMAttachment. Either this or better split it in two methods. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1850 > + bool cdmAttached = blockWaitingForCDMAttachment(message); isCDMAttached > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1862 > + GST_WARNING("waiting for a CDM failed, no CDM available"); _OBJECT > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:40 > + RefPtr<CDMProxyClearKey> proxyCDM; Arg! We are keeping the proxyCDM name here? Please give me a couple of diff lines more and a big satisfaction to both of us when we see proxyCDM wiped out even from our memories ;) > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:311 > +static gboolean installCDMProxyIfNotAvailable(WebKitMediaCommonEncryptionDecrypt* self) Nit: Even when the intention is using it only during the event handling, I'd keep this as bool instead of gboolean.

Comment on attachment 389384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389384&action=review >> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:115 >> + RELEASE_ASSERT(keyWithMatchingKeyID->value() == key->value(), "Can this really happen?"); > > This made me smile as I was really wondering the same thing when I read the code the first time. > > I don't really know if we need a RELEASE_ASSERT here is ok here. For me, it would be more user friendly to fail playback rather than crashing and we could keep this for debug build with an ASSERT. My thoughts were that I really want to fail hard when/if this happens. Propagating an error value appropriately to fail playback would be quite a bit of extra complexity and for a case that I don't think can happen. I chose RELEASE_ASSERT because a) only a small subset of developers (no users) actually run Debug builds and b) for the "impossible", a release assert seems like a good choice. WDYT? >> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:161 >> + UNREACHABLE(); > > Ditto. Ditto. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1777 >> +bool MediaPlayerPrivateGStreamer::blockWaitingForCDMAttachment(GstMessage* message) > > I think a bit better name for this would be fireInitDataEventsAndBlockForCDMAttachment. Either this or better split it in two methods. Done. Split into parseInitDataFromProtectionMessage & waitForCDMAttachment. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1850 >> + bool cdmAttached = blockWaitingForCDMAttachment(message); > > isCDMAttached Done. >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1862 >> + GST_WARNING("waiting for a CDM failed, no CDM available"); > > _OBJECT Done >> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:40 >> + RefPtr<CDMProxyClearKey> proxyCDM; > > Arg! We are keeping the proxyCDM name here? Please give me a couple of diff lines more and a big satisfaction to both of us when we see proxyCDM wiped out even from our memories ;) 😱 Done. >> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:311 >> +static gboolean installCDMProxyIfNotAvailable(WebKitMediaCommonEncryptionDecrypt* self) > > Nit: Even when the intention is using it only during the event handling, I'd keep this as bool instead of gboolean. Not done, the policy is to use g-types for g-APIs, since this is the return value for a gst callback, I will keep gboolean and avoid the potential for confusion between a bool type and an integer type (what glib typedefs its boolean to)

Created attachment 389648 [details] Patch Addresses all previous comments

(In reply to Charlie Turner from comment #20) > >> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:115 > >> + RELEASE_ASSERT(keyWithMatchingKeyID->value() == key->value(), "Can this really happen?"); > > > > This made me smile as I was really wondering the same thing when I read the code the first time. > > > > I don't really know if we need a RELEASE_ASSERT here is ok here. For me, it would be more user friendly to fail playback rather than crashing and we could keep this for debug build with an ASSERT. What I mean is not propagating the error, I think the error will propagate itself and there will be either decryption errors or decoding errors later, which are fine, IMHO, if we get the error with our Debug build later, once reported.

Comment on attachment 389648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389648&action=review > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:76 > + // NOTE: Do we care that we will not append a key in other if it matches a key ID > + // in the keystore and has different data. Should we overwrite? Which is "newer"? > + // Don't think we need this extra complexity. I wonder if we should ASSERT here just in case? > Source/WebCore/platform/encryptedmedia/CDMProxy.h:174 > + KeyStore m_keyStore; we have two key stores, wouldn't it be possible to have only one? Maybe a fixme for a next iterations? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.h:144 > + const uint8_t* keyID; > + size_t keyIDSizeInBytes; > + > + const uint8_t* iv; > + size_t ivSizeInBytes; > + > + uint8_t* encryptedBuffer; > + size_t encryptedBufferSizeInBytes; FIXME, encapsulate these in non-copied structures. > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:275 > +static CDMProxy* requestCDMProxyFromGstContext(WebKitMediaCommonEncryptionDecrypt* self) Nit, I think we are not requesting, more like getting, specially according to the long comment (yes, I wrote that myself)

Created attachment 390075 [details] Patch Addresses all comments. Submit to EWS to check Apple build status

Comment on attachment 390075 [details] Patch Setting r? again to see if I can trigger an EWS submission

Created attachment 390254 [details] Patch Attempt #1 at getting the Xcode build system to cooperate

Created attachment 390308 [details] Patch Attempt #2 at getting Apple ports building

The problem now is Apple port has a dependency on the CDMClearKey, which I initially thought could be removed since Apple doesn't support ClearKey, but avfoundation actually appears to! So, I think it would be best to move the CDMProxy ClearKey implementation into platform/graphics/gstreamer/eme instead and fix the now indirect dependency of Apple ports on GCrypt, which clearly is bad :-)

Created attachment 390350 [details] Patch Attempt #1 at moving G* specific ClearKey code to GStreamer platform directory

Created attachment 390354 [details] Patch Attempt #1.1.1ctually remove the gcrypt include now from CDMClearKey...

Created attachment 390377 [details] Patch Attempt #2 | The Mac bots have -Werror and -Wunused-parameter, GTK port does not :/

Created attachment 390398 [details] Patch Attempt #3 | Looks like CDMProxy.cpp is not getting compiled, since there's undefined references to methods defined in there. Noticed there's also a Sources.txt file that CDMFactory.cpp was in, try adding CDMProxy.cpp in there as well, maybe the Mac port needs that. After this I'm out of ideas of how to add these new files to the Apple build, it looks like I've added it in the relevant areas the X-files.

Created attachment 390404 [details] Patch Attempt #3 | More unusued parameter warnings I missed due to developing in Debug

Created attachment 390405 [details] Patch Remove global -Wunused-paramters used during testing for the Mac build requirements

Created attachment 390464 [details] Patch More unused parameters on Mac due to LOG_DISABLED=T && RELEASE_LOG_DISABLED=F, another difference to the GTK builds for me

Created attachment 390502 [details] Patch Patch for landing

Comment on attachment 390502 [details] Patch Clearing flags on attachment: 390502 Committed r256429: <https://trac.webkit.org/changeset/256429>

All reviewed patches have been landed. Closing bug.

(In reply to WebKit Commit Bot from comment #37) > Comment on attachment 390502 [details] > Patch > > Clearing flags on attachment: 390502 > > Committed r256429: <https://trac.webkit.org/changeset/256429> This seems to have broken the build on the GTK minimum dependencies bots (that build without jhbuild, using the distro dependencies). Debian 10: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/29727 Ubuntu 18.04: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/29264

Comment on attachment 390502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390502&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1792 > +InitData MediaPlayerPrivateGStreamer::parseInitDataFromProtectionMessage(GstMessage* message) Missing some encryptedMedia guards? https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/29264/steps/compile-webkit/logs/stdio

Thanks for pointing the build break out, I've fixed it in https://bugs.webkit.org/show_bug.cgi?id=207628