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

epub: svg images not shown #9298

Closed
dgcampea opened this issue Jul 5, 2022 · 34 comments · Fixed by #9510
Closed

epub: svg images not shown #9298

dgcampea opened this issue Jul 5, 2022 · 34 comments · Fixed by #9510
Labels
Milestone

Comments

@dgcampea
Copy link

dgcampea commented Jul 5, 2022

  • KOReader version: 2022.06
  • Device: Android

Issue

Embedded .svg images in epub are not shown.

Screenshots

KOReader Android screenshot:
Screenshot_2022-07-05-19-43-23

E-book viewer (calibre):
image

Steps to reproduce

Open sicp.epub

@poire-z
Copy link
Contributor

poire-z commented Jul 5, 2022

It uses <object> that we have no code to handle (dunno how standard this is):
image

If I modify it to be <img src=...>, we would get:
image

so, the lines/strokes are there, but not the text.
Which is a known limitation of the SVG library we use: #6757 (comment)

So, yes, sadly, our SVG support sucks.

@Frenzie
Copy link
Member

Frenzie commented Jul 5, 2022

It uses <object> that we have no code to handle (dunno how standard this is):

That was fairly common back when SVG in the <img> element was still (somewhat) new, and possibly before then as well.

I.e., like this:

<object src="vector.svg" type="image/svg+xml">
 <img src="raster-fallback.png">
</object>

I'm surprised to see it combined with <figcaption>.

@RokeJulianLockhart
Copy link

RokeJulianLockhart commented Jul 15, 2022

@poire-z, although http://github.com/memononen/nanosvg/issues/203 requests support for text by the parser of SVG that this project appears to utilize, this reponse to the proposition by whom I believe to be a maintainer of the parser rejects the proposition.

@poire-z
Copy link
Contributor

poire-z commented Jul 15, 2022

More interesting is the next comment, linking to https://github.com/asmwarrior/SvgPanel which is some continued work based on https://github.com/memononen/nanosvg/pull/94.
Which is something we could do as crengine, the external application, is able to draw text.
Although it feels a bit limited drawing text after all other things, when it may be covered by these other things - but it may be better than not having text, and good enough in real life/books.
I may look at it later this summer.

@NiLuJe
Copy link
Member

NiLuJe commented Jul 15, 2022

I'd also watch out for https://github.com/sammycage/lunasvg, which should ultimately gain pretty good text support ;).

@poire-z
Copy link
Contributor

poire-z commented Jul 29, 2022

should ultimately gain pretty good text support ;).

Nothing in sight :) The maintainer mentionned it had coded up to 60% of it - then lost the code :/
There is some fork that added a bit of text support, but only the parsing and data structures: https://github.com/yurablok/lunasvg
Anyway, lunasvg is not actively developped - and its code is libstd++ heavy - looks like another totally alien language to me than the C++ I'm used to with crengine :)

nanosvg also is not actively developped, but it's a bit more graspable. Although, comparing to lunasvg, it is quite limited and not really proper (limited XML parsing, no tree of elements, only a single-level list of shapes, so you don't have inheritance that would be needed for <text><tspan><tspan>.)

But SVG looks so complex, that I guess no little renderer can do it in full (it needs a proper CSS parser and applier, a DOM like tree structure, a text rendering stack with Harfbuzz, and even more complex that what we have in crengine to ensure some SVG options like lengthAdjust and text rotation/transformation, layout text along a textPath that can be an arc, ...), so there must be SVG files where any SVG engine would fail rendering them correctly...
A proper SVG rendering engine could be as complex as crengine... :/

I'm tempted to fork nanosvg, pick and cleanup a few of the unmerged PRs, pick and rewrite bits from https://github.com/asmwarrior/SvgPanel to at least get horizontal text.
I managed to get the following with quickly hacking stuff (text in blue is rendering by crengine code over the bitmap):
image
image
image
but there's so many stuff missing (like arrows in the last one), and a few other sample SVG with text fail - that it will always feel "half done and hope you're lucky that your book has simple SVG images"...

But I don't really want to have to maintain/enhance/fix stuff :) So, we could just wait a few more years for a new SVG library to come up that would do it all well.
(Does MuPDF support SVG files ? I can't really say looking at their web site.)

@Frenzie
Copy link
Member

Frenzie commented Jul 29, 2022

MuPDF does, remember we use it for the UI icons. :)

But until we update it support won't exceed nanosvg.

@poire-z
Copy link
Contributor

poire-z commented Aug 5, 2022

Another nanosvg fork has added more support for SVG stuff, including text and tspan:
https://github.com/mtyberg/nanosvg
@Frenzie : I see you're one of the 2 person that starred that repo :) Do you remember why, on which occasion, how did you meet it, is it used by some other project... ? No change to the rasterizer, so it must be used by some other software with its own rasterizer. It hasn't been updated for 2 years too...

Another standalone Lua library (that doest not do text either...):
https://github.com/Samsung/thorvg/
Another (probably unusable) heavily patched and augmented nanosvg in:
https://sourceforge.net/p/cloverefiboot/code/HEAD/tree/rEFIt_UEFI/libeg/

@Frenzie
Copy link
Member

Frenzie commented Aug 5, 2022

My guess would be I saw it in a PR on nanosvg?

@poire-z
Copy link
Contributor

poire-z commented Aug 14, 2022

Another nanosvg fork has added more support for SVG stuff, including text and tspan:
https://github.com/mtyberg/nanosvg

