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.

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

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

Merged
merged 3 commits into from
Jan 7, 2020

Conversation

ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Jan 7, 2020

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.

// 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

@ferhatb ferhatb merged commit 45bbbd7 into flutter:master Jan 7, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jan 8, 2020
flutter/engine@3f52888...a50f1ef

git log 3f52888..a50f1ef --first-parent --oneline
2020-01-08 gw280@google.com Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} (flutter/engine#15118)
2020-01-07 ferhat@gmail.com Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. (flutter/engine#15153)
2020-01-07 ferhat@gmail.com Recover when browser throws on ImageElement.decode due to too many images (flutter/engine#15160)
2020-01-07 garyq@google.com Fix RectHeightStyle::kMax ascent computation bug (flutter/engine#15106)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC garyq@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
bkonyi pushed a commit to flutter/flutter that referenced this pull request Jan 9, 2020
flutter/engine@3f52888...a50f1ef

git log 3f52888..a50f1ef --first-parent --oneline
2020-01-08 gw280@google.com Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} (flutter/engine#15118)
2020-01-07 ferhat@gmail.com Refactor BitmapCanvas, lazily allocate canvas, fix image composition bug. (flutter/engine#15153)
2020-01-07 ferhat@gmail.com Recover when browser throws on ImageElement.decode due to too many images (flutter/engine#15160)
2020-01-07 garyq@google.com Fix RectHeightStyle::kMax ascent computation bug (flutter/engine#15106)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC garyq@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
@ferhatb ferhatb deleted the image_decode_fix branch January 9, 2020 22:57
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web] Random images are not displayed after switching tabs
3 participants