[Impeller] Special case color filter in tiled texture. #39445
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
|
TBD how we should test this, texture allocation or whatnot. |
before
after
|
chinmaygarde
changed the title
impeller/renderer/allocator.cc
Outdated
| uint64_t Allocator::GetAllocatedSize() const { | ||
| return allocated_size_; | ||
| } | ||
|
|
||
| void Allocator::ResetAllocatedSize() { | ||
| allocated_size_ = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Perhaps we should add/remove these with some ifdef.
There was a problem hiding this comment.
Another idea is that if we do this only in builds for engine unit tests, we could probably track a vector of allocated sizes which make assertions easier
chinmaygarde
changed the title
impeller/renderer/allocator.h
Outdated
| @@ -37,6 +37,15 @@ class Allocator { | |||
| /// | |||
| virtual uint16_t MinimumBytesPerRow(PixelFormat format) const; | |||
|
|
|||
| // Debug/Profile only functionality for tracking the size of allocations | |||
| #if (FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE) | |||
| const std::vector<ISize>& GetAllocatedSizes() const; | |||
There was a problem hiding this comment.
I am not sure if you intend to incorporate the allocator instrumentation into this patch. If it helps speeds thing along for this patch, maybe we can back this out and land this patch while working on the instrumentation separately. Your call.
There was a problem hiding this comment.
I think now that I have an idea of how we will test this, I feel more comfortable backing this change out and working on it separately
| std::shared_ptr<Texture> texture_; | ||
| SamplerDescriptor sampler_descriptor_ = {}; | ||
| Entity::TileMode x_tile_mode_ = Entity::TileMode::kClamp; | ||
| Entity::TileMode y_tile_mode_ = Entity::TileMode::kClamp; | ||
| std::optional<ColorFilterProc> color_filter_; |
There was a problem hiding this comment.
It'd be good to note why this this is here as a naive implementation (like the one earlier) shouldn't have to care about the filter.
There was a problem hiding this comment.
Added a comment here and a breadcrumb in paint.cc
|
Backed out the allocator change. Will open a new PR with these features later |
jonahwilliams
added
the
autosubmit
…120761) * ab2dec516 Roll Skia from 8de7f68a3661 to 67a8177742e6 (4 revisions) (flutter/engine#39623) * a7ec1e50f Remove libpng dependency in the APNG decoder (flutter/engine#39622) * cb5393336 Add more info to malioc_diff.py output (flutter/engine#39625) * 60c199501 Roll Dart SDK from ea59504416a8 to 22ad11262460 (2 revisions) (flutter/engine#39624) * b1f76fbb0 [Impeller] Avoid stringstream usage in CreateUniformMemberKey (flutter/engine#39606) * a85220d2f Fix a flake in EmbedderTest.CompositorRenderTargetsNotRecycledWhenAvoidsCacheSet (flutter/engine#39596) * 3141b116d [impeller] Refactored backend specific feature checks to capabilities (flutter/engine#39598) * bf667254f [impellerc] Generate GLSL output for the runtime-stage-gles target (flutter/engine#39602) * 57e83aa16 [Impeller] Special case color filter in tiled texture. (flutter/engine#39445) * e1325a9de Roll Skia from 67a8177742e6 to a3fe5233eb10 (9 revisions) (flutter/engine#39627) * 9f2c4db50 Add app anatomy diagram (flutter/engine#39628) * e8c14e436 Roll Fuchsia Linux SDK from mWwKhmxRlXNJATVmu... to A15Lg2MzGSkbj33mo... (flutter/engine#39631) * 9bd50b706 [Impeller] read from framebuffer for advanced blends on iOS. (flutter/engine#39567) * 66715c928 Fix build output path of the malioc core list (flutter/engine#39629) * 727500b12 Roll Skia from a3fe5233eb10 to b7508e2f2577 (3 revisions) (flutter/engine#39634) * f193de9cc [Impeller] Make RenderTarget::CreateOffscreen utilities have a stencil by default (flutter/engine#39636) * 76b51e33a Roll Skia from b7508e2f2577 to 5974e36ea190 (4 revisions) (flutter/engine#39639) * aba44ff0b Roll Dart SDK from 22ad11262460 to 1dc26efa3c4c (1 revision) (flutter/engine#39640) * 657102f87 Roll Fuchsia Mac SDK from EFcCpAxOuQllDqP0F... to jsnm3dngrd9MveHrB... (flutter/engine#39641) * 44e36c9c0 Roll Skia from 5974e36ea190 to 2a7644cf4bd7 (1 revision) (flutter/engine#39642)
| // Image input types will directly set their color filter, | ||
| // if any. See `TiledTextureContents.SetColorFilter`. | ||
| if (color_source_type == ColorSourceType::kImage) { | ||
| return input; |
There was a problem hiding this comment.
Won't this will break if the filter is set after the color source?
There was a problem hiding this comment.
Oh, it's capturing a ref to the paint.
There was a problem hiding this comment.
We can lose this optimization if its making canvas weird. now that we're handling filters better in general its not as necessary.
There was a problem hiding this comment.
We can lose this optimization if its making canvas weird. now that we're handling filters better in general its not as necessary.
There was a problem hiding this comment.
Nah I think it's alright. Just had to rethink a refactor I'm working on.
Fixes flutter/flutter#119874
Currently there are two inefficiencies in the main wonderous carosel page:
Fixing these increases the FPS I see from ~90 up to 110. Getting closer!
I first attempt to generalize the passthrough code to handle tiled textures. In summary, this was a huge mess: https://github.com/flutter/engine/compare/main...jonahwilliams:engine:uv_stuff?expand=1
I tried to create a simpler implementation that takes advantage of the fact that the color filters we care about are almost always set on the same paint that is drawing the tiled texture. The tiled texture contents can be responsible for applying color filter in a subpass that is sized to the texture.