I tried to work with that one. Synced it with upstream, fixed a few bugs, got things that seemed to work and rendered mostly well, merged a few other PRs like support for clipPath, looked ok on some images - but not ok on others.
Once I had it working, I gave it to valgrind, which detected there were many structures not free'd.
While investigating the code, I got lost in the so many linked lists. It's quite hell.
The original nanosvg is quite limited, so it's quite clean with a limited number of such linked lists, and it's manageable. But once we try to support more stuff like group/defs/use/clippaths, you need more linked lists, some linking the same original structs, and it's a nighmare.
After contemplating the mtyberg fork for many hours, I went back to the original simpler nanosv to get the spirit of it - and it's quite nice and simple, clever with its simple linked lists when it only handle plain shapes.
But you can'd do much more than handling plain shapes with this linked list implementation...
(Also, I have no idea if mtyberg left his fork in a state he was happy with, or if he just gave up :) I assumed the former for a week, but now I'm tending for the latter :)
So, I gave up.

Anyway, lunasvg is not actively developped - and its code is libstd++ heavy - looks like another totally alien language to me than the C++ I'm used to with crengine :)

I then tried to add lunasvg to koreader-base and use it from crengine - and most of the things that didn't look right with nanosvg (groups, clipPath) did just look perfect with Lunasvg (except for the absent support for text).
Looks like it is still developped, some bugs are fixed - there's been a large refactoring in early 2021 where it was made simpler and nicer I guess, but it then lost support for text.
I looked at LunaSVG's code, and it is classes based ! and it's heaven trying to understand it. It's clean, nicely structured, except for the std:: stuff everywhere that I'm not familiar with.
But I eventually managed to understand it, and add some preliminary support for the <text> and <tspan> elements.
I had an intuition I could use the help from harfbuzz to ease this text integration, as since v4.0.0, it got a new API https://harfbuzz.github.io/harfbuzz-hb-draw.html which it uses to dump glyph shapes as SVG:
https://github.com/harfbuzz/harfbuzz/blob/4ab7e579cb8f4fd4f5ee2e1c0404e58edf8eb8e6/src/main.cc#L276-L288
I spent the day in despair trying to make the bridge beween lunasvg and crengine and have font->DrawTextString() callback into lunasvg to add a <path d="..."> for each glyph (thought I could use a classic func* for a callback, but I learned I need to use a std::function to get a closure like we are so used with Lua...)
And I finally got my PoC to work ! so sharing my undespair :)

image

image

It's quite neat that C++ and LunaSVG (with its proper architecture) allow doing that:

typedef struct {
    const char * family;
    double size;
    bool italic;
    int weight;
} external_font_spec_t;

typedef std::function<void(const char * path_d, double advance_x)> tspan_path_callback_t;

typedef void (*external_text_to_paths_func_t)( const char * text, external_font_spec_t * font_spec, tspan_path_callback_t callback );

typedef struct {
    external_text_to_paths_func_t text_to_paths_func;
    void * dummy;
} external_context_helper_t;

if ( !text.empty() && font_size > 0 ) {
    external_font_spec_t font_spec;
    std::string font_family = get(PropertyId::Font_Family);
    font_spec.family = font_family.c_str();
    font_spec.size = Parser::parseLength( find(PropertyId::Font_Size), ForbidNegativeLengths, Length::Zero).value(100.0);
    font_spec.italic = false; // XXX
    font_spec.weight = 400; // XXX

    // This will be called for each glyph
    auto add_glyph_callback = tspan_path_callback_t([&](const char * path_d, double advance_x) {
        auto path = Parser::parsePath(path_d);
        printf("adding shape n=%d advance=%g\n", path.points().size(), advance_x);
        auto shape = std::make_unique<LayoutShape>();
        shape->path = std::move(path);
        // Firefox and Edge don't handle a transform= on a <tspan>, so we can just set ours
        shape->transform = Transform::translated(cursor_x, cursor_y);
        shape->fillData = context->fillData(this);
        shape->strokeData = context->strokeData(this);
        shape->markerData = context->markerData(this, shape->path);
        shape->visibility = visibility();
        shape->clipRule = clip_rule();
        shape->opacity = opacity();
        shape->masker = context->getMasker(mask());
        shape->clipper = context->getClipper(clip_path());
        current->addChild(std::move(shape));
        cursor_x += advance_x;
    });
    context->getExternalContextHelper()->text_to_paths_func(text.c_str(), &font_spec, add_glyph_callback);
} 

So, I'm confident we can have more proper SVG support in crengine & KOReader.
I guess I'll have to add support for images via some other external crengine helper, for EPUBs that use a SVG wrapper for their cover - which are handled by a hack in crengine currently that I would have to remove to support <svg> embedded in <html>...

@poire-z
Copy link
Contributor

poire-z commented Sep 1, 2022

