Bug 283246 – [GTK][WPE] Regression: ot-svg font support lost in 2.46 with Skia

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

I saw the news about moving to skia in 2.46, so I fired up my mini py webkitgtk web browser, and visit:
https://pixelambacht.nl/chromacheck/
And found that ot-svg font support is indeed broken.

Downgrading to 2.44.3 (on fedora via the package manager), and visit the above page, ot-svg support is back.

There is a simple patch for qtwebengine to gain ot-svg support:
https://github.com/HinTak/Qt6WE-OT-SVG

Have a look - give me a few days, I can probably patch webkitgtk to regain the functionality again.

Safari webkit supports ot-svg, and so does Firefox.

I had a quick look at the duff of 2.44 and 2.46, and I can't quite figure out how it was done in 2.44 . The quickest way of restoring the functionality is the one-line code change - https://github.com/kyamagu/skia-python/commit/e5f2e6361d0e6a40e5077fa46897a556f793a18d - but you need to include skia's svg and skresource module. I think you might prefer to use webkit's own rendering/svg instead, which seems to offer similar functionality.

Would like a simple overview of how ot-svg font works in 2.44 or before though.

I see safari / coretext must work differently from freetype-based system (ie. Linux) in terms of ot-svg fonts.

The explanation of what the code does is at 
https://github.com/HinTak/chromium-mod-CI - since you don't use opentype sanitizer, you don't need half of part 1, and you used to be able to do ot-svg, it is unlikely you need patch 2. So mostly you should only need the one-line code addition as in skia-python.

Created attachment 473332 [details]
Patch 1 of 7

Created attachment 473333 [details]
Patch 2 of 7

Created attachment 473334 [details]
Patch 3 of 7

Created attachment 473335 [details]
Patch 4 of 7

Created attachment 473336 [details]
Patch 5 of 7

Created attachment 473337 [details]
Patch 6 of 7

Created attachment 473338 [details]
Patch 7 of 7

Here is a description of the patch series:

The main change is a one-line code addition in patch 3, hooking up the skia's svg module with the font loading code.

But before that, you need to bundle the svg module (which depends on the skunicode module, skshaper, skresource module), which are absent from the release the ball, but present in the repo. And adding them to the library file list in the cmake file. So that's patch 1 and 2 do.

Patch 4 to 7 are really about how wekbitgtk builds skia differently from how upstream does it, determining dependency and what other files you did not build are needed, etc. Patch 7 is a slight anomaly: I found you build the whole of wwbkitgtk as c++23, but upstream recommends c++17, and one particular file in skia does not build as c++23, but will build with c++17. This is documented elsewhere as how the behavior of unique_ptr has changed in c++23.

[Michael Catanzaro]

Hi, thanks for your contributions. I recommend opening one or more pull requests on GitHub where it's more likely to be reviewed. There is a restriction of only one commit per pull request, which unfortunately encourages grouping changes together into one commit instead of splitting them out into a nice series of commits. There is some guidance here: https://webkit.org/contributing-code/

You'll need to base your changes on the git repo, not the release tarball. To include more files in the tarballs, modify Tools/gtk/manifest.txt.in and Tools/wpe/manifest.txt.in.

I set up a repo https://github.com/HinTak/webkitgtk-mod-CI to rebuild the fedora rpm with that 7 patches, and tested then myself.

You mostly just need to visit either of these two urls (the bottom half of the latter) to see the difference:
https://pixelambacht.nl/chromacheck/
https://yoksel.github.io/color-fonts-demo/

Three more comments:

- I compare the size of libwebkitgtk before and after, on Linux. The patch series adds about 300k to the 75MB library. The 300k is essentially the size of skia's svg module as a separate shared library. This is small/negligible.

- I am just porting over previous work on skia-python (I am a co-maintainer) which has a complete copy of skia plus a few extra / optional modules. You seem to want to strip down skia a bit; I am just saying that my approach works in that context, but you might choose to do things differently. In particular, I probably add back more than strictly necessary. And putting the one line change where it is in patch 3 may not be optimal - it just needs to be run once somewhere near the start, and right after initing the font manager is an obvious place to do it. You may think of a better location. Also, webkitgtk has its own svg module, so this is somewhat duplicating functionality, although the actual size is small.

- I like to know how 2.44 or before with cairo works with ot-svg. I haven't figured out how it is done yet.

Oh, a 4th comment: the patch series are tested on desktop Linux. I see patch 6 or 5 makes EXPAT compulsory for linux too. Before, it was only a requirement for android. So it needs a little adjustment to not mess with android build, which now sees it as required twice. Apologies as I don't have android test build facilities (nor care enough about webkitgtk on android).

