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

Use new unresolvedCodePoints API from skia. #41991

Merged
merged 4 commits into from
May 16, 2023
Merged

Use new unresolvedCodePoints API from skia. #41991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5c9e95
Use new `unresolvedCodePoints` API from skia.
eyebrowsoffire May 12, 2023
291cb39
Add some comments.
eyebrowsoffire May 12, 2023
f46a25e
Fix unit tests.
eyebrowsoffire May 12, 2023
307ed3f
Merge branch 'main' into skia_missing_codepoints
eyebrowsoffire May 16, 2023
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
25 changes: 14 additions & 11 deletions lib/web_ui/lib/src/engine/font_fallbacks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,19 @@ class FontFallbackManager {
registry.getMissingCodePoints(codePoints, fontFamilies);

if (missingCodePoints.isNotEmpty) {
_codePointsToCheckAgainstFallbackFonts.addAll(missingCodePoints);
if (!_scheduledCodePointCheck) {
_scheduledCodePointCheck = true;
_idleFuture = Future<void>.delayed(Duration.zero, () async {
_ensureFallbackFonts();
_scheduledCodePointCheck = false;
await downloadQueue.waitForIdle();
});
}
addMissingCodePoints(codePoints);
}
}

void addMissingCodePoints(List<int> codePoints) {
_codePointsToCheckAgainstFallbackFonts.addAll(codePoints);
if (!_scheduledCodePointCheck) {
_scheduledCodePointCheck = true;
_idleFuture = Future<void>.delayed(Duration.zero, () async {
_ensureFallbackFonts();
_scheduledCodePointCheck = false;
await downloadQueue.waitForIdle();
});
}
}

Expand All @@ -144,8 +148,7 @@ class FontFallbackManager {
}
final List<int> codePoints = _codePointsToCheckAgainstFallbackFonts.toList();
_codePointsToCheckAgainstFallbackFonts.clear();
final List<int> missingCodePoints = registry.getMissingCodePoints(codePoints, globalFontFallbacks);
findFontsForMissingCodePoints(missingCodePoints);
findFontsForMissingCodePoints(codePoints);
}

