Round FT metrics, split text drawing by unicode script, and others by poire-z · Pull Request #318 · koreader/crengine

poire-z · 2019-11-21T19:42:54Z

See individual commit messages for detail.

Links to relevant discussions:

Fix decoding of recent MOBI files kobo aura hd Chinese mobi gibberish koreader#5599
Fonts: round FT metrics instead of floor'ing them Adds libkoreader-xtext.so: enhanced text shaping helper koreader-base#1010 (comment)
Fonts: switch to no hinting when native hinting fails cre: use 'best' (Harfbuzz) as the default kerning method koreader#5553 (comment) (with the font from https://www.mobileread.com/forums/showthread.php?t=273339)
Text: split measuring and word drawing by unicode script Right to left support (Arabic, Hebrew, ...) koreader#5359 (comment)

Before (with that Isidore font and native hinting, so "abc" not shown):

(at the bottom, no space on one side or both of the arabic "word" - might just be a stream of reandom arabic chars.)
After:

This change is

Happens with the Isidore font from: https://www.mobileread.com/forums/showthread.php?t=273339

Fix some issue (don't remember exactly what) when there's one single good cluster at end of segment, and previous part of segment is to be drawn with the fallback font. (Backported from xtext.cpp.)

When measuring with Harfbuzz, we should also split on Unicode script change, even in a same bidi level (mixed hebrew and arabic in a single text node should be handled as multiple segments, or Harfbuzz would shape the whole segment with the script of the first kind of text it meets, resulting in non-joined arabic if preceeded by hebrew). Also remove First-Strong-Isolate unicode char from the list of RTL chars. (Also fix wording of "bidi" as "bibi"...)

LVRendLineInfo created when splitting a large line (tall table row) into multiple pages were not freed. (Reported by valgring.)

Frenzie

Reviewed 7 of 8 files at r1.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @poire-z)

crengine/src/pdbfmt.cpp, line 193 at r1 (raw file):

        if ( mobiEncryption!=0 )
            return false; // encryption is not supported
        if ( hederLength >= 0xE4 ) {

That variable name. D:

NiLuJe

Reviewed 7 of 8 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @poire-z)

crengine/src/lvfntman.cpp, line 75 at r1 (raw file):

#define FONT_METRIC_TO_PX(x)    (((x)+32) >> 6) // ROUND + TRUNC

My maths is fuzzy, but isn't this missing the AND from ROUND?

Also, metrics, not metric ;).

crengine/src/lvfntman.cpp, line 1668 at r1 (raw file):

        else
            glyph->rsb =   (lInt16)(FONT_METRIC_TO_PX( (myabs(_slot->metrics.horiAdvance)
                                        - _slot->metrics.horiBearingX - _slot->metrics.width) ) );

@Frenzie : I CAN SEE DEAD WHITESPACE :D. <3.

NiLuJe · 2019-11-21T22:29:53Z

Yay, inline quotes still quoting unrelated code! :D.

The reviewable link actually points to the right place.

(EDIT: Ninja edit-fixed it).

Frenzie · 2019-11-21T22:36:26Z

Yeah, like I said in base I was almost already sold when I saw the whitespace. ;-)

NiLuJe

Reviewed 1 of 8 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @poire-z)

crengine/src/lvfntman.cpp, line 75 at r1 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

#define FONT_METRIC_TO_PX(x)    (((x)+32) >> 6) // ROUND + TRUNC
My maths is fuzzy, but isn't this missing the AND from ROUND?

Also, metrics, not metric ;).

A quick testcase shows that this indeed works as it should, so, yay, maths! :D

NiLuJe

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @poire-z)

poire-z · 2019-11-21T23:06:17Z

My maths is fuzzy, but isn't this missing the AND from ROUND?

I figured (and checked with some online C compiler with values 63, 65, 90, 100, 127, 130... and -63 -65... that both ways always give the same result) that & -64 (which do & 0xfffffffff000000, setting to 0 the 6 lower bits) is not needed if you do just after >> 6.

Also, metrics, not metric ;).

