Bug 205999 – [EME][ClearKey] Refactor CDMClearKey::update()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

[EME][ClearKey] Refactor CDMClearKey::update()

[Charlie Turner]

Created attachment 387218 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 387218 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        (WebCore::sharedBufferAsHexString): Helper method to convert a shared buffer containing bytes into a hex string, useful for debug prints.
> +        (WebCore::CDMInstanceClearKey::Key::keyIDAsString const): Uses the helper to return a string containing the hex values of the key ID.
> +        (WebCore::CDMInstanceClearKey::Key::keyValueAsString const): Ditto for the key value.
> +        (WebCore::operator==): Added comparison operators to refactor updateLicense's duplication of comparisons.
> +        (WebCore::operator<): Ditto.
> +        (WebCore::CDMInstanceSessionClearKey::updateLicense): Refactor to rely on equality comparisons in one place, the key class, and to remove thorny open-coded memcmp's in the middle of conditionals which made the method unnecessarily hard to reason about. The extra copying of the key status vector from the key store has been eliminated, instead the vector is sorted in-place directly inside the HashMap, which also cleaned up the method further.
> +        * platform/encryptedmedia/clearkey/CDMClearKey.h:

Aren't these lines two long? (This is funny coming from me)

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:543
> +static String sharedBufferAsHexString(const RefPtr<SharedBuffer> buf)

& or directly move this to SharedBuffer::toHexString?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:545
> +    StringBuilder sb;

stringBuilder

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:550
> +        if (byteOffset < buf->size() - 1)
> +            sb.append(' ');

Are appending a space after each byte? Do you think it is needed?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:565
> +bool operator==(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2)

Move the & next to the type, both parameters

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573
> +bool operator<(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2)

Ditto

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:582
> +    if (k1.keyIDData->size() != k2.keyIDData->size())
> +        return k1.keyIDData->size() < k2.keyIDData->size();
> +
> +    size_t minimumBufferSize = std::min(k1.keyIDData->size(), k2.keyIDData->size());
> +    return memcmp(k1.keyIDData->data(), k2.keyIDData->data(), minimumBufferSize);

Aren't the buffers going to have the same size after the if? I think you can remove the minimum size and use one of the sizes, right? Am I missing anything?

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 387218 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387218&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        (WebCore::sharedBufferAsHexString): Helper method to convert a shared buffer containing bytes into a hex string, useful for debug prints.
> > +        (WebCore::CDMInstanceClearKey::Key::keyIDAsString const): Uses the helper to return a string containing the hex values of the key ID.
> > +        (WebCore::CDMInstanceClearKey::Key::keyValueAsString const): Ditto for the key value.
> > +        (WebCore::operator==): Added comparison operators to refactor updateLicense's duplication of comparisons.
> > +        (WebCore::operator<): Ditto.
> > +        (WebCore::CDMInstanceSessionClearKey::updateLicense): Refactor to rely on equality comparisons in one place, the key class, and to remove thorny open-coded memcmp's in the middle of conditionals which made the method unnecessarily hard to reason about. The extra copying of the key status vector from the key store has been eliminated, instead the vector is sorted in-place directly inside the HashMap, which also cleaned up the method further.
> > +        * platform/encryptedmedia/clearkey/CDMClearKey.h:
> 
> Aren't these lines two long? (This is funny coming from me)

Hehe, fixed.

> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:543
> > +static String sharedBufferAsHexString(const RefPtr<SharedBuffer> buf)
> 
> & or directly move this to SharedBuffer::toHexString?
> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:545
> > +    StringBuilder sb;
> 
> stringBuilder

Fixed.

> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:550
> > +        if (byteOffset < buf->size() - 1)
> > +            sb.append(' ');
> 
> Are appending a space after each byte? Do you think it is needed?

I don't know how to get the `hex` modified to zero pad, without that it can be hard, without the added spaces, to see the individual byte values, since a byte value of 5 between, say, a1 and b2 would look like a15b2 and it's get confusing...

> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:565
> > +bool operator==(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2)
> 
> Move the & next to the type, both parameters

Fixed.

> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573
> > +bool operator<(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2)
> 
> Ditto

Fixed.

> 
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:582
> > +    if (k1.keyIDData->size() != k2.keyIDData->size())
> > +        return k1.keyIDData->size() < k2.keyIDData->size();
> > +
> > +    size_t minimumBufferSize = std::min(k1.keyIDData->size(), k2.keyIDData->size());
> > +    return memcmp(k1.keyIDData->data(), k2.keyIDData->data(), minimumBufferSize);
> 
> Aren't the buffers going to have the same size after the if? I think you can
> remove the minimum size and use one of the sizes, right? Am I missing
> anything?

You are right, the defensive code was from a previous iteration. Fixed.

[Charlie Turner]

Created attachment 387238 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387238 [details]
Patch

Rejecting attachment 387238 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 387238, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13301813

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #3)
> > 
> > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:550
> > > +        if (byteOffset < buf->size() - 1)
> > > +            sb.append(' ');
> > 
> > Are appending a space after each byte? Do you think it is needed?
> 
> I don't know how to get the `hex` modified to zero pad, without that it can
> be hard, without the added spaces, to see the individual byte values, since
> a byte value of 5 between, say, a1 and b2 would look like a15b2 and it's get
> confusing...

And wouldn't it be better to write a105b2 instead, cause the 0 is actually there.

[Xabier Rodríguez Calvar]

Comment on attachment 387238 [details]
Patch

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

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:543
> +static String sharedBufferAsHexString(const RefPtr<SharedBuffer>& buf)

The more I look at this, the more I think we should move it to SharedBuffer::toHexString...

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573
> +bool operator<(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2)

& next to the type!

[Charlie Turner]

Created attachment 387321 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387321 [details]
Patch

Clearing flags on attachment: 387321

Committed r254334: <https://trac.webkit.org/changeset/254334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.