Bug 162608

Summary: [GTK] Update WOFF2 decoder
Product: WebKit Reporter: Khaled Hosny <dr.khaled.hosny>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, commit-queue, fred.wang, mcatanzaro
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1305944
https://bugs.webkit.org/show_bug.cgi?id=162678
Bug Depends on:    
Bug Blocks: 162668    
Attachments:
Description Flags
Patch
none
Patch, v2
none
Patch, v3
mcatanzaro: review+
Patch, v4
mcatanzaro: review+
Patch, v5 commit-queue: commit-queue-

Description Khaled Hosny 2016-09-27 06:08:39 PDT
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.
Comment 1 Khaled Hosny 2016-09-27 08:08:50 PDT
Created attachment 289942 [details]
Patch
Comment 2 WebKit Commit Bot 2016-09-27 08:11:26 PDT
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.
Comment 3 Khaled Hosny 2016-09-27 08:35:54 PDT
Created attachment 289945 [details]
Patch, v2
Comment 4 Frédéric Wang (:fredw) 2016-09-27 08:45:37 PDT
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()
Comment 5 Khaled Hosny 2016-09-27 10:01:07 PDT
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.
Comment 6 Khaled Hosny 2016-09-27 10:02:09 PDT
Created attachment 289962 [details]
Patch, v3

This should address the comments on the previous patch.
Comment 7 Michael Catanzaro 2016-09-28 01:09:32 PDT
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.
Comment 8 Frédéric Wang (:fredw) 2016-09-28 01:34:23 PDT
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
Comment 9 Frédéric Wang (:fredw) 2016-09-28 02:10:34 PDT
(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.
Comment 10 Khaled Hosny 2016-09-28 02:44:05 PDT
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
Comment 11 Khaled Hosny 2016-09-28 02:46:04 PDT
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.
Comment 12 Michael Catanzaro 2016-09-28 03:15:02 PDT
(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.
Comment 13 Khaled Hosny 2016-09-28 03:37:37 PDT
Created attachment 290072 [details]
Patch, v5
Comment 14 WebKit Commit Bot 2016-09-28 05:32:15 PDT
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
Comment 15 Frédéric Wang (:fredw) 2016-09-28 05:48:46 PDT
Committed r206511: <http://trac.webkit.org/changeset/206511>