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

[canvaskit] Normalize color matrix translation values#32990

Merged
harryterkelsen merged 1 commit intoflutter:mainfrom
harryterkelsen:canvaskit-inverse-colormatrix
Apr 28, 2022
Merged

[canvaskit] Normalize color matrix translation values#32990
harryterkelsen merged 1 commit intoflutter:mainfrom
harryterkelsen:canvaskit-inverse-colormatrix

Conversation

@harryterkelsen
Copy link
Contributor

Flutter specifies that the values in the translation column of the color matrix are given in unnormalized 0..255 space. CanvasKit expects the translation values to be normalized. This change normalizes the color matrix. See https://api.flutter.dev/flutter/dart-ui/ColorFilter/ColorFilter.matrix.html

Fixes flutter/flutter#85707

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@harryterkelsen harryterkelsen requested a review from yjbanov April 28, 2022 19:52
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 28, 2022
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm

/// See [https://api.flutter.dev/flutter/dart-ui/ColorFilter/ColorFilter.matrix.html].
Float32List get _normalizedMatrix {
assert(matrix.length == 20, 'Color Matrix must have 20 entries.');
final Float32List result = Float32List(20);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be used as temporary storage. We could probably use CanvasKit.Malloc to avoid reallocating this matrix every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should only be called once (or once per resurrection if we don't have a finalizer registry)

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 1.
View them at https://flutter-engine-gold.skia.org/cl/github/32990

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #32990 at sha e1b4d9f

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-web Code specifically for the web engine will affect goldens

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ColorFiltered inverted map does not work on web

3 participants