Reenable HardwareBuffer backed Android Platform Views on SDK >= 29 #44790

johnmccutchan · 2023-08-17T00:32:50Z

Fix a bug in the SDK < 33 ImageReader construction code path.
Fix a bug that resulted in references to Images produced by a closed ImageReader.
Fix an order of operations bug in ImageReaderPlatformViewRenderTarget release/finalizer code path.
Enable HardwareBuffer backed Android Platform Views on SDK >= 29

Manually tested on device rotating and shutting down the app.

flutter-dashboard · 2023-08-17T00:32:52Z

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.

jason-simmons · 2023-08-17T00:57:47Z

Tried this PR on a Pixel 3.

If I run the https://github.com/flutter/flutter/tree/master/dev/integration_tests/abstract_method_smoke_test test and rotate the screen, then the PlatformViewAndroidJNIImpl::ImageGetHardwareBuffer JNI call gets an uncaught exception.

java.lang.IllegalStateException: Image is already closed
    at android.media.Image.throwISEIfImageIsInvalid(Image.java:73)
    at android.media.ImageReader$SurfaceImage.getHardwareBuffer(ImageReader.java:989)

navaronbracke · 2023-08-17T11:54:03Z

shell/platform/android/io/flutter/plugin/platform/ImageReaderPlatformViewRenderTarget.java

    }
