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

Let pushColorFilter accept all types of ColorFilters #9641

Merged
merged 19 commits into from Jul 10, 2019

Conversation

@dnfield
Copy link
Member

commented Jul 1, 2019

Fixes flutter/flutter#35415

This allows consumers to apply all supported types of color filters to a scene.

This is a breaking change, but we don't actually use this code anywhere in the framework so I expect this code to pass.

Breaking change announcement: https://groups.google.com/forum/#!topic/flutter-announce/bdhRbfwg6jA

@dnfield dnfield requested review from chinmaygarde, yjbanov and liyuqian Jul 1, 2019
@googlebot googlebot added the cla: yes label Jul 1, 2019
@dnfield

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

This has at least one bug in it right now that I'm working on... crashes when you pass it a matrix.

dnfield added 3 commits Jul 2, 2019
@dnfield

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

This should be good now. Will have tests upstream in framework.

@dnfield dnfield referenced this pull request Jul 2, 2019
8 of 9 tasks complete
default:
assert(false);
}
final ColorFilterEngineLayer layer = ColorFilterEngineLayer._(engineLayer);
assert(_debugPushLayer(layer));
return layer;
}
EngineLayer _pushColorFilter(int color, int blendMode) native 'SceneBuilder_pushColorFilter';

This comment has been minimized.

Copy link
@liyuqian

liyuqian Jul 2, 2019

Contributor

Instead of implementing 4 pushColorFilter variants, maybe it's cleaner to define a single pushColorFilter that takes a sk_sp<SkColorFilter>, and expose ExtractColorFilter from paint.cc to generate the SkColorFilter.

This comment has been minimized.

Copy link
@dnfield

dnfield Jul 3, 2019

Author Member

My concern with that is the Paint version is a bit hacky and was mainly done to maintain backwards compatibility.

Paint can afford to do this because it already encodes/serializes this data to the engine side. Doing that is not free. I'd rather use this approach because it avoids allocating unnecessary typed data (which is demonstrably slow), and avoids sending data over to thenative side that we don't need.

This comment has been minimized.

Copy link
@liyuqian

liyuqian Jul 3, 2019

Contributor

Thanks for pointing that out! I didn't realize that ColorFilter doesn't have a corresponding native SkColorFilter object. Is it possible to create such a native object during the construction of ColorFilter, so Paint can just store it inside _objects (like how Shader is stored), and pushColorFilter can also directly use it without triggering additional Dart to native conversion costs?

I guess that I'm Ok with having 4 pushColorFilter*** in scene_builder.cc but it just feels a little weird as every previous push*** in scene_builder.cc has a one-to-one relationship with the SceneBuilder.push*** in compositing.dart. I'll consult @chinmaygarde and @yjbanov to see how they feel about 4 pushColorFilter*** variants in scene_builder.cc.

This comment has been minimized.

Copy link
@dnfield

dnfield Jul 3, 2019

Author Member

That may be a better way to go - just make ColorFilter a NativeFieldWrapper2 class. Do you want that to be part of this pull request or should we split it out?

This comment has been minimized.

Copy link
@liyuqian

liyuqian Jul 3, 2019

Contributor

Let's have it here if it's not complicating this PR too much :) (I was hoping that it could even simplify this PR a little bit.)

This comment has been minimized.

Copy link
@dnfield

dnfield Jul 8, 2019

Author Member

I split that effort out into #9668 which has landed and been merged into this PR. PTAL.

