[webkit-reviews] review granted: [Bug 224276] Update SFrame implementation to latest version : [Attachment 425386] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 224276: Update SFrame implementation to latest version
https://bugs.webkit.org/show_bug.cgi?id=224276

Attachment 425386: Patch

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




--- Comment #4 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 425386
  --> https://bugs.webkit.org/attachment.cgi?id=425386
Patch

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

> Source/WebCore/ChangeLog:8
> +	   Update implementation according latest draft at
https://github.com/eomara/sframe/blob/master/draft-omara-sframe.md.

s/according latest draft/according to latest draft/

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:74
>  static inline Vector<uint8_t> computeIV(uint64_t counter, const
Vector<uint8_t>& saltKey)
>  {
>      Vector<uint8_t> iv(16);

A comment about what this does would be helpful. Maybe you should quote the
current spec text "The key for the encryption is the sframe_key and the nonce
is formed by XORing ..."?

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80
> +    for (unsigned i = 11; i >= 4; --i) {
> +	   auto value = counter & 0xff;
> +	   counter = counter >> 8;
>	   iv[i] = value ^ saltKey[i];

`value` isn't used.

Can you convert the salt key to big-endian before XORing to make this clearer?

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:167
> +    ASSERT(saltKeyResult.returnValue().size() >= 12);

It would be nice to have this magic value in a named constant to help someone
not completely familiar with the spec who reads this