[GStreamer][EME][OpenCDM] Implement OpenCDM support

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

We're implementing the OpenCDM support.

[Xabier Rodríguez Calvar]

Created attachment 402635 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 402657 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 402721 [details]
Patch

[Charlie Turner]

Comment on attachment 402721 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:126
> +const uint8_t* SharedBuffer::dataAsUInt8Ptr() const

It bothers me that the public API has to change like this, but I don't know how to make it better. There's https://en.cppreference.com/w/cpp/language/cast_operator, but that's probably a little too magical. Shame we can't overload based on return types :-)

Maybe even an out-of-class function that wraps the static casting.

I find it odd the API returns a signed type for the bytes anyway, AFAICT uint8_t is more natural for this class as a default anyway.

> Source/WebCore/platform/SharedBuffer.h:109
> +    const uint8_t* dataAsUInt8Ptr() const;

Could inline the definition here.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:107
> +    return "[opaque key value]"_s;

When does this happen?

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:127
> +{

This is a default operator=, the compiler will do this for you.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:435
> +void CDMInstanceSessionProxy::getRemovedFromInstanceProxy()

removeFromInstanceProxy

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:49
> +using KeyHandleValueVariant = Variant<

Note that, only when ENABLE(OPENCDM), does this variant make sense. If only 1 out of 2 options make use of a variant, then the variant is unnecessary. Instead, do this,

#if ENABLE(OPENCDM)
using KeyValueType = BoxPtr<OpenCDMSession>
#else
using KeyValueType = Vector<uint8_t>
#endif

and then, all call-sites using this value go back to being simple without requiring (*constructs)->likethat().

It would be even better if instead of using #if guards, you moved the definition of these types into the classes, and then used templates or inheritance, as you wish, to condition the value of types on the implementation of the CDM.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:128
> +    KeyStore(MergeStrategy mergeStrategy = MergeStrategy::NoOp) { setMergeStrategy(mergeStrategy); }

Use a class initializer for default, then the ctor can be remove (let the compiler do it for you).

I commented this on the last bug without response, but this merge strategy pattern is overkill here IMO. Feel free to ignore again, but for prosperity I repeat :-)

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:175
> +    void setMergeStrategy(KeyStore::MergeStrategy strategy) { m_keyStore.setMergeStrategy(strategy); }

Why do we need this level of runtime configuration? Does the strategy ever change at runtime in practice?

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:246
> +    explicit CDMInstanceProxy(const String& keySystem, KeyStore::MergeStrategy mergeStrategy = KeyStore::MergeStrategy::NoOp)

Ditto, use a class initializer to clean this up. Again though, don't think we need this complexity.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:299
> +        unsigned openCDMRank = isOpenCDMRanked() ? 300 : 100;

Please add an explanation for this one.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:402
> +    Vector<uint8_t> value;

Nit of year? But I prefer vec to value here :-)

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:403
> +    value.append(data(), size());

I've seen this sort of thing enough times now that I propose adding another constructor to Vector, taking a byte array and a size.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:219
> +    if (m_cdmInstance && !g_strcmp0(g_getenv("WEBKIT_EME_RELEASE_OPENCDM_WITH_PLAYER_DESTRUCTION"), "yes"))

