Thanks! Now that that change has two +1's, would anyone be able to push it?
Best regards, Dmitry Batrak On Tue, Jan 21, 2020 at 8:05 AM Sergey Bylokhov <sergey.bylok...@oracle.com> wrote: > Looks fine. > Thank you for contribution! > > On 1/20/20 12:14 am, Dmitry Batrak wrote: > > > Ok approved. Seems it is making a few things better if not ideal, but > nothing worse. > > > > Thanks! > > Anyone else volunteering to review? > > > > Best regards, > > Dmitry Batrak > > > > On Tue, Jan 14, 2020 at 8:58 PM Phil Race <philip.r...@oracle.com > <mailto:philip.r...@oracle.com>> wrote: > > > > Ok approved. Seems it is making a few things better if not ideal, > but nothing worse. > > > > -phil. > > > > On 1/14/20 8:14 AM, Dmitry Batrak wrote: > >> > So this is a workaround for a buggy font that doesn't play well > with GDI ? > >> > >> This is a workaround for all cases (or the vast majority of them) > of broken > >> rendering reported by our customers. The case with Roboto is just > the one we > >> have steps to reproduce for. There can be other cases where GDI's > logic is not > >> matched by JDK. Even if all of them are caused by 'mis-constructed' > fonts, I'm > >> afraid, this will not be considered as a good excuse by our > customers, as only > >> Java applications have such problems with these fonts. > >> > >> See JDK-8192972, still unsolved in OpenJDK, as an example of the > problems which > >> will be, at least partially, solved with this fix (correct glyphs > will be > >> rendered, albeit using FreeType). > >> > >> I did test the fix with fonts preinstalled in Windows 10. Fallback > was actually > >> triggered for one font (bold italic 'Segoe UI Semibold'), which is > not a 'false' > >> positive, but actually a manifestation of another JDK bug from the > same family - > >> I've just raised https://bugs.openjdk.java.net/browse/JDK-8237085 > for it. That's > >> yet another example of an issue which will be (mostly) solved by > the proposed > >> fix. > >> > >> Using file length as a 'checksum' certainly doesn't guarantee we > choose the > >> right font, but the probability of error is very low, and this > value seems to be > >> the best candidate in our circumstances in terms of cost vs. > benefit. Even if > >> the validation mistreats a different font (having the same length) > as a correct > >> one, we'll not be in a worse position than before. > >> > >> Of course, there's a certain risk that rendering for unaffected > fonts might > >> change, but, given quite straightforward contract of GetFontData > function, I > >> would consider it very low. > >> > >> > Since you aren't retrieving the data, just asking what the size > is, I'd expect > >> > it to be unmeasurable. > >> > >> Well, we don't know how GetFontData works exactly, but it does seem > to add some > >> overhead. On my Windows 10 machine OpenJDK with the proposed fix > yields about 7% > >> larger result for the following benchmark program. The reported > value does > >> fluctuate from run to run, but the impact of the fix seems to be > larger than the > >> fluctuations. > >> > >> --- Benchmark source code ---- > >> import java.awt.*; > >> import java.awt.font.GlyphVector; > >> import java.awt.image.BufferedImage; > >> > >> public class PerfTestOneFont { > >> private static final Font FONT = new Font("Segoe UI", > Font.PLAIN, 12); > >> > >> public static void main(String[] args) { > >> FONT.getFamily(); // preload font > >> > >> BufferedImage image = new BufferedImage(1, 1, > BufferedImage.TYPE_INT_RGB); > >> Graphics2D g = image.createGraphics(); > >> g.setRenderingHint(RenderingHints.KEY_TEXT_ANTIALIASING, > >> RenderingHints.VALUE_TEXT_ANTIALIAS_LCD_HRGB); > >> int glyphCount = FONT.getNumGlyphs(); > >> long startTime = System.currentTimeMillis(); > >> for (int glyphCode = 0; glyphCode < glyphCount; > glyphCode++) { > >> GlyphVector gv = > FONT.createGlyphVector(g.getFontRenderContext(), > >> new int[]{glyphCode}); > >> g.drawGlyphVector(gv, 0, 0); > >> } > >> long endTime = System.currentTimeMillis(); > >> g.dispose(); > >> System.out.println(endTime - startTime); > >> } > >> } > >> ------------------------------ > >> > >> Best regards, > >> Dmitry Batrak > >> > >> On Mon, Jan 13, 2020 at 11:09 PM Phil Race <philip.r...@oracle.com > <mailto:philip.r...@oracle.com>> wrote: > >> > >> So this is a workaround for a buggy font that doesn't play well > with GDI ? > >> > >> It does rely on the fonts always being different sizes which is > highly > >> likely if not guaranteed. > >> I suppose it is OK so long as we aren't getting any "false" > positives. > >> > >> What I mean is that almost no one will have these Roboto fonts > >> installed, so the fix > >> is solving a problem they don't have, but if it is wrong in > some way, > >> then they could lose > >> GDI rendering of LCD glyphs and that could affect a lot of > people. > >> > >> So have you tested this with the full set of Windows 10 fonts - > >> including Indic, CJK, etc - to be sure > >> there are no cases where it fails for these or other spurious > failures. > >> > >> > As for performance impact, during testing I didn't observe > average > >> glyph generation time increase of more than 15%. > >> > >> Since you aren't retrieving the data, just asking what the size > is, I'd > >> expect it to be unmeasurable. > >> > >> -phil. > >> > >> On 1/13/20 1:25 AM, Dmitry Batrak wrote: > >> > Hello, > >> > > >> > I'd like to submit a patch for JDK-8236996. I'm not a > Committer, so > >> > I'll need someone to sponsor this change. > >> > > >> > Issue: https://bugs.openjdk.java.net/browse/JDK-8236996 > >> > Webrev: > http://cr.openjdk.java.net/~dbatrak/8236996/webrev.00/ > >> > > >> > The problem described in JDK-8236996 is from a group of > issues (see > >> > also e.g. JDK-8078382 and JDK-8192972), where JDK > >> > uses one font to perform char-to-glyph conversion, but GDI, > when asked > >> > to render the glyph is picking a different font, > >> > leading to completely random glyphs being rendered, as > char-to-glyph > >> > mapping obviously differs for different fonts. > >> > > >> > Specific version of Roboto font, mentioned in JDK-8236996, is > most > >> > probably causing the issue because it's not following > >> > the naming guidelines from OpenType specification > >> > ( > https://docs.microsoft.com/en-us/typography/opentype/spec/name), > >> > having more than 4 variants (regular, bold, italic and bold > italic) > >> > with the same 'Font Family name' (name ID = 1). So, > >> > GDI gets confused and picks Roboto Black for rendering, when > asked to > >> > choose a regular font from Roboto family (Roboto > >> > Black having weight of 400, just like Roboto Regular, > probably adds to > >> > the confusion). > >> > > >> > But the reasoning, given above, about the issue cause is only > a guess. > >> > GDI is not an open-source subsystem, so we cannot > >> > know for sure how it selects the font for rendering, and > cannot > >> > implement matching logic in JDK. Ideally, we'd want to > >> > select the font by specifying its file path, but that's not > possible > >> > with GDI. Luckily, it allows us to query file data > >> > for the selected font using GetFontData function > >> > ( > https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getfontdata > ), > >> > which we can use to validate that the > >> > selected font is the one we need. > >> > > >> > The proposed solution is to check the file size of the font, > selected > >> > by GDI, before using it for rendering. If a mismatch > >> > is detected, fallback to FreeType is performed. It can > produce a > >> > somewhat different glyph representation, but, at least, > >> > the correct glyph will be rendered. For members of font > collections, > >> > file size for validation is calculated in a special > >> > way, in accordance with GetFontData logic described in the > >> > documentation. I've verified that it works for font > collections > >> > bundled with Windows 10. > >> > > >> > As for performance impact, during testing I didn't observe > average > >> > glyph generation time increase of more than 15%. > >> > Taking glyph caching into account, it shouldn't be that > significant > >> > for typical UI applications, I think. Performance > >> > impact can be made even smaller - by performing the > validation only > >> > once per font, but, I believe, having a Java > >> > application always render correct glyphs (even if fonts are > added or > >> > removed while application is running) is more > >> > important. > >> > > >> > Proposed patch doesn't add any tests, as reproducing the issue > >> > requires installation of fonts. Existing automated > >> > OpenJDK tests pass after the fix. Proposed approach has been > used in > >> > JetBrains Runtime without known issues for about 3 > >> > months in testing and for about 1 month in production. > >> > > >> > Best regards, > >> > Dmitry Batrak > >> > >> > > > > > > > > > -- > Best regards, Sergey. >
