Bug 152616

Summary: [GTK] Need support for WOFF2
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, fred.wang, kling, mcatanzaro, mmaxfield
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 154859, 154956    
Attachments:
Description Flags
Experimental patch
none
Experimental patch
none
Alternative patch with brotli & woff2 in Source/ThirdParty
none
Patch
none
Patch
none
Patch
none
Patch cgarcia: review+

Description Martin Robinson 2015-12-31 09:49:43 PST
The GTK+ port does not have support for WOFF2 fonts. The easiest way to add support seems to be a re-add Chromium's support for the OpenType Santiser and transcode OpenType fonts.
Comment 1 Frédéric Wang (:fredw) 2016-02-01 12:44:09 PST
Created attachment 270411 [details]
Experimental patch

This is an attempt to use OTS. The library is correctly compiled with jhbuild and found by cmake. However during the link phase, I got error about unsafe memmove. I suspect this is related to https://github.com/google/brotli/issues/307 and https://github.com/google/brotli/issues/308 (OTS's submodules point to old commits of brotli/woff2 ).

Alternatively, we could also directly use https://github.com/google/woff2. But this library does not have a release (https://github.com/google/woff2/issues/40) nor way to build and install a library.
Comment 2 Frédéric Wang (:fredw) 2016-02-01 12:48:01 PST
Created attachment 270412 [details]
Experimental patch
Comment 3 Martin Robinson 2016-02-01 12:48:33 PST
Is the open type santiser available in as a package in distributions? If not, it probably needs to be imported directly into the WebKit source tree.
Comment 4 Frédéric Wang (:fredw) 2016-02-01 12:52:16 PST
At the moment ots is available on github and uses woff2 a
Comment 5 Frédéric Wang (:fredw) 2016-02-01 12:54:50 PST
At the moment ots is available on github and uses woff2 and brotli as submodules. None of the three libraries really seem to be ready for packaging in distributions. So maybe we should do like Mozilla and just put the code in the source tree. I think for this bug we can only use woff2 and brotli decoders, though.
Comment 6 Frédéric Wang (:fredw) 2016-02-26 06:35:44 PST
Created attachment 272323 [details]
Alternative patch with brotli & woff2 in Source/ThirdParty

Here is an alternative patch that puts brotli/woff2 into the WebKit tree. That seems to work after a quick test with my WOFF2 math fonts, but other woff 2 tests such as https://hail2u.net/pub/test/534.html fail (for some reason isWOFF2 and a fortiori convertWOFF2ToSfnt is never called for these tests).
Comment 7 Frédéric Wang (:fredw) 2016-03-01 06:51:11 PST
Created attachment 272562 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2016-03-01 07:01:41 PST
(In reply to comment #6)
> Created attachment 272323 [details]
> Alternative patch with brotli & woff2 in Source/ThirdParty
> 
> Here is an alternative patch that puts brotli/woff2 into the WebKit tree.
> That seems to work after a quick test with my WOFF2 math fonts, but other
> woff 2 tests such as https://hail2u.net/pub/test/534.html fail (for some
> reason isWOFF2 and a fortiori convertWOFF2ToSfnt is never called for these
> tests).

OK, I had forgotten adding "woff2" in FontCustomPlatformData::supportsFormat. But https://hail2u.net/pub/test/534.html seems to have bad web font signature anyway. I checked that it works with the LayoutTests and with http://codepen.io/yisi/pen/dGzeKJ/?js-preprocessor=none
Comment 9 Michael Catanzaro 2016-03-01 08:56:13 PST
Comment on attachment 272562 [details]
Patch

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

> Source/CMakeLists.txt:23
> +add_subdirectory(ThirdParty/brotli)