Why do we use environment variables for this? Seems a bit dodgy.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3518
> +        GST_DEBUG("scheduling initializationDataEncountered %s event of size %zu", initData.payloadContainerType().utf8().data()

Something seems odd with the whitespace here

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:59
> +using OpenCDMKeyStatus = KeyStatus;

Why?

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:106
> +static RefPtr<JSON::Object> parseJSONObject(const SharedBuffer& buffer)

This function is copied from the ClearKey file. Instead of copying, place it in a common utility file.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:152
> +            ASSERT_NOT_REACHED_WITH_MESSAGE("No key systems supported on OpenCDM and OpenCDM is preferred. Is Thunder running? Are the plugins built?");

This message doesn't make sense to me.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:272
> +    callback(!opencdm_system_set_server_certificate(m_openCDMSystem.get(), const_cast<uint8_t*>(certificate->dataAsUInt8Ptr()), certificate->size())

Nit: I'd use a variable to tidy this ?: up.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:286
> +        , const uint16_t challengeLength) {

Nit: I don't think the commas are supposed to drop down like this? o_O

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:332
> +static std::pair<RefPtr<SharedBuffer>, Optional<WebCore::MediaKeyMessageType>> parseResponseMessage(const RefPtr<SharedBuffer>& buffer)

Nit: I admit this is my personal taste, but I've come to find std::pair an abomination. Structs come with lovely names that hide all of the >><><<><><. They also don't force you to name your variables first and second.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:335
> +        return std::make_pair(SharedBuffer::create(), WTF::nullopt);

Seems we should return both an optional RefPtr<SharedBuffer> to go with the optional MediaKeyMessageType

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:338
> +    String message(StringImpl::createWithoutCopying(reinterpret_cast<const LChar*>(buffer->data()), buffer->size()));

That looks like string could use a new static constructor method.
Maybe StringView::toStringWithoutCopying would help a little.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:340
> +    String requestType(message.characters8(), typePosition != notFound ? typePosition : 0);

I would do this spliting / parsing using a StringView to both make the code's intent cleaner and avoid unnecessary copies. That should extend to the rest of this method as well I reckon.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:355
> +    // This can be called as a result of e.g. requestLicense() but update() or remove() as well.

Do you mean all three? Or that it's most common for requestLicence, but can happen for update() and remove() too? I'd simply say "Called as a result of requestLicense(), update() or remove()"

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:356
> +    // This called not as a response to API call is also possible.

Doesn't make any sense.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:364
> +    } else if (!m_sessionChangedCallbacks.isEmpty()) {

Is it intentional that session changed callbacks are not fired if there's any challenge callbacks fired?

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:370
> +            m_client->sendMessage(static_cast<CDMMessageType>(messageAndRequestType.second.value()), messageAndRequestType.first.releaseNonNull());

This makes me push harder on the nit above, that the std::pair is hindering more than it's helping. A dedicated struct for the type shared buffer with a helper method for ^ seems a good idea.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:399
> +CDMInstanceSession::KeyStatus CDMInstanceSessionOpenCDM::status(const KeyIDType& keyID) const

In keeping with the other enum helpers, name it toKeyStatus.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:444
> +        m_client->updateKeyStatuses(m_keyStore.convertToJSKeyStatusVector());

Why must it run only if there's no callbacks? Please leave a comment since this is weird.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:512
> +        generateChallenge();

Why must be fire these callbacks now? Both conditions are handled in the lambda already. I worry there's some timing condition hidden here.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:533
> +            if (!buffer) {

buffer is too generally named, i'd call it responseMessage, or responseMessageBuffer if you like types in the names.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:563
> +    m_sessionChangedCallbacks.append([this, callback = WTFMove(callback)](bool success, RefPtr<SharedBuffer>&& buffer) mutable {

ditto about buffer

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:570
> +                // will even be other parts of the spec where not having structured data will be bad.

This is all looking very repetitive. Create a new class to hold a parsed object for the response payloads seems a good idea. E.g parsedReponseMessage should return an object, OpenCDMResponseMessage, which has methods like type() and payload().

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:576
> +                        , messageAndRequestType.first.releaseNonNull()), SuccessValue::Succeeded, SessionLoadFailure::None);

In line with the above, this needs tidying up.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:587
> +            callback(WTF::nullopt, WTF::nullopt, WTF::nullopt, SuccessValue::Failed, sessionLoadFailureFromOpenCDM(response));

Note in particular the WTF::nullopt, WTF::nullopt, WTF::nullopt repitition, that can be factored using another lambda and make this code more readable.

[Charlie Turner]

Comment on attachment 402721 [details]
Patch

Reset review flag, I accidentally hit it as an informal reviewer.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #4)
> Comment on attachment 402721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402721&action=review
> 
> > Source/WebCore/platform/SharedBuffer.cpp:126
> > +const uint8_t* SharedBuffer::dataAsUInt8Ptr() const
> 
> It bothers me that the public API has to change like this, but I don't know
> how to make it better. There's
> https://en.cppreference.com/w/cpp/language/cast_operator, but that's
> probably a little too magical. Shame we can't overload based on return types
> :-)
> 
> Maybe even an out-of-class function that wraps the static casting.
> 
> I find it odd the API returns a signed type for the bytes anyway, AFAICT
> uint8_t is more natural for this class as a default anyway.

I agree... But I think any of the solutions is out of the scope of this bug other than giving you a quick function that does the cast for you.

> > Source/WebCore/platform/SharedBuffer.h:109
> > +    const uint8_t* dataAsUInt8Ptr() const;
> 
> Could inline the definition here.

Nope!

  Running check-webkit-style.
  ERROR: Source/WebCore/platform/SharedBuffer.h:109:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:107
> > +    return "[opaque key value]"_s;
> 
> When does this happen?

Good question! This was already there but it's currently unused so I remove the whole method, anyone can add it if needed.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:127
> > +{
> 
> This is a default operator=, the compiler will do this for you.

No, I can't because it is deleted the ThreadSafeRefCountedBase but after rethinking if merge is needed, it looks like it isn't so copying a key is not needed (I added an ASSERT for this just in case). I removed all the code related to merging.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:435
> > +void CDMInstanceSessionProxy::getRemovedFromInstanceProxy()
> 
> removeFromInstanceProxy

Done

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:49
> > +using KeyHandleValueVariant = Variant<
> 
> Note that, only when ENABLE(OPENCDM), does this variant make sense. If only
> 1 out of 2 options make use of a variant, then the variant is unnecessary.
> Instead, do this,
> 
> #if ENABLE(OPENCDM)
> using KeyValueType = BoxPtr<OpenCDMSession>
> #else
> using KeyValueType = Vector<uint8_t>
> #endif
> 
> and then, all call-sites using this value go back to being simple without
> requiring (*constructs)->likethat().
> 
> It would be even better if instead of using #if guards, you moved the
> definition of these types into the classes, and then used templates or
> inheritance, as you wish, to condition the value of types on the
> implementation of the CDM.

I don't think doing this is a good idea because if I do this I either build OpenCDM or ClearKey. This way I can do both at the same time. I think this is something to remove if we can make ClearKey work with OpenCDM but I don't see it feasible.
 
> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:128
> > +    KeyStore(MergeStrategy mergeStrategy = MergeStrategy::NoOp) { setMergeStrategy(mergeStrategy); }
> 
> Use a class initializer for default, then the ctor can be remove (let the
> compiler do it for you).
> 
> I commented this on the last bug without response, but this merge strategy
> pattern is overkill here IMO. Feel free to ignore again, but for prosperity
> I repeat :-)

This is already reworked and this constructor is default.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:175
> > +    void setMergeStrategy(KeyStore::MergeStrategy strategy) { m_keyStore.setMergeStrategy(strategy); }
> 
> Why do we need this level of runtime configuration? Does the strategy ever
> change at runtime in practice?

Not anymore

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:246
> > +    explicit CDMInstanceProxy(const String& keySystem, KeyStore::MergeStrategy mergeStrategy = KeyStore::MergeStrategy::NoOp)
> 
> Ditto, use a class initializer to clean this up. Again though, don't think
> we need this complexity.

Done.

> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:299
> > +        unsigned openCDMRank = isOpenCDMRanked() ? 300 : 100;
> 
> Please add an explanation for this one.

Done.

> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:402
> > +    Vector<uint8_t> value;
> 
> Nit of year? But I prefer vec to value here :-)

Done.

> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:403
> > +    value.append(data(), size());
> 
> I've seen this sort of thing enough times now that I propose adding another
> constructor to Vector, taking a byte array and a size.

I think this is an improvement that can be tackled later.
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:219
> > +    if (m_cdmInstance && !g_strcmp0(g_getenv("WEBKIT_EME_RELEASE_OPENCDM_WITH_PLAYER_DESTRUCTION"), "yes"))
> 
> Why do we use environment variables for this? Seems a bit dodgy.

We have the same problem with libgcrypt in ClearKey so I made it general and not depending on an env var.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3518
> > +        GST_DEBUG("scheduling initializationDataEncountered %s event of size %zu", initData.payloadContainerType().utf8().data()
> 
> Something seems odd with the whitespace here

I don't know what you mean.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:59
> > +using OpenCDMKeyStatus = KeyStatus;
> 
> Why?

Cause unfortunately OpenCDM declares a KeyStatus and it conflicts with the CDMInstanceSession one but I added a comment to make it clear.
 
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:106
> > +static RefPtr<JSON::Object> parseJSONObject(const SharedBuffer& buffer)
> 
> This function is copied from the ClearKey file. Instead of copying, place it
> in a common utility file.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:152
> > +            ASSERT_NOT_REACHED_WITH_MESSAGE("No key systems supported on OpenCDM and OpenCDM is preferred. Is Thunder running? Are the plugins built?");
> 
> This message doesn't make sense to me.

Improved, I hope.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:272
> > +    callback(!opencdm_system_set_server_certificate(m_openCDMSystem.get(), const_cast<uint8_t*>(certificate->dataAsUInt8Ptr()), certificate->size())
> 
> Nit: I'd use a variable to tidy this ?: up.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:286
> > +        , const uint16_t challengeLength) {
> 
> Nit: I don't think the commas are supposed to drop down like this? o_O

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:332
> > +static std::pair<RefPtr<SharedBuffer>, Optional<WebCore::MediaKeyMessageType>> parseResponseMessage(const RefPtr<SharedBuffer>& buffer)
> 
> Nit: I admit this is my personal taste, but I've come to find std::pair an
> abomination. Structs come with lovely names that hide all of the >><><<><><.
> They also don't force you to name your variables first and second.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:335
> > +        return std::make_pair(SharedBuffer::create(), WTF::nullopt);
> 
> Seems we should return both an optional RefPtr<SharedBuffer> to go with the
> optional MediaKeyMessageType

Done. If it's optional, it becomes Ref.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:338
> > +    String message(StringImpl::createWithoutCopying(reinterpret_cast<const LChar*>(buffer->data()), buffer->size()));
> 
> That looks like string could use a new static constructor method.
> Maybe StringView::toStringWithoutCopying would help a little.
>
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:340
> > +    String requestType(message.characters8(), typePosition != notFound ? typePosition : 0);
> 
> I would do this spliting / parsing using a StringView to both make the
> code's intent cleaner and avoid unnecessary copies. That should extend to
> the rest of this method as well I reckon.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:355
> > +    // This can be called as a result of e.g. requestLicense() but update() or remove() as well.
> 
> Do you mean all three? Or that it's most common for requestLicence, but can
> happen for update() and remove() too? I'd simply say "Called as a result of
> requestLicense(), update() or remove()"
>
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:356
> > +    // This called not as a response to API call is also possible.
> 
> Doesn't make any sense.

I think the comment does not make sense anymore so I removed it.
 
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:370
> > +            m_client->sendMessage(static_cast<CDMMessageType>(messageAndRequestType.second.value()), messageAndRequestType.first.releaseNonNull());
> 
> This makes me push harder on the nit above, that the std::pair is hindering
> more than it's helping. A dedicated struct for the type shared buffer with a
> helper method for ^ seems a good idea.

Done.
 
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:399
> > +CDMInstanceSession::KeyStatus CDMInstanceSessionOpenCDM::status(const KeyIDType& keyID) const
> 
> In keeping with the other enum helpers, name it toKeyStatus.

This is not a toKeyStatus method. It gets the status for a key ID.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:512
> > +        generateChallenge();
> 
> Why must be fire these callbacks now? Both conditions are handled in the
> lambda already. I worry there's some timing condition hidden here.

In some cases the OpenCDM callback is not called and we need to handle them as well, that's why we force the callback to be called here.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:533
> > +            if (!buffer) {
> 
> buffer is too generally named, i'd call it responseMessage, or
> responseMessageBuffer if you like types in the names.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:563
> > +    m_sessionChangedCallbacks.append([this, callback = WTFMove(callback)](bool success, RefPtr<SharedBuffer>&& buffer) mutable {
> 
> ditto about buffer

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:570
> > +                // will even be other parts of the spec where not having structured data will be bad.
> 
> This is all looking very repetitive. Create a new class to hold a parsed
> object for the response payloads seems a good idea. E.g parsedReponseMessage
> should return an object, OpenCDMResponseMessage, which has methods like
> type() and payload().

Done.
 
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:576
> > +                        , messageAndRequestType.first.releaseNonNull()), SuccessValue::Succeeded, SessionLoadFailure::None);
> 
> In line with the above, this needs tidying up.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMOpenCDM.cpp:587
> > +            callback(WTF::nullopt, WTF::nullopt, WTF::nullopt, SuccessValue::Failed, sessionLoadFailureFromOpenCDM(response));
> 
> Note in particular the WTF::nullopt, WTF::nullopt, WTF::nullopt repitition,
> that can be factored using another lambda and make this code more readable.

No, because the callbacks are CompletionHandlers that would need to be moved into the lambda causing to not be able to used them outside the lambda for the success cases.

And all this, together switching back from BoxPtr to std::shared_ptr and a couple of more improvements with come soon to your screen.

[Xabier Rodríguez Calvar]

Created attachment 403462 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 403519 [details]
Patch

Trying to fix Apple build

[Xabier Rodríguez Calvar]

Created attachment 403685 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 403689 [details]
Patch

[Philippe Normand]

Comment on attachment 403689 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:292
> +    const char* value = g_getenv("WEBKIT_EME_RANK_PRIORITY");

WEBKIT_GST_...

[Xabier Rodríguez Calvar]

Created attachment 403957 [details]
Patch

(In reply to Philippe Normand from comment #11)
> Comment on attachment 403689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403689&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:292
> > +    const char* value = g_getenv("WEBKIT_EME_RANK_PRIORITY");
> 
> WEBKIT_GST_...

Done, renamed OpenCDM into Thunder, moved from shared_ptr into the new BoxPtr.

Maybe a final review and we would be good to land.

[EWS]

Committed r264219: <https://trac.webkit.org/changeset/264219>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403957 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65340424>

[Aakash Jain]

(In reply to EWS from comment #13)
> Committed r264219: <https://trac.webkit.org/changeset/264219>
This seems to have broken gtk api test: /WebKit2Gtk/TestWebKitWebView:/webkit/WebKitWebView/is-playing-audio

EWS also warned about this failure in https://ews-build.webkit.org/#/builders/34/builds/12940

[Philippe Normand]

https://trac.webkit.org/r264225
https://trac.webkit.org/r264226

[Philippe Normand]

Comment on attachment 403957 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1610
> +    if (GST_MESSAGE_TYPE(message) == GST_MESSAGE_ERROR && !g_strcmp0(g_getenv("WEBKIT_CRASH_ON_GSTREAMER_ERROR"), "sync")) {
> +        GUniqueOutPtr<GError> err;
> +        GUniqueOutPtr<gchar> debug;
> +        gst_message_parse_error(message, &err.outPtr(), &debug.outPtr());
> +        GST_ERROR("Error %d from %s: %s (url=%s)", err->code, GST_MESSAGE_SRC_NAME(message), err->message, m_url.string().utf8().data());
> +        GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "webkit-video-sync-error");
> +        ASSERT_NOT_REACHED();
> +    }

Was this meant for debug purpose or is it useful upstream?

[Adrian Perez]

Comment on attachment 403957 [details]
Patch

I have added a couple of comments which you may want to address in
a follow-up patch.

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

> Source/cmake/FindThunder.cmake:10
> +#     True if the requested version of gcrypt was found

Surely you meant to do s/gcrypt/Thunder/ here (and in the lines below).

> Source/cmake/FindThunder.cmake:50
> +)

Also, is there any reason why this is not using an imported target?

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #17)
> Comment on attachment 403957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403957&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1610
> > +    if (GST_MESSAGE_TYPE(message) == GST_MESSAGE_ERROR && !g_strcmp0(g_getenv("WEBKIT_CRASH_ON_GSTREAMER_ERROR"), "sync")) {
> > +        GUniqueOutPtr<GError> err;
> > +        GUniqueOutPtr<gchar> debug;
> > +        gst_message_parse_error(message, &err.outPtr(), &debug.outPtr());
> > +        GST_ERROR("Error %d from %s: %s (url=%s)", err->code, GST_MESSAGE_SRC_NAME(message), err->message, m_url.string().utf8().data());
> > +        GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "webkit-video-sync-error");
> > +        ASSERT_NOT_REACHED();
> > +    }
> 
> Was this meant for debug purpose or is it useful upstream?

It was meant for debug, but it could be useful upstream. I'll fix it.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #19)
> (In reply to Philippe Normand from comment #17)
> > Comment on attachment 403957 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403957&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1610
> > > +    if (GST_MESSAGE_TYPE(message) == GST_MESSAGE_ERROR && !g_strcmp0(g_getenv("WEBKIT_CRASH_ON_GSTREAMER_ERROR"), "sync")) {
> > > +        GUniqueOutPtr<GError> err;
> > > +        GUniqueOutPtr<gchar> debug;
> > > +        gst_message_parse_error(message, &err.outPtr(), &debug.outPtr());
> > > +        GST_ERROR("Error %d from %s: %s (url=%s)", err->code, GST_MESSAGE_SRC_NAME(message), err->message, m_url.string().utf8().data());
> > > +        GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "webkit-video-sync-error");
> > > +        ASSERT_NOT_REACHED();
> > > +    }
> > 
> > Was this meant for debug purpose or is it useful upstream?
> 
> It was meant for debug, but it could be useful upstream. I'll fix it.

Done in r264296.

[Xabier Rodríguez Calvar]

(In reply to Adrian Perez from comment #18)
> Comment on attachment 403957 [details]
> Patch
> 
> I have added a couple of comments which you may want to address in
> a follow-up patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403957&action=review
> 
> > Source/cmake/FindThunder.cmake:10
> > +#     True if the requested version of gcrypt was found
> 
> Surely you meant to do s/gcrypt/Thunder/ here (and in the lines below).

Done in r264300.

> > Source/cmake/FindThunder.cmake:50
> > +)
> 
> Also, is there any reason why this is not using an imported target?

Nope, I just used GCrypt one as template as comment above suggests :) . I guess it can be done later.

[Adrian Perez]

(In reply to Xabier Rodríguez Calvar from comment #21)
> (In reply to Adrian Perez from comment #18)
> > Comment on attachment 403957 [details]
> > Patch
> > 
> > I have added a couple of comments which you may want to address in
> > a follow-up patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403957&action=review
> > 
> > > Source/cmake/FindThunder.cmake:10
> > > +#     True if the requested version of gcrypt was found
> > 
> > Surely you meant to do s/gcrypt/Thunder/ here (and in the lines below).
> 
> Done in r264300.

Neat, thanks!

> > > Source/cmake/FindThunder.cmake:50
> > > +)
> > 
> > Also, is there any reason why this is not using an imported target?
> 
> Nope, I just used GCrypt one as template as comment above suggests :) . I
> guess it can be done later.

Sure, we can convert it to use imported targets later on.