Allow drawing images inverted + Some CSS fixes #276
Conversation
Will be requested when in night mode: inverted images will then get inverted back by night mode and displayed normally.
By default, don't adjust returned Rect for glyphs left or right side bearing, as this could cause have some side effects (ex: when checking for link coherency). Only use it in getSegmentRects() when we really want finely adjusted rects for highlighting.
E F (descendant combinator) and E ~ F (general sibling combinator) had a limited implementation, that could miss some matches. This implements the correct logic: check the multiple possible paths, possibly all, until a match is found. (This may be expensive on books with lots of such selectors, and may increase the loading and re-rendering times.)
|
A quick note related to the book mentionned at https://www.mobileread.com/forums/showthread.php?t=321683 (investigated there by @NiLuJe :). Loading time takes 27s on my emulator (far less powerful than @NiLuJe 's desktop :) - but only 14s if I disable embedded styles. (My reference book for timing is also a Bible (french TOB, with a quite simple stylesheet) which loads in 17s on the emulator and 80s on my Kobo - so this book should indeed load in 2-3 minutes on a device - no idea why it would not...) There's another strange thing with this book: when you change some rendering parameters (font size, margins), it triggers a |
|
Note that I was also cheating a bit by using a release build, and I think I remember you mentioning those timings were from debug builds ;). And it was also run from a tmpfs, so, very very fast I/O, too ;). That said, it does indeed fall squarely on the 2 min mark on my H2O (unplugged, which might be a significant factor on the H2O with its wonky CPUFreq driver)). I assume that that popup showing up means that closing/re-opening the book will indeed trigger a re-render? |
|
It took under 6 seconds on my debug build, so it'd definitely take a minute on my H2O. |
Yes (*): if you don't answer OK to the popup, it will happen on next load (in spite of the cache, considered invalid). *: but more than a re-render: a re-loading, re parsing the EPUB's HTML to build a new DOM. crengine/crengine/src/lvtinydom.cpp Lines 10943 to 10949 in 3433c07 crengine/crengine/src/lvtinydom.cpp Lines 10964 to 10972 in 3433c07 and when this hash changes, you get that popup. When only other CSS properties have changed (e.g. by style tweaks), the DOM is still valid, so no "loading" needed - just a re-rendering, which works on the current and cached DOM, so it just drops all nodes' styles and recompute them, and re-layout boxes and text. Note that on a first load/no cache, a good amount of the styles work is done in the "loading" phase, so the "rendering" phase is quicker than the next re-renderings. Initial loading with no cache: 17s (loading/parsing HTML + computing styles) + 4s (rendering boxes and text) Changing font size: 13s (computing styles + rendering boxes and text) |
|
Gotcha! Thanks for the detailed walk-through ;). |
|
OK, the popup was triggered because of docclass117.css having: Issue can be triggered with this simple document: <?xml version="1.0" encoding="utf-8" ?>
<html>
<head>
<style>
th:last-child, td:last-child {display: none; }
</style>
</head>
<body>
<div>Start of text.</div>
<table>
<tr>
<th>abc</th>
<th>def</th>
</tr>
<tr>
<td>abc</td>
<td>def</td>
</tr>
</table>
<div>End of text.</div>
</body>
</html>On initial loading, no table is displayed (wrong). |
:( Dunno if that's solvable (except by just not computing styles in the loading phase... and delay it to the rendering phase, which I had occasionally though about to solve some bugs, but always ended up not having to, which I've kinda been proud of :) |
|
Or just set a flag somewhere when one of these is met, so we just consider the styles made on loading invalid, and immediately go on with a re-rendering (like we may already do in some cases when there are embedded fonts). Books with these will load slower, but those without will be fine. |
|
The flag idea sounds nifty. :-) |
|
I quickly crafted a fix, but it still needs a bit of work, and as I'm off soon, it will have to wait a few days. Regarding the italian version given on the forum, it was still loading after a few minutes, parsing and checking CSS rules each time I gdb ctrl-z it. |
|
It loads, but it took about 15 minutes. I don't know what's so different about that Italian one… or maybe "only" about 7-8 + another 7-8 for the new Italian hyphenation? https://www.mobileread.com/forums/showthread.php?p=3870310#post3870310 |
I don't think the hyphenation is the cause, rather some embedded font specified in CSS (even if there is no font file in the EPUB...): The italian one has the same number of xhtml files (~4000) as the english one, but the CSS in the italian one are horrible: 3 huge files, that must be included by each HTML files (the english one has more, but smaller CSS files, and each xhtml file only includes 2 or 3 small ones). May be there are some tools to clean CSS files and keep only the selectors that are matched, or merge them. That book could benefit from some cleanup :| May be just remove 1 or 2 of the biggest CSS Files as a first try... The answer as to why other readers open it is that may be they don't render the whole book, or their styles check&apply code is better than crengine's one. @Frenzie: I'd like a qcachegrind output on the english one if you can provide it - just to see if something jumps out, although I know the ancestor selectors are doomed to be slow, and there's not much obvious to be done, except discarding them (as it was done before I fixed them :| ). |
There are some tools that can help clean up styles a little, e.g. see here. I suppose they should work well enough on an extracted EPUB, or maybe Sigil even integrates something like it.
|
|
Thanks. Lines 149 to 156 in 3433c07 But if I delete these, load & render time doesn't change (30s with or without)... Otherwise, nothing much strange. |
|
But the self value of lowercase is a mere 0.01. It looks to me like the unexpected heavy hitter is TrimDoubleSpaces()?
|
Well, I don't see the same things as you do :) I use some portable Qcachegrind. You don't see that big block on the left I pasted above?
I don't see that either :) Anyway, it's in LVCssDeclaration:parse(), which shouldn't be hit much. |
Sure, if I enable the callee map. :-)
|
|
Just some quick timing notes on the emulator about that italian one: I made 2 of the 3 huge CSS empty, keeping only the smaller one (80Kb). No real clue about how to speed that up, but it's probably in lvstsheet.cpp that stuff might be optimized. (According to https://www.mobileread.com/forums/showthread.php?p=3864541#post3864541, many reading apps may not support the expensive CSS combinators as much as we do.) |
Nice work @poire-z ;-)
Maybe there's some stuff that could be cached that isn't? I dunno, I think the only real way to open ridiculously large documents fast is to do only one XHTML file at a time (cheating, so to speak). I imagine facilitating such behavior to be part of the reason behind the various individual chapter files in the first place. Also I just finally looked at that CSS and HOLY @@#$ what is that thing?! Those styles are just ridiculously complex. O_o I assume a preprocessor was involved but still… Also they contain some doozies: inline-block doesn't combine with float:right. Stuff like this makes a good testcase for how we handle crappy CSS. ;-) |
|
Reminding @Frenzie we did investigate that https://www.mobileread.com/forums/showthread.php?p=3940093#post3940093 above around #276 (comment) If I remember correctly, the 2 of the 3 CSS files were really similar, so I'm sure removing one of these 2 from the EPUB zip file will improve loading time, and might not change much the look. |
DrawBuf: allow drawing images inverted
Will be requested when in night mode: inverted images will then get inverted back by night mode and displayed normally. See koreader/koreader#2571 (comment)
ldomXPointer::getRect(): adds adjusted=false parameter
Commit 12c8e4e from #263 (lvtinydom: Adjust ldomXPointer::getRectEx(), used to get the rect of a char, to include its glyph's left and right overflows) is nice for getting nice rects to highlight, but should generally not be done for other things - so have it done only when needed. Will avoid having "not a coherent xpointer" as seen in koreader/koreader#4840 (comment).
CSS: fix parsing of "! important" with spaces
That's allowed, and used in the testsuite:
CSS: fix non-deterministic combinators
E F (descendant combinator) and E ~ F (general sibling combinator) had a limited implementation, that could miss some matches.
This implements the correct logic: check the multiple possible paths, possibly all, until a match is found.
This may be expensive on books with lots of such selectors, and may increase the loading and re-rendering times (some quick test on a book with an absurdly lot of these shows a +20% load time).
It fixes the css3-modsel-86, css3-modsel-88 (and others css3-modsel-8xx) from #176, see #176 (comment).