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.

Sign up for GitHub

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

Jump to bottom

Some LVFormatter::measureText() changes #574

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Some LVFormatter::measureText() changes #574

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50c2178
text: optimize tab handling
bbshelper Jun 26, 2024
07a143e
text: optimize measureText()'s bidi handling
bbshelper Jun 26, 2024
e391bbb
text: simplify measureText()'s harfbuzz check
bbshelper Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 17 additions & 30 deletions crengine/src/lvtextfm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1845,14 +1845,12 @@ class LVFormatter {
#define MAX_TEXT_CHUNK_SIZE 4096
static lUInt16 widths[MAX_TEXT_CHUNK_SIZE+1];
static lUInt8 flags[MAX_TEXT_CHUNK_SIZE+1];
int tabIndex = -1;
#if (USE_FRIBIDI==1)
FriBidiLevel lastBidiLevel = 0;
FriBidiLevel newBidiLevel;
#endif
#if (USE_HARFBUZZ==1)
bool checkIfHarfbuzz = true;
bool usingHarfbuzz = false;
bool usingHarfbuzz = m_kerning_mode == KERNING_MODE_HARFBUZZ;
Comment on lines -1854 to +1853
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glad to hear that :)

// Unicode script change (note: hb_script_t is uint32_t)
lUInt32 prevScript = HB_SCRIPT_COMMON;
hb_unicode_funcs_t* _hb_unicode_funcs = hb_unicode_funcs_get_default();
Expand All @@ -1864,25 +1862,13 @@ class LVFormatter {
LVFont * newFont = NULL;
lInt16 newLetterSpacing = 0;
src_text_fragment_t * newSrc = NULL;
if ( tabIndex<0 && m_text[i]=='\t' ) {
tabIndex = i;
}
bool isObject = false;
bool prevCharIsObject = false;
if ( i<m_length ) {
newSrc = m_srcs[i];
isObject = m_flags[i] & LCHAR_IS_OBJECT; // image, float or inline box
newFont = isObject ? NULL : (LVFont *)newSrc->t.font;
newLetterSpacing = newSrc->letter_spacing; // 0 for objects
#if (USE_HARFBUZZ==1)
// Check if we are using Harfbuzz kerning with the first font met
if ( checkIfHarfbuzz && newFont ) {
if ( m_kerning_mode == KERNING_MODE_HARFBUZZ ) {
usingHarfbuzz = true;
}
checkIfHarfbuzz = false;
}
#endif
}
if (i > 0)
prevCharIsObject = m_flags[i-1] & LCHAR_IS_OBJECT; // image, float or inline box
Expand All @@ -1904,17 +1890,13 @@ class LVFormatter {
}
#endif
bool bidiLevelChanged = false;
int lastDirection = 0; // unknown
#if (USE_FRIBIDI==1)
lastDirection = 1; // direction known: LTR if no bidi found
if (m_has_bidi) {
newBidiLevel = m_bidi_levels[i];
if (i == 0)
lastBidiLevel = newBidiLevel;
else if ( newBidiLevel != lastBidiLevel )
bidiLevelChanged = true;
if ( FRIBIDI_LEVEL_IS_RTL(lastBidiLevel) )
lastDirection = -1; // RTL
}
#endif
// When measuring with Harfbuzz, we should also split on Unicode script change,
Expand Down Expand Up @@ -1976,11 +1958,13 @@ class LVFormatter {
lUInt32 hints = 0;
if ( start == 0 ) hints |= LFNT_HINT_BEGINS_PARAGRAPH;
if ( i == m_length ) hints |= LFNT_HINT_ENDS_PARAGRAPH;
if ( lastDirection ) {
hints |= LFNT_HINT_DIRECTION_KNOWN;
if ( lastDirection < 0 )
hints |= LFNT_HINT_DIRECTION_IS_RTL;
}
#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
Comment on lines +1961 to +1967
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int chars_measured = lastFont->measureText(
m_text + start,
len,
Expand Down Expand Up @@ -2306,7 +2290,7 @@ class LVFormatter {
// (In the context of inline elements, margin/border/padding-inline-start/end
// would be more natural to use than -left/right - but it's a more recent CSS
// addition that we don't support.)
bool is_mirrored = lastDirection < 0;
bool is_mirrored = FRIBIDI_LEVEL_IS_RTL(lastBidiLevel);
if ( is_right_pad != is_mirrored ) { // unmirrored right pad, or mirrored left pad
// Use right margin/border/padding values
margin = lengthToPx( node, style->margin[1], base_width );
Expand Down Expand Up @@ -2422,14 +2406,17 @@ class LVFormatter {
lastBidiLevel = newBidiLevel;
#endif
}
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;
}
Comment on lines -2425 to +2419
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the case. I still see the condition to be true in some epubs, but as you say it's rare.

}
}
// // debug dump
Expand Down
Loading