Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextWidget/TextBoxWidget: enhanced text shaping #5598

Merged
merged 6 commits into from Nov 16, 2019

Conversation

@poire-z
Copy link
Contributor

poire-z commented Nov 15, 2019

Have TextWdiget and TextBoxWidget use the new Lua C module (added in koreader/koreader-base#1010) as an alternative text layout method, using Harfbuzz, FriBiDi and libunibreak.
This is needed to correctly shape Arabic scripts (cursive glyphs), propertly draw RTL text (Hebrew,
Arabic) and bidi text (arabic/hebrew filenames in english UI).
Screenshots and discussion in #5359.

With this PR, we globally switch to use Harfbuzz for text shaping. It will feel like a change (text more condensed, I didn't like it initially, but switching back to the previous Freetype-only rendering, that old rendering really feels ugly :)

This should be enough to render Arabic text correctly. It still needs more work on the UI widgets and the language bootstrap code, and gettext/L to properly bidi isolate some bit of strings - to properly align arabic and hebrew to the right.
(We'll also need more unrelated work to mirror the UI elements when the UI language is RTL - but this PR and a next one should have addressed all things related to text.)

For now, for testing, to enable RTL magic, add somewhere in reader.lua (this will have to be properly handled in language.lua)

require("libs/libkoreader-xtext")
xtext.setDefaultParaDirection(true)
xtext.setDefaultLang("ar")
-- xtext.setDefaultLang("zh_CN")
-- xtext.setDefaultLang("ja") -- get alt CJK glyphs
-- xtext.setDefaultLang("fr") -- get libunibreak french guillemets non-breaks

Also, I had to rework #5496 as it was using TextBoxWidget internals. So, I ended adding stuff to TextBoxWidget to do the work, and Menu.lua ended up quite shorter :) @robert00s : can you check last commit, and see if the code, with my comments, address correctly what you did with #5496.
I also took the liberty of adding a new setting Items > Reduce font size to show more text to bring back the previous behaviour that I prefer :) See #5496 (comment).

Some misc technical notes:
I initially went with touching as little TextWidget and TextBoxWidget, and add my stuff in xtextwidget.lua and xtextboxwidget.lua, and use something like:

function XTextBoxWidget:replaceTextBoxWidget()
    if TextBoxWidget._replaced_by_XTextBoxWidget then
        return
    end
    TextBoxWidget._replaced_by_XTextBoxWidget = true
    TextBoxWidget.initText = XTextBoxWidget.initText
    TextBoxWidget._splitToLines = XTextBoxWidget._splitToLines
    TextBoxWidget._renderText = XTextBoxWidget._renderText
    TextBoxWidget._shapeLine = XTextBoxWidget._shapeLine
    TextBoxWidget._getXYForCharPos = XTextBoxWidget._getXYForCharPos
    TextBoxWidget.getCharPosAtXY = XTextBoxWidget.getCharPosAtXY
    TextBoxWidget.onHoldReleaseText = XTextBoxWidget.onHoldReleaseText
    TextBoxWidget._alt_color_for_rtl = true -- for debugging
end

to enable it. It resulted in some redundant code (the code that deals with y coordinates), as this updates only the work on placing glyphs on the x asis. So, I ended up merging the new code into our TextWidget and TextBoxWidget, using a simple boolean to toggle one or the other (use_xtext = true/false).
For TextBoxWidget:_splitToLines(), I had to use a ugly goto idx_continue and ::idx_continue:: -- (Label for goto when use_xtext=true) - just because I didn't want to indent the previous code and have it part of the diff/git blame. I'll get rid of it later by indenting the whole thing in a later commit if that's really too ugly :)

Haven't benchmarked that code. But on my device, it feels as smooth as before.
I know (from the code and library we use) this needs quite more processor cycles than before, so it feels a bit gratuitous as the previous rendering code was good enough. But I have no idea how much this added work compares to all the other CPU intensive work, like blitting blitbuffers, and if it is negligeable :)
If needed, we could optimize the other widgets to cache more their TextWidgets (our menu create new ones each time we switch menu or menu pages) - or use other cache strategies.

Also, we might need to work on our bold font use and handling. In many cases, we use the normal font, and provide bold=true which does synthetized bold (while we may have a real nicer bold font).
With XText/Harfbuzz, we use another Freetype emboldening API that don't change metrics, and we can tune the boldness to our liking. Also, as it doesn't change metrics, it feels different, and I quite like it that the metrics don't change, things get more aligned, eg:
Before (that Chapter 6 sticks out from the other ones):
image
After with bold_strength_factor = 3/8 -- as crengine, lighter:
image
With bold_strength_factor = 1/2 -- bold enough:
image
With bold_strength_factor = 1 -- really too bold:
image

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 15, 2019

Will need a base bump for koreader/koreader-base#1010 in 3rd commit.

-- (Could be tweaked by font if needed. Note that NotoSans does not
-- have common ligatures, like for "fi" or "fl", so we won't see
-- them in the UI.)
face_obj.hb_features = { "+kern", "+liga" }

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

So is this technically redundant with HB defaults? I'm slightly confused by the comment following this line.

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

Half :) I've indeed been unclear, but that's mostly just cut and pasted from crengine.
And what the HB defaults are are also hard to track. It's in its src/hb-ot-shape.cc.
So, it looks to me +liga is the default, but not +kern (or may be it is...).
I'm just setting here the defaults we use in crengine HB full/best, even if one of them is already the default.
The commented next line is the list of all stuff mentionned in HB "good", that I just cut and pasted with all disabled to see if enabling +liga / +kern made a difference :)
I don't plan on being the reference as I'm not sure what it is :|. I'll update the comment to not state anything about "default" but "If we'd wanted to disable all the features HB may enable" :)

