Bug 162608 – [GTK] Update WOFF2 decoder

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Khaled Hosny]

The WOFF2 reference implementation have been updated to match the latest version of the spec in handling totalSfntSize (See https://github.com/google/woff2/pull/48). We need to update the code and use the new API otherwise some valid WOFF2 fonts will not be decoded.

[Khaled Hosny]

Created attachment 289942 [details]
Patch

[WebKit Commit Bot]

Attachment 289942 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/WOFFFileFormat.cpp:117:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Khaled Hosny]

Created attachment 289945 [details]
Patch, v2

[Frédéric Wang (:fredw)]

Comment on attachment 289945 [details]
Patch, v2

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

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:103
> +        return Write(data, m_size, n);

I think this should be removed.

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:104
> +        if (!m_vector.tryReserveCapacity(m_size + n))

Is it needed? It seems that WTF::Vector::append already calls expandCapacity.

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:113
> +        if (!m_vector.tryReserveCapacity(m_size + n))

Same here.

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:116
> +        m_vector.insert(offset, static_cast<const char*>(data), n);

I wonder if it's possible to copy the data in one step.

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:128
> +    size_t m_size;

Do you really need this member? It seems that you can just use m_vector::size()

[Khaled Hosny]

Comment on attachment 289945 [details]
Patch, v2

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

>> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:103
>> +        return Write(data, m_size, n);
> 
> I think this should be removed.

Good catch, I think it was a left over from previous trials.

>> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:104
>> +        if (!m_vector.tryReserveCapacity(m_size + n))
> 
> Is it needed? It seems that WTF::Vector::append already calls expandCapacity.

I’m not sure what is going on, but if I don’t do that decoding will always fail.

>> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:116
>> +        m_vector.insert(offset, static_cast<const char*>(data), n);
> 
> I wonder if it's possible to copy the data in one step.

I’m not sure if Vector has an API to do that, I didn’t seem to find any.

>> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:128
>> +    size_t m_size;
> 
> Do you really need this member? It seems that you can just use m_vector::size()

I removed it.

[Khaled Hosny]

Created attachment 289962 [details]
Patch, v3

This should address the comments on the previous patch.

[Michael Catanzaro]

Comment on attachment 289962 [details]
Patch, v3

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

r=me on the condition that Frederic says it's good.

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:100
> +    bool Write(const void *data, size_t n)

void* data, not void *data

Is this a virtual method? If so you need to mark it override. If not, it should be named write with a lowercase W, since WebKit methods are camelCase.

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:108
> +    bool Write(const void *data, size_t offset, size_t n)

Ditto

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:125
> +    Vector<char>& m_vector;

Optional: I think it would be safer to keep a plain Vector<char> here instead of a reference, and sink it to WOFF2VectorOut by changing the constructor to take a Vector<char>&& instead of a Vector<char>. Then you will need to use WTFMove to pass the vector to the constructor. The advantage of doing it that way is the WOFF2VectorOut becomes harder to misuse (more robust to future modification); currently it breaks badly if the passed in vector goes out of scope before the WOFF2VectorOut does. But this is optional; the current way is OK too.

[Frédéric Wang (:fredw)]

Comment on attachment 289962 [details]
Patch, v3

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

Yes, this looks good to me with Michael's comments addressed.

>> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:100
>> +    bool Write(const void *data, size_t n)
> 
> void* data, not void *data
> 
> Is this a virtual method? If so you need to mark it override. If not, it should be named write with a lowercase W, since WebKit methods are camelCase.

Yes, they are pure virtual and should indeed be marked override.
https://github.com/google/woff2/blob/4e698b8c6c5e070d53c340db9ddf160e21070ede/src/woff2_out.h#L59

[Frédéric Wang (:fredw)]

(In reply to comment #8)
> Comment on attachment 289962 [details]
> Patch, v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289962&action=review
> 
> Yes, this looks good to me with Michael's comments addressed.

We should also import

http://test.csswg.org/harness/test/woff2_dev/single/header-totalsfntsize-001/

as the patch makes this test passes.

[Khaled Hosny]

Comment on attachment 289962 [details]
Patch, v3

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

>> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:125
>> +    Vector<char>& m_vector;
> 
> Optional: I think it would be safer to keep a plain Vector<char> here instead of a reference, and sink it to WOFF2VectorOut by changing the constructor to take a Vector<char>&& instead of a Vector<char>. Then you will need to use WTFMove to pass the vector to the constructor. The advantage of doing it that way is the WOFF2VectorOut becomes harder to misuse (more robust to future modification); currently it breaks badly if the passed in vector goes out of scope before the WOFF2VectorOut does. But this is optional; the current way is OK too.

I tried doing that, but then nothing was written to the `sfnt` vector, not sure if I’m doing something wrong:
diff --git a/Source/WebCore/platform/graphics/WOFFFileFormat.cpp b/Source/WebCore/platform/graphics/WOFFFileFormat.cpp
index c2323cf..60eb7df 100644
--- a/Source/WebCore/platform/graphics/WOFFFileFormat.cpp
+++ b/Source/WebCore/platform/graphics/WOFFFileFormat.cpp
@@ -93,8 +93,8 @@ bool isWOFF(SharedBuffer& buffer)
 #if USE(WOFF2)
 class WOFF2VectorOut : public woff2::WOFF2Out {
 public:
-    WOFF2VectorOut(Vector<char>& vector)
-        : m_vector(vector)
+    WOFF2VectorOut(Vector<char>&& vector)
+        : m_vector(WTFMove(vector))
     { }
 
     bool Write(const void* data, size_t n) override
@@ -122,7 +122,7 @@ public:
     }
 
 private:
-    Vector<char>& m_vector;
+    Vector<char> m_vector;
 };
 #endif
 
@@ -148,7 +148,7 @@ bool convertWOFFToSfnt(SharedBuffer& woff, Vector<char>& sfnt)
         if (!sfnt.tryReserveCapacity(sfntSize))
             return false;
 
-        WOFF2VectorOut out(sfnt);
+        WOFF2VectorOut out(WTFMove(sfnt));
         return woff2::ConvertWOFF2ToTTF(woffData, woffSize, &out);
     }
 #endif

[Khaled Hosny]

Created attachment 290070 [details]
Patch, v4

Updated the patch to address the style comments and add a new test based on the one in WOFF2 test suite.

[Michael Catanzaro]

(In reply to comment #10)
> I tried doing that, but then nothing was written to the `sfnt` vector, not
> sure if I’m doing something wrong:

No, I simply missed that sfnt was a parameter passed by reference and needs to be kept valid, so my suggestion was wrong.

[Khaled Hosny]

Created attachment 290072 [details]
Patch, v5

[WebKit Commit Bot]

Comment on attachment 290072 [details]
Patch, v5

Rejecting attachment 290072 [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', 290072, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2161227

[Frédéric Wang (:fredw)]

Committed r206511: <http://trac.webkit.org/changeset/206511>