@Frenzie @NiLuJe
I was about to finish including LunaSVG use by crengine, to discover that the libs have grown unexpectedly :(

koreader + base + crengine current master:
 1718464 Sep  1 21:13 liblunasvg.so
21868624 Sep  1 21:19 libcrengine.so
    4096 Sep  1 21:19 .
  955528 Sep  1 21:19 libkoreader-cre.so
stripped:
  141112 Sep  1 21:24 libkoreader-cre.so
 4587904 Sep  1 21:24 libcrengine.so
 1718464 Sep  1 21:24 liblunasvg.so

koreader+base with the patch below, but crengine & cre.cpp master (so, without any Lunasvg use):
stripped:
 1430272 Sep  1 21:31 libkoreader-cre.so          <--------- !!!!
 3212400 Sep  1 21:31 libcrengine.so
 1718464 Sep  1 21:31 liblunasvg.so

readelf on libkoreader-cre.so shows a lot of stdc stuff (including some moneypunct stuff!).
Any idea what is happening ?

Here's enough to make this happen (copy & pasted from other section, I have really not much idea what all this does :)

diff --git a/Makefile.defs b/Makefile.defs
index 6031fc1..dcde160 100644
--- a/Makefile.defs
+++ b/Makefile.defs
@@ -796,2 +796,7 @@ LIBWEBP_DIR=$(CURDIR)/$(LIBWEBP_BUILD_DIR)/libwebp-prefix/src/libwebp-build

+LUNASVG_LIB_EXT=$(if $(WIN32),.dll,$(if $(DARWIN),.dylib,.so))
+LUNASVG_LIB=$(OUTPUT_DIR)/libs/liblunasvg$(LUNASVG_LIB_EXT)
+LUNASVG_BUILD_DIR=$(THIRDPARTY_DIR)/lunasvg/build/$(MACHINE)
+LUNASVG_DIR=$(CURDIR)/$(LUNASVG_BUILD_DIR)/lunasvg-prefix/src/lunasvg-build
+
 LIBFFI_BUILD_DIR=$(THIRDPARTY_DIR)/libffi/build/$(MACHINE)
@@ -976,3 +981,3 @@ CMAKE_THIRDPARTY_LIBS := $(CMAKE_THIRDPARTY_LIBS),lj-wpaclient
 CMAKE_THIRDPARTY_LIBS := $(CMAKE_THIRDPARTY_LIBS),lua-htmlparser,lua-rapidjson,lua-Spore,luasec,luasocket,libffi,glib,lodepng,minizip
-CMAKE_THIRDPARTY_LIBS := $(CMAKE_THIRDPARTY_LIBS),djvulibre,mupdf,freetype2,harfbuzz,giflib,libpng,zlib,libwebp,libwebpdemux
+CMAKE_THIRDPARTY_LIBS := $(CMAKE_THIRDPARTY_LIBS),djvulibre,mupdf,freetype2,harfbuzz,giflib,libpng,zlib,libwebp,lunasvg
 CMAKE_THIRDPARTY_LIBS := $(CMAKE_THIRDPARTY_LIBS),tar,sdcv,libiconv,gettext,libjpeg-turbo,popen-noshell,sqlite
diff --git a/Makefile.third b/Makefile.third
index e0548a8..f389726 100644
--- a/Makefile.third
+++ b/Makefile.third
@@ -176,2 +176,16 @@ $(LIBWEBP_LIB) $(LIBWEBP_DIR): $(THIRDPARTY_DIR)/libwebp/*.*

+$(LUNASVG_LIB): $(THIRDPARTY_DIR)/lunasvg/*.*
+       install -d $(LUNASVG_BUILD_DIR)
+       cd $(LUNASVG_BUILD_DIR) && \
+               $(CMAKE) $(CMAKE_FLAGS) \
+               $(CURDIR)/$(THIRDPARTY_DIR)/lunasvg && \
+               $(CMAKE_MAKE_PROGRAM) $(CMAKE_MAKE_PROGRAM_FLAGS)
+       cp -fL $(LUNASVG_DIR)/$(if $(WIN32),,lib)/$(notdir $(LUNASVG_LIB)) $(LUNASVG_LIB)
+       # cp -fL $(THIRDPARTY_DIR)/lunasvg/stb_image_write.h $(LUNASVG_DIR)/include
+ifdef DARWIN
+       install_name_tool -id \
+               libs/$(notdir $(LUNASVG_LIB)) \
+               $(LUNASVG_LIB)
+endif
+
 $(DJVULIBRE_LIB): $(JPEG_LIB) $(THIRDPARTY_DIR)/djvulibre/*.*
@@ -194,3 +208,3 @@ endif
 $(CRENGINE_LIB): $(ZLIB) $(ZSTD_LIB) $(PNG_LIB) $(FREETYPE_LIB) $(HARFBUZZ_LIB) $(FRIBIDI_LIB) \
-               $(LIBUNIBREAK_LIB) $(UTF8PROC_LIB) $(JPEG_LIB) $(NANOSVG_HEADERS) $(LIBWEBP_LIB) \
+               $(LIBUNIBREAK_LIB) $(UTF8PROC_LIB) $(JPEG_LIB) $(LIBWEBP_LIB) $(LUNASVG_LIB) \
                $(CRENGINE_SRC_FILES) $(THIRDPARTY_DIR)/kpvcrlib/*.*
@@ -205,2 +219,3 @@ $(CRENGINE_LIB): $(ZLIB) $(ZSTD_LIB) $(PNG_LIB) $(FREETYPE_LIB) $(HARFBUZZ_LIB)
                LIBWEBPDEMUX_LIB="$(CURDIR)/$(LIBWEBPDEMUX_LIB)" \
+               LUNASVG_LIB="$(CURDIR)/$(LUNASVG_LIB)" \
                LIBUNIBREAK_LIB="$(CURDIR)/$(LIBUNIBREAK_LIB)" \
@@ -214,3 +229,3 @@ $(CRENGINE_LIB): $(ZLIB) $(ZSTD_LIB) $(PNG_LIB) $(FREETYPE_LIB) $(HARFBUZZ_LIB)
                -DPNG_INCLUDE_DIR="$(PNG_DIR)/include" \
-               -DNANOSVG_INCLUDE_DIR="$(NANOSVG_INCLUDE_DIR)" \
+               -DLUNASVG_INCLUDE_DIR="$(LUNASVG_DIR)/include" \
                -DZLIB_INCLUDE_DIR="$(ZLIB_DIR)/include" \
diff --git a/cre.cpp b/cre.cpp
index a11e7ff..f59b43f 100644
--- a/cre.cpp
+++ b/cre.cpp
@@ -3696,2 +3696,3 @@ static int getImageDataFromPosition(lua_State *L) {
                 unsigned char *cbuf = (unsigned char*) buffer; // cast same void pointer to char pointer
+                /*
                 // if buffer starts with <?xml or <svg, it's probably SVG
@@ -3712,2 +3713,3 @@ static int getImageDataFromPosition(lua_State *L) {
                 }
+                */
                 lua_pushlightuserdata(L, buffer);
diff --git a/thirdparty/kpvcrlib/CMakeLists.txt b/thirdparty/kpvcrlib/CMakeLists.txt
index 29cb6fe..d042175 100644
--- a/thirdparty/kpvcrlib/CMakeLists.txt
+++ b/thirdparty/kpvcrlib/CMakeLists.txt
@@ -32,3 +32,3 @@ assert_var_defined(JPEGLIB_INCLUDE_DIR)
 assert_var_defined(LIBWEBP_INCLUDE_DIR)
-assert_var_defined(NANOSVG_INCLUDE_DIR)
+assert_var_defined(LUNASVG_INCLUDE_DIR)
 assert_var_defined(ZSTD_INCLUDE_DIR)
@@ -48,3 +48,3 @@ include_directories(
     ${LIBWEBP_INCLUDE_DIR}
-    ${NANOSVG_INCLUDE_DIR}
+    ${LUNASVG_INCLUDE_DIR}
     ${ZSTD_INCLUDE_DIR}
@@ -82,3 +82,4 @@ add_definitions(
     -DUSE_UTF8PROC=1
-    -DUSE_NANOSVG=1
+    -DUSE_NANOSVG=0 # better SVG support via LunaSVG
+    -DUSE_LUNASVG=1
     -DUSE_LIBWEBP=1
@@ -112,5 +113,6 @@ if(DEFINED ENV{WIN32})
     find_library(JPEG_LIB NAMES "jpeg-8" PATHS $ENV{LIBS_DIR})
+    find_library(LUNASVG_LIB NAMES "lunasvg" PATHS $ENV{LIBS_DIR})
     find_library(LIBWEBP_LIB NAMES "webp-7" PATHS $ENV{LIBS_DIR})
     find_library(LIBWEBPDEMUX_LIB NAMES "webpdemux-2" PATHS $ENV{LIBS_DIR})
-    set(THIRDPARTY_LIBS ${ZLIB} ${ZSTD} ${FREETYPE_LIB} ${HARFBUZZ_LIB} ${FRIBIDI_LIB} ${LIBUNIBREAK_LIB} ${UTF8PROC_LIB} ${JPEG_LIB} ${PNG_LIB} ${LIBWEBP_LIB} ${LIBWEBPDEMUX_LIB})
+    set(THIRDPARTY_LIBS ${ZLIB} ${ZSTD} ${FREETYPE_LIB} ${HARFBUZZ_LIB} ${FRIBIDI_LIB} ${LIBUNIBREAK_LIB} ${UTF8PROC_LIB} ${JPEG_LIB} ${PNG_LIB} ${LIBWEBP_LIB} ${LIBWEBPDEMUX_LIB} ${LUNASVG_LIB})
 else()
@@ -134,2 +136,4 @@ else()
     set_target_properties(LIBWEBPDEMUX_LIB PROPERTIES IMPORTED_LOCATION $ENV{LIBWEBPDEMUX_LIB})
+    add_library(LUNASVG_LIB SHARED IMPORTED)
+    set_target_properties(LUNASVG_LIB PROPERTIES IMPORTED_LOCATION $ENV{LUNASVG_LIB})
     add_library(ZLIB SHARED IMPORTED)
@@ -138,3 +142,3 @@ else()
     set_target_properties(ZSTD PROPERTIES IMPORTED_LOCATION $ENV{ZSTD_LIB})
-    set(THIRDPARTY_LIBS ZLIB ZSTD FREETYPE_LIB HARFBUZZ_LIB FRIBIDI_LIB LIBUNIBREAK_LIB UTF8PROC_LIB JPEG_LIB PNG_LIB LIBWEBP_LIB LIBWEBPDEMUX_LIB)
+    set(THIRDPARTY_LIBS ZLIB ZSTD FREETYPE_LIB HARFBUZZ_LIB FRIBIDI_LIB LIBUNIBREAK_LIB UTF8PROC_LIB JPEG_LIB PNG_LIB LIBWEBP_LIB LIBWEBPDEMUX_LIB LUNASVG_LIB)
 endif()

(Even if USE_LUNASVG=1, nothing checks it - there is no #include lunasvg.h anywhere.
Also, there nanosvg is disabled, which may explain the size reduction of libcrengine.so, although that much is strange...)

and

$ cat base/thirdparty/lunasvg/CMakeLists.txt

project(lunasvg)
cmake_minimum_required(VERSION 3.5.1)

set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_LIST_DIR}/../cmake_modules")
include("koreader_thirdparty_common")
include("koreader_thirdparty_git")

enable_language(C CXX)

ep_get_source_dir(SOURCE_DIR)
ep_get_binary_dir(BINARY_DIR)

# CMake hell.
# We expect lib later on in Makefile.third, even on multilib systems...
list(APPEND CMAKE_ARGS "-DCMAKE_INSTALL_PREFIX=${BINARY_DIR}")
list(APPEND CMAKE_ARGS "-DCMAKE_INSTALL_LIBDIR=lib")
list(APPEND CMAKE_ARGS "-DCMAKE_INSTALL_INCLUDEDIR=include")
# list(APPEND CMAKE_ARGS "-DCMAKE_SKIP_BUILD_RPATH:BOOL=True")

# c.f., https://android.googlesource.com/platform/ndk/+/master/build/cmake/android.toolchain.cmake
# and   https://github.com/taka-no-me/android-cmake
# In the meantime, I'll be sitting in the corner, crying hysterically.
if(DEFINED ENV{ANDROID})
    list(APPEND CMAKE_ARGS "-DCMAKE_SYSTEM_NAME=Linux")
    # Magical value that inhibits all of CMake's own NDK handling code. (Or shit goes boom.)
    list(APPEND CMAKE_ARGS "-DCMAKE_SYSTEM_VERSION=1")
endif()

list(APPEND CMAKE_ARGS "-DBUILD_SHARED_LIBS=ON")

set(PATCH_CMD1 "${KO_PATCH} ${CMAKE_CURRENT_SOURCE_DIR}/extended.patch")
set(PATCH_CMD2 sh -c "cp -rp -v ${CMAKE_CURRENT_SOURCE_DIR}/xtended ${SOURCE_DIR}/")

ko_write_gitclone_script(
    GIT_CLONE_SCRIPT_FILENAME
    https://github.com/sammycage/lunasvg.git
    637121f89218544ec2f154ae6949ab0ea9b47898
    ${SOURCE_DIR}
)

include(ExternalProject)
ExternalProject_Add(
    lunasvg
    DOWNLOAD_COMMAND ${CMAKE_COMMAND} -P ${GIT_CLONE_SCRIPT_FILENAME}
    PATCH_COMMAND COMMAND ${PATCH_CMD1} COMMAND ${PATCH_CMD2}
    CMAKE_ARGS "${CMAKE_ARGS}"
    CMAKE_GENERATOR "Unix Makefiles"
    BUILD_COMMAND ${KO_MAKE_RECURSIVE} -j${PARALLEL_JOBS}
    INSTALL_COMMAND ${KO_MAKE_RECURSIVE} -j${PARALLEL_JOBS} install
)

(You can remove the 2 PATCH_COMMAND if you wanna try it, they don't do/use anything special not already used in lunasvg...)

Moreover, compiling for my Kobo (with a gcc-8.3 from an older debian, I had bad luck with current debian gcc-10) I also get such big sized libs - and moreover, I get a segfault when I open an epub... (and no gdb on my Kobo).
In case it would matter, I get this kind of warnings when compiling lunasvg for arm:

[ 12%] Building CXX object CMakeFiles/lunasvg.dir/source/parser.cpp.o
In file included from gnu_arm-linux-gnueabihf/usr/arm-linux-gnueabihf/include/c++/8/vector:69,
                 from koreader/base/thirdparty/lunasvg/build/arm-linux-gnueabihf/lunasvg-prefix/src/lunasvg/source/property.h:4,
                 from koreader/base/thirdparty/lunasvg/build/arm-linux-gnueabihf/lunasvg-prefix/src/lunasvg/source/property.cpp:1:
gnu_arm-linux-gnueabihf/usr/arm-linux-gnueabihf/include/c++/8/bits/vector.tcc: In member function 'void std::vector<_Tp, _Alloc>::_M_realloc_insert(std::vector<_Tp, _Alloc>::iterator, _Args&& ...) [with _Args = {double&, double&}; _Tp = lunasvg::Point; _Alloc = std::allocator<lunasvg::Point>]':
gnu_arm-linux-gnueabihf/usr/arm-linux-gnueabihf/include/c++/8/bits/vector.tcc:413:7: note: parameter passing for argument of type 'std::vector<lunasvg::Point>::iterator' {aka '__gnu_cxx::__normal_iterator<lunasvg::Point*, std::vector<lunasvg::Point> >'} changed in GCC 7.1
       vector<_Tp, _Alloc>::
       ^~~~~~~~~~~~~~~~~~~
gnu_arm-linux-gnueabihf/usr/arm-linux-gnueabihf/include/c++/8/bits/vector.tcc: In member function 'void std::vector<_Tp, _Alloc>::emplace_back(_Args&& ...) [with _Args = {double&, double&}; _Tp = lunasvg::Point; _Alloc = std::allocator<lunasvg::Point>]':
gnu_arm-linux-gnueabihf/usr/arm-linux-gnueabihf/include/c++/8/bits/vector.tcc:109:4: note: parameter passing for argument of type '__gnu_cxx::__normal_iterator<lunasvg::Point*, std::vector<lunasvg::Point> >' changed in GCC 7.1
    _M_realloc_insert(end(), std::forward<_Args>(__args)...);
    ^~~~~~~~~~~~~~~~~

Any thought (and solution) ? :)

(In spite of the libs sizes, this works wonders on the emulator...)

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

Yeah, Luna is C++, and we build with -static-libstdc++, so it sorta makes sense.

One possibly saner approach (especially vis-à-vis the ODR) would be to NOT do that, and instead ship the TC's shared libstdc++. The bloat would be limited to the actual STL lib (it's 1.4M over here), and we'd actually be doing C++ properly ;p (e.g., I'm pretty sure what we're currently doing happens to be working by pure blind luck, I fully expect this to horribly crash on a libc++ based TC, for instance).

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

As for the build error, I can't remember how/when we enforce a default minimum C++ standard version, which sorta coulda explain this, but you'll have better luck just using the koxtoolchain TC ;o).

