[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR: 8233006: freetype incorrectly adjusts advances when emboldening rotated gl
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2020-04-17 19:13:38
Message-ID: a3e1e1a5-7038-74ff-afd7-4dfee69211d9 () oracle ! com
[Download RAW message or body]

Looks fine.

On 4/16/20 7:58 am, Philip Race wrote:
> 
> 
> On 4/16/20, 6:31 AM, Sergey Bylokhov wrote:
> > Hi, Phil.
> > I have only the question about the new comment:
> > 
> > 340     // Let's not adjust the metrics of any glyph that is zero advance.
> > 341     if (slot->linearHoriAdvance == 0) {
> > 342         return;
> > 343     }
> > 
> > The comments said that we do not want to adjust the metrics and return, but we \
> > already adjusted it a little bit before: 335     slot->metrics.width        += \
> > extra; 336     slot->metrics.height       += extra;
> 
> That is stored in metrics but it is the bounding box of the glyph image.
> So we definitely need to adjust that before we return since we widened the glyph \
> outline.
> > 
> > I do not know the exact reason to check linearHoriAdvance at line 341, but then \
> > why we skip the check of "linearVertAdvance"?
> 
> A few reasons.
> First, you'd need a font with vertical layout support (very rare) and for JDK to \
> then actually support vertical text layout for it to matter - so not needed. \
> Second, it is not clear if we'd want to do it even if we had such support. We \
> weren't previously getting scaled linear horizontal advance from freetype and it \
> was "OK". Just a tiny bit more less spaced than ideal because bold glyphs are \
> wider. With vertical it is less clear to me that you'd scale the advance in the \
> same way. So until such a day comes it is fine as it is. Third, freetype didn't \
> adjust it either, just like it wasn't adjusting the horizontal case. 
> Even this line below .. I am not sure is used for anything ..
> +    slot->metrics.vertAdvance  += extra;
> 
> .. but freetype added it, so I did so too to be safe.
> 
> -phil.
> > 
> > 
> > On 4/15/20 2:00 pm, Philip Race wrote:
> > > Bug : https://bugs.openjdk.java.net/browse/JDK-8233006
> > > Webrev : http://cr.openjdk.java.net/~prr/8233006
> > > 
> > > The bug here is that the freetype function for synthesising bold is not ready \
> > > to handle rotation. 
> > > In the process I noticed it did not adjust the advance used by the fractional \
> > > metrics case, even though the outline is bolded.
> > > 
> > > Also, in what seems to be a completely wrong thing to do, freetype would
> > > widen the advance of glyphs which have zero advance.
> > > 
> > > So I decided that the best thing to do was to write our own.
> > > A chunk of the heavy lifting - widening the outline - is still done by freetype
> > > but there were a lot of details to get right and test.
> > > 
> > > I wrote a test to visualise the problem but the actual test checks by looking
> > > at the bounding rectangle of the drawn pixels and compares its height to
> > > the declared metrics of the font, failing if they disagree by too much.
> > > 
> > > Note that the code path is only exercised when synthetic bolding is needed.
> > > So real bold fonts don't test this code.
> > > Since there's not an easy way to say which fonts have real bold, I decided the
> > > test should use a BOLD version of every font on the system, which on almost
> > > all systems will test some significant number of such cases.
> > > I kept the UI for visualising as it will be useful for later debugging of \
> > > failures. 
> > > Also it made me notice that the case where the text was not rotated at all was
> > > drawing shorter than all the other cases.
> > > I traced this back to the fix for 8203485 which added a macro FT26Dot6ToInt
> > > and used it to get the integer advance in the unrotated, integer metrics case.
> > > The idea there wasn't completely wrong, but I don't think it was completely \
> > > right either. I got rid of the macro and instead used the same FT26Dot6ToFloat \
> > > macro as used in the rotation cases. So we now return the exact floating point \
> > > value to the calling Java code. That then can round appropriately as it needs \
> > > to. This fixed the inconsistency and the test for 8203485 still passes as do \
> > > all other tests. This change will likely lead to some cases where unrotated \
> > > advances now round up one pixel wider, but so far it looks correct to me. \
> > > They'll be restored to something more like what they were before 8203485, since \
> > > that removed rounding and added truncation instead to fix a problem with the \
> > > rounding being incorrect for rotations because it could round down when it \
> > > should round up. Now we just let the Java code handle it.
> > > 
> > > I've run these tests on all platforms and they pass. Mac isn't using this \
> > > freetype path so it is not affected but it is still good to know the tests pass \
> > > there ... 
> > > -phil
> > 
> > 


-- 
Best regards, Sergey.


[prev in list] [next in list] [prev in thread] [next in thread]