Bug 205999

Summary: [EME][ClearKey] Refactor CDMClearKey::update()
Product: WebKit Reporter: Charlie Turner <cturner>
Component: WebKitGTKAssignee: Charlie Turner <cturner>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Charlie Turner 2020-01-09 06:35:13 PST
[EME][ClearKey] Refactor CDMClearKey::update()
Comment 1 Charlie Turner 2020-01-09 06:39:33 PST
Created attachment 387218 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2020-01-09 08:18:47 PST
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?
Comment 3 Charlie Turner 2020-01-09 08:58:32 PST
(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.
Comment 4 Charlie Turner 2020-01-09 08:59:41 PST
Created attachment 387238 [details]
Patch
Comment 5 WebKit Commit Bot 2020-01-09 09:37:52 PST
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
Comment 6 Xabier Rodríguez Calvar 2020-01-10 01:38:34 PST
(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.
Comment 7 Xabier Rodríguez Calvar 2020-01-10 01:42:44 PST
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!
Comment 8 Charlie Turner 2020-01-10 02:58:43 PST
Created attachment 387321 [details]
Patch
Comment 9 WebKit Commit Bot 2020-01-10 04:32:38 PST
Comment on attachment 387321 [details]
Patch

Clearing flags on attachment: 387321

Committed r254334: <https://trac.webkit.org/changeset/254334>
Comment 10 WebKit Commit Bot 2020-01-10 04:32:40 PST
All reviewed patches have been landed.  Closing bug.