@@ -376,6 +376,20 @@ function FileManagerMenu:setUpdateItemTable()
end,
})
end
table.insert(self.menu_items.developer_options.sub_item_table, {
text = _("Disable UI enhanced text shaping (xtext)"),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

The order sounds a bit weird to me.

Disable enhanced UI text shaping

self._height = math.ceil(face_height) + 2*self.padding
self._baseline_h = Math.round(face_ascender) + self.padding
-- With our UI fonts, this usually gives 0.72 to 0.74, so text is aligned
-- a bit lower than before with the hardcoded 0.7

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

Excellent!

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

(Nothing excellent :) that's some previous comment that I just moved - it appears added here, but it's the same that is deleted below :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

Oh right, I remember now. 🤦‍♂

Still excellent though. ;-D

-- In case we draw truncated text, keep original self.text
-- so caller can fetch it again
self._text_to_draw = self.text

if self.text and type(self.text) ~= "string" then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

When does this happen?

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

Should not, but can if we provide a number or else.
We'll see it displayed instead of crashing, and it simplify my next check:
if not self.text or #self.text == 0 then self._is_empty = true self._length = 0
It avoid having nil fixed/handled in lower level code, like in:

function RenderText:renderUtf8Text(dest_bb, x, baseline, face, text, kerning, bold, fgcolor, width, char_pads)
if not text then
logger.warn("renderUtf8Text called without text");
return 0
end

koreader/frontend/util.lua

Lines 311 to 313 in e166a77

function util.splitToChars(text)
local tab = {}
if text ~= nil then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

Would something this work in this context? (That'd go underneath the method.) It's a convenient way to ensure conformity without bothering users.

dbg:guard(TextWidget, "updateSize",
    function()
        assert(type(self.text) == "string",
            "Wrong text type (expected string)")
    end)

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

I can, it works as you expect (crash on the emulator). Same in setText(text) then.
I think I've seen some output from logger.warn("renderUtf8Text called without text") in the past, so it may happen we get a nil. It's ok if we crash then when that happens?
(Can't cause any crash while tapping around - but I'm sure we'll get some.)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

Assuming it only crashes when it comes across a problem so that the problem can be addressed, that's indeed the idea. ;-)

But if it's too big an issue atm then it'd probably be best to leave that for a follow-up PR.

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

I've already added and tested it, so it will go in :)
If you confirm dbg.guard() does not kick in nor add overhead on our devices, it's fine with me - as long as I get there the safety of my tostring() (othewise, XText Lua check will throw an error).

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

Yeah, we possibly have a few nil renderUtf8Text lying around, that does ring a bell ;).

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

No overhead, when debug is disabled it's just nothing:

function Dbg.guard() end

And when enabled:

Dbg.guard = function(_, mod, method, pre_guard, post_guard)
local old_method = mod[method]
mod[method] = function(...)
if pre_guard then
pre_guard(...)
end
local values = {old_method(...)}
if post_guard then
post_guard(...)
end
return unpack(values)
end
end

end

-- Only when not self.use_xtext:

-- Note: we use kerning=true in all RenderText calls
--- @todo Don't use kerning for monospaced fonts. (houqp)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

This still relevant?

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

Not with the use_xtext code (Haven't checked, but I hope HB does the right thing with monospace fonts.
But for the legacy code, it might (it would still use kerning with monospaced fonts).

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

OK, I'll remove the @todo and keep that comment in parenthesis. As we'll probably don't have to fix it if we keep using xtext.

local face = self.face.getFallbackFont(xglyph.font_num) -- callback (not a method)
local glyph = RenderText:getGlyphByIndex(face, xglyph.glyph, self.bold)
local color = self.fgcolor
if self._alt_color_for_rtl then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

So this is always disabled unless you change it in the code?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

(Might be of some interest in that developer settings menu.)

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

So this is always disabled unless you change it in the code?

Right, always disabled.
Not worth busying developper settings with it I think - was useful when doing this PR, but I guess RTL is alright now, and I shouldn't need it - unless we feel it's needed when we play with mirroring the UI, but I doubt it.

@@ -580,6 +831,17 @@ function TextBoxWidget:paintTo(bb, x, y)
bb:blitFrom(self._bb, x, y, 0, 0, self.width, self._bb:getHeight())
end

function TextBoxWidget:onCloseWidget()
-- free when UIManager:close() was called

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

Free when UIManager:close() was called.

(I mean, it just looks so out of place with the next comment. :-P)

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

Yes :) That's verbatim from the other widgets that have it, so, I just kept it for coherency even if it sounds gramatically strange.
And... it's I that introduced that strange wording :)

@@ -580,6 +831,17 @@ function TextBoxWidget:paintTo(bb, x, y)
bb:blitFrom(self._bb, x, y, 0, 0, self.width, self._bb:getHeight())
end

function TextBoxWidget:onCloseWidget()
-- free when UIManager:close() was called
-- Free the between-renderings (between page scrolls) freeable ressources

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 15, 2019

Member

ressources is French ^_^

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

French ressources need freeing too ! :)

@Frenzie Frenzie added this to the 2019.12 milestone Nov 15, 2019
@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 15, 2019

Yeah, I'd probably +1 using a real Bold font (which we already ship anyway ^^) ;).

@poire-z poire-z force-pushed the poire-z:xtext_front branch from 909253c to 88ae574 Nov 15, 2019
Copy link
Member

NiLuJe left a comment

Had to missclick, the simple "Single comment" button just plain disappeared on me -_-".

-- We do the revert of what's done in :init():
-- self.line_height_px = Math.round( (1 + self.line_height) * self.face.size )
local font_size = height_px / nb_lines / (1 + line_height_em)
font_size = font_size * 1000000 / Screen:scaleBySize(1000000) -- invert scaleBySize

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

Doing the proper math in a real matching function in ffi/framebuffer.lua might be worth it?

(i.e., having a real px to pt helper function available).

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

Probably, we used that same trick in coverbrowser some weeks ago.
(But I suck at proper math :)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

Apparently as simple as basically copying scaleBySize but changing the return to math.floor(px / (size_scale + dpi_scale) * 2) ;).

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 15, 2019

Author Contributor

So, should it be that (scaleBySize() input being pt and not px)?

-function fb:scaleBySize(px)
+function fb:scaleBySize(pt)
     -- larger screen needs larger scale
@@ -299,3 +299,15 @@ function fb:scaleBySize(px)
     -- scaled positive px should also be positive
-    return math.ceil(px * (size_scale + dpi_scale) / 2)
+    return math.ceil(pt * (size_scale + dpi_scale) / 2)
+end
+
+function fb:unscaleBySize(px)
+    -- Revert of previous function:
+    -- px = scaleBySize(pt)
+    -- pt = unscaleBySize(px)
+    local size_scale = math.min(self:getWidth(), self:getHeight())/600
+    local dpi_scale = size_scale
+    if self.device and self.device.display_dpi ~= self.dpi then
+        dpi_scale = self.dpi / 160
+    end
+    return math.floor(px / (size_scale + dpi_scale) * 2)
 end

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

Well, it's not technically points per-se, as that's pretty much set in stone as px = round(dpi / 72.0 * pt), which isn't what we're doing.

It's more something like absolute pixels or whatever (I only mentioned point earlier because the theory is similar ;)).

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

Not a Python quirk ;).

