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

Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreateSnapshotSurface' #30590

Merged
merged 3 commits into from
Jan 13, 2022
Merged

Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreateSnapshotSurface' #30590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
499f597
Only creates 'onscreen_surface_' when it's not available in 'AndroidS…
ColdPaleLight Dec 30, 2021
822d4af
Rename the function and add some unit tests
ColdPaleLight Jan 13, 2022
87b3d63
Update android_surface_gl.h
dnfield Jan 13, 2022
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
96 changes: 96 additions & 0 deletions shell/platform/android/android_context_gl_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,62 @@
#include "flutter/shell/common/thread_host.h"
#include "flutter/shell/platform/android/android_context_gl.h"
#include "flutter/shell/platform/android/android_environment_gl.h"
#include "flutter/shell/platform/android/android_surface_gl.h"
#include "flutter/shell/platform/android/jni/platform_view_android_jni.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace flutter {
namespace testing {
namespace android {
namespace {
class MockPlatformViewAndroidJNI : public PlatformViewAndroidJNI {
public:
MOCK_METHOD2(FlutterViewHandlePlatformMessage,
void(std::unique_ptr<flutter::PlatformMessage> message,
int responseId));
MOCK_METHOD2(FlutterViewHandlePlatformMessageResponse,
void(int responseId, std::unique_ptr<fml::Mapping> data));
MOCK_METHOD3(FlutterViewUpdateSemantics,
void(std::vector<uint8_t> buffer,
std::vector<std::string> strings,
std::vector<std::vector<uint8_t>> string_attribute_args));
MOCK_METHOD2(FlutterViewUpdateCustomAccessibilityActions,
void(std::vector<uint8_t> actions_buffer,
std::vector<std::string> strings));
MOCK_METHOD0(FlutterViewOnFirstFrame, void());
MOCK_METHOD0(FlutterViewOnPreEngineRestart, void());
MOCK_METHOD2(SurfaceTextureAttachToGLContext,
void(JavaLocalRef surface_texture, int textureId));
MOCK_METHOD1(SurfaceTextureUpdateTexImage,
void(JavaLocalRef surface_texture));
MOCK_METHOD2(SurfaceTextureGetTransformMatrix,
void(JavaLocalRef surface_texture, SkMatrix& transform));
MOCK_METHOD1(SurfaceTextureDetachFromGLContext,
void(JavaLocalRef surface_texture));
MOCK_METHOD8(FlutterViewOnDisplayPlatformView,
void(int view_id,
int x,
int y,
int width,
int height,
int viewWidth,
int viewHeight,
MutatorsStack mutators_stack));
MOCK_METHOD5(FlutterViewDisplayOverlaySurface,
void(int surface_id, int x, int y, int width, int height));
MOCK_METHOD0(FlutterViewBeginFrame, void());
MOCK_METHOD0(FlutterViewEndFrame, void());
MOCK_METHOD0(FlutterViewCreateOverlaySurface,
std::unique_ptr<PlatformViewAndroidJNI::OverlayMetadata>());
MOCK_METHOD0(FlutterViewDestroyOverlaySurfaces, void());
MOCK_METHOD1(FlutterViewComputePlatformResolvedLocale,
std::unique_ptr<std::vector<std::string>>(
std::vector<std::string> supported_locales_data));
MOCK_METHOD0(GetDisplayRefreshRate, double());
MOCK_METHOD1(RequestDartDeferredLibrary, bool(int loading_unit_id));
};

TaskRunners MakeTaskRunners(const std::string& thread_label,
const ThreadHost& thread_host) {
fml::MessageLoop::EnsureInitializedForCurrentThread();
Expand Down Expand Up @@ -62,6 +112,52 @@ TEST(AndroidContextGl, CreateSingleThread) {
context.reset();
EXPECT_TRUE(main_context->abandoned());
}

TEST(AndroidSurfaceGL, CreateSnapshopSurfaceWhenOnscreenSurfaceIsNotNull) {
GrMockOptions main_context_options;
sk_sp<GrDirectContext> main_context =
GrDirectContext::MakeMock(&main_context_options);
auto environment = fml::MakeRefCounted<AndroidEnvironmentGL>();
std::string thread_label =
::testing::UnitTest::GetInstance()->current_test_info()->name();
ThreadHost thread_host(thread_label, ThreadHost::Type::UI |
ThreadHost::Type::RASTER |
ThreadHost::Type::IO);
TaskRunners task_runners = MakeTaskRunners(thread_label, thread_host);
auto android_context = std::make_shared<AndroidContextGL>(
AndroidRenderingAPI::kOpenGLES, environment, task_runners);
auto jni = std::make_shared<MockPlatformViewAndroidJNI>();
auto android_surface =
std::make_unique<AndroidSurfaceGL>(android_context, jni);
auto window = fml::MakeRefCounted<AndroidNativeWindow>(
nullptr, /*is_fake_window=*/true);
android_surface->SetNativeWindow(window);
auto onscreen_surface = android_surface->GetOnscreenSurface();
EXPECT_NE(onscreen_surface, nullptr);
android_surface->CreateSnapshotSurface();
EXPECT_EQ(onscreen_surface, android_surface->GetOnscreenSurface());
}

TEST(AndroidSurfaceGL, CreateSnapshopSurfaceWhenOnscreenSurfaceIsNull) {
GrMockOptions main_context_options;
sk_sp<GrDirectContext> main_context =
GrDirectContext::MakeMock(&main_context_options);
auto environment = fml::MakeRefCounted<AndroidEnvironmentGL>();
std::string thread_label =
::testing::UnitTest::GetInstance()->current_test_info()->name();
ThreadHost thread_host(thread_label, ThreadHost::Type::UI |
ThreadHost::Type::RASTER |
ThreadHost::Type::IO);
TaskRunners task_runners = MakeTaskRunners(thread_label, thread_host);
auto android_context = std::make_shared<AndroidContextGL>(
AndroidRenderingAPI::kOpenGLES, environment, task_runners);
auto jni = std::make_shared<MockPlatformViewAndroidJNI>();
auto android_surface =
std::make_unique<AndroidSurfaceGL>(android_context, jni);
EXPECT_EQ(android_surface->GetOnscreenSurface(), nullptr);
android_surface->CreateSnapshotSurface();
EXPECT_NE(android_surface->GetOnscreenSurface(), nullptr);
}
} // namespace android
} // namespace testing
} // namespace flutter
6 changes: 4 additions & 2 deletions shell/platform/android/android_surface_gl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,10 @@ AndroidContextGL* AndroidSurfaceGL::GLContextPtr() const {
return reinterpret_cast<AndroidContextGL*>(android_context_.get());
}

std::unique_ptr<Surface> AndroidSurfaceGL::CreatePbufferSurface() {
onscreen_surface_ = GLContextPtr()->CreatePbufferSurface();
std::unique_ptr<Surface> AndroidSurfaceGL::CreateSnapshotSurface() {
if (!onscreen_surface_ || !onscreen_surface_->IsValid()) {
Copy link
Contributor

@dnfield dnfield Dec 30, 2021

Choose a reason for hiding this comment

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

Maybe just an early return instead? This needs a test too.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If we early return, then Rasterizer::DoMakeRasterSnapshot will execute the following logic, which will change from hardware rasterization to software rasterization. The performance will be a little bit worse, but the logic should be correct. If you think an early return is more reasonable, I'm happy to tweak the code.

engine/shell/common/rasterizer.cc

Lines 292 to 297 in 30f048d

if (!snapshot_surface) {
// Raster surface is fine if there is no on screen surface. This might
// happen in case of software rendering.
sk_sp<SkSurface> sk_surface = SkSurface::MakeRaster(image_info);
result = DrawSnapshot(sk_surface, draw_callback);
} else {

And about test, I'm not sure how to test this, would you mind giving me some suggestions ? Thank you :) .

Copy link
Member Author

@ColdPaleLight ColdPaleLight Dec 30, 2021

Choose a reason for hiding this comment

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

I thought about it again, the early return will introduce a new problem. If Rasterizer::DoMakeRasterSnapshot is executed multiple times when the app is in the background. The first time there is no problem, and AndroidSurfaceGL::on_screen_surface_ which size is 1x1 is created, but the second time, because AndroidSurfaceGL::on_screen_surface_ already exists, it is returned early. As a result, Rasterizer::DoMakeRasterSnapshot has changed from hardware rasterization to software rasterization.

Copy link
Contributor

@dnfield dnfield Jan 12, 2022

Choose a reason for hiding this comment

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

I think the part I'm not clear on is how we'd get here.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Jan 12, 2022

Choose a reason for hiding this comment

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

I think the part I'm not clear on is how we'd get here.

Although I can't provide a reproducible sample, it appeared many times in my application, and I finally found the reason for the problem by printing the Log, and the problem was triggered by this line of code.
https://github.com/flutter/flutter/blob/019f03374c5b106c3fde05c8eee930cd26b8ffff/packages/flutter/lib/src/painting/shader_warm_up.dart#L95

Copy link
Contributor

@dnfield dnfield Jan 12, 2022

Choose a reason for hiding this comment

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

I think this is a combination of a few things:

  • The platform thread creates the onscreen surface
  • It then calls Shell::NotifyCreated
  • NotifyCreated fire and forgets the UI task runner, which might schedule an image decode (this is a recent change, from November or Decemberish)
  • The rasterizer setup is called, but now there's a race where the UI thread might have asked the rasterizer to make a snapshot before it has been setup
  • If the UI thread beats rasterizer setup, the rasterizer will say it doesn't have a surface to work with and ask for a snapshotting surface. The AndroidContextGL creates a pbuffer surface for this and sets it as the onscreen_surface_, which destroys the one we actually want.

Copy link
Contributor

@dnfield dnfield Jan 12, 2022

Choose a reason for hiding this comment

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

So what I think should happen here is the following:

  • Rename AndroidSurfaceGL::CreatePbufferSurface to something like AndroidSurfaceGL::GetOrCreateSnapshotSurface.
  • Have that renamed method only create the Pbuffer surface if onscreen_surface_ is null/invalid. Otherwise, return onscreen_surface_.
  • Write a test that exercises this, e.g. latch the raster task runner before NotifyCreated is called, run some Dart code that calls and completes toImage, unlatch the raster task runner, make sure the rasterizer surface is the right one.

Copy link

@blasten blasten Jan 12, 2022

Choose a reason for hiding this comment

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

Have that renamed method only create the Pbuffer surface if onscreen_surface_ is null/invalid. Otherwise, return onscreen_surface_.

What happens if the buffer isn't a Pbuffer at this time? Does it make sense to destroy the existing surface and then create the actual Pbuffer?

Copy link
Contributor

@dnfield dnfield Jan 12, 2022

Choose a reason for hiding this comment

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

No. We just want to make sure that we have a GPU backed surface and don't end up in the software backend code.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Jan 13, 2022

Choose a reason for hiding this comment

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

So what I think should happen here is the following:

  • Rename AndroidSurfaceGL::CreatePbufferSurface to something like AndroidSurfaceGL::GetOrCreateSnapshotSurface.
  • Have that renamed method only create the Pbuffer surface if onscreen_surface_ is null/invalid. Otherwise, return onscreen_surface_.
  • Write a test that exercises this, e.g. latch the raster task runner before NotifyCreated is called, run some Dart code that calls and completes toImage, unlatch the raster task runner, make sure the rasterizer surface is the right one.

@dnfield
We can't return onscreen_surface_ because the type of the return value is std::unique_ptr<Surface>, but the type of onscreen_surface_ is std::unique_ptr<AndroidEGLSurface>. So in either case we have to create a new std::unique_ptr<Surface>. so I renamed it to AndroidSurfaceGL::CreateSnapshotSurface. I also added test cases as you said in discord.

onscreen_surface_ = GLContextPtr()->CreatePbufferSurface();
}
sk_sp<GrDirectContext> main_skia_context =
GLContextPtr()->GetMainSkiaContext();
if (!main_skia_context) {
Expand Down
10 changes: 9 additions & 1 deletion shell/platform/android/android_surface_gl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class AndroidSurfaceGL final : public GPUSurfaceGLDelegate,
bool SetNativeWindow(fml::RefPtr<AndroidNativeWindow> window) override;

// |AndroidSurface|
virtual std::unique_ptr<Surface> CreatePbufferSurface() override;
virtual std::unique_ptr<Surface> CreateSnapshotSurface() override;

// |GPUSurfaceGLDelegate|
std::unique_ptr<GLContextResult> GLContextMakeCurrent() override;
Expand All @@ -66,6 +66,14 @@ class AndroidSurfaceGL final : public GPUSurfaceGLDelegate,
// |GPUSurfaceGLDelegate|
sk_sp<const GrGLInterface> GetGLInterface() const override;

// Obtain a raw pointer to the on-screen AndroidEGLSurface.
//
// This method is intended for use in tests. Callers must not
// delete the returned pointer.
AndroidEGLSurface* GetOnscreenSurface() const {
Copy link
Contributor

@dnfield dnfield Jan 13, 2022

Choose a reason for hiding this comment

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

Please add a doc comment, probably explaining that this is only intended for tests.

e.g.

  // Obtain a raw pointer to the AndroidEGLSurface. This method is intended for use in tests. Callers must not delete this pointer.

Copy link
Contributor

@dnfield dnfield Jan 13, 2022

Choose a reason for hiding this comment

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

(I went ahead and made this change so we can just get this landed, rather than wait for another time zone cycle :)

return onscreen_surface_.get();
}

private:
fml::RefPtr<AndroidNativeWindow> native_window_;
std::unique_ptr<AndroidEGLSurface> onscreen_surface_;
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/surface/android_surface.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ AndroidSurface::AndroidSurface(

AndroidSurface::~AndroidSurface() = default;

std::unique_ptr<Surface> AndroidSurface::CreatePbufferSurface() {
std::unique_ptr<Surface> AndroidSurface::CreateSnapshotSurface() {
return nullptr;
}

Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/surface/android_surface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class AndroidSurface {

virtual bool SetNativeWindow(fml::RefPtr<AndroidNativeWindow> window) = 0;

virtual std::unique_ptr<Surface> CreatePbufferSurface();
virtual std::unique_ptr<Surface> CreateSnapshotSurface();

protected:
explicit AndroidSurface(
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/surface/snapshot_surface_producer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ AndroidSnapshotSurfaceProducer::AndroidSnapshotSurfaceProducer(

std::unique_ptr<Surface>
AndroidSnapshotSurfaceProducer::CreateSnapshotSurface() {
return android_surface_.CreatePbufferSurface();
return android_surface_.CreateSnapshotSurface();
}

} // namespace flutter