Skip to content

Commit fe5b23e

Browse files
Revert: [android]: prewarms shaders when the context is created (flutter#196468)
Reverts: [[android]: prewarms shaders when the context is created](flutter#196069) Initiated by: @andywolff Reason for reverting: broke android_hardware_smoke_opengles_instrumented_tests shard, seems to have introduced a deadlock on opengles flutter#196453 Original PR Author: @gaaclarke Reviewed By: @walley892 The original PR description is provided below: This preemptively starts compiling shaders as soon as the Context is created instead of waiting for the surface to be created. This has been measured to reduce startup time for mokey by 160ms. One gotcha is that on vulkan the pixel format is compiled into the shaders, getting it wrong is very wasteful. Today we only use r8g8b8a8, but in the future that may change when we support wide gamut. I've added a debug assert to make sure what we warm up is what we use, which i'm pretty sure will be correct. See the linked issue for more info ( flutter#195920 ) fixes flutter#195920 ### before <img src="https://private-user-images.githubusercontent.com/30870216/696606728-5df99815-84ba-4767-8652-f62b1868deab.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3OTQyNzExNTAsIm5iZiI6MTc5NDI3MDg1MCwicGF0aCI6Ii8zMDg3MDIxNi82OTY2MDY3MjgtNWRmOTk4MTUtODRiYS00NzY3LTg2NTItZjYyYjE4NjhkZWFiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjExMTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYxMTEwVDAwMzQxMFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTEwOTU4ZmY2ODY4M2U2MGM2NWYwODdmMTVmNzVkMjBlZmE2NGVhOWViYmNjNDE1NjE0NzE1ZjdmOTEyOWNhZGMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JnJlc3BvbnNlLWNvbnRlbnQtdHlwZT1pbWFnZSUyRnBuZyJ9.Pby0uXVIz5VeI6D2yIjY7esMaH_YFrXLyxvRm6C2UgY"> ### after <img src="https://private-user-images.githubusercontent.com/30870216/696783031-e835a389-c85a-43c4-8b82-2624073a351e.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3OTQyNzExNTEsIm5iZiI6MTc5NDI3MDg1MSwicGF0aCI6Ii8zMDg3MDIxNi82OTY3ODMwMzEtZTgzNWEzODktYzg1YS00M2M0LThiODItMjYyNDA3M2EzNTFlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjExMTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYxMTEwVDAwMzQxMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWU2M2FmNWFmOTkzZGFjOTM3Mzg1MjU5NzEzNmViZmZmNTBiYWM4MWYyOTI0ZjU5NGZhNTFiZWZhNzU1YjRjNzYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JnJlc3BvbnNlLWNvbnRlbnQtdHlwZT1pbWFnZSUyRnBuZyJ9.OYVGbEZQScWwQ4R1z8vA_2HqSbfvIH5W_2qRE4w4GuA"> ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/CONTRIBUTING.ai.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md Co-authored-by: Aaron Clarke <[email protected]>
1 parent 1b95854 commit fe5b23e

9 files changed

Lines changed: 224 additions & 669 deletions

File tree

‎engine/src/flutter/impeller/entity/BUILD.gn‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,6 @@ impeller_component("entity_unittests") {
287287

288288
sources = [
289289
"clip_stack_unittests.cc",
290-
"contents/content_context_unittests.cc",
291290
"contents/filters/blend_filter_contents_unittests.cc",
292291
"contents/filters/gaussian_blur_filter_contents_unittests.cc",
293292
"contents/filters/inputs/filter_input_unittests.cc",

‎engine/src/flutter/impeller/entity/contents/content_context.cc‎

Lines changed: 223 additions & 242 deletions
Large diffs are not rendered by default.

‎engine/src/flutter/impeller/entity/contents/content_context.h‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -129,26 +129,6 @@ class ContentContext {
129129

130130
~ContentContext();
131131

132-
//----------------------------------------------------------------------------
133-
/// @brief Requests the default pipelines that every `ContentContext`
134-
/// creates, without creating a `ContentContext`.
135-
///
136-
/// The pipelines are requested asynchronously from the pipeline
137-
/// library of `context`, which keeps them for the life of the
138-
/// context. `ContentContext`s created with `context` afterwards
139-
/// find them there, compiled or in flight, instead of compiling
140-
/// them again. This lets embedders start compiling pipelines as
141-
/// soon as a context is available, before there is a surface to
142-
/// render to.
143-
///
144-
/// This must be called on the thread that the `ContentContext`s
145-
/// for `context` are used on, since some pipeline libraries
146-
/// (OpenGL ES) are not thread-safe.
147-
///
148-
/// @param[in] context The context whose pipeline library to warm.
149-
///
150-
static void WarmDefaultPipelines(const Context& context);
151-
152132
bool IsValid() const;
153133

154134
Tessellator& GetTessellator() const;
@@ -382,14 +362,6 @@ class ContentContext {
382362
struct Pipelines;
383363
std::unique_ptr<Pipelines> pipelines_;
384364

385-
/// Requests the default variant of every pipeline from the pipeline library
386-
/// of `context`.
387-
///
388-
/// @return The pipelines holding the default variants, or nullptr if a
389-
/// default pipeline descriptor could not be created.
390-
static std::unique_ptr<Pipelines> CreateDefaultPipelines(
391-
const Context& context);
392-
393365
bool is_valid_ = false;
394366
std::shared_ptr<Tessellator> tessellator_;
395367
std::shared_ptr<RenderTargetAllocator> render_target_cache_;

‎engine/src/flutter/impeller/entity/contents/content_context_unittests.cc‎

Lines changed: 0 additions & 141 deletions
This file was deleted.

‎engine/src/flutter/impeller/renderer/testing/mocks.h‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@
1919
#include "impeller/renderer/render_pass.h"
2020
#include "impeller/renderer/render_target.h"
2121
#include "impeller/renderer/sampler_library.h"
22-
#include "impeller/renderer/shader_function.h"
23-
#include "impeller/renderer/shader_library.h"
2422

2523
#define FLT_FORWARD(mock, real, method) \
2624
EXPECT_CALL(*mock, method()) \
@@ -345,25 +343,6 @@ class MockSampler : public Sampler {
345343
explicit MockSampler(const SamplerDescriptor& desc) : Sampler(desc) {}
346344
};
347345

348-
class MockShaderFunction : public ShaderFunction {
349-
public:
350-
MockShaderFunction(std::string name, ShaderStage stage)
351-
: ShaderFunction(UniqueID{}, std::move(name), stage) {}
352-
};
353-
354-
class MockShaderLibrary : public ShaderLibrary {
355-
public:
356-
MOCK_METHOD(bool, IsValid, (), (const, override));
357-
MOCK_METHOD(std::shared_ptr<const ShaderFunction>,
358-
GetFunction,
359-
(std::string_view name, ShaderStage stage),
360-
(override));
361-
MOCK_METHOD(void,
362-
UnregisterFunction,
363-
(std::string name, ShaderStage stage),
364-
(override));
365-
};
366-
367346
} // namespace testing
368347
} // namespace impeller
369348

‎engine/src/flutter/shell/platform/android/BUILD.gn‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ executable("flutter_shell_native_unittests") {
6262
deps = [
6363
":flutter_shell_native_src",
6464
"//flutter/fml",
65-
"//flutter/impeller/renderer/testing:mocks",
6665
"//flutter/shell/platform/android/jni:jni_mock",
6766
"//flutter/shell/platform/android/jni:mock_jni_env",
6867
"//flutter/third_party/googletest:gmock",

‎engine/src/flutter/shell/platform/android/platform_view_android.cc‎

Lines changed: 1 addition & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434

3535
#include "fml/logging.h"
3636
#include "impeller/display_list/aiks_context.h"
37-
#include "impeller/entity/contents/content_context.h"
3837
#if IMPELLER_ENABLE_VULKAN // b/258506856 for why this is behind an if
3938
#include "flutter/shell/platform/android/android_surface_vk_impeller.h"
4039
#include "flutter/shell/platform/android/image_external_texture_vk_impeller.h"
@@ -436,34 +435,8 @@ std::unique_ptr<Surface> PlatformViewAndroid::CreateRenderingSurface() {
436435
if (!android_surface_) {
437436
return nullptr;
438437
}
439-
std::unique_ptr<Surface> surface = android_surface_->CreateGPUSurface(
438+
return android_surface_->CreateGPUSurface(
440439
android_context_->GetMainSkiaContext().get());
441-
// Creating the GPU surface sets up the swapchain, which is what may change
442-
// the context's default color format.
443-
CheckWarmedColorFormat();
444-
return surface;
445-
}
446-
447-
void PlatformViewAndroid::CheckWarmedColorFormat() const {
448-
if (!warmed_color_format_.has_value()) {
449-
return;
450-
}
451-
std::shared_ptr<impeller::Context> impeller_context =
452-
android_context_->GetImpellerContext();
453-
if (!impeller_context) {
454-
return;
455-
}
456-
// Only read by the DCHECK below, which compiles out of release builds.
457-
[[maybe_unused]] const impeller::PixelFormat surface_color_format =
458-
impeller_context->GetCapabilities()->GetDefaultColorFormat();
459-
FML_DCHECK(surface_color_format == warmed_color_format_.value())
460-
<< "The default pipelines were warmed for "
461-
<< impeller::PixelFormatToString(warmed_color_format_.value())
462-
<< " but the surface renders to "
463-
<< impeller::PixelFormatToString(surface_color_format)
464-
<< ", so they will be compiled again. Reevaluate the call to "
465-
"ContentContext::WarmDefaultPipelines in "
466-
"PlatformViewAndroid::SetupImpellerContext.";
467440
}
468441

469442
// |PlatformView|
@@ -602,23 +575,6 @@ bool PlatformViewAndroid::IsSurfaceControlEnabled() const {
602575
void PlatformViewAndroid::SetupImpellerContext() {
603576
android_context_->SetupImpellerContext();
604577
android_surface_->SetupImpellerSurface();
605-
606-
// Start compiling the default pipelines now, on the raster thread where the
607-
// surface's ContentContext will later look them up, instead of waiting for
608-
// the first surface to be created. Compilation happens off this thread (on
609-
// the IO thread for OpenGL ES, on worker threads for Vulkan), so no rendering
610-
// context needs to be current here.
611-
//
612-
// The pipelines are keyed on the context's default color format, which the
613-
// Vulkan swapchain may change once a surface exists. On Android it is always
614-
// R8G8B8A8, so the pipelines warmed here are the ones the surface uses.
615-
// |CheckWarmedColorFormat| asserts that this stays true.
616-
if (std::shared_ptr<impeller::Context> impeller_context =
617-
android_context_->GetImpellerContext()) {
618-
warmed_color_format_ =
619-
impeller_context->GetCapabilities()->GetDefaultColorFormat();
620-
impeller::ContentContext::WarmDefaultPipelines(*impeller_context);
621-
}
622578
}
623579

624580
} // namespace flutter

‎engine/src/flutter/shell/platform/android/platform_view_android.h‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,11 @@
66
#define FLUTTER_SHELL_PLATFORM_ANDROID_PLATFORM_VIEW_ANDROID_H_
77

88
#include <memory>
9-
#include <optional>
109
#include <string>
1110
#include <vector>
1211

1312
#include <android/hardware_buffer_jni.h>
1413
#include "flutter/fml/platform/android/scoped_java_ref.h"
15-
#include "flutter/impeller/core/formats.h"
1614
#include "flutter/lib/ui/window/platform_message.h"
1715
#include "flutter/shell/common/platform_view.h"
1816
#include "flutter/shell/common/snapshot_surface_producer.h"
@@ -145,14 +143,6 @@ class PlatformViewAndroid final : public PlatformView {
145143
std::unique_ptr<AndroidSurface> android_surface_;
146144
std::shared_ptr<PlatformMessageHandlerAndroid> platform_message_handler_;
147145
bool android_meets_hcpp_criteria_ = false;
148-
// The color format that |SetupImpellerContext| warmed the default pipelines
149-
// for, if it warmed them. Raster thread only.
150-
std::optional<impeller::PixelFormat> warmed_color_format_;
151-
152-
// Asserts that the surface renders to the color format that the default
153-
// pipelines were warmed for. Otherwise the warm-up was wasted. Raster thread
154-
// only.
155-
void CheckWarmedColorFormat() const;
156146

157147
// |PlatformView|
158148
void UpdateSemantics(

0 commit comments

Comments
 (0)