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

Let pushColorFilter accept all types of ColorFilters #9641

Merged
dnfield merged 19 commits into from
Jul 10, 2019
Merged

Let pushColorFilter accept all types of ColorFilters #9641

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5af08d0
Let pushColorFilter accept all types of ColorFilters
dnfield Jul 1, 2019
059bf04
release typed data, check matrix len
dnfield Jul 2, 2019
b7b4aa8
20 entries, not 25
dnfield Jul 2, 2019
a1e9319
20 entries, not 25
dnfield Jul 2, 2019
ed6fa6d
move const to right spot
dnfield Jul 2, 2019
a3b9075
tests
dnfield Jul 3, 2019
ff4ed6b
Refactor ColorFilter to have a native wrapper
dnfield Jul 3, 2019
4b820ed
comment
dnfield Jul 3, 2019
8935830
tests
dnfield Jul 8, 2019
1bfc97c
typo
dnfield Jul 8, 2019
919f2d3
merge
dnfield Jul 8, 2019
529803d
Merge remote-tracking branch 'upstream/master' into push_color_filter
dnfield Jul 8, 2019
f10ecfd
merge
dnfield Jul 8, 2019
c8a8c35
Merge remote-tracking branch 'upstream/master' into native_color_filter
dnfield Jul 8, 2019
884f0e3
Use right accessor
dnfield Jul 8, 2019
c6f3421
Merge branch 'native_color_filter' into push_color_filter
dnfield Jul 8, 2019
9f132cf
merge
dnfield Jul 8, 2019
b6a06a9
merge!
dnfield Jul 8, 2019
75a88a5
remove dead code
dnfield Jul 8, 2019
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
6 changes: 3 additions & 3 deletions flow/layers/color_filter_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