(I can't easily test debian TCs anyway).

@poire-z
Copy link
Contributor

poire-z commented Sep 1, 2022

ODR ?

I think there are a few bits of std:: stuff in some of the libs, and I think just the "needed bits" from std get included.

I'd be fine with any std bits getting into liblunasvg.so - and possibly some bits in libcrengine.so.
But I don't understand why things get put into libkoreader-cre - when libcrengine is not even including anything liblunasvg.
And I made sure to make the cre.cpp change not depending on any std:: stuff - it's supposed to just call libcrengine - which may interact with lunasvg.

So, I think it may be CMake doing too much. No ?

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

The One Definition Rule ;). (In practical terms: -static-libstdc++ is a terrible idea that *will * horribly crash as soon as two C++ apps built that way interact).

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

We're passing the actual CRe lib to the CRe Lua module, which is probably how the bloat gets introduced ;).

(I'd need a full build log to confirm; if we're passing -lcrengine, we're doing it right; if we're passing the absolute path to the actual lib: we're doing it wrong).

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

I'll see if I can throw a PoC about the stdc++ stuff tomorrow (because it is going to blow up in our faces one day regardless), I'll double-check how the Lua module is built in the process ;).

@poire-z
Copy link
Contributor

poire-z commented Sep 1, 2022

