Switch ui.window.devicePixelRatio from browser logical to physical. #17209

ferhatb · 2020-03-19T16:34:19Z

This enabled image loading for multi dpi assets and fixes correctness issues in framework due to incorrectly using logical browser pixels at 1.0 dpr.
Adds integration test for image loading of higher resolution.
Adds missing platform replymessage calls.

Related issues:
flutter/flutter#46186
flutter/flutter#41328
material-components/material-components-flutter-gallery#310

Fixes: flutter/flutter#52929

…st for multi res image loading

.cirrus.yml

lib/web_ui/lib/src/engine/surface/scene_builder.dart

yjbanov · 2020-03-19T21:40:24Z

lib/web_ui/lib/src/engine/window.dart

+  /// This is useful in tests to emulate screens of different dimensions
+  /// including integration tests.
+  void overrideDevicePixelRatio(double value) {
+    _debugDevicePixelRatio = value;


Why this change? It's still used only in tests, so the "debug" prefix and assert seem appropriate.

It is now also used in integration test (the one that loads an image at framework level).

Right, but that's still a test :)

Do integration tests strip out asserts?

If they don't, we should add the option to enable them. dart2js supports asserts (we use dart2js for engine unit-test with assertions enabled).

/cc @nturgut

My comment is non-blocking though. I don't think there's major risk. It just doesn't feel right for a method that's not annotated as "debug" to manipulate a "debug" field without an assertion.

e2etests/web/regular_integration_tests/test_driver/image_loading_e2e_test.dart

lib/web_ui/lib/src/engine/window.dart

e2etests/web/regular_integration_tests/lib/image_loading_main.dart

mdebbar · 2020-03-20T00:49:00Z

lib/web_ui/lib/src/engine/window.dart

+  /// This is useful in tests to emulate screens of different dimensions
+  /// including integration tests.
+  void overrideDevicePixelRatio(double value) {
+    _debugDevicePixelRatio = value;


Do integration tests strip out asserts?

lib/web_ui/lib/src/engine/window.dart

yjbanov · 2020-03-20T03:35:16Z

LGTM if LGT @mdebbar

yjbanov · 2020-03-24T17:19:23Z

lib/web_ui/lib/src/engine/surface/scene_builder.dart

+      // scene to devicepixelratio. Use identity instead since CSS uses
+      // logical device pixels.
+      assert(matrix4[0] == EngineWindow.browserDevicePixelRatio &&
+          matrix4[5] == EngineWindow.browserDevicePixelRatio);


This assertion error would be visible to developers and Flutter contributors writing tests. It would be very confusing without a good error message. Consider moving the comment into the assertion error message.

Removed the check for test environment since there are many SceneBuilder raw tests outside the context of an app that pushes root transform for now (we really need a ui.Window level debugDevicePixelRatio, i noticed some hacked in TestWindow.devicePixelRatioTestValue etc in the code base used for viewport_test).

…flutter_test

yjbanov · 2020-03-25T17:53:55Z

lib/web_ui/lib/src/engine/semantics/accessibility.dart


  /// Decodes the message coming from the 'flutter/accessibility' channel.
-  void handleMessage(ByteData data) {
+   void handleMessage(StandardMessageCodec codec, ByteData data) {


unnecessary extra leading space

yjbanov · 2020-03-25T18:00:49Z

lib/web_ui/lib/src/engine/surface/scene_builder.dart

+        assert(matrix4[0] == window.devicePixelRatio &&
+           matrix4[5] == window.devicePixelRatio);
+      }
+      matrix4 = Matrix4.identity().storage;


Actually, now that I think about it, the correct fix would be to inject a transform that scales by 1/DPR. This way we don't have to rely on a particular framework behavior. To avoid extra div nodes we can do something like:

if (the scene contains a root transform) { reuse the root transform by scale it by 1/DPR. } else { apply 1/DPR to the `<flt-scene>` element. }

In most cases we'll hit the if branch, not the else branch. Also in most cases the root transform will be scale by DPR. So the resulting root transform will automatically become identity and we won't apply any transform at all.

The framework itself injects pushTransform using ViewConfiguration. Ideally this code would not live in framework but in engine. See view.dart:39. Or instead of calling pushTransform , framework would call SceneBuilder.setViewConfiguration in engine.

Right, but you can use dart:ui without the Flutter framework where you can paint a scene without a root transform. This typically happens in tests and benchmarks. I don't expect there to be many apps that don't use the framework.

yjbanov · 2020-03-25T18:02:21Z

lib/web_ui/lib/src/engine/window.dart

    return (ratio == null || ratio == 0.0) ? 1.0 : ratio;
  }

+


yjbanov · 2020-03-25T18:05:16Z

lib/web_ui/lib/src/ui/test_embedding.dart

-  engine.window.debugOverrideDevicePixelRatio(devicePixelRatio);
-  engine.window.webOnlyDebugPhysicalSizeOverride =
-      Size(800 * devicePixelRatio, 600 * devicePixelRatio);
+  assert(() {


Why is this assert necessary? This code emulates the flutter test environment (it sets debugEmulateFlutterTesterEnvironment to true below). So it shouldn't affect e2e tests. Those should not hit this code.

yjbanov · 2020-03-25T18:06:49Z

lib/web_ui/test/golden_tests/engine/compositing_golden_test.dart

+
+    builder.pushTransform(Matrix4.diagonal3Values(
+        EngineWindow.browserDevicePixelRatio,
+        EngineWindow.browserDevicePixelRatio, 1.0).storage);


This change wouldn't be necessary if we applied 1/DPR to <flt-scene> when the root transform is missing.

I think some of our benchmarks will fail for the same reason.

Agreed. Would like to change ViewConfiguration so we don't have to do this in the first place on the framework side.

ferhatb · 2020-03-25T19:40:39Z

merging on luci-engine infra fail.

…sical. (flutter/engine#17209)

ferhatb added 4 commits March 18, 2020 11:52

Switch ui.window.devicePixelRatio to browser dpi

65fb5d3

Rename debugOverrideDevicePixelRation to override. Add integration te…

bd58d35

…st for multi res image loading

Merge remote-tracking branch 'upstream/master' into devicepixels

2165d86

Update cirrus for new integration test

542be23

ferhatb requested review from harryterkelsen, nturgut and yjbanov March 19, 2020 16:34

googlebot added the cla: yes label Mar 19, 2020

Merge remote-tracking branch 'upstream/master' into devicepixels

8705651

harryterkelsen approved these changes Mar 19, 2020

View reviewed changes

update canvas golden test root transform

aab46b4

nturgut reviewed Mar 19, 2020

View reviewed changes

.cirrus.yml Show resolved Hide resolved

yjbanov reviewed Mar 19, 2020

View reviewed changes

ferhatb added 3 commits March 19, 2020 14:48

Update compositing golden test with dpr transform

101083d

Address review comment

a328874

remove print

3af31ef

nturgut reviewed Mar 19, 2020

View reviewed changes

lib/web_ui/lib/src/engine/window.dart Show resolved Hide resolved

nturgut approved these changes Mar 19, 2020

View reviewed changes

mdebbar reviewed Mar 20, 2020

View reviewed changes

yjbanov mentioned this pull request Mar 20, 2020

Web engine does not respond to all platform messages flutter/flutter#52929

Closed

yjbanov approved these changes Mar 20, 2020

View reviewed changes

ferhatb added 8 commits March 20, 2020 09:01

Merge remote-tracking branch 'upstream/master' into devicepixels

472615b

Merge remote-tracking branch 'upstream/master' into devicepixels

489ad43

update test

7d31069

Merge remote-tracking branch 'upstream/master' into devicepixels

3ae5e40

Merge remote-tracking branch 'upstream/master' into devicepixels

16a0f0f

Merge remote-tracking branch 'upstream/master' into devicepixels

d8907b0

Addressed review comments/replyChannel

a2aaafb

Merge remote-tracking branch 'upstream/master' into devicepixels

19da8b6

ferhatb added 5 commits March 24, 2020 10:51

fix assert return

c34111b

CI test

47da9ce

CI test

81fd917

CI test

940e44d

CI test

1d864dc

yjbanov reviewed Mar 24, 2020

View reviewed changes

ferhatb added 14 commits March 24, 2020 13:07

CI Test

2419fdb

CI Test

d455d74

Test CI

be2113d

Test CI

6b227a7

rename override to debugOverride

5f21300

Revert to pre CI test code that was reviewed

2eaae7e

Merge remote-tracking branch 'upstream/master' into devicepixels

50f6c64

fix typo and address review comment

158533c

Fix check for top level matrix

de3b6a7

Merge remote-tracking branch 'upstream/master' into devicepixels

fcffbec

Update assert for top level matrix to allow subclassing of window in …

b710b87

…flutter_test

Merge remote-tracking branch 'upstream/master' into devicepixels

68e85ca

update assert to handle null _debugDevicePixels

01c5bf5

Restrict top level transform check

47a11e5

ferhatb requested a review from yjbanov March 25, 2020 17:48

yjbanov approved these changes Mar 25, 2020

View reviewed changes

address review comments

ea15a3c

ferhatb merged commit 04182cd into flutter:master Mar 25, 2020

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2020

04182cd Switch ui.window.devicePixelRatio from browser logical to phy…

7511025

…sical. (flutter/engine#17209)

engine-flutter-autoroll mentioned this pull request Mar 25, 2020

Roll engine e79aef61d166..f1008d9bf99e (4 commits) flutter/flutter#53312

Merged

rxlabz mentioned this pull request Apr 30, 2020

Flutter web crashes when moving between two screens with different pixelRatio flutter/flutter#56050

Closed

ferhatb deleted the devicepixels branch February 11, 2021 17:43

kenNg1 mentioned this pull request Aug 7, 2022

Flutter web cannot render high resolution images which are resolution aware flutter/flutter#109109

Closed

Switch ui.window.devicePixelRatio from browser logical to physical. #17209

Switch ui.window.devicePixelRatio from browser logical to physical. #17209

Uh oh!

Conversation

ferhatb commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yjbanov commented Mar 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferhatb commented Mar 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ferhatb commented Mar 19, 2020 •

edited

Loading