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

Conversation

@iskakaushik
Copy link
Contributor

Before: go/gl-context-before-fltt took ~2.6 seconds

After: go/gl-context-after-fltt took ~7 ms

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@dnfield
Copy link
Contributor

dnfield commented Apr 27, 2022

Change looks good but should have a test.

One way to test this woul dbe to delegate the eglMakeCurrent call and make sure it doesn't get called when eglGetCurrent returns the current one.

@dnfield
Copy link
Contributor

dnfield commented Apr 27, 2022

I think at worst this should be freeing up CPU time where we are currently blocking on a buffer dequeue, but I'm curious how much raster time it actually gets us.

}

bool AndroidEGLSurface::MakeCurrent() const {
EGLContext current_egl_context = eglGetCurrentContext();
Copy link
Member

@jason-simmons jason-simmons Apr 27, 2022

Choose a reason for hiding this comment

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

Check eglGetCurrentSurface and eglGetCurrentDisplay in addition to the EGL context

@iskakaushik iskakaushik force-pushed the only-call-make-curr branch 2 times, most recently from 829781f to b7c5868 Compare April 27, 2022 20:33
@iskakaushik
Copy link
Contributor Author

@dnfield I was looking into writing a test for this. This seems very close to the OpenGL + Android specific, there aren't any tests that test things in AndroidEGLSurface. If we were to write a test that ensures that we would not call MakeCurrent if context is current, we would want to essentially mock out IsContextCurrent altogether which seems to add very little value.

There aren't any tests for CreatePbufferSurface which will be the real test for this situation. I'm open to other ideas on what a good place to test would be.

@iskakaushik iskakaushik force-pushed the only-call-make-curr branch from b7c5868 to 4223813 Compare April 27, 2022 21:20
@dnfield
Copy link
Contributor

dnfield commented Apr 27, 2022

I think you can add a test in android_context_gl_unittests that does something like this:

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(ThreadHost::ThreadHostConfig(
      thread_label,
      ThreadHost::Type::UI | ThreadHost::Type::RASTER | ThreadHost::Type::IO));
  TaskRunners task_runners = MakeTaskRunners(thread_label, thread_host);
  auto context = std::make_unique<AndroidContextGL>(
      AndroidRenderingAPI::kOpenGLES, environment, task_runners, 0);
  auto pbuffer = context->CreatePBufferSurface();
// etc.

@iskakaushik iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 28, 2022
@iskakaushik iskakaushik merged commit c825073 into flutter:main Apr 28, 2022
@iskakaushik iskakaushik deleted the only-call-make-curr branch April 28, 2022 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants