Closed Bug 1357952 Opened 3 years ago Closed 3 years ago

UnscaledFontFreeType does not properly serialize system fonts with WebRender

Categories

(Core :: Graphics: WebRender, defect, P3)

55 Branch
All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files)

As a result of bug 1355931, UnscaledFontFreeType was temporarily regressed with respect to serializing fonts for WebRender. It holds either an FT_Face for webfonts, or a filename/index for system fonts, and was only previously reading fonts by querying tables from the face.

As part of fixing this bug, I made a slight compat-breaking change in an upstream WebRender pull request: https://github.com/servo/webrender/commit/6b8000def1e11fe61a438ab1a71f4f102e1b89fd
The pull request basically just added support for supplying an index along with the add_raw_font API call, so that if we're giving FreeType a font collection instead of a single font, it can properly index said collection. The compat issue was noted in bug 1357392 comment 5 (https://bugzilla.mozilla.org/show_bug.cgi?id=1357392#c5).
This handles the proper routing of the font index through WebRenderAPI. This is also dependent on updating our in-tree WebRender to drag in the requisite upstream pull request: https://github.com/servo/webrender/commit/6b8000def1e11fe61a438ab1a71f4f102e1b89fd

Kats or Jeff, how to properly stage this with the necessary tree webrender update?
Flags: needinfo?(bugmail)
Attachment #8859797 - Flags: review?(jmuizelaar)
(In reply to Lee Salzman [:lsalzman] from comment #2)
> Created attachment 8859798 [details] [diff] [review]
> fix UnscaledFontFreeType::GetFontFileData to read file contents when
> filename is used instead of an FT_Face

Should be noted that I chose to use mmap here as we were already using it in places in thebes in the FT2 font code. I discovered during debugging that font files were usually a couple hundred KB in size up to a MB, so mmaping here to get rid of extra expensive allocations and reads made sense.

Ultimately we may want to work around the clunky GetFontFileData interface entirely so that we oculd supply mmap'd fonts directly to ipdl rather than also copying them into a ByteBuffer downstream. But by then, we may also want to just consider shmem-based font sharing to get rid of the need to ship file contents through pipes entirely. Anyway, wishful thinking for a future time.

For now, this patch should now just bring Linux up to parity with the other platforms that currently fling everything through AddRawFont.
Keywords: regression
Version: 51 Branch → 55 Branch
Attachment #8859797 - Flags: review?(jmuizelaar) → review+
Attachment #8859798 - Flags: review?(jmuizelaar) → review+
(In reply to Lee Salzman [:lsalzman] from comment #1)
> Kats or Jeff, how to properly stage this with the necessary tree webrender
> update?

Based on the try pushes in bug 1357392 it seems like I can land the webrender update first (with the index set to 0) and it shouldn't break anything, and then you can land this afterwards whenever you like. I'll try to get the WR update landed today if I can. Tomorrow by latest.
Flags: needinfo?(bugmail)
Or in case you're wondering how to test these patches locally, you'll want to apply the patches from one of the try pushes in bug 1357392 locally and put these patches on top. If you want I can add these patches to a try push pretty trivially as well. Or you could just wait until the WR update lands and then test as you usually do.
I landed the WR update, so feel free to land these patches.
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/bec7523b36c6
update WebRenderAPI to allow index in add_raw_font. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/44890ebd9c5b
fix UnscaledFontFreeType::GetFontFileData to read file contents when filename is used instead of an FT_Face. r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.