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 <mailto: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>
<mailto: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>
<mailto: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/
<http://cr.openjdk.java.net/%7Edbatrak/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.
