Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix compatibility issues with SurfaceTexture on Android Q. #31698

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Fix compatibility issues with SurfaceTexture on Android Q. #31698

merged 2 commits into from
Mar 7, 2022

Conversation

0xZOne
Copy link
Member

@0xZOne 0xZOne commented Feb 26, 2022 •
edited
Loading

We've observed on Android Q that we have to wait for the consumer of SurfaceTexture to consume the last image before continuing to draw, otherwise, subsequent calls to dequeueBuffer to request a free buffer from the BufferQueue will fail.

/cc @blasten @dnfield

Fixes: flutter/flutter#98722

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@0xZOne 0xZOne force-pushed the task/compatible_with_android10 branch 3 times, most recently from 025e773 to 451e49c Compare February 27, 2022 01:10
blasten
blasten reviewed Mar 5, 2022
View reviewed changes
shell/platform/android/io/flutter/embedding/engine/renderer/SurfaceTextureWrapper.java
Comment on lines +30 to +39
public SurfaceTextureWrapper(
@NonNull SurfaceTexture surfaceTexture, @Nullable Runnable onFrameConsumed) {
Copy link

Choose a reason for hiding this comment

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

java doc for this constructor as well as the previous one to document when to use which

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

blasten
blasten reviewed Mar 5, 2022
View reviewed changes
shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java
surfaceCanvas.drawColor(Color.TRANSPARENT, BlendMode.CLEAR);
} else {
surfaceCanvas.drawColor(Color.TRANSPARENT);
// We've observed on Android Q that we have to wait for the consumer of {@link SurfaceTexture}
Copy link

Choose a reason for hiding this comment

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

did you test with versions below Q?

Copy link

Choose a reason for hiding this comment

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

cc @jreck

Copy link
Member Author

Choose a reason for hiding this comment

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

did you test with versions below Q?

Yes, I did. They work fine.

Copy link

Choose a reason for hiding this comment

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

Just to clarify, which versions did you test on? I'm getting reports that it may be several versions. See googleads/googleads-mobile-flutter#269 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, which versions did you test on? I'm getting reports that it may be several versions. See googleads/googleads-mobile-flutter#269 (comment)

I have tested on real devices from Android9 to Android12.

blasten
blasten reviewed Mar 5, 2022
View reviewed changes
shell/platform/android/io/flutter/view/TextureRegistry.java Outdated
/** Listener invoked when a new image frame becomes available and has been consumed. */
interface ImageFrameListener {
/** This method will to be invoked when a new image frame becomes available. */
void onFrameAvailable();
Copy link

Choose a reason for hiding this comment

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

is this method used?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

blasten
blasten reviewed Mar 5, 2022
View reviewed changes
shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java Outdated
@@ -195,6 +209,11 @@ protected void finalize() throws Throwable {
super.finalize();
}
}

@Override
public void setImageFrameListener(@Nullable ImageFrameListener listener) {
Copy link

Choose a reason for hiding this comment

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

I don't think this is used or related to bug. If this is indeed the case, please remove.

blasten
blasten reviewed Mar 5, 2022
View reviewed changes
shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java Outdated
@@ -109,6 +141,10 @@ public void setTexture(@Nullable SurfaceTexture newTx) {
} else {
canvas.drawColor(Color.TRANSPARENT);
}

if (Build.VERSION.SDK_INT == Build.VERSION_CODES.Q) {
Copy link

Choose a reason for hiding this comment

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

nit: we decided that it's more readable to use a number than the constant. e.g.
consider: Build.VERSION_CODES.Q vs 29. Fewer characters, and also more clear to someone reading this code.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@blasten
Copy link

blasten commented Mar 5, 2022

Thank you for looking at this @0xZOne! This is a very useful finding

@0xZOne
Copy link
Member Author

0xZOne commented Mar 5, 2022

Thank you for looking at this @0xZOne! This is a very useful finding

It is my pleasure to contribute code to Flutter. : )

@0xZOne 0xZOne requested a review from blasten March 5, 2022 09:33
@0xZOne 0xZOne force-pushed the task/compatible_with_android10 branch from 64d75b2 to d19e052 Compare March 5, 2022 13:25
@0xZOne 0xZOne force-pushed the task/compatible_with_android10 branch from d19e052 to 4d3a607 Compare March 5, 2022 23:35
@blasten blasten mentioned this pull request Mar 5, 2022
Closed
blasten
blasten approved these changes Mar 6, 2022
View reviewed changes
Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM - This is pretty awesome! Thank you

@blasten blasten 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 Mar 7, 2022
@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 7, 2022
@flutter flutter deleted a comment from fluttergithubbot Mar 7, 2022
@blasten blasten merged commit f6dd681 into flutter:main Mar 7, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 7, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2022
@engine-flutter-autoroll
f6dd681 Fix compatibility issues with SurfaceTexture on Android Q. (f…
8a98284 
…lutter/engine#31698)
@0xZOne 0xZOne mentioned this pull request Jul 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blasten blasten blasten approved these changes

@GaryQian GaryQian Awaiting requested review from GaryQian

Assignees
No one assigned
Labels
platform-android
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[android][platform_views] Views are not properly updated on Android Q
3 participants
@0xZOne @blasten @fluttergithubbot