Merged with KOReader fork of crengine #125
Conversation
… how did it work in koreader
- Delay computation of alignment when vertical-align: top or bottom, as we need the full line to be laid out to know the final line height and baseline. - Also make the image vertical alignment code not assuming that baseline_to_bottom=0 (even if it's true for images), so it's ready to be re-used when implementing "display: inline-block" (where the baseline is not the bottom of the box). - Fix half_leading by setting half_leading_bottom, as otherwise 1px could be lost, causing possible alignment issues (but actually not anymore with the added delayed computation).
Floats among floats should be cleared and shouldn't overflow the top float box (they were overflowing its margin bottom). Also, when they are cleared, the space cleared should not be removed from the bottom margin, so it's fully shown. Also fix uninitialized value (reported by clang-tidy).
Generic support for baseline of blocks. Will be needed to support "display: inline-block" (boxes whose baseline is the baseline of the last line box it contains) and "inline-table" (boxes whose baseline is the baseline of the first line box, or the bottom of the first table row). - FlowState::addContentLine() takes an additional parameter, that we set when adding a line (ignored when adding paddings and margins). - renderBlockElement() takes an optional *baseline so callers can get it additionally to the returned height. - Also allows storing a baseline in RenderRectAccessor.
- Enhanced Block Rendering: add a new toggable feature to allow rendering correctly an element with display: inline-block as an inline box (like an image, part of a line). - initNodeRendMethod() and lvrend.cpp setNodeStyle(): wrap elements with display: inline-block or inline-table in an internal inlineBox element (mostly just like we wrap an element with float: in a floatBox element). An inlineBox is erm_inline, and only needs to get the vertical-align property of its child. - renderBlockElementEnhanced(): deal with inlineBox specifically, but mostly the same as we handle a floatBox element. - getRenderedWidths(): recursive call to measure an inlineBox while measuring its containing final block. - renderFinalBlock(): add an inlineBox to the LFormattedText object the same way we add images. - lvtextfm: handle inlineBox mostly as we handle images (same vertical alignment code), and render them when measuring text to get their width, height, and baseline. Store their size and position in their RenderRectAccessor (once the words are aligned, and their position on the line known). Move some code from the float drawing section in an added getAbsMarksFromMarks() function, as we need it too with inlineBoxes when drawing selected text. - ldomDocument::createXPointer(pt): allow recursive call, used when pt is inside an inlineBox or a floatBox (former code for floatBox replaced by this cleaner way).
crengine native text selection (and internal bookmarks) highlighting was expecting single-column full-width paragraphs with nothing else on the line, and may be ugly when floats, table cells, or RTL/BiDi text are involved. This adds an alternative method for drawing them by using getSegmentRects(), and getting multiple small rects that will each compose the final highlighting. It can be toggled by using ldomXRange.setFlags(2) on the range to highlight (the original method will still be used when using ldomXRange.setFlags(1)). Also avoid division by zero (reported by clang-tidy).
|
I see you haven't brought our epub.css, which might be fine, dunno. On KOReader side, we save a I remember that I have thought a few times "they will have to take care of that if it's brought back into upstream" - but right now, I can't remember the specific points, except the one above. I can't remember about all the things I added over the last 2 years and which may affect you - but the PRs I opened in koreader/crengine, and my commit messages, are usually quite verbose :) so you might want to spend some time browsing thru them to see what you get with this merge (possible issues, or enhancements that need to be enabled from frontends). Another (a lot more concise :) source for what's available for your frontends might be the history of our single-file crengine interface: https://github.com/koreader/koreader-base/commits/master/cre.cpp TLDR: be careful, and test a lot before making a release with this :) |
|
Having the two come closer together is a welcome and doubtless quite time consuming endeavor. :-) |
@poire-z thank you very much for bringing this to our attention. |
|
Sharing more thoughs (as they are visiting me:) about things you may want to check/look at:
|
No, it can switch to recently opened document but that's all. CoolReader is a single document app. |
Oh, allright. The lvtinydom code allows for multiple document instances. And I just remember we reduced the max number of such instances from 256 to 16, to allow for more nodes and bigger books (koreader/crengine#121 (comment), koreader/crengine#254). |
| convertBookmarks = historyRecord->getBookmarks().length() > 1; | ||
| } | ||
| } else | ||
| gDOMVersionRequested = gDOMVersionCurrent; |
There was a problem hiding this comment.
Been wondering how much of that CRFileHistRecord stuff in that LVDocView::LoadDocument() I can keep without risk for KOReader, as we don't use CRFileHistRecord, so all that stuff should be no-op.
Is that the case too with CR on Android?
But this line bothers me: needToConvertBookmarks() is called even if there's no CRFileHistRecord (meaning all that is managed from outside), so it looks like this else is wrongly killing the gDOMVersionRequested the outside code may have requested?
There was a problem hiding this comment.
If we change needToConvertBookmarks() like below:
diff --git a/crengine/src/lvdocview.cpp b/crengine/src/lvdocview.cpp
index d2ea6dc..ff7928e 100644
--- a/crengine/src/lvdocview.cpp
+++ b/crengine/src/lvdocview.cpp
@@ -3751,13 +3751,12 @@ static void FileToArcProps(CRPropRef props) {
static bool needToConvertBookmarks(CRFileHistRecord* historyRecord)
{
bool convertBookmarks = false;
- if(historyRecord) {
+ if(historyRecord && historyRecord->getBookmarks().length() > 1) {
gDOMVersionRequested = historyRecord->getDOMversion();
if(gDOMVersionRequested < 20180528) {
- convertBookmarks = historyRecord->getBookmarks().length() > 1;
+ convertBookmarks = true;
}
- } else
- gDOMVersionRequested = gDOMVersionCurrent;
+ }
return convertBookmarks;
}
This way it will not affect you, right?
There was a problem hiding this comment.
That looks fine. (So, that way, you're not resetting at all gDOMVersionRequested , but it will be gDOMVersionCurrent when you have no historyRecord because... you set/reset it to gDOMVersionCurrent elsewhere?)
There was a problem hiding this comment.
Initially gDOMVersionRequested is equal to gDOMVersionCurrent. I did not find were is it changed. I assume it is changed in a frontend or I missed something. So the one who changes gDOMVersionRequested is responsible for resetting it.
I did some more changes, please take a look: https://gist.github.com/pkb/64714978e3a467b687b738c3a0f3b51b
Does it make sense?
There was a problem hiding this comment.
So the one who changes gDOMVersionRequested is responsible for resetting it.
OK, In KOReader, we set it from frontend before each book loading (from the value saved in our book settings, or to the gDOMVersionCurrent that we have fetched from CRE, when the book has no setting yet).
I get that in your case, for each book, you set it first to the value saved in your CRHistFile (so, the oldest if none), always migrate bookmarks and always end up setting it to gDOMVersionCurrent. So I guess both you and me are fine (as long as you don't tweak gDOMVersionRequested when there is no CRHistFile :)
Your patch looks sane to me, when reading it in your and my contexts.
But I don't know how that work for CR Android, You told me bookmarks were saved in a SQLite DB, so I assume it's not using CRHistFile, right?. Is that gDOMVersion business and bookmarks migration taken care in any way on CR Android?
Also, a question, just because I don't know much about CRHistFile: is it always fed from outside frontends? So, if that is not done by the frontend, it's always NULL and checks with it are just skipped, right?
Btw, in your merged code and patch, you might want to use (instead of your hardcoded < 20180528):
#define DOM_VERSION_WITH_XPOINTERS_V2 20180528
or
#define DOM_VERSION_WITH_NORMALIZED_XPOINTERS 20180528(because for me, that won't be 20180528, but something like 20200214, depends on when I'll get that ready :) or you could have used anything 2020xyxz that I could reuse - but minor diffs, no problem).
historyRecord->getBookmarks().length() > 1 - another (less critical) thingy that might need conversion is the last position in book. After migration and switch to the latest DOMVersion, the last position might not resolve - unless crengine handles that somehow (I've seen it doing that in some other contexts, like re-renderings, but not sure if it uses a xpointer or a y-coordinate).
(In CRFileHistRecord::convertBookmarks(), your toggling of gDOMVersionRequested between old and new versions is just to pick the right version of createXPointer() and toString(), right? would have worked the same by using createXPointerV1 and toStringUsingNames(New), right?)
There was a problem hiding this comment.
I don't know much about Android part. Let's wait and see what @virxkane has to say about it.
AFAIU new CRFileHistRecord gets created whenever LVDocView::savePosition() is called. So you might have a record if you call LVDocView::goLink() for example. It will not be saved to a file, so on start record list will be empty. But anyway getBookmarks().length() will be at most 1 so
Btw, in your merged code and patch, you might want to use
Ok
another (less critical) thingy that might need conversion is the last position in book
Yes I'm aware of this. Maybe I'm wrong but I've decided to skip this conversion. There is a chance that reading position will be lost.
In CRFileHistRecord::convertBookmarks(), your toggling of gDOMVersionRequested
Right.
@virxkane could you please check the patch https://gist.github.com/pkb/64714978e3a467b687b738c3a0f3b51b and apply it along with your other changes if you find it ok? Thanks
There was a problem hiding this comment.
@pkb Ok, it's done. https://github.com/virxkane/coolreader/commit/999a484ee1f9c4a9287932537fae9697f96f7d28
@poire-z About your question: yes, on Android bookmarks saved in SQLite DB, so CRHistFile is unused. Bookmark migration on Android for now not implemented yet. In latest commit https://github.com/virxkane/coolreader/commit/3189393b1067ddab0b33375ebbf1f40014258edb I implement saving & loading DOM version of each document in DB, so at least old bookmark must be handled properly. In the future, the conversion of bookmarks will probably be implemented.
There was a problem hiding this comment.
Some update from my side: I brought these normalize xpointers in koreader/crengine#328 - with some additional minor tweaks and renames.
AFAIU new CRFileHistRecord gets created whenever LVDocView::savePosition() is called. So you might have a record if you call LVDocView::goLink() for example.
Ok, we're indeed using that, only when resizing or changing orientations. Didn't take any risk and let your CRHistFile conversion code out.
|
Another thing I forgot and that I should mention: |
I found root of this problem. I think it's affected only Android version. Method LVDocView::getBookmark() Are these cycles really useful and necessary? I tried to increase the increment of the loop counter to 10, 100, it works much faster. But there is a risk of not finding anything at the end of the desired interval... What is the preferred increment value of the loop counter in your opinion? Or may be use Bisection method? Or something else? |
Well, usually, the for loops should not go on much, there's always a break that should get out of it really early. But also see #125 (comment) just above - and try to increase these cache sizes. It's possible these affect createXPointer(lvPoint) as it needs to go thru many RenderRectAccessor - and if they can't get cached, there might be some recomputing that would cause some slowdown.
Can't really find out how that works, and where you need that and how often you call it... Android source tree is quite obscure to me :) |
On a smartphone with page animation turned on - 9 times per page turn. The latter is executes few times. On e-ink bookreader (without animation) - 2 times per page turn. Increasing the maximum allowed cache size not fixed this slowness. But I found integer overflow in cache size calculations :) |
|
For info, with KOReader, we do not use any of the CMD stuff. Anyway, in your samples above, if for a page turn, wouldn't they return always the same value, and would benefit being cached locally so you don't call them 9 times? |
Did your fix (https://github.com/virxkane/coolreader/commit/36299e6e14) and increasing cache size help with the slowness? (I noticed some slowness in some page turn after I added a new feature, that resulted in the for loop iterating all over the height - it had to be fixed elsewhere koreader/crengine@1cec90fbc4 . So, if you really notice the for loops being done fully, it might be a bug - hopefully only on some specific pages of some specific book, but not all?) |
No, it not helps. This commit for future increasing cache that has not yet been applied.
Yes, the slowdown is not on all books, I hope your fix corrects this behavior. |
Possibly not, that fix was for taller-than-page elements with |
|
@poire-z DocumentsForTestingRTL.zip that you upload about month ago, file Petra.AR.epub. Slowdown occurs when I turn pages around contents with numeric list. |
|
@poire-z Forgot to add: with DOMVersion and BlockRenderingFlags both equals 0. |
|
OK, took that epub from that zip file. With KOReader, and just adding: ldomXPointer LVDocView::getBookmark() {
LVLock lock(getMutex());
checkPos();
ldomXPointer ptr;
if (m_doc) {
if (isPageMode()) {
if (_page >= 0 && _page < m_pages.length()) {
// In some edge cases, getting the xpointer for y=m_pages[_page]->start
// could resolve back to the previous page. We need to check for that
// and increase y until we find a coherent one.
// (In the following, we always want to find the first logical word/char.)
LVRendPageInfo * page = m_pages[_page];
bool found = false;
ldomXPointer fallback_ptr;
for (int y = page->start; y < page->start + page->height; y++) {
+ printf("page.start = %d : y=%d\n", page->start, y);
ptr = m_doc->createXPointer(lvPoint(0, y), PT_DIR_SCAN_FORWARD_LOGICAL_FIRST);
lvPoint pt = ptr.toPoint();In enhanced mode, I always see a single loop iteration (ok, twice because we must call this twice per page turn, one to go to, one to get the xpointer to restore to): Still a single one on the TOC at start, and on the footnotes at end. There's one page when I get 100 loops: Confirmed with an other LTR EPUB that this happens when some float is split among 2 pages, on the 2nd page: It looks logical: when you ask what xpointer is at top of that 2nd page, it gives you the float (or the image inside the float) that starts on previous page, you resolved it back to a y, which happens to be smaller than the one you asked for, so it is bad: so, you continue the loop, until you pass 100px (the height of the 2nd part of the image on that 2nd page), and you finally reach some node that is on this page (y > the y you asked for), and it's fine. In "legacy" mode, I notice all is fine, in the TOC and footnotes, BUT not at end, after the footnotes, when there is a table: there, in that table, there are many loops (but 100 to 200 px only). But well, legacy mode :) not worth investigating. So, I don't notice what you notice on page with numeric list (TOC at start or footnotes at end?). Some possibly related commits I made that you haven't picked yet, that may be worth quick applying/testing to see if they solve your issue? |
Uh ! that's quite a huge forgetting :) Well, at minima, DOMVersion should be >= 20180528 (and you do that migration automatically, no?) - so you get at least proper display for elements, and the most essential fixes. Not fixing it as having DOMVersion=20180528 is actually the fix to many things :) |
|
@poire-z Ok, I copied all your last commits via cherry-pick and slowdown are fixed now. Thank you. After: Logcat at page turn action with some debug noise: |
|
Hmm, no, I found slowdown in other file - Petra.FR.epub Again: As before, the reason is the same - LVDocView::getPageDocumentRange(int), LVDocView::getBookmark(). DOMVersion=last, BlockRenderingFlags=max |
|
Can't reproduce on this one. But even with a DOMVersion=20100101 and rend flags=0, I get the image centered. I have it off like you do when I switched from "web" to "legacy" (but koreader tells me the cache is invalid and I need to reload the book - and then I get the image centered). And why are you testing with DOMVersion=0/rendflags=0 ? :) |
No, DOMVersion=last, BlockRenderingFlags=max. |
|
Ok, after cleaning cache directory problem fixes. So, on our side incorrectly switched DOMVersion (option in settings dialog), but in other cases not specified, the slowdown may return. This can be a trap. |
|
Yep, I have it like that ^ , with a proper float :) |
You picked https://github.com/virxkane/coolreader/commit/2aa255072d8e0b8d873759f7ae7454dfc4cf03c9 - so it should be automatically handled: cache trashed when dom version different. |
|
Just mentionning something I just saw while trying to implement support for CSS min-width/max-height: In lvtextfm.cpp, there is a resizeImage() function that is supposed to reduce if needed the image size to fit in the paragraph width/page height, but also to apply some of CoolReader options, the Image scaling options. Looks like nobody noticed it since this was merged :) and may be it's just fine. |
|
For info, I restored the maxScale stuff in a commit part of koreader/crengine#409 : |
This PR should close #91. It includes a lot of enhancements in css processing, thanks to koreader devs and especially to poire-z for work on crengine.
It wasn't that easy to merge, in case of conflicts I mostly accepted their side. Except for kerning mode setting, We handle it differently - we use a combination of new setting "Text shaping mode" and the old Kerning enable/disable.
They have added qimagescale from QT, I moved it to thirdpatrty folder. I also added nanosvg library to the thirdparty folder, harfbuzz I updated to 2.6.4 version.
Note: this change is not yet ready for Android, @virxkane will create a separate request for it.