g++ -I/koreader/base/thirdparty/kpvcrlib/crengine/crengine/include/ -L/koreader/base/build/x86_64-linux-gnu-debug/libs -Og -g -pipe -fno-omit-frame-pointer   -march=native -fPIC -I/koreader/base/thirdparty/luajit/build/x86_64-linux-gnu-debug/luajit-prefix/src/luajit/src -shared -Wl,-E -Wl,-rpath,'$ORIGIN' \
    -DLDOM_USE_OWN_MEM_MAN=1 -DUSE_SRELL_REGEX=1 \
         -fvisibility=hidden -static-libstdc++ -o build/x86_64-linux-gnu-debug/libs/libkoreader-cre.so cre.cpp build/x86_64-linux-gnu-debug/libs/libcrengine.so

I guess both lunasvg and libcrengine will need the same std:: bits to be able to talk via std::unique_ptr and std::function :/
But anyway, all this work on the emulator/x86_64.

@poire-z
Copy link
Contributor

poire-z commented Sep 1, 2022

Actually, just removing -static-libstdc++ from the libkoreader-cre.so build make it small again :) and it still works on the emulator:

 $(OUTPUT_DIR)/libs/libkoreader-cre.so: cre.cpp \
                        $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),) \
                        $(CRENGINE_LIB)
        $(CXX) -I$(CRENGINE_SRC_DIR)/crengine/include/ $(DYNLIB_CXXFLAGS) \
                -DLDOM_USE_OWN_MEM_MAN=$(if $(WIN32),0,1) -DUSE_SRELL_REGEX=1 \