#!/usr/bin/env luajit

dpi = 167
width = 600
height = 800

function scaleBySize(px)
   -- larger screen needs larger scale
   size_scale = math.min(width, height)/600
   -- if users custom screen dpi, also scale by dpi
   dpi_scale = size_scale

   dpi_scale = dpi / 160

   -- scaled positive px should also be positive
   scaled_px = px * (size_scale + dpi_scale) / 2
   return {scaled_px, math.ceil(scaled_px)}
end

function unScaleBySize(px)
   -- larger screen needs larger scale
   size_scale = math.min(width, height)/600
   -- if users custom screen dpi, also scale by dpi
   dpi_scale = size_scale

   dpi_scale = dpi / 160

   -- scaled positive px should also be positive
   unscaled_px = px / (size_scale + dpi_scale) * 2
   return {unscaled_px, math.floor(unscaled_px)}
end

---

function tprint(tbl, indent)
  if not indent then indent = 0 end
  for k, v in pairs(tbl) do
    formatting = string.rep("  ", indent) .. k .. ": "
    if type(v) == "table" then
      print(formatting)
      tprint(v, indent+1)
    elseif type(v) == 'boolean' then
      print(formatting .. tostring(v))
    else
      print(formatting .. v)
    end
  end
end

print("scaleBySize(500)")
tprint(scaleBySize(500))
print("unScaleBySize(511)")
tprint(unScaleBySize(511))

