[OpenJDK 2D-Dev] RFR: 8233006: freetype incorrectly adjusts advances when emboldening rotated glyphs

[Philip Race]

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
>
>