(In reply to Michael Catanzaro from comment #12)
> Hi, thanks for your contributions. I recommend opening one or more pull
> requests on GitHub where it's more likely to be reviewed. There is a
> restriction of only one commit per pull request, which unfortunately
> encourages grouping changes together into one commit instead of splitting
> them out into a nice series of commits. There is some guidance here:
> https://webkit.org/contributing-code/
> 
> You'll need to base your changes on the git repo, not the release tarball.
> To include more files in the tarballs, modify Tools/gtk/manifest.txt.in and
> Tools/wpe/manifest.txt.in.

I did not need to add my own files - they are just files present in the repo but not in the release tar ball.

Thanks for the advice - that was my first thought, except even just downloading a snapshot tar ball is 1.6GB (compared to the release tar ball at 40MB), so it is going to be a while before I have the disk space to do a clone to create a pull :-(.

It is also a bit weird that the issue tracker is here and the pull is over there....

This is my quickest way of restoring the lost functionality, based on my  earlier familiarity with skia and skia-python. It is quite likely the webkitgtk people like to do it differently - use the existing webkit svg code, rather than adding the depending on the optional skia's svg module.

I am okay if one of you want to collapse the 7 patches and submit that on my behalf. They were done by "git init" an expanded the ball, then work on it, then "git format-patch ..." so they have the correct e-mail contact and signed-off lines.

Obviously, patch #1 (restoring 4 missing entire directories in repo but not in release tar ball) needs to be done differently, and so is patch #6 (expat was an optional requirement for android only, now applies to desktop Linux, too) need some adjustment to not break android build, due to specifying expat twice.

I am unlikely to do the pull ever myself (simply don't have the disk space to clone...), but it is probably acceptable for one of the regular contributors to fix patch #1 & #6, and adding my signed-off. Even to build the patched RPMs, I do it via github CI.

[Adrian Perez]

@htl10: I will take them and prepare them into a pull request that could be merged, adding a build option for the feature, and making sure release tarballs will have the needed files included. We will credit you in the commit message, thanks a lot for the patch set!

(In reply to Adrian Perez from comment #16)
> @htl10: I will take them and prepare them into a pull request that could be
> merged, adding a build option for the feature, and making sure release
> tarballs will have the needed files included. We will credit you in the
> commit message, thanks a lot for the patch set!

Thanks. Is that option going to be default on or off? Or just depending on availability of expat?

Btw, I'd like some pointer (e.g. which files are involved, etc) how 2.44 or before with cairo works with ot-svg. I haven't figured out how it is done yet. In the longer term, it might be a good idea to try to use webkit's older svg code instead of the newer and optional skia svg module, or offer that as an option to compare the svg rendering of the two.

I have setup 
https://github.com/HinTak/webkitgtk-mod-CI to rebuild both the fedora gtk3 and gtk4 rpms. I have to do it in parallel lots of 3:40 hours, as gitub has a 6 hour limit.

[Adrian Perez]

(In reply to htl10 from comment #17)
> (In reply to Adrian Perez from comment #16)
> > @htl10: I will take them and prepare them into a pull request that could be
> > merged, adding a build option for the feature, and making sure release
> > tarballs will have the needed files included. We will credit you in the
> > commit message, thanks a lot for the patch set!
> 
> Thanks. Is that option going to be default on or off? Or just depending on
> availability of expat?

The CMake build option is going to be enabled by default, because we
expect most distributions to have an Expat package.

Nevertheless, our policy is having a build build option for new
dependencies. This makes it easier for packagers to update to newer
versions without changes to their base system -- which is important
to ensure people can get security updates. Also, in the case of WPE
people may want to make minimal builds to reduce space and memory
usage on embedded devices.

Thanks for the explanation. For longer term, and also the minimal-size builds, it might be useful to have an option to use the existing webkit svg code (which the cairo build was probably using?) vs skia's svg module. I probably will have a browse through the webkit mailing list / git history to see how it was done.

[Adrian Perez]

Pull request: https://github.com/WebKit/WebKit/pull/37847

[Adrian Perez]

(In reply to htl10 from comment #20)
> Thanks for the explanation. For longer term, and also the minimal-size
> builds, it might be useful to have an option to use the existing webkit svg
> code (which the cairo build was probably using?) vs skia's svg module. I
> probably will have a browse through the webkit mailing list / git history to
> see how it was done.

Carlos Garcia tried reusing WebKit's SVG renderer, but it turned out to be problematic: rendering SVGs inside fonts happens while holding font-related locks, and rendering SVG with WebKit will itself try to enumerate and get information about fonts, which in turn will attempy to acquire locks... resulting in deadlocks. I also added a note about this in the commit log, which you can see in the pull request I uploaded some moments ago :) 

Before using Skia, it also happened that the rendering of SVGs embedded in fonts happened behind the scenes (using Cairo + Pango), outside of WebKit's SVG rendering. In that regard it's not different to let Skia's SVG module do the job.

[EWS]

Committed 287859@main (7bf0c3997b61): <https://commits.webkit.org/287859@main>

Reviewed commits have been landed. Closing PR #37847 and removing active labels.