On Thu, 18 Apr 2024 04:03:38 GMT, Tejesh R <t...@openjdk.org> wrote:
>> The main problem here is that we do not curate what font point size we
>> passed to freetype,
>> and would pass in one larger than freetype's maximum of FT_USHORT_MAX which
>> is USHRT_MAX.
>> This isn't documented, SFAICS, and is checked a couple of calls deep from
>> the specific API we use.
>> But generally anywhere near that size seems to cause freetype to choke as it
>> uses signed 16.16
>> values, so 32767 is really the max.
>> But we have no need to go anywhere near that - 16384 seems like a plenty
>> large enough pt size.
>> And we can request bigger sizes than that by making use of the transform.
>> At normal size ranges we use that just to account for rotation and decompose
>> the glyph transform
>> into point size and that rotation.
>>
>> But at large sizes - which are not usefully rendered anyway - there are no
>> hints etc to be lost
>> from not specifying the target point size. So we can extend the range of
>> sizes we allow.
>>
>> If this is still too large to be held decomposed into a pt size in the
>> range less than 16384 and a scale of up to 32766 then we substitute the null
>> scaler, as we generally do when values are out of range, such
>> a for a NaN transform.
>>
>> These extreme values aren't useful.
>>
>> In looking at this I did find that getGlyphPixelBounds doesn't follow the
>> maximum image size we use
>> for rendering. Which is not useful and could lead to inconsistent results. I
>> fixed that.
>>
>> Also whilst macOS didn't have these problems it could be cleaned up a bit
>> for consistency in the reported sizes for some cases.
>>
>> The test is mainly around making sure that "sensible" things are returned
>> for not sensible input.
>> There's no 100% right answer to these extreme unrenderable sizes.
>
> src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 537:
>
>> 535: if (TOO_LARGE(dmat[0], ptsz) || TOO_LARGE(dmat[1], ptsz) ||
>> 536: TOO_LARGE(dmat[2], ptsz) || TOO_LARGE(dmat[3], ptsz))
>> 537: {
>
> Suggestion:
>
> TOO_LARGE(dmat[2], ptsz) || TOO_LARGE(dmat[3], ptsz)) {
No, that was deliberate. We generally prefer the { on a new line if the
conditional is multi-line.
Still looking for an actual "reviewer" review here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18703#discussion_r1571126135