Even when I'm dealing with a single one (height, or advance, or another) of the multiple font metrics?
Should it be FONT_METRICS_TO_PX(single_metrics) ?!

I CAN SEE DEAD WHITESPACE

That's some comment about that "reviewable" site, or my use of added white space to keep things aligned?

Anyway, not fond of "reviewable" (except for the larger width & smaller font size when viewing code) - and the noise it adds here, and it forcing me to resolve conversation there, and masquerading as an additional CI check. But if you guys like it... (Except that now, as I don't want to use it, I can reply to code comments in the main PR thread and not miss some of them :)

NiLuJe · 2019-11-22T00:21:03Z

My maths is fuzzy, but isn't this missing the AND from ROUND?

I figured (and checked with some online C compiler with values 63, 65, 90, 100, 127, 130... and -63 -65... that both ways always give the same result) that & -64 (which do & 0xfffffffff000000, setting to 0 the 6 lower bits) is not needed if you do just after >> 6.

Yeah, ran a quick for (int32_t i = INT32_MIN; i < INT32_MAX; i++) loop locally, too ;).

Also, metrics, not metric ;).

Even when I'm dealing with a single one (height, or advance, or another) of the multiple font metrics?
Should it be FONT_METRICS_TO_PX(single_metrics) ?!

Yup. c.f., OED ;).

I don't think I've ever seen it being used in another form in the context of font design/rendering.

I CAN SEE DEAD WHITESPACE

That's some comment about that "reviewable" site, or my use of added white space to keep things aligned?

Anyway, not fond of "reviewable" (except for the larger width & smaller font size when viewing code) - and the noise it adds here, and it forcing me to resolve conversation there, and masquerading as an additional CI check. But if you guys like it... (Except that now, as I don't want to use it, I can reply to code comments in the main PR thread and not miss some of them :)

Yeah, that was in response to @Frenzie in another issue and/or on gitter ;).

NiLuJe · 2019-11-22T00:35:57Z

Just to clear up the maths in my head:

Two's complement arithmetic say that (for an int) -x == (~x) + 1 == ~(x - 1), and that ~x == (x ^ 0xFFFFFFFF).
Meaning -64 == (63 ^ 0xFFFFFFFF). That's 4294967232, or 0b11111111111111111111111111000000.

Hence the 6 low bits, which we then punt off with the shift ;).

(ref: https://www.nayuki.io/page/some-bit-twiddling-functions-explained because no, I did not know the two's complement rules off the top of my head ;))

KCalc in programmer's mode has the handy binary representation visible at all times, which helps for this kind of stuff ;).

NiLuJe

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @poire-z)

NiLuJe

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @poire-z)

poire-z · 2019-11-22T08:03:37Z

I don't think I've ever seen it being used in another form in the context of font design/rendering.

I'm not convinced :) Your screenshots explicitely states its use in plural, and show examples in plurals.
Looking at the number of results of a google search (for what it's worth) for "one metric" vs "one metrics", or "single metric" vs "single metrics", gives more result for the singular "metric" - and there's some links to scientific papers (which I guess go thru grammar reviews :) that use the singular.
And there's a few singular "metric" instances in FreeType sources (albeit far less than plural "metrics" - but there are more generic comments not specifying a specific single metric :)

Newer MOBI files have larger headers. Not fetching the extradata flags would cause not cleaning extradata at PDB block boundaries, and result in garbage in the HTML content (possibly resulting in invalid UTF-8 and not triggering UTF-8 parsing).

So it gets a fixed id and we get more cache stability.

poire-z · 2019-11-22T08:25:31Z

(Small optimization to the "background-color black" commit.)

NiLuJe · 2019-11-22T18:43:28Z

Oh, that's because the singular form is for sure more common in general ;). A slightly different meaning, and different contexts, though ;).

In FreeType, the only singular I can find are related to the PCF driver, which may be using it because of said slightly different meaning (and/or because it's just much older) ;).

poire-z · 2019-11-22T19:34:15Z

Well, I'm older too :) But ok, history will blame that on me (and it looks so odd to my french-grammar-rooted english :)

Frenzie · 2019-11-22T20:09:53Z

Afaik a metric is basically just a synonym for measurement (e.g., what metric should we use to judge X). One metric, two metrics. Height, ascent, descent, and whatever else there is are all individually a single font metric.

As in this quote (2011 April 10, Financial Times, according to here):

As for the large number of official statements that Spain is safe, I think they are merely a metric of the complacency that has characterised the European crisis from the start.

Also:

https://docs.microsoft.com/en-us/dotnet/framework/winforms/advanced/how-to-obtain-font-metrics

Note that this is the same as the number (rounded up to an integer) obtained by converting the line-spacing metric to pixels.

NiLuJe · 2019-11-22T21:24:40Z

Yeah, I'm a bit torn on the specific usage when it's used to specifically point to a single specific type like in that last quote ;).

That said, here, I'm taking the macro to basically say "convert some metrics to pixels", so the plural seems justified nonetheless.

Grammar is fun. Feel free to poke holes in that ;).