@@ -274,7 +274,13 @@ void main() {
return builder.pushOpacity(100, oldLayer: oldLayer);
});
testNoSharing((SceneBuilder builder, EngineLayer oldLayer) {
return builder.pushColorFilter(const Color.fromARGB(0, 0, 0, 0), BlendMode.color, oldLayer: oldLayer);
return builder.pushColorFilter(

This comment has been minimized.

Copy link
@liyuqian

liyuqian Jul 2, 2019

Contributor

Let's add test coverage for all 4 types of ColorFilters. I think they don't have to be put in the SceneBuilder does not share a layer between addRetained and push* test. Maybe just add a new test pushColorFilter handles all 4 types?

This comment has been minimized.

Copy link
@dnfield

dnfield Jul 3, 2019

Author Member

Will do

dnfield added 2 commits Jul 3, 2019
dnfield added 11 commits Jul 3, 2019
@dnfield dnfield force-pushed the dnfield:push_color_filter branch from a226e13 to 882ca85 Jul 8, 2019
@dnfield dnfield force-pushed the dnfield:push_color_filter branch from 882ca85 to 75a88a5 Jul 8, 2019
Copy link
Contributor

left a comment

LGTM

@dnfield dnfield merged commit 56885f7 into flutter:master Jul 10, 2019
17 checks passed
17 checks passed
WIP Ready for review
Details
build_and_test_host Task Summary
Details
build_and_test_host
Details
build_and_test_host_profile Task Summary
Details
build_and_test_host_profile
Details
build_and_test_host_release Task Summary
Details
build_and_test_host_release
Details
build_android Task Summary
Details
build_android
Details
build_windows_debug Task Summary
Details
build_windows_debug
Details
build_windows_debug_unopt Task Summary
Details
build_windows_debug_unopt
Details
cla/google All necessary CLAs are signed
format_and_dart_test Task Summary
Details
format_and_dart_test
Details
luci-engine
Details
@dnfield dnfield deleted the dnfield:push_color_filter branch Jul 10, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jul 11, 2019
flutter/engine@75387db...49445ce

git log 75387dbc147d1cee8164f5d61582eab8ba1e5058..49445ce4949bf0d28506a6b22e28b4264f624e5f --no-merges --oneline
49445ce49 FLEViewController/Engine API changes (flutter/engine#9750)
2a79462fd Add Fuchsia build CI presubmit steps (flutter/engine#9736)
67cebdb92 Roll fuchsia/sdk/core/linux-amd64 from KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC to ehWVT9QJbC-vFMM6SkkQM9HJ9oITFCws7FC9JnrFq2gC (flutter/engine#9765)
089c74084 Roll fuchsia/sdk/core/mac-amd64 from lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C to EYnRdXFT9l-d8Qkz4zeTRXnqfV3KQzpQhoPs1r0-740C (flutter/engine#9759)
b22410ef6 Include SkParagraph headers only when the enable-skshaper flag is on (flutter/engine#9758)
2cd650d26 Minimal integration with the Skia text shaper module (flutter/engine#9556)
f775f5e4d Re-enable the Wuffs GIF decoder (flutter/engine#9466)
aca048236 Make all shell unit tests use the OpenGL rasterizer. (flutter/engine#9746)
bc57291ff Make FLEViewController&#39;s view an internal detail (flutter/engine#9741)
9776043ea Synchronize main thread and gpu thread for first render frame (flutter/engine#9506)
f600ae830 Use libc&#43;&#43; variant of string view and remove the FML variant. (flutter/engine#9737)
564f53f0a Revert &#34;Improve caching limits for Skia (#9503)&#34; (flutter/engine#9740)
b453d3c3d libtxt: fix reference counting of SkFontStyleSets held by font asset providers (flutter/engine#9561)
fa7627d17 Fix backspace crash on Chinese devices (flutter/engine#9734)
56885f79b Let pushColorFilter accept all types of ColorFilters (flutter/engine#9641)
6dccb21e6 Roll src/third_party/skia 96fdfe0fe88e..af4e7b6cf616 (1 commits) (flutter/engine#9735)
8511d9b47 Roll fuchsia/sdk/core/mac-amd64 from byM-kyxL4bemlTYNqhKUfJfZoIUrCSzS6XzsFr4n9-MC to lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C (flutter/engine#9742)
b3bf0a182 Roll fuchsia/sdk/core/linux-amd64 from I2Qe1zxgckzIzMBTztvzeWYsDgcb9Fw-idSI16oIlx8C to KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC (flutter/engine#9743)
7e568232a Fix windows test by not attempting to open a directory as a file. (flutter/engine#9745)
6cf0d1350 Roll src/third_party/skia a3ffaabcc4f2..96fdfe0fe88e (5 commits) (flutter/engine#9731)
49a00aed8 Fix Fuchsia build. (flutter/engine#9730)
b3bb39b0b Roll src/third_party/dart 06c3d7ad3a...09fc76bc51 (flutter/engine#9728)
2284210f4 Make the license script compatible with recently changed Dart I/O stream APIs (flutter/engine#9725)
ad582b508 Rework image &amp; texture management to use concurrent message queues. (flutter/engine#9486)
1dcd5f52b Roll src/third_party/skia 6b82cf638682..a3ffaabcc4f2 (24 commits) (flutter/engine#9726)
129979cab Revert &#34;Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)&#34; (flutter/engine#9724)
8020d7e43 Roll src/third_party/skia 56065d9b875f..6b82cf638682 (3 commits) (flutter/engine#9718)
e24bd7846 Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)
3d2668c35 Reland isolate group changes
802bd1518 iOS platform view opacity (flutter/engine#9667)
3b6265b74 Roll src/third_party/dart b5aeaa6796..06c3d7ad3a (44 commits)
887e05233 Refactor ColorFilter to have a native wrapper (flutter/engine#9668)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (garyq@google.com), and stop
the roller if necessary.
johnsonmh added a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
flutter/engine@75387db...49445ce

git log 75387dbc147d1cee8164f5d61582eab8ba1e5058..49445ce4949bf0d28506a6b22e28b4264f624e5f --no-merges --oneline
49445ce49 FLEViewController/Engine API changes (flutter/engine#9750)
2a79462fd Add Fuchsia build CI presubmit steps (flutter/engine#9736)
67cebdb92 Roll fuchsia/sdk/core/linux-amd64 from KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC to ehWVT9QJbC-vFMM6SkkQM9HJ9oITFCws7FC9JnrFq2gC (flutter/engine#9765)
089c74084 Roll fuchsia/sdk/core/mac-amd64 from lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C to EYnRdXFT9l-d8Qkz4zeTRXnqfV3KQzpQhoPs1r0-740C (flutter/engine#9759)
b22410ef6 Include SkParagraph headers only when the enable-skshaper flag is on (flutter/engine#9758)
2cd650d26 Minimal integration with the Skia text shaper module (flutter/engine#9556)
f775f5e4d Re-enable the Wuffs GIF decoder (flutter/engine#9466)
aca048236 Make all shell unit tests use the OpenGL rasterizer. (flutter/engine#9746)
bc57291ff Make FLEViewController&flutter#39;s view an internal detail (flutter/engine#9741)
9776043ea Synchronize main thread and gpu thread for first render frame (flutter/engine#9506)
f600ae830 Use libc&flutter#43;&flutter#43; variant of string view and remove the FML variant. (flutter/engine#9737)
564f53f0a Revert &flutter#34;Improve caching limits for Skia (flutter#9503)&flutter#34; (flutter/engine#9740)
b453d3c3d libtxt: fix reference counting of SkFontStyleSets held by font asset providers (flutter/engine#9561)
fa7627d17 Fix backspace crash on Chinese devices (flutter/engine#9734)
56885f79b Let pushColorFilter accept all types of ColorFilters (flutter/engine#9641)
6dccb21e6 Roll src/third_party/skia 96fdfe0fe88e..af4e7b6cf616 (1 commits) (flutter/engine#9735)
8511d9b47 Roll fuchsia/sdk/core/mac-amd64 from byM-kyxL4bemlTYNqhKUfJfZoIUrCSzS6XzsFr4n9-MC to lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C (flutter/engine#9742)
b3bf0a182 Roll fuchsia/sdk/core/linux-amd64 from I2Qe1zxgckzIzMBTztvzeWYsDgcb9Fw-idSI16oIlx8C to KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC (flutter/engine#9743)
7e568232a Fix windows test by not attempting to open a directory as a file. (flutter/engine#9745)
6cf0d1350 Roll src/third_party/skia a3ffaabcc4f2..96fdfe0fe88e (5 commits) (flutter/engine#9731)
49a00aed8 Fix Fuchsia build. (flutter/engine#9730)
b3bb39b0b Roll src/third_party/dart 06c3d7ad3a...09fc76bc51 (flutter/engine#9728)
2284210f4 Make the license script compatible with recently changed Dart I/O stream APIs (flutter/engine#9725)
ad582b508 Rework image &amp; texture management to use concurrent message queues. (flutter/engine#9486)
1dcd5f52b Roll src/third_party/skia 6b82cf638682..a3ffaabcc4f2 (24 commits) (flutter/engine#9726)
129979cab Revert &flutter#34;Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)&flutter#34; (flutter/engine#9724)
8020d7e43 Roll src/third_party/skia 56065d9b875f..6b82cf638682 (3 commits) (flutter/engine#9718)
e24bd7846 Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)
3d2668c35 Reland isolate group changes
802bd1518 iOS platform view opacity (flutter/engine#9667)
3b6265b74 Roll src/third_party/dart b5aeaa6796..06c3d7ad3a (44 commits)
887e05233 Refactor ColorFilter to have a native wrapper (flutter/engine#9668)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (garyq@google.com), and stop
the roller if necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.