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

Fix HiDPI platform view transforms in CanvasKit #23244

Closed
cbenhagen wants to merge 1 commit into from
Closed

Fix HiDPI platform view transforms in CanvasKit #23244

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class HtmlViewEmbedder {
final double inverseScale = 1 / scale;
final Matrix4 scaleMatrix =
Matrix4.diagonal3Values(inverseScale, inverseScale, 1);
headTransform.multiply(scaleMatrix);
headTransform = scaleMatrix.multiplied(headTransform);
Copy link
Contributor

@yjbanov yjbanov Jan 5, 2021

Choose a reason for hiding this comment

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

Can we initialize headTransform to Matrix4.diagonal3Values(inverseScale, inverseScale, 1) instead of applying the scaled transform at the end? This should eliminate 2 extra matrix allocations (Matrix4.identity() and scaleMatrix.multiplied).

Copy link
Contributor Author

@cbenhagen cbenhagen Jan 5, 2021

Choose a reason for hiding this comment

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

I am not sure if I fully understand what you mean. Please feel free to take over this PR and commit changes to my branch. I must confess, I merely applied the fix applied in #22945 without deep understanding of how this all fits together.

Copy link
Contributor

@yjbanov yjbanov Jan 6, 2021

Choose a reason for hiding this comment

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

The headTransform was IDENTITY x T1 x T2 x ... x Tk x inverseScale. After this change it becomes inverseScale x IDENTITY x T1 x T2 x ... x Tk, which means we can remove IDENTITY and start with inverseScale. But the devil is probably in the detail. For example, if there's a clipping transform we reset it back to IDENTITY. I'm wondering if applying the scale at the end works in the presence of clips at all. If I'm reading the code correctly, it seems it doesn't?

Example: T1 > CLIP1 > T2 becomes T1 > CLIP1 > inverseScale > T2, but should probably be inverseScale > T1 > CLIP1 > T2. IIUC the inverseScale is there to undo the top-level transform that the flutter framework injects in https://github.com/flutter/flutter/blob/47bad77287effb3467f27937884036f51da6e325/packages/flutter/lib/src/rendering/view.dart#L144, so in order to undo that we should apply the inverse at the top level too.

I can take it over if you'd like, but also happy to provide guidance. I think we also need @hterkelsen's thoughts on this to make sure we're not breaking it altogether :)

Copy link
Contributor Author

@cbenhagen cbenhagen Jan 7, 2021

Choose a reason for hiding this comment

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

Yes please take it over. Feel free to close this and create a separate PR or just push to my branch.

Copy link
Contributor

@yjbanov yjbanov Jan 8, 2021

Choose a reason for hiding this comment

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

SGTM. I'll start a new PR. Thanks for discovering the bug!

head.style.transform = float64ListToCssTransform(headTransform.storage);
}

Expand Down