Some LVFormatter::measureText() changes#574
Some LVFormatter::measureText() changes#574bbshelper wants to merge 3 commits intokoreader:masterfrom
Conversation
Find first tab as needed, and don't check beyond marker width.
Calculate bidi direction on measuring, not on every char.
It doesn't seem to be necessary to set usingHarfbuzz on the first font met: the variable doesn't change, and when it's checked, there is always a valid font.
bbshelper
changed the title
poire-z
left a comment
poire-z
left a comment
There was a problem hiding this comment.
I'm not sure these microoptimizations are worth your time and the 60 minutes I had to spend on reviewing to get back into the code and made sure you did not change the behaviour :/
I'm finalizing a major optimization to LVFormatter::measureText()
I'm really fearing that :\ (If you PR that and don't hear from me, it's because I'll soon be on vacation, not because of fear :))
| if ( tabIndex >= 0 && m_srcs[0]->indent < 0) { | ||
| if (m_srcs[0] && m_srcs[0]->indent < 0) { | ||
| // Used by obsolete rendering of css_d_list_item_legacy when css_lsp_outside, | ||
| // where the marker width is provided as negative/hanging indent. | ||
| int tabPosition = -m_srcs[0]->indent; // has been set to marker_width | ||
| if ( tabPosition>0 && tabPosition > m_widths[tabIndex] ) { | ||
| int dx = tabPosition - m_widths[tabIndex]; | ||
| for ( i=tabIndex; i<m_length; i++ ) | ||
| m_widths[i] += dx; | ||
| for (int j = 0; j < m_length && m_widths[j] >= tabPosition; ++j) { | ||
| if (m_text[j] == '\t') { | ||
| int dx = tabPosition - m_widths[j]; | ||
| for ( int i=j; i<m_length; i++ ) | ||
| m_widths[i] += dx; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Not really familiar with what that does (even now just trying to understand the purpose).
I get that now, we won't do anything as long as m_srcs[0]->indent < 0 is false, and if I trust my comment, this should rarely be true. So ok with avoiding these checks in the main loop.
I also think that you too probably don't get what this does, and that you just tried to ensure the same logic (looks like you did), so trusting you :)
In crengine, there are mostly only i++, --i is rare. As in the inner loop you kept the i++, may be make the outer loop also use the j++ style.
There was a problem hiding this comment.
That's exactly the case. I still see the condition to be true in some epubs, but as you say it's rare.
| #if (USE_FRIBIDI==1) | ||
| if (m_has_bidi) { | ||
| hints |= LFNT_HINT_DIRECTION_KNOWN; | ||
| if (FRIBIDI_LEVEL_IS_RTL(lastBidiLevel)) | ||
| hints |= LFNT_HINT_DIRECTION_IS_RTL; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
It looks like in the existing code, we set hints |= LFNT_HINT_DIRECTION_KNOWN always (when #if (USE_FRIBIDI==1), so: always).
Now, you set it only when if (m_has_bidi). I guess this first hints |= should be moved above.
Ok, I get what you're doing. The commit message should be:
Calculate bidi direction on measuring segment change only, not on every char.
The other (now it is a second call) FRIBIDI_LEVEL_IS_RTL(lastBidiLevel) below would only be done when PAD_CHAR_INDEX (inline margin/border/padding) which should be rare.
(Is that FRIBIDI_LEVEL_IS_RTL() really expensive, and worth the pain for you and me ? :) It's just #define FRIBIDI_LEVEL_IS_RTL(lev) ((lev) & 1)).
There was a problem hiding this comment.
The intention was to remove writes to lastDirection in the main loop. As you've pointed out, this commit is problematic. I'll rework it.
| bool checkIfHarfbuzz = true; | ||
| bool usingHarfbuzz = false; | ||
| bool usingHarfbuzz = m_kerning_mode == KERNING_MODE_HARFBUZZ; |
There was a problem hiding this comment.
It doesn't seem to be necessary to set usingHarfbuzz on the first font
met: the variable doesn't change, and when it's checked, there is always
a valid font.
Ok, right.
These usingHarfbuzz were introcuded by 737f37e in 2019.
m_kerning_mode by a7cea02 only in 2022. I could have removed all that at the time, so better late than never :)
There was a problem hiding this comment.
glad to hear that :)
I'm not sure either :) I must have spent more time optimizing crengine than what I can ever save in reading on e-ink in my lifetime.
I try very hard not to. I've setup a regression test, which is quite useful in catching corner cases. That said, I don't really know a thing about bidi and harfbuzz, and they are not covered by my regression test.
I promise that the new code is clearer than the current one. It's a major optimization and not a rewrite. :) |
2 participants
I'm finalizing a major optimization to
LVFormatter::measureText(), so I'd like these smaller ones to be reviewed and merged first.The first 2 commits have around 0.5% and 0.2% performance gain in my Load and Render test.
This change is