-               $(if $(WIN32),-DQT_GL=1) $(SYMVIS_FLAGS) -static-libstdc++ -o $@ $^
+               $(if $(WIN32),-DQT_GL=1) $(SYMVIS_FLAGS) -o $@ $^

But I guess it won't find that on our Kobo:
libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

Okay, so we're doing it wrong, yay.

And, yeah, the emulator doesn't need -static-libstdc++ (and, in fact, shouldn't be using it), but we do almost everywhere else (unless we start shipping the TC's STL).

I'll look into it tomorrow ;).

@poire-z
Copy link
Contributor

poire-z commented Sep 1, 2022

-               $(if $(WIN32),-DQT_GL=1) $(SYMVIS_FLAGS) -static-libstdc++ -o $@ $^
+               $(if $(WIN32),-DQT_GL=1) $(SYMVIS_FLAGS) $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),) -o $@ cre.cpp -lcrengine

doesn't change much: small and working on the emulator - not small and crashing on my kobo.

@NiLuJe
Copy link
Member

NiLuJe commented Sep 1, 2022

(It does weird shit on some binutils version, and it's just not The Right Way(TM) to do things ;)).

@Frenzie
Copy link
Member

Frenzie commented Sep 2, 2022

Get some sleep guys. :-P

@poire-z
Copy link
Contributor

poire-z commented Sep 2, 2022

Got some sleep, but nothing more :)

I still don't understand, in spite of everything @NiLuJe wrote, why the linker would decide NOW to include moneypunct stuff from stdc into libkoreader-cre.so when nothing even uses lunasvg.
LunaSVG is just mentionned in thirdparty/kpvcrlib/CMakeLists.txt - and somehow, this is enough to make that happen.
I tried variations aound:

-target_link_libraries(crengine chmlib antiword ${THIRDPARTY_LIBS})
+# target_link_libraries(crengine chmlib antiword ${THIRDPARTY_LIBS})
+# target_link_libraries(crengine PRIVATE -lstdc++ chmlib antiword ${THIRDPARTY_LIBS})
+target_link_libraries(crengine PRIVATE -static-libstdc++ chmlib antiword ${THIRDPARTY_LIBS})

and for the non-CMake build of libkoreader-cre:

