Remove non-fontconfig platform fontlist codepath

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: emk, Assigned: jrmuizel)

Tracking

Trunk
mozilla55
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox55 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

No description provided.
Whiteboard: [gfx-noted]
Assignee: nobody → jmuizelaar
Attachment #8849736 - Flags: review?(jfkthame)
Attachment #8849736 - Flags: review?(lsalzman)
Attachment #8849736 - Flags: review?(lsalzman) → review+
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa327aeb6572
Remove non-fontconfig platform fontlist codepath. r=lsalzman
This will break the about:config workaround that some Linux users are currently using in order to keep bitmap fonts working; e.g. see bug 1180383, bug 1283802, bug 1243194 and similar. I hope we're prepared to field the resulting support questions / bug reports.
https://hg.mozilla.org/mozilla-central/rev/aa327aeb6572
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> This will break the about:config workaround that some Linux users are
> currently using in order to keep bitmap fonts working; e.g. see bug 1180383,
> bug 1283802, bug 1243194 and similar. I hope we're prepared to field the
> resulting support questions / bug reports.

Is this merely a case of fontconfig somehow being broken with the embeddedbitmap option, or does the selection of available fonts itself somehow not include these fonts?
Flags: needinfo?(jfkthame)
I'm not referring to embedded bitmaps within OpenType fonts (which I assume is what embeddedbitmap is about), but legacy bitmap-only fonts. These are explicitly excluded by the code at https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/gfx/thebes/gfxFcPlatformFontList.cpp#1009-1014 (and I presume that if we didn't exclude them from the font list here, they wouldn't actually work properly because they're not supported all the way through skia etc; see also bug 1056479 comment 13).

Some users have specific bitmap fonts they really want to keep using, and for the time being, have been setting gfx.font_rendering.fontconfig.fontlist.enabled to false to allow them to continue working.
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> I'm not referring to embedded bitmaps within OpenType fonts (which I assume
> is what embeddedbitmap is about), but legacy bitmap-only fonts. These are
> explicitly excluded by the code at
> https://dxr.mozilla.org/mozilla-central/rev/
> e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/gfx/thebes/gfxFcPlatformFontList.
> cpp#1009-1014 (and I presume that if we didn't exclude them from the font
> list here, they wouldn't actually work properly because they're not
> supported all the way through skia etc; see also bug 1056479 comment 13).
> 
> Some users have specific bitmap fonts they really want to keep using, and
> for the time being, have been setting
> gfx.font_rendering.fontconfig.fontlist.enabled to false to allow them to
> continue working.

It's a far bigger pain on our side to keep the old legacy font list code just for this use case, as opposed to doing the "Right Thing (tm)", which would be to make the new font list code support this use case in some form. I don't know why in principle our Skia font host shouldn't support these (famous last words), as much has changed since a couple years ago. So let me do some investigation as to whether/how to make this work within our new font list code. If it turns out there is no way to make the new font list code support it, and we deem it a serious enough issue, we can always do a backout.