+    throw new UnsupportedOperationException(
+        "ImageReaderPlatformViewRenderTarget requires API version 29+");
  }

  public ImageReaderPlatformViewRenderTarget(ImageTextureEntry textureEntry) {
    if (Build.VERSION.SDK_INT < 26) {


This should be 29, instead of 26.

Good catch. Done.

johnmccutchan · 2023-08-17T17:15:57Z

@jason-simmons This is ready for review.

dnfield

Few nits

dnfield · 2023-08-17T17:23:48Z

shell/platform/android/hardware_buffer_external_texture.cc

+    FML_LOG(WARNING)
+        << "No DlImage available for HardwareBufferExternalTexture to paint.";


What's the developer action to take here?

If there is none, maybe this should be a DLOG/DCHECK?

This is useful information for debugging the engine and I'd like to keep it in place for now until this code is more solid.

dnfield · 2023-08-17T17:24:53Z

shell/platform/android/io/flutter/plugin/platform/ImageReaderPlatformViewRenderTarget.java

+    // to read back texture data. If we don't always want to use it, how do we
+    // decide when
+    // to use it or not? Perhaps PlatformViews can indicate if they may contain
+    // DRM'd content.


nit: formatting
nit: please link an issue here explaining what has to be done (even if just what investigation is left to do or what resources someone interested in this should read about)

Fixed added more context to the TODO.

dnfield · 2023-08-17T17:25:46Z

shell/platform/android/surface_texture_external_texture.cc

@@ -65,7 +65,8 @@ void SurfaceTextureExternalTexture::Paint(PaintContext& context,
        flutter::DlCanvas::SrcRectConstraint::kStrict  // enforce edges
    );
  } else {
-    FML_LOG(WARNING) << "No DlImage available.";
+    FML_LOG(WARNING)
+        << "No DlImage available for SurfaceTextureExternalTexture to paint.";


Ditto on log message.

- Fix a bug in the SDK < 33 ImageReader construction code path. - Fix a bug that resulted in references to Images produced by a closed ImageReader. - Fix an order of operations bug in ImageReaderPlatformViewRenderTarget release/finalizer code path. - Enable HardwareBuffer backed Android Platform Views on SDK >= 29

jason-simmons · 2023-08-17T17:37:43Z

LGTM

jonahwilliams

RSLGTM

…K >= 29 (flutter/engine#44790)

…132812) flutter/engine@5019d6d...866b43f 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from 1bec2899ace8 to 1e62a2d4c429 (1 revision) (flutter/engine#44829) 2023-08-17 skia-flutter-autoroll@skia.org Roll Dart SDK from 7e4e5796ee99 to 7101eb7569ac (2 revisions) (flutter/engine#44828) 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from e4be2cab442f to 1bec2899ace8 (1 revision) (flutter/engine#44826) 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from bfd45173e5e3 to e4be2cab442f (3 revisions) (flutter/engine#44824) 2023-08-17 gspencergoog@users.noreply.github.com Add Doxygen doc generation for iOS, macOS, Linux, Windows, and Impeller (flutter/engine#43915) 2023-08-17 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from VW7WAVPT3Cj5erlae... to Tnp43n_nAR2N0l_gY... (flutter/engine#44823) 2023-08-17 jason-simmons@users.noreply.github.com Fix FlutterInjectorTest assumptions about how the executor service assigns tasks to threads (flutter/engine#44775) 2023-08-17 john@johnmccutchan.com Reenable HardwareBuffer backed Android Platform Views on SDK >= 29 (flutter/engine#44790) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from VW7WAVPT3Cj5 to Tnp43n_nAR2N If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#44790) - Fix a bug in the SDK < 33 ImageReader construction code path. - Fix a bug that resulted in references to Images produced by a closed ImageReader. - Fix an order of operations bug in ImageReaderPlatformViewRenderTarget release/finalizer code path. - Enable HardwareBuffer backed Android Platform Views on SDK >= 29 Manually tested on device rotating and shutting down the app.

…>= 29 (flutter#44790)" This reverts commit d259a52.

johnmccutchan requested a review from jason-simmons August 17, 2023 00:32

github-actions bot added the platform-android label Aug 17, 2023

navaronbracke reviewed Aug 17, 2023

View reviewed changes

johnmccutchan force-pushed the reenable_hb_apvs branch 2 times, most recently from 2d7cb2f to 2efc833 Compare August 17, 2023 17:13

dnfield reviewed Aug 17, 2023

View reviewed changes

johnmccutchan force-pushed the reenable_hb_apvs branch from 2efc833 to 1a7eee1 Compare August 17, 2023 17:30

Merge branch 'main' into reenable_hb_apvs

19cf39a

jonahwilliams approved these changes Aug 17, 2023

View reviewed changes

johnmccutchan merged commit d259a52 into flutter:main Aug 17, 2023

engine-flutter-autoroll mentioned this pull request Aug 17, 2023

Roll Flutter Engine from 5019d6da655c to 714aa98750c0 (7 revisions) flutter/flutter#132810

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 17, 2023

d259a5260 Reenable HardwareBuffer backed Android Platform Views on SD…

f15d57c

…K >= 29 (flutter/engine#44790)

engine-flutter-autoroll mentioned this pull request Aug 18, 2023

Roll Flutter Engine from 5019d6da655c to 866b43f656e4 (8 revisions) flutter/flutter#132812

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2023

d259a5260 Reenable HardwareBuffer backed Android Platform Views on SD…

0ca10b4

…K >= 29 (flutter/engine#44790)

jiahaog added a commit to jiahaog/flutter-engine that referenced this pull request Sep 18, 2023

Revert "Reenable HardwareBuffer backed Android Platform Views on SDK …

1278ce2

…>= 29 (flutter#44790)" This reverts commit d259a52.

jiahaog mentioned this pull request Sep 18, 2023

Revert "Reenable HardwareBuffer backed Android Platform Views on SDK … #45973

Closed

8 tasks

jiahaog added a commit to jiahaog/flutter-engine that referenced this pull request Sep 18, 2023

Revert "Reenable HardwareBuffer backed Android Platform Views on SDK …

fdf7eec

…>= 29 (flutter#44790)" This reverts commit d259a52.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reenable HardwareBuffer backed Android Platform Views on SDK >= 29 #44790

Reenable HardwareBuffer backed Android Platform Views on SDK >= 29 #44790

Uh oh!

johnmccutchan commented Aug 17, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Aug 17, 2023

Uh oh!

jason-simmons commented Aug 17, 2023 •

edited

Loading

Uh oh!

navaronbracke Aug 17, 2023

Uh oh!

johnmccutchan Aug 17, 2023

Uh oh!

johnmccutchan commented Aug 17, 2023

Uh oh!

dnfield left a comment

Uh oh!

dnfield Aug 17, 2023

Uh oh!

johnmccutchan Aug 17, 2023

Uh oh!

dnfield Aug 17, 2023

Uh oh!

johnmccutchan Aug 17, 2023

Uh oh!

dnfield Aug 17, 2023

Uh oh!

johnmccutchan Aug 17, 2023

Uh oh!

jason-simmons commented Aug 17, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

Uh oh!

		FML_LOG(WARNING)
		<< "No DlImage available for HardwareBufferExternalTexture to paint.";

Reenable HardwareBuffer backed Android Platform Views on SDK >= 29 #44790

Reenable HardwareBuffer backed Android Platform Views on SDK >= 29 #44790

Uh oh!

Conversation

johnmccutchan commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Aug 17, 2023

Uh oh!

jason-simmons commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnmccutchan commented Aug 17, 2023

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Aug 17, 2023

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnmccutchan commented Aug 17, 2023 •

edited

Loading

jason-simmons commented Aug 17, 2023 •

edited

Loading