[webkit-reviews] review denied: [Bug 213550] [GStreamer][EME][OpenCDM] Implement OpenCDM support : [Attachment 402721] Patch

Mon Jun 29 06:37:59 PDT 2020

Charlie Turner <cturner at igalia.com> has denied Xabier Rodríguez Calvar
<calvaris at igalia.com>'s request for review:
Bug 213550: [GStreamer][EME][OpenCDM] Implement OpenCDM support
https://bugs.webkit.org/show_bug.cgi?id=213550

Attachment 402721: Patch

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

--- Comment #4 from Charlie Turner <cturner at igalia.com> ---
Comment on attachment 402721
  --> https://bugs.webkit.org/attachment.cgi?id=402721
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.