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

Recover when browser throws on ImageElement.decode due to too many images #15160

Merged
merged 3 commits into from
Jan 7, 2020
Merged

Recover when browser throws on ImageElement.decode due to too many images #15160

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91cdfae
Recover when browser throws on ImageElement.decode due to too many im…
ferhatb Jan 7, 2020
e745fa4
Fix comment
ferhatb Jan 7, 2020
74aea55
Merge remote-tracking branch 'upstream/master' into image_decode_fix
ferhatb Jan 7, 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
59 changes: 33 additions & 26 deletions lib/web_ui/lib/src/engine/html_image_codec.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// found in the LICENSE file.

part of engine;

final bool _supportsDecode = js_util.getProperty(
js_util.getProperty(
js_util.getProperty(html.window, 'Image'), 'prototype'),
Expand All @@ -23,46 +22,54 @@ class HtmlCodec implements ui.Codec {

@override
Future<ui.FrameInfo> getNextFrame() async {
StreamSubscription<html.Event> loadSubscription;
StreamSubscription<html.Event> errorSubscription;
final Completer<ui.FrameInfo> completer = Completer<ui.FrameInfo>();
final html.ImageElement imgElement = html.ImageElement();
// If the browser doesn't support asynchronous decoding of an image,
// then use the `onload` event to decide when it's ready to paint to the
// DOM. Unfortunately, this will case the image to be decoded synchronously
// on the main thread, and may cause dropped framed.
if (!_supportsDecode) {
loadSubscription = imgElement.onLoad.listen((html.Event event) {
loadSubscription.cancel();
errorSubscription.cancel();
if (_supportsDecode) {
final html.ImageElement imgElement = html.ImageElement();
imgElement.src = src;
js_util.setProperty(imgElement, 'decoding', 'async');
imgElement.decode().then((dynamic _) {
final HtmlImage image = HtmlImage(
imgElement,
imgElement.naturalWidth,
imgElement.naturalHeight,
);
completer.complete(SingleFrameInfo(image));
}).catchError((e) {
// This code path is hit on Chrome 80.0.3987.16 when too many
// images are on the page (~1000).
// Fallback here is to load using onLoad instead.
_decodeUsingOnLoad(completer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better strategy might be to have a configurable limit on decoding concurrency. We'd then just pick a limit per browser. That may not obviate the need for this fallback, but it would be good to not rely on the fallback during normal app execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if we set a limit for Chrome and monitored rate of decoding it could change over time with no way to detect. Since it is edge case and likely to be fixed (other js image libraries reverted usage, its a known issue), fallback seemed best option.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

});
} else {
_decodeUsingOnLoad(completer);
}
return completer.future;
}

void _decodeUsingOnLoad(Completer completer) {
StreamSubscription<html.Event> loadSubscription;
StreamSubscription<html.Event> errorSubscription;
final html.ImageElement imgElement = html.ImageElement();
// If the browser doesn't support asynchronous decoding of an image,
// then use the `onload` event to decide when it's ready to paint to the
// DOM. Unfortunately, this will case the image to be decoded synchronously
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: case => cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// on the main thread, and may cause dropped framed.
errorSubscription = imgElement.onError.listen((html.Event event) {
loadSubscription?.cancel();
errorSubscription.cancel();
completer.completeError(event);
});
loadSubscription = imgElement.onLoad.listen((html.Event event) {
loadSubscription.cancel();
errorSubscription.cancel();
final HtmlImage image = HtmlImage(
imgElement,
imgElement.naturalWidth,
imgElement.naturalHeight,
);
completer.complete(SingleFrameInfo(image));
});
imgElement.src = src;
// If the browser supports asynchronous image decoding, use that instead
// of `onload`.
if (_supportsDecode) {
imgElement.decode().then((dynamic _) {
errorSubscription.cancel();
final HtmlImage image = HtmlImage(
imgElement,
imgElement.naturalWidth,
imgElement.naturalHeight,
);
completer.complete(SingleFrameInfo(image));
});
}
return completer.future;
}

@override
Expand Down