poire-z added 5 commits November 21, 2019 20:13

Fonts: round FT metrics instead of floor'ing them

02dd811

Fonts: switch to no hinting when native hinting fails

4c27f0e

Happens with the Isidore font from: https://www.mobileread.com/forums/showthread.php?t=273339

Fonts: fix issue with Harfbuzz fallback font drawing

ac58ba6

Fix some issue (don't remember exactly what) when there's one single good cluster at end of segment, and previous part of segment is to be drawn with the fallback font. (Backported from xtext.cpp.)

Page splitting: fix small memory leak

922323e

LVRendLineInfo created when splitting a large line (tall table row) into multiple pages were not freed. (Reported by valgring.)

Frenzie approved these changes Nov 21, 2019

View reviewed changes

NiLuJe requested changes Nov 21, 2019

View reviewed changes

NiLuJe approved these changes Nov 21, 2019

View reviewed changes

NiLuJe approved these changes Nov 22, 2019

View reviewed changes

poire-z added 3 commits November 22, 2019 09:23

Fix "background-color: black" ignored on inline elements

0503653

Fix decoding of recent MOBI files

0f27ae1

Newer MOBI files have larger headers. Not fetching the extradata flags would cause not cleaning extradata at PDB block boundaries, and result in garbage in the HTML content (possibly resulting in invalid UTF-8 and not triggering UTF-8 parsing).

Hardcoded elements list: add <font>

7370434

So it gets a fixed id and we get more cache stability.

poire-z force-pushed the various_fixes branch from 49c6705 to 7370434 Compare November 22, 2019 08:24

poire-z merged commit cc9af0a into koreader:master Nov 22, 2019

poire-z deleted the various_fixes branch November 22, 2019 11:34

poire-z mentioned this pull request Nov 22, 2019

bump crengine: round FT metrics, split text drawing by script koreader/koreader-base#1014

Merged

poire-z mentioned this pull request Nov 22, 2019

bump crengine: round FT metrics, split text drawing by script koreader/koreader#5628

Merged

Conversation

poire-z commented Nov 21, 2019 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

NiLuJe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Nov 21, 2019

Uh oh!

NiLuJe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

poire-z commented Nov 21, 2019

Uh oh!

NiLuJe commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

poire-z commented Nov 22, 2019

Uh oh!

poire-z commented Nov 22, 2019

Uh oh!

NiLuJe commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Nov 22, 2019

Uh oh!

Frenzie commented Nov 22, 2019

Uh oh!

NiLuJe commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

poire-z commented Nov 21, 2019 •

edited by Frenzie

Loading

NiLuJe left a comment •

edited

Loading

NiLuJe commented Nov 21, 2019 •

edited

Loading

NiLuJe left a comment •

edited

Loading

NiLuJe commented Nov 22, 2019 •

edited

Loading

NiLuJe commented Nov 22, 2019 •

edited

Loading

NiLuJe commented Nov 22, 2019 •

edited

Loading

NiLuJe commented Nov 22, 2019 •

edited

Loading