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

Avoid sending a 0 DPR to framework #21389

Merged
nturgut merged 2 commits into from
Sep 25, 2020
Merged

Avoid sending a 0 DPR to framework #21389

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fa289f
Avoid sending a 0 DPR to framework
dnfield Sep 24, 2020
05e8cba
check width and height as well
dnfield Sep 24, 2020
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
11 changes: 1 addition & 10 deletions lib/ui/window/viewport_metrics.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,22 +42,13 @@ ViewportMetrics::ViewportMetrics(double p_device_pixel_ratio,
physical_system_gesture_inset_bottom(
p_physical_system_gesture_inset_bottom),
physical_system_gesture_inset_left(p_physical_system_gesture_inset_left) {
// Ensure we don't have nonsensical dimensions.
FML_DCHECK(physical_width >= 0);
FML_DCHECK(physical_height >= 0);
FML_DCHECK(device_pixel_ratio > 0);
}

ViewportMetrics::ViewportMetrics(double p_device_pixel_ratio,
double p_physical_width,
double p_physical_height)
: device_pixel_ratio(p_device_pixel_ratio),
physical_width(p_physical_width),
physical_height(p_physical_height) {
// Ensure we don't have nonsensical dimensions.
FML_DCHECK(physical_width >= 0);
FML_DCHECK(physical_height >= 0);
FML_DCHECK(device_pixel_ratio > 0);
}
physical_height(p_physical_height) {}

} // namespace flutter
9 changes: 9 additions & 0 deletions shell/common/fixtures/shell_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ void onPointerDataPacketMain() {
@pragma('vm:entry-point')
void emptyMain() {}

@pragma('vm:entry-point')
void reportDevicePixelRatio() {
window.onMetricsChanged = () {
_reportDevicePixelRatio(window.devicePixelRatio);
};
}

void _reportDevicePixelRatio(double devicePixelRatio) native 'ReportDevicePixelRatio';

@pragma('vm:entry-point')
void dummyReportTimingsMain() {
window.onReportTimings = (List<FrameTiming> timings) {};
Expand Down
6 changes: 6 additions & 0 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,12 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {
FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

if (metrics.device_pixel_ratio <= 0) {
Copy link
Contributor

@arbreng arbreng Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should also check the width and height for positivity, or a 2nd check could handle that. Otherwise, there seems to be nothing in the engine guarding against negative or 0 dimensions

A new test case below can also verify the width/height behavior

Copy link
Member

@chinmaygarde chinmaygarde Sep 24, 2020

Choose a reason for hiding this comment

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

IIRC a zero sized viewport just disengages the animator and is a valid case. That is also the initial viewport size.

Copy link
Contributor Author

@dnfield dnfield Sep 24, 2020

Choose a reason for hiding this comment

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

Done, and just updated the same test case.

Copy link
Contributor Author

@dnfield dnfield Sep 24, 2020

Choose a reason for hiding this comment

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

It is the initial size, but I'm not seeing where it disengages the animator.

What would it mean to actually set the viewport to zero width or height?

Copy link
Member

@chinmaygarde chinmaygarde Sep 24, 2020

Choose a reason for hiding this comment

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

Hmm, you're right. I thought Shell::OnPlatformViewSetViewportMetrics used to pause the animator when the size was zero. That doesn't not seem to be the case.

Copy link
Contributor

@arbreng arbreng Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The Engine skips rendering the generated LayerTree here: https://github.com/flutter/engine/blob/master/shell/common/engine.cc#L483

Perhaps there should be a DLOG there, or here in the shell

Copy link
Contributor

@arbreng arbreng Sep 24, 2020

Choose a reason for hiding this comment

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

Also note that LayerTree has another DCHECK for non-zero DPR: https://github.com/flutter/engine/blob/master/flow/layers/layer_tree.cc#L20

Copy link
Contributor Author

@dnfield dnfield Sep 24, 2020

Choose a reason for hiding this comment

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

That one's a CHECK. That'll just crash the whole process in release mode. It's probably .. ok to crash there rather than later.

FML_DLOG(ERROR)
<< "Embedding reported a device pixel ratio of 0, ignoring update.";
return;
}

// This is the formula Android uses.
// https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/renderthread/CacheManager.cpp#41
size_t max_bytes = metrics.physical_width * metrics.physical_height * 12 * 4;
Expand Down
45 changes: 45 additions & 0 deletions shell/common/shell_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2066,5 +2066,50 @@ TEST_F(ShellTest, DiscardLayerTreeOnResize) {
DestroyShell(std::move(shell));
}

TEST_F(ShellTest, IgnoresZeroDPR) {
fml::AutoResetWaitableEvent latch;
double last_device_pixel_ratio;
auto native_report_device_pixel_ratio = [&](Dart_NativeArguments args) {
auto dpr_handle = Dart_GetNativeArgument(args, 0);
ASSERT_TRUE(Dart_IsDouble(dpr_handle));
Dart_DoubleValue(dpr_handle, &last_device_pixel_ratio);
ASSERT_FALSE(last_device_pixel_ratio == 0.0);
latch.Signal();
};

Settings settings = CreateSettingsForFixture();
auto task_runner = CreateNewThread();
TaskRunners task_runners("test", task_runner, task_runner, task_runner,
task_runner);

AddNativeCallback("ReportDevicePixelRatio",
CREATE_NATIVE_ENTRY(native_report_device_pixel_ratio));

std::unique_ptr<Shell> shell =
CreateShell(std::move(settings), std::move(task_runners));

auto configuration = RunConfiguration::InferFromSettings(settings);
configuration.SetEntrypoint("reportDevicePixelRatio");

RunEngine(shell.get(), std::move(configuration));

task_runner->PostTask([&]() {
shell->GetPlatformView()->SetViewportMetrics({0.0, 400, 200});
task_runner->PostTask([&]() {
shell->GetPlatformView()->SetViewportMetrics({0.8, 400, 200});
});
});
latch.Wait();
ASSERT_EQ(last_device_pixel_ratio, 0.8);
latch.Reset();
task_runner->PostTask([&]() {
shell->GetPlatformView()->SetViewportMetrics({1.2, 400, 200});
});
latch.Wait();
ASSERT_EQ(last_device_pixel_ratio, 1.2);

DestroyShell(std::move(shell), std::move(task_runners));
}

} // namespace testing
} // namespace flutter