Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] roll CanvasKit 0.36.1 (attempt 3) #36005

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Sep 8, 2022

This roll was reverted in #35837 because of golden failures in the engine=>framework roll, which turned out to be improvements. The new version of CK renders more consistently with non-web versions of Flutter (examples).

Roll CanvasKit to 0.36.1. This brings a fix for certain old Android GPUs that trigger a bug when used through WebGL.

Fixes flutter/flutter#74990
Fixes flutter/flutter#110684

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Sep 8, 2022
@yjbanov yjbanov changed the title [web] roll CanvasKit 0.36.1 [web] roll CanvasKit 0.36.1 (attempt 3) Sep 8, 2022
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Small nitpicks/questions.

Comment on lines +1578 to +1580
void expectAlmost(double actual, double expected) {
expect(actual, within<double>(distance: actual / 100, from: expected));
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you need your own implementation to validate this?

There's already moreOrLessEquals and closeTo that might help here (and probably more!).

expect(actual, moreOrLessEquals(expected, epsilon: actual/100));

(Also not sure about using actual as part of the epsilon value... shouldn't it be fixed to guarantee accuracy to X decimal points? Like 0.1 or maybe 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.

moreOrLessEquals is from package:flutter_test, which we don't have access to. closeTo is interesting. Right now within is standard in the code base, so I think for now I'll prefer to continue relying on it.

expectAlmost(paragraph.getLongestLine(), 263);
expectAlmost(paragraph.getMaxIntrinsicWidth(), 263);
expectAlmost(paragraph.getMinIntrinsicWidth(), 135);
expectAlmost(paragraph.getMaxWidth(), 500);
Copy link
Member

Choose a reason for hiding this comment

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

In this case, the width can be +-5 of 500 (I assume pixels?). Isn't that delta starting to be "too large" that could start masking actual measurement/rounding errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole test is just a smoke test. More precise text measurement tests are in dedicated files. So I'm not worried about the precision here. TBH I'd probably be happy if we just checked that the returned value is not null. That we are also checking for a specific value is a bonus.

..decoration = 0x2
..decorationThickness = 2.0
..decorationColor = Float32List.fromList(<double>[13, 14, 15, 16])
..decorationStyle = canvasKit.DecorationStyle.Dotted
..textBaseline = canvasKit.TextBaseline.Ideographic
..fontSize = 24
..fontSize = 48
Copy link
Member

Choose a reason for hiding this comment

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

Why these changes in fontSize, heightMultiplier and others? (I understand the colors were maybe invalid before, but the rest?)

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 made this test a golden test. Previously the text we rendered was complete nonsense. Now it is displaying something reasonable: https://flutter-engine-gold.skia.org/diff?test=paragraph_kitchen_sink&left=c8f1c90d490826fdbcaf4e2a4762fc76&right=eb3fc116f84bdf3e401fe0345ccde4f3

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 9, 2022
@auto-submit auto-submit bot merged commit 15e9258 into flutter:main Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
cfontas pushed a commit to cfontas/engine that referenced this pull request Sep 14, 2022
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Engine -> Framework roll blocked on skia diffs Web app with CanvasKit doesn't render text on some Android phones
3 participants