void registerFallbackFont(String family) {
Expand Down
3 changes: 2 additions & 1 deletion lib/web_ui/lib/src/engine/skwasm/skwasm_impl/font_collection.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,9 @@ class SkwasmFontCollection implements FlutterFontCollection {

@override
void debugResetFallbackFonts() {
setDefaultFontFamilies(<String>[]);
setDefaultFontFamilies(<String>['Roboto']);
fontFallbackManager = FontFallbackManager(SkwasmFallbackRegistry(this));
fontCollectionClearCaches(handle);
}
}

Expand Down
49 changes: 25 additions & 24 deletions lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ class SkwasmLineMetrics implements ui.LineMetrics {
class SkwasmParagraph implements ui.Paragraph {
SkwasmParagraph(this.handle);

ParagraphHandle handle;
final ParagraphHandle handle;
bool _isDisposed = false;
bool _hasCheckedForMissingCodePoints = false;

@override
double get width => paragraphGetWidth(handle);
Expand All @@ -102,8 +103,30 @@ class SkwasmParagraph implements ui.Paragraph {
bool get didExceedMaxLines => paragraphGetDidExceedMaxLines(handle);

@override
void layout(ui.ParagraphConstraints constraints) =>
void layout(ui.ParagraphConstraints constraints) {
paragraphLayout(handle, constraints.width);
if (!_hasCheckedForMissingCodePoints) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question: is it possible for paragraphGetUnresolvedCodePoints to return a different result for the same paragraph later? E.g. when extra fallback fonts are downloaded.

I guess what I'm really asking is whether there's ever a case where we need to reset paragraphGetUnresolvedCodePoints to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe it can. The reason is that you can't actually change the list of font families on the text styles or paragraph style once it is a paragraph, that's specified at the ParagraphBuilder stage. Once you've already created the paragraph, the only way to add more fallback fonts to it is by actually rebuilding the paragraph with new text styles.

_hasCheckedForMissingCodePoints = true;
final int missingCodePointCount = paragraphGetUnresolvedCodePoints(handle, nullptr, 0);
if (missingCodePointCount > 0) {
withStackScope((StackScope scope) {
final Pointer<Uint32> codePointBuffer = scope.allocUint32Array(missingCodePointCount);
final int returnedCodePointCount = paragraphGetUnresolvedCodePoints(
handle,
codePointBuffer,
missingCodePointCount
);
assert(missingCodePointCount == returnedCodePointCount);
renderer.fontCollection.fontFallbackManager!.addMissingCodePoints(
List<int>.generate(
missingCodePointCount,
(int index) => codePointBuffer[index]
)
Comment on lines +121 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is going to be a common occurrence, consider making an extension method for it:

extension PointerToList<T extends NativeType> on Pointer<T> {
  List<T> toList(int length) {
    return List<T>.generate(length, (int index) => this[index]);
  }
}

but when I tried this, the analyzer didn't like this[index] apparently because the [] operator is defined as a separate extension for each T 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think probably the reason this is failing is because the T in Pointer<T> the native type, but the T in List<T> is probably the dart type (int in this case). That makes this kind of difficult to create a generic helper for.

);
});
}
}
}

List<ui.TextBox> _convertTextBoxList(TextBoxListHandle listHandle) {
final int length = textBoxListGetLength(listHandle);
Expand Down Expand Up @@ -562,30 +585,8 @@ class SkwasmParagraphBuilder implements ui.ParagraphBuilder {
placeholderScales.add(scale);
}

List<String> _getEffectiveFonts() {
final List<String> fallbackFonts = renderer.fontCollection.fontFallbackManager!.globalFontFallbacks;
final List<String>? currentFonts =
textStyleStack.isEmpty ? null : textStyleStack.last.style.fontFamilies;
if (currentFonts != null) {
return <String>[
...currentFonts,
...fallbackFonts,
];
} else if (style.defaultFontFamily != null) {
return <String>[
style.defaultFontFamily!,
...fallbackFonts,
];
} else {
return fallbackFonts;
}
}

@override
void addText(String text) {
renderer.fontCollection.fontFallbackManager!.ensureFontsSupportText(
text, _getEffectiveFonts()
);
final SkString16Handle stringHandle = skString16FromDartString(text);
paragraphBuilderAddText(handle, stringHandle);
skString16Free(stringHandle);
Expand Down
5 changes: 5 additions & 0 deletions lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/raw_fonts.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,8 @@ external void fontCollectionRegisterTypeface(
TypefaceHandle typeface,
SkStringHandle fontName,
);

@Native<Void Function(
FontCollectionHandle
)>(symbol: 'fontCollection_clearCaches', isLeaf: true)
external void fontCollectionClearCaches(FontCollectionHandle handle);
18 changes: 18 additions & 0 deletions lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/text/raw_paragraph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,21 @@ external TextBoxListHandle paragraphGetBoxesForRange(
@Native<TextBoxListHandle Function(ParagraphHandle)>(
symbol: 'paragraph_getBoxesForPlaceholders', isLeaf: true)
external TextBoxListHandle paragraphGetBoxesForPlaceholders(ParagraphHandle handle);

// Returns a list of the code points that were unable to be rendered with the
// selected fonts. The list is deduplicated, so each code point in the output
// is unique.
// If `nullptr` is passed in for `outCodePoints`, we simply return the count
// of the code points.
// Note: This must be called after the paragraph has been laid out at least
// once in order to get valid data.
@Native<Int Function(
ParagraphHandle,
Pointer<Uint32>,
Int,
)>(symbol: 'paragraph_getUnresolvedCodePoints', isLeaf: true)
external int paragraphGetUnresolvedCodePoints(
ParagraphHandle handle,
Pointer<Uint32> outCodePoints,
int outLength,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this third outLength parameter is for. Could you clarify please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

outLength is the length of the outCodePoints buffer that we are passing to the API. This is so that the C++ code doesn't overwrite the buffer into some other region of the memory. However... because we actually got that value from the C++ code in the first place, it is true that we can just omit this parameter altogether and the C++ code can always trust that the buffer is exactly the length that is needed. I can remove it if you think it's confusing, I think this is just my standard "C memory safety" guardrails I automatically added.

);
5 changes: 5 additions & 0 deletions lib/web_ui/skwasm/fonts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,8 @@ SKWASM_EXPORT void fontCollection_registerTypeface(
collection->provider->registerTypeface(sk_sp<SkTypeface>(typeface));
}
}

SKWASM_EXPORT void fontCollection_clearCaches(
FlutterFontCollection* collection) {
collection->collection->clearCaches();
}
25 changes: 25 additions & 0 deletions lib/web_ui/skwasm/text/paragraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,28 @@ SKWASM_EXPORT TextBoxList* paragraph_getBoxesForPlaceholders(
Paragraph* paragraph) {
return new TextBoxList{paragraph->getRectsForPlaceholders()};
}

// Returns a list of the code points that were unable to be rendered with the
// selected fonts. The list is deduplicated, so each code point in the output
// is unique.
// If `nullptr` is passed in for `outCodePoints`, we simply return the count
// of the code points.
// Note: This must be called after the paragraph has been laid out at least
// once in order to get valid data.
SKWASM_EXPORT int paragraph_getUnresolvedCodePoints(Paragraph* paragraph,
SkUnichar* outCodePoints,
int outLength) {
if (!outCodePoints) {
return paragraph->unresolvedCodepoints().size();
}
Comment on lines +132 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it looks like you are handling the case of hasUnresolvedCodePoints here. Is there really a need for outLength?

int outIndex = 0;
for (SkUnichar character : paragraph->unresolvedCodepoints()) {
if (outIndex < outLength) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Hmm this is a bit confusing and could use a better name or an API change.

For example, it might be a good idea to split this into 2 APIs:

  • bool paragraph_hasUnresolvedCodePoints(Paragraph* paragraph)
  • int paragraph_getUnresolvedCodePoints(Paragraph* paragraph, SkUnichar* outCodePoints)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A boolean is actually not good enough, we want the API to return the length of the buffer needed to hold all the code points so we can allocate the correct amount. This pattern is actually pretty common for C APIs, where the user of the API passes in null first, and the API returns the size of the buffer needed to hold the output, and then the user allocates that memory and calls the API a second time with the actual buffer to have it filled.

outCodePoints[outIndex] = character;
outIndex++;
} else {
break;
}
}
return outIndex;
}
22 changes: 18 additions & 4 deletions lib/web_ui/test/ui/fallback_fonts_golden_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ void testMain() {
ui.ParagraphStyle(),
);
pb.addText('مرحبا');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));

await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();

Expand Down Expand Up @@ -100,6 +101,7 @@ void testMain() {
ui.ParagraphStyle(),
);
pb.addText('مرحبا');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));