Not sure why the style checker isn't complaining about this, but you need to indent here
Comment 10 Frédéric Wang (:fredw) 2016-03-01 23:54:21 PST
Created attachment 272640 [details]
Patch
Comment 11 Frédéric Wang (:fredw) 2016-03-01 23:57:31 PST
(In reply to comment #9)
> > Source/CMakeLists.txt:23
> > +add_subdirectory(ThirdParty/brotli)
> 
> Not sure why the style checker isn't complaining about this, but you need to
> indent here

Thanks, I've fixed that and changed my email address in the ChangeLogs at the same time.
Comment 12 Carlos Garcia Campos 2016-03-03 00:45:42 PST
Comment on attachment 272640 [details]
Patch

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

Thanks for the patch! I have only a few comments/questions. I guess third party licenses are compatible.

> Source/CMakeLists.txt:22
> +if (ENABLE_WOFF2)

Why do we need an option for this if it's always available? Does this actually depend on every port more than whether this is available or not? Because in that case I think it's more appropriate to use USE() instead of ENABLE()

> Source/ThirdParty/brotli/CMakeLists.txt:17
> +add_library(brotli SHARED ${BROTLI_SOURCES})

This is a private lib used only by WebCore, right? Can we make it static instead then?

> Source/ThirdParty/woff2/CMakeLists.txt:16
> +add_library(woff2 SHARED ${WOFF2_SOURCES})

Same here, could this be static?

> Source/WebCore/CMakeLists.txt:3405
> +  list(APPEND WebCore_INCLUDE_DIRECTORIES
> +    "${THIRDPARTY_DIR}/woff2/src"
> +  )
> +  list(APPEND WebCore_LIBRARIES
> +    brotli
> +    woff2
> +  )
> +endif ()

Does WebCore actually use brotli? or is it a dependency of woff2? If that is the case maybe we don't need to explicitly link to brotli here.

> Source/WebCore/loader/cache/CachedFont.cpp:119
> +#if ENABLE(WOFF2)
> +        } else if (isWOFF2(buffer.get())) {
> +            Vector<char> convertedFont;
> +            if (!convertWOFF2ToSfnt(buffer.get(), convertedFont))
> +                buffer = nullptr;
> +            else
> +                buffer = SharedBuffer::adoptVector(convertedFont);
> +#endif

Instead of duplicating this, I wonder if we could make this automatically in existing isWOFF and convertWOFFToSfnt
Comment 13 Frédéric Wang (:fredw) 2016-03-03 00:59:06 PST
(In reply to comment #12)

Thanks for the review. I'll come back later with a new patch. Comments inline.

> I have only a few comments/questions. I guess third party licenses are compatible.

It's MIT license so I think so.

> 
> > Source/CMakeLists.txt:22
> > +if (ENABLE_WOFF2)
> 
> Why do we need an option for this if it's always available? Does this
> actually depend on every port more than whether this is available or not?
> Because in that case I think it's more appropriate to use USE() instead of
> ENABLE()

Yes, it depends on whether the port will use it or not (e.g. Apple bundles the librareis in its ElCapitan OS IIUC), I don't think we want to disable WOFF2 support. I'll try using USE()

> 
> > Source/ThirdParty/brotli/CMakeLists.txt:17
> > +add_library(brotli SHARED ${BROTLI_SOURCES})
> 
> This is a private lib used only by WebCore, right? Can we make it static
> instead then?
> > Source/ThirdParty/woff2/CMakeLists.txt:16
> > +add_library(woff2 SHARED ${WOFF2_SOURCES})

Yes they are private lib. I'll try making them static.

> Does WebCore actually use brotli? or is it a dependency of woff2? If that is
> the case maybe we don't need to explicitly link to brotli here.

Not at the moment. Although that might be needed for bug 154859 in the future.

> Instead of duplicating this, I wonder if we could make this automatically in
> existing isWOFF and convertWOFFToSfnt

OK, I'll check that too.
Comment 14 Frédéric Wang (:fredw) 2016-03-04 09:26:50 PST
Created attachment 273004 [details]
Patch
Comment 15 Frédéric Wang (:fredw) 2016-03-04 09:27:54 PST
I think I fixed almost all review comments. Switching to brotli/woff2 static cause build errors asking to recompile with option -fPIC. That worked for woff2 but not for brotli.
Comment 16 Michael Catanzaro 2016-03-04 10:53:49 PST
Why didn't it work for brotli? I agree with Carlos, these should be linked statically to WebCore.
Comment 17 Michael Catanzaro 2016-03-04 14:39:56 PST
(In reply to comment #16)
> Why didn't it work for brotli?

Aha, by chance I was looking at WEBKIT_SET_EXTRA_COMPILER_FLAGS(), which is where we set -fPIC for static libraries... I bet you forgot to use that?
Comment 18 Frédéric Wang (:fredw) 2016-03-05 01:20:25 PST
(In reply to comment #16)
> Why didn't it work for brotli? I agree with Carlos, these should be linked
> statically to WebCore.

Sorry was lazy giving details at the end of the work day... Here are they:

Changing woff2 to static gives an error like

lib/libwoff2.a(lib/../Source/ThirdParty/woff2/CMakeFiles/woff2.dir/src/variable_length.cc.o) : requires dynamic realloc R_X86_64_PC32 against « _Znwm » which may overflow at runtime; recompile with -fPIC

Adding set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC") to the woff2 cmake file fixes the problem.

Then changing brotli to static gives an error like

lib/libbrotli.a(lib/../Source/ThirdParty/brotli/CMakeFiles/brotli.dir/dec/state.c.o) : requires dynamic realloc 11 not supported; recompile with -fPIC

Adding -fPIC to the brotli cmake file does not help.

(I tried translating the errors from French)
Comment 19 Frédéric Wang (:fredw) 2016-03-05 01:20:40 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Why didn't it work for brotli?
> 
> Aha, by chance I was looking at WEBKIT_SET_EXTRA_COMPILER_FLAGS(), which is
> where we set -fPIC for static libraries... I bet you forgot to use that?

Indeed, this works. Thanks, I'll upload a new patch.
Comment 20 Frédéric Wang (:fredw) 2016-03-05 01:23:14 PST
Created attachment 273076 [details]
Patch
Comment 21 Carlos Garcia Campos 2016-03-09 23:48:20 PST
Comment on attachment 273076 [details]
Patch

Looks great, thanks!
Comment 22 Frédéric Wang (:fredw) 2016-03-10 07:30:39 PST
Committed r197933: <http://trac.webkit.org/changeset/197933>