namespace flutter {

ColorFilterLayer::ColorFilterLayer(SkColor color, SkBlendMode blend_mode)
: color_(color), blend_mode_(blend_mode) {}
ColorFilterLayer::ColorFilterLayer(sk_sp<SkColorFilter> filter)
: filter_(std::move(filter)) {}

ColorFilterLayer::~ColorFilterLayer() = default;

Expand All @@ -16,7 +16,7 @@ void ColorFilterLayer::Paint(PaintContext& context) const {
FML_DCHECK(needs_painting());

SkPaint paint;
paint.setColorFilter(SkColorFilters::Blend(color_, blend_mode_));
paint.setColorFilter(filter_);

Layer::AutoSaveLayer save =
Layer::AutoSaveLayer::Create(context, paint_bounds(), &paint);
Expand Down
7 changes: 4 additions & 3 deletions flow/layers/color_filter_layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,19 @@

#include "flutter/flow/layers/container_layer.h"

#include "third_party/skia/include/core/SkColorFilter.h"

namespace flutter {

class ColorFilterLayer : public ContainerLayer {
public:
ColorFilterLayer(SkColor color, SkBlendMode blend_mode);
ColorFilterLayer(sk_sp<SkColorFilter> filter);
~ColorFilterLayer() override;

void Paint(PaintContext& context) const override;

private:
SkColor color_;
SkBlendMode blend_mode_;
sk_sp<SkColorFilter> filter_;

FML_DISALLOW_COPY_AND_ASSIGN(ColorFilterLayer);
};
Expand Down
2 changes: 1 addition & 1 deletion lib/stub_ui/lib/src/engine/compositor/layer_scene_builder.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class LayerSceneBuilder implements ui.SceneBuilder {
}

@override
ui.ColorFilterEngineLayer pushColorFilter(ui.Color color, ui.BlendMode blendMode,
ui.ColorFilterEngineLayer pushColorFilter(ui.ColorFilter filter,
{ui.ColorFilterEngineLayer oldLayer}) {
throw new UnimplementedError();
}
Expand Down
2 changes: 1 addition & 1 deletion lib/stub_ui/lib/src/ui/compositing.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ class SceneBuilder {
/// blend mode.
///
/// See [pop] for details about the operation stack.
ColorFilterEngineLayer pushColorFilter(Color color, BlendMode blendMode, { ColorFilterEngineLayer oldLayer }) {
ColorFilterEngineLayer pushColorFilter(ColorFilter filter, { ColorFilterEngineLayer oldLayer }) {
throw new UnimplementedError();
}

Expand Down
26 changes: 24 additions & 2 deletions lib/ui/compositing.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,13 +387,35 @@ class SceneBuilder extends NativeFieldWrapperClass2 {
/// {@macro dart.ui.sceneBuilder.oldLayerVsRetained}
///
/// See [pop] for details about the operation stack.
ColorFilterEngineLayer pushColorFilter(Color color, BlendMode blendMode, { ColorFilterEngineLayer oldLayer }) {
ColorFilterEngineLayer pushColorFilter(ColorFilter filter, { ColorFilterEngineLayer oldLayer }) {
assert(filter != null);
assert(filter._type != null && filter._type != ColorFilter._TypeNone);
assert(_debugCheckCanBeUsedAsOldLayer(oldLayer, 'pushColorFilter'));
final ColorFilterEngineLayer layer = ColorFilterEngineLayer._(_pushColorFilter(color.value, blendMode.index));
EngineLayer engineLayer;
switch(filter._type) {
case ColorFilter._TypeMode:
engineLayer = _pushColorFilter(filter._color.value, filter._blendMode.index);
break;
case ColorFilter._TypeMatrix:
engineLayer = _pushColorFilterMatrix(Float32List.fromList(filter._matrix));
break;
case ColorFilter._TypeLinearToSrgbGamma:
engineLayer = _pushColorFilterLinearToSrgbGamma();
break;
case ColorFilter._TypeSrgbToLinearGamma:
engineLayer = _pushColorFilterSrgbToLinearGamma();
break;
default:
assert(false);
}
final ColorFilterEngineLayer layer = ColorFilterEngineLayer._(engineLayer);
assert(_debugPushLayer(layer));
return layer;
}
EngineLayer _pushColorFilter(int color, int blendMode) native 'SceneBuilder_pushColorFilter';

@liyuqian liyuqian Jul 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@dnfield dnfield Jul 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@liyuqian liyuqian Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@dnfield dnfield Jul 3, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@liyuqian liyuqian Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@dnfield dnfield Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

EngineLayer _pushColorFilterMatrix(Float32List matrix) native 'SceneBuilder_pushColorFilterMatrix';
EngineLayer _pushColorFilterLinearToSrgbGamma() native 'SceneBuilder_pushColorFilterLinearToSrgbGamma';
EngineLayer _pushColorFilterSrgbToLinearGamma() native 'SceneBuilder_pushColorFilterSrgbToLinearGamma';

/// Pushes a backdrop filter operation onto the operation stack.
///
Expand Down
31 changes: 30 additions & 1 deletion lib/ui/compositing/scene_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ IMPLEMENT_WRAPPERTYPEINFO(ui, SceneBuilder);
V(SceneBuilder, pushClipPath) \
V(SceneBuilder, pushOpacity) \
V(SceneBuilder, pushColorFilter) \
V(SceneBuilder, pushColorFilterMatrix) \
V(SceneBuilder, pushColorFilterLinearToSrgbGamma) \
V(SceneBuilder, pushColorFilterSrgbToLinearGamma) \
V(SceneBuilder, pushBackdropFilter) \
V(SceneBuilder, pushShaderMask) \
V(SceneBuilder, pushPhysicalShape) \
Expand Down Expand Up @@ -143,8 +146,34 @@ fml::RefPtr<EngineLayer> SceneBuilder::pushOpacity(int alpha,

fml::RefPtr<EngineLayer> SceneBuilder::pushColorFilter(int color,
int blendMode) {
auto layer =
std::make_shared<flutter::ColorFilterLayer>(SkColorFilters::Blend(
static_cast<SkColor>(color), static_cast<SkBlendMode>(blendMode)));
PushLayer(layer);
return EngineLayer::MakeRetained(layer);
}

fml::RefPtr<EngineLayer> SceneBuilder::pushColorFilterMatrix(
tonic::Float32List& matrix) {
FML_DCHECK(matrix.num_elements() == 20);

auto layer = std::make_shared<flutter::ColorFilterLayer>(
MakeColorMatrixFilter255(matrix.data()));
matrix.Release();
PushLayer(layer);
return EngineLayer::MakeRetained(layer);
}

fml::RefPtr<EngineLayer> SceneBuilder::pushColorFilterLinearToSrgbGamma() {
auto layer = std::make_shared<flutter::ColorFilterLayer>(
SkColorFilters::LinearToSRGBGamma());
PushLayer(layer);
return EngineLayer::MakeRetained(layer);
}

fml::RefPtr<EngineLayer> SceneBuilder::pushColorFilterSrgbToLinearGamma() {
auto layer = std::make_shared<flutter::ColorFilterLayer>(
static_cast<SkColor>(color), static_cast<SkBlendMode>(blendMode));
SkColorFilters::SRGBToLinearGamma());
PushLayer(layer);
return EngineLayer::MakeRetained(layer);
}
Expand Down
3 changes: 3 additions & 0 deletions lib/ui/compositing/scene_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class SceneBuilder : public RefCountedDartWrappable<SceneBuilder> {
int clipBehavior);
fml::RefPtr<EngineLayer> pushOpacity(int alpha, double dx = 0, double dy = 0);
fml::RefPtr<EngineLayer> pushColorFilter(int color, int blendMode);
fml::RefPtr<EngineLayer> pushColorFilterMatrix(tonic::Float32List& matrix);
fml::RefPtr<EngineLayer> pushColorFilterLinearToSrgbGamma();
fml::RefPtr<EngineLayer> pushColorFilterSrgbToLinearGamma();
fml::RefPtr<EngineLayer> pushBackdropFilter(ImageFilter* filter);
fml::RefPtr<EngineLayer> pushShaderMask(Shader* shader,
double maskRectLeft,
Expand Down
4 changes: 3 additions & 1 deletion lib/ui/painting.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2529,7 +2529,9 @@ class ColorFilter {
/// matrix is in row-major order and the translation column is specified in
/// unnormalized, 0...255, space.
const ColorFilter.matrix(List<double> matrix)
: _color = null,
: assert(matrix != null, 'Color Matrix argument was null.'),
assert(matrix.length == 20, 'Color Matrix must have 20 entries.'),
_color = null,
_blendMode = null,
_matrix = matrix,
_type = _TypeMatrix;
Expand Down
10 changes: 10 additions & 0 deletions lib/ui/painting/matrix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,14 @@ tonic::Float64List ToMatrix4(const SkMatrix& sk_matrix) {
return matrix4;
}

sk_sp<SkColorFilter> MakeColorMatrixFilter255(const float color_matrix[20]) {
float tmp[20];
memcpy(tmp, color_matrix, sizeof(tmp));
tmp[4] *= 1.0f / 255;
tmp[9] *= 1.0f / 255;
tmp[14] *= 1.0f / 255;
tmp[19] *= 1.0f / 255;
return SkColorFilters::Matrix(tmp);
}

} // namespace flutter
6 changes: 6 additions & 0 deletions lib/ui/painting/matrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_LIB_UI_PAINTING_MATRIX_H_
#define FLUTTER_LIB_UI_PAINTING_MATRIX_H_

#include "third_party/skia/include/core/SkColorFilter.h"
#include "third_party/skia/include/core/SkMatrix.h"
#include "third_party/tonic/typed_data/typed_list.h"

Expand All @@ -13,6 +14,11 @@ namespace flutter {
SkMatrix ToSkMatrix(const tonic::Float64List& matrix4);
tonic::Float64List ToMatrix4(const SkMatrix& sk_matrix);

// Flutter still defines the matrix to be biased by 255 in the last column
// (translate). skia is normalized, treating the last column as 0...1, so we
// post-scale here before calling the skia factory.
sk_sp<SkColorFilter> MakeColorMatrixFilter255(const float color_matrix[20]);

} // namespace flutter

#endif // FLUTTER_LIB_UI_PAINTING_MATRIX_H_
14 changes: 1 addition & 13 deletions lib/ui/painting/paint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "flutter/fml/logging.h"
#include "flutter/lib/ui/painting/image_filter.h"
#include "flutter/lib/ui/painting/matrix.h"
#include "flutter/lib/ui/painting/shader.h"
#include "third_party/skia/include/core/SkColorFilter.h"
#include "third_party/skia/include/core/SkImageFilter.h"
Expand Down Expand Up @@ -75,19 +76,6 @@ enum ColorFilterType {
SRGBToLinearGamma
};

// Flutter still defines the matrix to be biased by 255 in the last column
// (translate). skia is normalized, treating the last column as 0...1, so we
// post-scale here before calling the skia factory.
static sk_sp<SkColorFilter> MakeColorMatrixFilter255(const float array[20]) {
float tmp[20];
memcpy(tmp, array, sizeof(tmp));
tmp[4] *= 1.0f / 255;
tmp[9] *= 1.0f / 255;
tmp[14] *= 1.0f / 255;
tmp[19] *= 1.0f / 255;
return SkColorFilters::Matrix(tmp);
}

sk_sp<SkColorFilter> ExtractColorFilter(const uint32_t* uint_data,
Dart_Handle* values) {
switch (uint_data[kColorFilterIndex]) {
Expand Down
8 changes: 7 additions & 1 deletion testing/dart/compositing_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(

@liyuqian liyuqian Jul 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@dnfield dnfield Jul 3, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do

const ColorFilter.mode(
Color.fromARGB(0, 0, 0, 0),
BlendMode.color,
),
oldLayer: oldLayer,
);
});
testNoSharing((SceneBuilder builder, EngineLayer oldLayer) {
return builder.pushBackdropFilter(ImageFilter.blur(), oldLayer: oldLayer);
Expand Down