I think it is good that forcing this change is bringing this issue to light, otherwise it is highly likely it would have continued to be swept under the rug indefinitely. Keeping the old code around only for this use case is very not ideal.
(In reply to Lee Salzman [:lsalzman] from comment #7)
> It's a far bigger pain on our side to keep the old legacy font list code
> just for this use case, as opposed to doing the "Right Thing (tm)", which
> would be to make the new font list code support this use case in some form.
> I don't know why in principle our Skia font host shouldn't support these
> (famous last words), as much has changed since a couple years ago. So let me
> do some investigation as to whether/how to make this work within our new
> font list code. If it turns out there is no way to make the new font list
> code support it, and we deem it a serious enough issue, we can always do a
> backout.

I doubt we'd consider it that serious. We should either implement support in the new code, or make it explicit that we've discontinued support for such fonts.

> I think it is good that forcing this change is bringing this issue to light,
> otherwise it is highly likely it would have continued to be swept under the
> rug indefinitely. Keeping the old code around only for this use case is very
> not ideal.

Yes, definitely agreed. I don't know how much work it would take to support this in the new code; obviously, at the time John didn't think it was worth doing, but I'm not sure how deeply he looked into it. If it's going to be a lot of effort, it may well not be justified, but it's still a use case that at least a few people care about, so permanently disabling it should be consciously evaluated rather than just a side-effect of cleanup.
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> (In reply to Lee Salzman [:lsalzman] from comment #7)
> > It's a far bigger pain on our side to keep the old legacy font list code
> > just for this use case, as opposed to doing the "Right Thing (tm)", which
> > would be to make the new font list code support this use case in some form.
> > I don't know why in principle our Skia font host shouldn't support these
> > (famous last words), as much has changed since a couple years ago. So let me
> > do some investigation as to whether/how to make this work within our new
> > font list code. If it turns out there is no way to make the new font list
> > code support it, and we deem it a serious enough issue, we can always do a
> > backout.
> 
> I doubt we'd consider it that serious. We should either implement support in
> the new code, or make it explicit that we've discontinued support for such
> fonts.
> 
> > I think it is good that forcing this change is bringing this issue to light,
> > otherwise it is highly likely it would have continued to be swept under the
> > rug indefinitely. Keeping the old code around only for this use case is very
> > not ideal.
> 
> Yes, definitely agreed. I don't know how much work it would take to support
> this in the new code; obviously, at the time John didn't think it was worth
> doing, but I'm not sure how deeply he looked into it. If it's going to be a
> lot of effort, it may well not be justified, but it's still a use case that
> at least a few people care about, so permanently disabling it should be
> consciously evaluated rather than just a side-effect of cleanup.

So after removing the FC_SCALABLE junk in gfxFcPlatformFontList, installing xfonts-terminus, and overriding font settings to use the Terminus font family, things just worked. Skia (SkFontHost_cairo) ate the font just fine, no indigestion.

Am I missing something here, or can we just removing the scalable restriction and be done with it?
Flags: needinfo?(jfkthame)
I'm not sure. How does it cope if you try using the font at various sizes? How about if you use it with effects like text-shadow or CSS transforms, or within SVG content?
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #10)
> I'm not sure. How does it cope if you try using the font at various sizes?
> How about if you use it with effects like text-shadow or CSS transforms, or
> within SVG content?

When the scale doesn't exactly match the size, it chooses the available size that is closest, and will scale it to match. While this isn't necessarily the prettiest, it works.
Hmm. IIUC the behavior required by CSS Fonts 3 is to select a font based on the requested size, to within some UA-determined tolerance, and then use that font at its proper (unscaled) size. And if the family doesn't have a face that's near enough to the requested size, then it fails the font matching algorithm and we fall back to the next in the font-family list.

So with current Firefox, and using the old fontconfig backend, the Terminus font WFM at sizes from 11 to 39 device pixels (with the used size snapping to the nearest available face); outside that range it gets rejected and we fall back to the next (or default) font rather than scaling the bitmaps up (or down) into great ugly blocks.
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Hmm. IIUC the behavior required by CSS Fonts 3 is to select a font based on
> the requested size, to within some UA-determined tolerance, and then use
> that font at its proper (unscaled) size. And if the family doesn't have a
> face that's near enough to the requested size, then it fails the font
> matching algorithm and we fall back to the next in the font-family list.
> 
> So with current Firefox, and using the old fontconfig backend, the Terminus
> font WFM at sizes from 11 to 39 device pixels (with the used size snapping
> to the nearest available face); outside that range it gets rejected and we
> fall back to the next (or default) font rather than scaling the bitmaps up
> (or down) into great ugly blocks.

Isn't that pretty much be automatically controlled by Fontconfig? I don't do see anything special going on in either font list code to check that for bitmap fonts.
(In reply to Lee Salzman [:lsalzman] from comment #13)
> Isn't that pretty much be automatically controlled by Fontconfig? I don't do
> see anything special going on in either font list code to check that for
> bitmap fonts.

Possibly; but when you said "will scale it to match ... isn't necessarily the prettiest", I understood it to perhaps be scaling bitmaps in a way that the old code didn't do (AFAIK). I'll try to experiment a bit....
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> (In reply to Lee Salzman [:lsalzman] from comment #13)
> > Isn't that pretty much be automatically controlled by Fontconfig? I don't do
> > see anything special going on in either font list code to check that for
> > bitmap fonts.
> 
> Possibly; but when you said "will scale it to match ... isn't necessarily
> the prettiest", I understood it to perhaps be scaling bitmaps in a way that
> the old code didn't do (AFAIK). I'll try to experiment a bit....

Okay, looking at the old code, there is a SizeIsAcceptable check that does the 20% matching you described. Just glancing around, if gfxFontFamily::FindFontForStyle is made virtual, then overridden on gfxFontconfigFontFamily in the new code to filter the available font entries based on that same criteria, it miiiight more or less allow us to do the same?
From a (very) quick'n'dirty test comparing a hacked-to-allow-nonscalable Nightly to a Firefox 50 with pref set to use the old code, it looks like we're getting blurry "antialiased-like" rendering in Nightly, whereas FF50 rendered the bitmap font crisply with no blurring. :(  Also, adding font-style:italic (which should slant the font, assuming there's no actual italic face) doesn't work well. And I'm not seeing the right line-height metrics, though that may be something I broke when I hacked out the FC_SCALABLE stuff.

(If you think there's a chance of getting this to work reasonably, it'd probably be best to file a separate bug where we can consider a possible patch, rather than prolonged discussion in an already-closed bug.)
Blocks: 1350783
Attachment #8849736 - Flags: review?(jfkthame) → review+
Blocks: 1364089
You need to log in before you can comment on or make changes to this bug.