print("scaleBySize(20)")
tprint(scaleBySize(20))
print("unScaleBySize(21)")
tprint(unScaleBySize(21))

dpi = 167 * 3

print("scaleBySize(18)")
tprint(scaleBySize(18))
print("unScaleBySize(38)")
tprint(unScaleBySize(38))

print("18 via * 10**6")
tprint(scaleBySize(18 * 1000000 / scaleBySize(1000000)[2]))
scaleBySize(500)
1: 510.9375
2: 511
unScaleBySize(511)
1: 500.06116207951
2: 500
scaleBySize(20)
1: 20.4375
2: 21
unScaleBySize(21)
1: 20.550458715596
2: 20
scaleBySize(18)
1: 37.18125
2: 38
unScaleBySize(38)
1: 18.39636913767
2: 18
18 via * 10**6
1: 18
2: 18

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

TL;DR: Smells like yet another case of "I don't know what the hell's happening inside the testsuite, but it doesn't make any sense" ;).

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

(GH is still being clunky, I've only just now got your previous answers interleaved in the thread -_-").

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 16, 2019

Member

GH has recently been "helpfully" hiding comments behind a "More comments" type thing for me. D:

The presumed default used to be 800x600 @ 167 but that was changed to 160 for easier math. See koreader/koreader-base@c978857

Otherwise the unit test would make no sense! ;-)

        fb:setDPI(167)
        assert.are.equals(31, fb:scaleBySize(30))

        fb:setDPI(167 * 3)
        assert.are.equals(62, fb:scaleBySize(30))

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 16, 2019

Member

Or at least, I supported it for that reason: koreader/koreader-base#843 (comment)

I'm wondering if maybe we should switch the other one to 160 instead since it's basically just a bit of legacy. It's practically the same thing. I doubt you'll even notice a difference in pixels except at really high DPI, except 160 DPI is exactly one inch making for much easier (head) calculations.

-- In case we draw truncated text, keep original self.text
-- so caller can fetch it again
self._text_to_draw = self.text

