Summary: | [GTK] Need support for WOFF2 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||||||||
Component: | WebKitGTK | Assignee: | 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
Martin Robinson
2015-12-31 09:49:43 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. Created attachment 270412 [details]
Experimental patch
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. At the moment ots is available on github and uses woff2 a 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. 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). Created attachment 272562 [details]
Patch
(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 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 Created attachment 272640 [details]
Patch
(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 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 (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. Created attachment 273004 [details]
Patch
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. Why didn't it work for brotli? I agree with Carlos, these should be linked statically to WebCore. (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? (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) (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. Created attachment 273076 [details]
Patch
Comment on attachment 273076 [details]
Patch
Looks great, thanks!
Committed r197933: <http://trac.webkit.org/changeset/197933> |