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

[web] Support gif/webp animations, Speed up image drawing in BitmapCanvas. #13748

Merged
merged 11 commits into from
Nov 8, 2019

Conversation

ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Nov 8, 2019

Updated BitmapCanvas for:

Animated GIFS/Webp
Fixes: flutter/flutter#33618

drawImage API fails on canvas transform
Fixes: flutter/flutter#44362

ListView performance with grid of images:
flutter/flutter#42568

..position = 'absolute'
..transform = 'translate(${p.dx}px, ${p.dy}px)';
rootElement.append(imgElement);
..position = 'absolute';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assign this when we instantiate the element so we don't have to repeatedly assign it on every frame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to HtmlImage.

..position = 'absolute';
if (isClipped) {
final List<html.Element> clipElements =
_clipContent(_clipStack, imgElement, p, currentTransform);
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 this may result in undesired AA artifacts. If we clip multiple drawing operations (e.g. image and text) with the same clip, they will be placed on top of each other. Is there a way to create one clipping element per clip and reuse it across all the content that's being clipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see _clipContent implementation. There was a TODO to reduce/merge multiple clips which should be addressed separately. We need benchmarks to make sure it is not too expensive since some are complex svg paths.

..position = 'absolute';
if (isClipped) {
final List<html.Element> clipElements =
_clipContent(_clipStack, imgElement, p, currentTransform);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's something off with the indent

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. dartfmt.

}
} else {
final String cssTransform =
matrix4ToCssTransform(transformWithOffset(currentTransform, p));
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

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. dartfmt.

if (requiresClipping) {
if (src.width != image.width) {
double leftMargin = -src.left * (dst.width / src.width);
imageStyle.marginLeft = '${leftMargin.toStringAsFixed(2)}px';
Copy link
Contributor

Choose a reason for hiding this comment

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

IICR, margin is a layout-affecting CSS property. Dunno how much it matters here, but it may be slower than a transform, which only affects painting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx. switched to use offset.

rc.drawImageRect(testImage, Rect.fromLTRB(testWidth / 2, 0, testWidth, testHeight),
Rect.fromLTRB(100, 30, 2 * testWidth, 2 * testHeight), new Paint());
await _checkScreenshot(rc, 'draw_image_rect_with_transform_source');
});
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 tests with clips would be very useful too, since that part is a little tricky in the implementations. Particular the cases when two images are clipped by a single clip, or when an image and a paragraph are both clipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The transform with src test clips before rendering off center. It actually uncovered a bug when compared to drawImageRect screenshot that was taken with prior canvas drawImageScaled call.

@ferhatb ferhatb merged commit 7413304 into flutter:master Nov 8, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Nov 9, 2019
git@github.com:flutter/engine.git/compare/5f5713e33971...af04338

git log 5f5713e..af04338 --no-merges --oneline
2019-11-08 a-siva@users.noreply.github.com Manual roll of Dart e68ca9b652acdb642668a6acb5f630d5be6c03da...fa4379946109467c8a48f20f19d83d7c72968a3e (flutter/engine#13756)
2019-11-08 ychris@google.com Revert "Reland "Guarding EAGLContext used by Flutter #13314" (#13755)" (flutter/engine#13757)
2019-11-08 ferhat@gmail.com [web] Support gif/webp animations, Speed up image drawing in BitmapCanvas.  (flutter/engine#13748)
2019-11-08 ychris@google.com Reland "Guarding EAGLContext used by Flutter #13314" (flutter/engine#13755)
2019-11-08 gspencergoog@users.noreply.github.com Move TextRange from the framework to dart:ui. (flutter/engine#13747)
2019-11-08 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8c1e265f6f81..c88d1774ed50 (7 commits) (flutter/engine#13754)
2019-11-08 ychris@google.com Revert "Always use `IOSGLContextSwitch` to access EAGLContexts to prevent plugins from polluting Flutter's EAGLContext (#13314)" (flutter/engine#13753)
2019-11-08 ychris@google.com Always use `IOSGLContextSwitch` to access EAGLContexts to prevent plugins from polluting Flutter's EAGLContext (flutter/engine#13314)


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_speed0 branch November 11, 2019 17:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web] drawImage API fails on canvas transform [web] Implement / fix animated images (GIF/WebP)
4 participants