-       $(CXX) -I$(CRENGINE_SRC_DIR)/crengine/include/ $(DYNLIB_CXXFLAGS) \
+       $(CXX) -I$(CRENGINE_SRC_DIR)/crengine/include/ $(DYNLIB_CXXFLAGS) -nodefaultlibs -fno-exceptions \
                -DLDOM_USE_OWN_MEM_MAN=$(if $(WIN32),0,1) -DUSE_SRELL_REGEX=1 \
-               $(if $(WIN32),-DQT_GL=1) $(SYMVIS_FLAGS) -static-libstdc++ -o $@ $^
+               $(if $(WIN32),-DQT_GL=1) $(SYMVIS_FLAGS) -static-libstdc++ -lgcc -o $@ $^

(even putting the full resulting command line with the multiple -static-libstdc++ removed)
and most of these variations work on the emulator, and get a small libkoreader-cre.so (readelf syaing it only NEEDED libcrengine.so, or libcrengine.so + a few other libm.so, libgcc...)
But it still segfault on my Kobo...

@NiLuJe
Copy link
Member

NiLuJe commented Sep 2, 2022

I got sidetracked a bit because I decided to reinstall my phone yesterday (EoL'ed by the vendor -> Lineage ;)), but I'll be starting on that now :).

I can't quite answer why which bits of the STL get pulled or not, but the moneypunct stuff very vaguely rings a bell somehow, but I can't quite put my finger on it yet :}.

As for the crash, it's probably because of -static-libstdc++ breaking the ODR.

@poire-z
Copy link
Contributor

poire-z commented Sep 2, 2022