await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();

Expand Down Expand Up @@ -134,6 +136,7 @@ void testMain() {
ui.ParagraphStyle(),
);
pb.addText('Hello 😊');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));

await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();

Expand Down Expand Up @@ -170,7 +173,10 @@ void testMain() {
() async {
// Try rendering text that requires fallback fonts, initially before the fonts are loaded.

ui.ParagraphBuilder(ui.ParagraphStyle()).addText('ヽಠ');
ui.ParagraphBuilder pb = ui.ParagraphBuilder(ui.ParagraphStyle());
pb.addText('ヽಠ');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));

await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();
expect(
downloadedFontFamilies,
Expand All @@ -182,15 +188,20 @@ void testMain() {

// Do the same thing but this time with loaded fonts.
downloadedFontFamilies.clear();
ui.ParagraphBuilder(ui.ParagraphStyle()).addText('ヽಠ');
pb = ui.ParagraphBuilder(ui.ParagraphStyle());
pb.addText('ヽಠ');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));
await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();
expect(downloadedFontFamilies, isEmpty);
});

test('can find glyph for 2/3 symbol', () async {
// Try rendering text that requires fallback fonts, initially before the fonts are loaded.

ui.ParagraphBuilder(ui.ParagraphStyle()).addText('⅔');
ui.ParagraphBuilder pb = ui.ParagraphBuilder(ui.ParagraphStyle());
pb.addText('⅔');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));

await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();
expect(
downloadedFontFamilies,
Expand All @@ -201,7 +212,10 @@ void testMain() {

// Do the same thing but this time with loaded fonts.
downloadedFontFamilies.clear();
ui.ParagraphBuilder(ui.ParagraphStyle()).addText('⅔');
pb = ui.ParagraphBuilder(ui.ParagraphStyle());
pb.addText('⅔');
pb.build().layout(const ui.ParagraphConstraints(width: 1000));

await renderer.fontCollection.fontFallbackManager!.debugWhenIdle();
expect(downloadedFontFamilies, isEmpty);
});
Expand Down