if self.text and type(self.text) ~= "string" then

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Nov 15, 2019

Member

Yeah, we possibly have a few nil renderUtf8Text lying around, that does ring a bell ;).

@poire-z poire-z force-pushed the poire-z:xtext_front branch from 88ae574 to 300ca39 Nov 15, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 15, 2019

I have not spend much time on the cursor positionning in RTL/bidi contexts. So, this will need some more work. See @todo in TextBoxWidget:_getXYForCharPos().

bump base for libkoreader-xtext.so:
- ffi/pic.lua: fix memory leak with some unsupported PNG files
- FreeType ffi: add methods for use with Harfbuzz shaping
- thirdparty: adds libunibreak
- Adds libkoreader-xtext.so: enhanced text shaping

Add a getFallbackFont(N) to each Lua font object to instantiate
(if not yet done) and return the Nth fallback font for the font.

Fallback fonts list: add NotoSansArabicUI-Regular.ttf

Add RenderText:getGlyphByIndex() to get a glyph bitmap by glyph
index, which is what we'll get from Harfbuzz shaping.
(RenderText:getGlyph() works with unicode charcode).
@poire-z poire-z force-pushed the poire-z:xtext_front branch from 300ca39 to 3358273 Nov 16, 2019
Alternative code to size and draw text with the help
of the xtext Lua C module.
Enabled by default (can be disabled via an added menu
item in "Developer options").

New properties can be specified by calling widgets, only
used when xtext is used:
- lang: use specified language instead of the UI language
- para_direction_rtl: true/false to override the default
  direction for the UI language
- auto_para_direction: detect direction of each paragraph
  in text
@poire-z poire-z force-pushed the poire-z:xtext_front branch from 3358273 to ac03013 Nov 16, 2019
Alternative code to size, split lines and draw text with
the help of the xtext Lua C module.
Enabled by default (can be disabled via an added menu
item in "Developer options").

New properties can be specified by calling widgets, only
used when xtext is used:
- lang: use specified language instead of the UI language
- para_direction_rtl: true/false to override the default
  direction for the UI language
- auto_para_direction: detect direction of each paragraph
  in text
- alignment_strict: prevent the inversion of the specified
  alignment= that is done when a paragraph direction is
  set or detected as RTL.

Also: fix possible memory leak (present even when not using xtext)
by calling :free() in onCloseWidget() like it's done in ImageWidget.
@poire-z poire-z force-pushed the poire-z:xtext_front branch 2 times, most recently from ffb3908 to 8a16056 Nov 16, 2019
- height_adjust: if true, reduce height to a multiple of
  line_height (for nicer centering)
- height_overflow_show_ellipsis: if height overflow, append
  ellipsis to last shown line
(Implemented in both use_xtext and legacy code path.)

Use them in Menu.lua to clean up/shorten the code used for multiline
menu items by delegating the work to TextBoxWidget, or using
TextWidget when we end up needing only a single line.
@poire-z poire-z force-pushed the poire-z:xtext_front branch from 8a16056 to 2c885bc Nov 16, 2019
@poire-z poire-z merged commit dc8696b into koreader:master Nov 16, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z poire-z deleted the poire-z:xtext_front branch Nov 16, 2019
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 17, 2019

Tbh I thought we already did.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 17, 2019