Thanks !
(Following your progress at https://github.com/NiLuJe/koreader-base/commits/ship_shared_stl)
Can you upload here your libstdc++.so.6 , so I have it tomorrow if I wanna try more ?
Among the various things I tried this morning, I copied the libstdc++.so.6 from my old debian arm toolchain to my kobo, and got this happening:

error loading module 'libs/libkoreader-cre' from file './libs/libkoreader-cre.so':
    /lib/libc.so.6: version `GLIBC_2.18' not found (required by /mnt/onboard/.adds/koreader/libs/libstdc++.so.6)

So, I guess I need a one that will work with the Kobo firmware (including old ones like fw3.19 ?), and I guess that's what your toolchains are all about :)
(I hope I can still compile libcrengine/liblunasvg with that old debian arm stuff and have it work with your libstdc++.so.6 ... I really don't want to install that koxtoolchain monstruosity :) last time I thought I would have to make that jump, I realized I wouldn't even have enough disk space :)

@NiLuJe
Copy link
Member

NiLuJe commented Sep 2, 2022

Nope, cross-TC (most likely) won't work ;).

FWIW: 252M /home/niluje/x-tools/arm-kobo-linux-gnueabihf


I've got something that builds (and generates smaller binaries, because linker magic).

I've... left the actual CRe build itself alone, because it does something similar (by which I mean passing absolute paths to shared libraries to the final link stage), but it does so via CMake add_library crap, so fuck that ;o).

The leptonica tools also appear to do something similar for libpng, but, again, fuck CMake ;).

@NiLuJe
Copy link
Member

NiLuJe commented Sep 2, 2022

That said, you can still try ;).

You can pull the lib from the prebuilt in https://github.com/koreader/koxtoolchain/releases/tag/2021.12 (it'll be @ arm-kobo-linux-gnueabihf/arm-kobo-linux-gnueabihf/sysroot/lib/libstdc++.so.6)

@poire-z
Copy link
Contributor

poire-z commented Sep 2, 2022

I've... left the actual CRe build itself alone

Might be just a matter of adding -lstdc++ to the last line: ?
target_link_libraries(crengine PRIVATE -lstdc++ chmlib antiword ${THIRDPARTY_LIBS})

FWIW: 252M /home/niluje/x-tools/arm-kobo-linux-gnueabihf

I'll be happy to get that final binary build :) It's building the whole thing that required GBs.

Oh, I didn't even know you have made prebuilt binaries ! I will pick one if need arises !

@NiLuJe
Copy link
Member

NiLuJe commented Sep 2, 2022

I've... left the actual CRe build itself alone

Might be just a matter of adding -lstdc++ to the last line: ? target_link_libraries(crengine PRIVATE -lstdc++ chmlib antiword ${THIRDPARTY_LIBS})

Oh, no, I was talking of my crusade against absolute paths, the STL stuff is unnecessary, it'll be picked up automatically ;).

Oh, I didn't even know you have made prebuilt binaries ! I will pick one if need arises !

That's thanks to @yparitcher ;).


Note that, if you can actually still build stuff with your shady TCs, you just need to ship that STL (which is essentially what I'm doing in the branch, the actual lib is pulled from the TC, it's not hard-coded).

@NiLuJe
Copy link
Member

NiLuJe commented Sep 2, 2022

Appears to behave on my end, so I'll PR all that to watch how massively it broke stuff on Android :D.

┌─(ROOT@europa:pts/0)──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────(~)─┐
└─(2.11:20%:01:03:90%:#)── cat /proc/6724/maps | ag stdc++                                                                                                                                                                                                                                                                                             ──(Sat, Sep 03)─┘
b6015000-b6160000 r-xp 00000000 b3:03 80668      /mnt/onboard/.adds/koreader/libs/libstdc++.so.6
b6160000-b6170000 ---p 0014b000 b3:03 80668      /mnt/onboard/.adds/koreader/libs/libstdc++.so.6
b6170000-b6175000 r--p 0014b000 b3:03 80668      /mnt/onboard/.adds/koreader/libs/libstdc++.so.6
b6175000-b6177000 rw-p 00150000 b3:03 80668      /mnt/onboard/.adds/koreader/libs/libstdc++.so.6

@NiLuJe
Copy link
Member

NiLuJe commented Sep 2, 2022

It's building the whole thing that required GBs.

LifeHack: I build 'em in a tmpfs ;).

@poire-z
Copy link
Contributor

poire-z commented Sep 3, 2022

Reporting success !

I have just rebuilld the things I have worked on (I have some scripts to just "make mycrengine" and various subsections of our main Makefile), on the emulator and for kobo, no issue.
For kobo, with your libstdc++.so.6 and still with my old debian arm toolchain: no problem (went first look for filenames in Arabic to validate xtext, as cre was still crashing).

The sizes on Kobo - all the expected small/smaller sizes:

   71816 Aug 18 08:39 libkoreader-xtext.so.ORIG     there are from a nightly
   85320 Aug 18 08:39 libkoreader-cre.so.ORIG
 3297536 Aug 18 08:39 libcrengine.so.ORIG

 1385008 Sep  3 10:19 libstdc++.so.6
   38648 Sep  3 10:28 libkoreader-xtext.so
   93556 Sep  3 12:20 libkoreader-cre.so
 2299384 Sep  3 13:29 libcrengine.so
  254128 Sep  3 12:20 liblunasvg.so

and they all get libstdc++.so.6 in their ldd output.
(I always got libcrengine.so smaller with my toolchain than what's in our nightly, dunno why, we'll have to see their size on some future nightly.)

I still got the segfault on Kobo, but i figured it was happening in cre.initCache(...).
The reason was that I have tweaked the SerialBuf stuff and its serialization of string: it was storing the string size in a lUInt16, which limited the length of such strings to 65535. We store HTML attributes values via this in the cache, and with <img src="data:image/png;base64,321321312">, such src attributes can be longer than that (so, images worked, but when reopening the book from cache, images were unserialized and the attribute value truncated, and didn't work anymore). So, I made the string size stored in a lUInt32.
But, the bit that reads cr3cache/cr3cache.inx also stores filenames in the cache that way, and it was crashing because of this lUInt16/lUIn32 change.
I'll solve this with:

-static const char * doccache_magic = "CoolReader3 Document Cache Directory Index\nV1.00\n";
+static const char * doccache_magic = "CoolReader3 Document Cache Directory Index\nV1.01\n";

and hopefully, this won't happen anywhere else :|
So, I think this segfault has nothing to do with libstd or the ODR.

After that (or before, I was running it without the cache), all my SVG stuff works nicely, and loading and drawing huge SVGs is not excessively slow on my old Kobo.

Thank you a lot for having taken the time to clean that libstdc++ mess ! :)

Nothing was needed in thirdparty/kpvcrlib/CMakeLists.txt, but don't you think it would be saner to use PRIVATE (if that actually does something, because it didn't change anything in my previous experiments):

- target_link_libraries(crengine chmlib antiword ${THIRDPARTY_LIBS})
+ target_link_libraries(crengine PRIVATE chmlib antiword ${THIRDPARTY_LIBS})

In the lunasvg build with:

$(LUNASVG_LIB): $(THIRDPARTY_DIR)/lunasvg/*.*
        install -d $(LUNASVG_BUILD_DIR)
        cd $(LUNASVG_BUILD_DIR) && \
                $(CMAKE) $(CMAKE_FLAGS) -DCC="$(CC)" -DCXX="$(CXX)" -DCFLAGS="$(CFLAGS)" \
                -DCXXFLAGS="$(CXXFLAGS)" -DLDFLAGS="$(LDFLAGS)" -DCHOST=$(CHOST) \
                -DCPPFLAGS="$(CPPFLAGS)" \
                $(CURDIR)/$(THIRDPARTY_DIR)/lunasvg && \
                $(CMAKE_MAKE_PROGRAM) $(CMAKE_MAKE_PROGRAM_FLAGS)
        cp -fL $(LUNASVG_DIR)/$(if $(WIN32),,lib)/$(notdir $(LUNASVG_LIB)) $(LUNASVG_LIB)
        # cp -fL $(THIRDPARTY_DIR)/lunasvg/stb_image_write.h $(LUNASVG_DIR)/include
ifdef DARWIN
        install_name_tool -id \
                libs/$(notdir $(LUNASVG_LIB)) \
                $(LUNASVG_LIB)
endif

and the thirdparty/lunasvg/CMakeLists.txt I pasted in #9298 (comment)
I get:

  Manually-specified variables were not used by the project:

    CC
    CFLAGS
    CHOST
    CPPFLAGS
    CXXFLAGS
    LDFLAGS

CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_INSTALL_INCLUDEDIR
    CMAKE_INSTALL_LIBDIR

Anything special to do about that, and what (remove them in Makefile.third, or use them in lunasvg/CMakeLists.txt) ?

@NiLuJe
Copy link
Member

NiLuJe commented Sep 3, 2022

Nothing was needed in thirdparty/kpvcrlib/CMakeLists.txt, but don't you think it would be saner to use PRIVATE (if that actually does something, because it didn't change anything in my previous experiments)

IIRC, those are built as static libraries anyway, so, yeah, definitely makes sense, although it might not make an actual difference in this case :}.

Anything special to do about that, and what (remove them in Makefile.third, or use them in lunasvg/CMakeLists.txt) ?

It's CMake all the way down, so, yeah, you can drop the extra stuff from both Makefile.third & our lunasvg CMakeLists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants