Bug 152616 – [GTK] Need support for WOFF2

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

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

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.

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

Created attachment 270412 [details]
Experimental patch

[Martin Robinson]

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.

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

At the moment ots is available on github and uses woff2 a

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

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.

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

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).

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

Created attachment 272562 [details]
Patch

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

(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

[Michael Catanzaro]

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

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

Created attachment 272640 [details]
Patch

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

(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.

[Carlos Garcia Campos]

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

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

(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.

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

Created attachment 273004 [details]
Patch

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

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.

[Michael Catanzaro]

Why didn't it work for brotli? I agree with Carlos, these should be linked statically to WebCore.

[Michael Catanzaro]

(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?

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

(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)

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

(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.

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

Created attachment 273076 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 273076 [details]
Patch

Looks great, thanks!

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

Committed r197933: <http://trac.webkit.org/changeset/197933>