(I don't have any gtk stuff installed, which might explain why I was unaffected.)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 17, 2019

I'm not wrong, I did put that in the script. :-P

# export load library path
export LD_LIBRARY_PATH=${KOREADER_DIR}/libs:$LD_LIBRARY_PATH

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 17, 2019

But this would mean I can no longer just run ./reader.lua? :-/

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

@Frenzie: Which explains why you couldn't repro w/ the AppImage. Yay! :).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

But this would mean I can no longer just run ./reader.lua? :-/

Hmm, yeah, I can't think of a neat solution to that.

I can think of a very terrible one, though: an LD_PRELOAD hack that sets the env var. Kindles do that >_<".
It'd be doubly terrible here because that implies /etc/ld.so.preload to avoid having to set an env var for each run ^^.

(Kids, don't do that at home!)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 17, 2019

FWIW, we'll need static-libstdc++, I'm pretty sure it won't load on some Kindles otherwise

You mean that in relation to the Android5 issue?
Btw, adding -static-libstdc++ does not make my non-debug arm libkoreader-xtext.so grows by a single bit. If I add somewhere std::vector<int> v = {7, 5, 16, 8};, it does grow by 36 bytes.
It does grow more in debug mode for x86.
So, dunno if it really changes something if I don't pull any stdc++. Would it on Kindle?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 17, 2019

@NiLuJe This doesn't seem to actually do anything?

diff --git a/kodev b/kodev
index 0c1ac28f..a41c842e 100755
--- a/kodev
+++ b/kodev
@@ -101,6 +101,7 @@ function setup_env() {
     files=$(find . -maxdepth 1 -name 'koreader-emulator-*' | ${SETUP_ENV_GREP_COMMAND})/koreader
     assert_ret_zero $? "Emulator not found. Please build it first."
     export EMU_DIR=${files[0]}
+    export LD_LIBRARY_PATH=${EMU_DIR}/libs:$LD_LIBRARY_PATH
 }
 
 function kodev-fetch-thirdparty() {
@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

@poire-z:

The simple fact of linking against the STL implies ABI compat checks.

Here:

 Version needs section '.gnu.version_r' contains 2 entries:
  Addr: 0x0000000000001360  Offset: 0x001360  Link: 3 (.dynstr)
   000000: Version: 1  File: libstdc++.so.6  Cnt: 2
   0x0010:   Name: CXXABI_1.3.9  Flags: none  Version: 5
   0x0020:   Name: GLIBCXX_3.4  Flags: none  Version: 3

That won't do on Kindle ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

@Frenzie: PWD changes between when that's set and when we run ;).

Something like

diff --git a/kodev b/kodev
index 0c1ac28f..84bca093 100755
--- a/kodev
+++ b/kodev
@@ -100,7 +100,8 @@ function setup_env() {
     fi
     files=$(find . -maxdepth 1 -name 'koreader-emulator-*' | ${SETUP_ENV_GREP_COMMAND})/koreader
     assert_ret_zero $? "Emulator not found. Please build it first."
-    export EMU_DIR=${files[0]}
+    export EMU_DIR="${files[0]}"
+    export LD_LIBRARY_PATH="$(realpath ${EMU_DIR})/libs:${LD_LIBRARY_PATH}"
 }
 
 function kodev-fetch-thirdparty() {

Works, by ensuring that's a canonical absolute path ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

That, or readlink -f.

One, or both of those, will gloriously not work on macOS, because Apple. Unless one install coreutils.

@yparitcher

This comment has been minimized.

Copy link
Contributor

yparitcher commented Nov 17, 2019

btw with archlinux i also get the system versions of FT and harfbuzz, but the emulator works even with xtext

/usr/lib/libharfbuzz.so.0.20600.4
/AUR/koreader/base/build/x86_64-pc-linux-gnu-debug/libs/libfreetype.so.6
/usr/lib/libfreetype.so.6.17.1

i do have gtk installed, and it works both if run via kodev or via ./reader.lua

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

I'm running FT snapshots on my system, and @Frenzie is on a Debian thingy that may be running some older versions, while you're exactly matching the vendroed version on Arch, which might explain why nothing blows up.

Having two copies of the same library initialized in the same process is still a highly volatile proposition ;).

@yparitcher

This comment has been minimized.

Copy link
Contributor

yparitcher commented Nov 17, 2019

@Frenzie I found a difference, my libkoreader-xtext.so pulls in libstdc++ yours does not.
i do not know if it is connected.
lddtree.txt

my results

libkoreader-xtext.so (interpreter => None)
    libfreetype.so.6 => /AUR/koreader/base/build/x86_64-pc-linux-gnu-debug/libs/libfreetype.so.6
    libharfbuzz.so.0 => /AUR/koreader/base/build/x86_64-pc-linux-gnu-debug/libs/libharfbuzz.so.0
    libfribidi.so.0 => /AUR/koreader/base/build/x86_64-pc-linux-gnu-debug/libs/libfribidi.so.0
    libunibreak.so.3 => /AUR/koreader/base/build/x86_64-pc-linux-gnu-debug/libs/libunibreak.so.3
    libstdc++.so.6 => /usr/lib/libstdc++.so.6
        ld-linux-x86-64.so.2 => /usr/lib/ld-linux-x86-64.so.2
    libm.so.6 => /usr/lib/libm.so.6
    libgcc_s.so.1 => /usr/lib/libgcc_s.so.1
    libc.so.6 => /usr/lib/libc.so.6

your results

libkoreader-xtext.so => ./libkoreader-xtext.so (interpreter => none)
    libfreetype.so.6 => ./libfreetype.so.6
    libharfbuzz.so.0 => ./libharfbuzz.so.0
    libfribidi.so.0 => ./libfribidi.so.0
    libunibreak.so.3 => ./libunibreak.so.3
    libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6
    libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6
    ld-linux-x86-64.so.2 => /lib/x86_64-linux-gnu/ld-linux-x86-64.so.2
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 17, 2019

a Debian thingy

I generally run Debian stable or testing on my desktop. That's currently stable, it'll turn into testing whenever I start to feel it's a bit outdated, i.e., when I start to feel the need to backport more than a select handful of programs. Right now my only backport is po4a, because Debian comes with 0.55 (even in unstable ;_;) and 0.56 contains this fix: https://github.com/mquinson/po4a/pull/173.

On my laptop I normally run the latest *buntu. Then I've got another old laptop which varies (currently Mint), doesn't do too much since I got my current sidegrade in '16, and an external drive with a bunch of live images. I've also got an old netbook that runs the latest Xubuntu LTS. And of course Ubuntu 16.04 in Docker. I think that's about all of it.

So yeah, mostly Debubuntu in various degrees of "outdated" (compared to Arch/Gentoo).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

@yparitcher: I'm guessing that was a simply a post -static-libstdc++ build ;).

@yparitcher

This comment has been minimized.

Copy link
Contributor

yparitcher commented Nov 17, 2019

nope koreader @ 89c0bd0 base was @6c48019 before -static-libstdc++,

however my kindle KT4 build, built last night has

lddtree libkoreader-xtext.so 
libkoreader-xtext.so (interpreter => None)
    libfreetype.so.6 => /home/yechiel/Downloads/dtmpfs/kindle/mnt/us/koreader/libs/libfreetype.so.6
    libharfbuzz.so.0 => /home/yechiel/Downloads/dtmpfs/kindle/mnt/us/koreader/libs/libharfbuzz.so.0
    libfribidi.so.0 => /home/yechiel/Downloads/dtmpfs/kindle/mnt/us/koreader/libs/libfribidi.so.0
    libunibreak.so.3 => /home/yechiel/Downloads/dtmpfs/kindle/mnt/us/koreader/libs/libunibreak.so.3
    libm.so.6 => None
    libgcc_s.so.1 => None
    libc.so.6 => None
    ld-linux.so.3 => None

and it is working just fine without -static-libstdc++

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Nov 17, 2019

Frenzie added a commit to Frenzie/koreader that referenced this pull request Nov 18, 2019
As per @NiLuJe's suggestions from <koreader#5598 (comment)>.

I'm not super happy with this but being able to easily run the emulator is the most important thing.
Frenzie added a commit that referenced this pull request Nov 18, 2019
As per @NiLuJe's suggestions from <#5598 (comment)>.

I'm not super happy with this but being able to easily run the emulator is the most important thing.
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Dec 15, 2019

Note: I forgot to implement this in the use_xtext variant of TextBoxWidget:

-- either a very long english word occupying more than one line,
-- or the excessive char is itself splittable:
-- we let that excessive char for next line
if adjusted_idx == offset then -- let the fact a long word was splitted be known
self.has_split_inside_word = true
end

It's only use is in CoverBrowser Mosaic mode, with "fake" text covers, where we can now have words splitted in the middle :| (but it also avoids going to excessive small font size)
local textboxes_ok = true
if (authors_wg and authors_wg.has_split_inside_word) or (title_wg and title_wg.has_split_inside_word) then
-- We may get a nicer cover at next lower font size
textboxes_ok = false
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.