[Impeller] A toolkit for managed handles to Android NDK vended objects. #51334

chinmaygarde · 2024-03-11T21:55:50Z

Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch.

Part of #51213 being chopped up.

jonahwilliams

Do we intented to (eventually) replace the existing AHardwareBuffer and NDK wrappers used in the android embedder with this entirely?

jonahwilliams · 2024-03-13T02:01:12Z

impeller/toolkit/android/hardware_buffer.cc

+  // Zero sized hardware buffers cannot be allocated.
+  desc.size = size.Max(ISize{1u, 1u});


This seems like it should be an error state (std::option or statusOr) instead of returning a size that wasn't requested.

I documented that restriction in the headerdoc with a large warning sign so it shows up more prominently. It took me some trial and error to understand that this was the cause of buffers not being allocatable and the Android API doesn't say why a size wasn't allocatable. So making the callers figure this out was a pretty poor experience. Also, we do the same clamping in other spots (like the pbuffer sizes for offscreen GLES surfaces). So it seemed like in keeping with the theme. There are IsAllocatable checks either way though.

jonahwilliams · 2024-03-13T02:01:59Z

impeller/toolkit/android/hardware_buffer.h

+  /// @see        Vulkan Format: VK_FORMAT_R8G8B8A8_UNORM
+  /// @see        OpenGL ES Format: GL_RGBA8
+  ///
+  /// Why have many format when one format do trick?


jonahwilliams · 2024-03-13T02:02:16Z

impeller/toolkit/android/hardware_buffer.h

+  ///             the descriptor after this call is made to determine the size
+  ///             of the allocated hardware buffer.
+  ///
+  /// @param[in]  size  The size.


Note size requirements here.

I just put the size restrictions in the @warning block right above. Hoping to draw attention to it. But that was obviously not enough. I'll add a note reiterating the warning.

jonahwilliams · 2024-03-13T02:02:54Z

impeller/toolkit/android/native_window.h

+  bool IsValid() const;
+
+  //----------------------------------------------------------------------------
+  /// @return     The current of the native window.


current size?

jonahwilliams · 2024-03-13T02:05:21Z

impeller/toolkit/android/surface_control.h

+///
+///             This wrapper is only available on Android API 29 and above.
+///
+class SurfaceControl {


Does the surface control have any threading restrictions? In general, if we're aware of any threading restrictions for parts of these APIs it would be good to note them.

I am not aware of any threading restrictions on any of the handle types. The only major quirk is the transaction completion callback being made on a system managed thread (presumably a binder thread). I've documented that and also the lifecycle implications to the transaction because of it in the headerdoc comment for it.

I audited all calls, and beyond the completion callback that I have already documented, there is little guidance from the Android docs about the threading restrictions of any of the calls. I'm just going to be safe and call these as needing to be externally synchronized.

jonahwilliams · 2024-03-13T02:07:56Z

impeller/toolkit/android/toolkit_android_unittests.cc

+    // tests everywhere else.
+    if (__builtin_available(android 29, *)) {
+    } else {
+      GTEST_SKIP() << "Platform too old for this test.";


Given that we just went through a fire drill where tests weren't actually running (because they failed unsafely) - how can we guarantee that we get a failure somewhere if we accidentally stop running this on API 29+ ?

We have the same problem with the existing ndk_helpers_unittests.

We do, in fact, currently run emulators that will run this test, but we should probably patch ... something to fail if this test never runs at all anywhere.

Should I just do a GTEST_FAIL instead?

GTEST_FAIL SGTM

We have infra that runs an API 28 emulator.

We should probably have a test that verifies we have sane/non-crashing behavior if you use this API below 29 though...

runs an API 28 emulator...

So, I can't make this a fail right?

we have sane/non-crashing behavior if you use this API below 29 though

We are definitely missing a trick not using static analysis (like at-availability on iOS) for this. I spent a non-trivial amount of time the other day wiring it up. But I haven't been able to get it to work reliably with our toolchain. Something to consider in addition to just checking on all emulator versions.

impeller/toolkit/android/README.md

impeller/toolkit/android/toolkit_android_unittests.cc

dnfield · 2024-03-13T17:27:00Z

impeller/toolkit/android/toolkit_android_unittests.cc

+
+namespace impeller::android::testing {
+
+class ToolkitAndroidTest : public ::testing::Test {


I don't know if you want to do this as a follow up, but related to Jonah's question:

The tests here don't have as much coverage as the ndk_helpers_unittests (specifcially, there aren't assertions that the API works and that we can allocate/destroy these objects or that the API symbols are correctly resolved). Maybe this will be good for a follow up patch, but consider porting those tests to this toolkit, deleting the FML versions, and porting the Android embedding to use this toolkit?

If that's not possible, I'd rather not land this in Impeller - I'm somewhat concerned about having more places that we do these lookups and introduce typos/untested code, especially since we just got that cleaned up.

If that's not possible, I'd rather not land this in Impeller - I'm somewhat concerned about having more places that we do these lookups and introduce typos/untested code, especially since we just got that cleaned up.

+1

API works and that we can allocate/destroy these objects.

CanCreateHardwareBuffer for hardware buffers. CanApplySurfaceTransaction for surface transactions. Same as the NDK helpers.

API symbols are correctly resolved.

CanCreateProcTable. You'll only get a valid proc table if the API symbols are correctly resolved and the version checks pass. For the individual subcomponents, this is also checked with IsAvailableOnPlatform.

The tests here don't have as much coverage.

I'm surprised by this statement. There are only two tests in NDK helpers related to anything in this patch. HardwareBuffer and SurfaceTransaction. The entire corpus is only 5 tests in total (unless I am missing something). The two tests I mentioned above more than cover the the assertions in NDK helpers.

I want to use the toolkit for everything and remove the NDK helpers after porting all existing tests. But, at least till I get to the choreographer, there isn't going to be anything to port as it should already be covered.

We seem to be missing coverage here of AHardwareBuffer_describe, AHardwareBuffer_isSupported, and AHardwareBuffer_getId, which are needed by the current Android implementation.

Yeah, but the toolkit has no ability to describe or get the ID of those objects yet. Once I work on adding those features get rid of NDK helpers, I can patch those in.

(Sorry, I did not mean to post this one! too many tabs open)

Yeah, but the toolkit has no ability to describe or get the ID of those objects yet. Once I work on adding those features get rid of NDK helpers, I can patch those in.

I would rather see either Impeller use the FML utilities until it's ready to replace them with something better, or the rest of the code base using new improved utilities with similar test coverage. Otherwise we're likely to end up in a state where we have multiple locations to do NDK related lookups with different available/missing test coverage ...

All I want to do is to facilitate landing #51213. The ask on that patch was that it was too large and it should be split up. I have split it up into #51295, #51298, and this one. Two more patches are incoming after this to facilitate the request for smaller, more self-contained PRs.

I am on the hook for killing the FML utilities. I can kill the FML utilities in this patch if this gets us past the bikeshedding but it will make this patch bigger. Or, kill the FML utilities immediately after this lands if that seems more elegant.

Ping @dnfield. Do you have a preference? Want to unblock this review ASAP.

I would rather eithe rkill it in this patch or update the AHB swapchain patch to use the FML utilities.

chinmaygarde

Do we intented to (eventually) replace the existing AHardwareBuffer and NDK wrappers used in the android embedder with this entirely?

Yes, I was going to replace the few uses of the helpers immediately after landing the swapchain patch and get rid of the libAndroid stuff in FML.

chinmaygarde · 2024-03-13T18:02:51Z

impeller/toolkit/android/hardware_buffer.h

+  ///             the descriptor after this call is made to determine the size
+  ///             of the allocated hardware buffer.
+  ///
+  /// @param[in]  size  The size.


I just put the size restrictions in the @warning block right above. Hoping to draw attention to it. But that was obviously not enough. I'll add a note reiterating the warning.

chinmaygarde · 2024-03-13T18:04:26Z

impeller/toolkit/android/native_window.h

+  bool IsValid() const;
+
+  //----------------------------------------------------------------------------
+  /// @return     The current of the native window.


chinmaygarde · 2024-03-13T18:07:28Z

impeller/toolkit/android/hardware_buffer.cc

+  // Zero sized hardware buffers cannot be allocated.
+  desc.size = size.Max(ISize{1u, 1u});


I documented that restriction in the headerdoc with a large warning sign so it shows up more prominently. It took me some trial and error to understand that this was the cause of buffers not being allocatable and the Android API doesn't say why a size wasn't allocatable. So making the callers figure this out was a pretty poor experience. Also, we do the same clamping in other spots (like the pbuffer sizes for offscreen GLES surfaces). So it seemed like in keeping with the theme. There are IsAllocatable checks either way though.

chinmaygarde · 2024-03-13T18:11:36Z

impeller/toolkit/android/surface_control.h

+///
+///             This wrapper is only available on Android API 29 and above.
+///
+class SurfaceControl {


I am not aware of any threading restrictions on any of the handle types. The only major quirk is the transaction completion callback being made on a system managed thread (presumably a binder thread). I've documented that and also the lifecycle implications to the transaction because of it in the headerdoc comment for it.

chinmaygarde · 2024-03-13T18:14:48Z

impeller/toolkit/android/surface_control.h

+///
+///             This wrapper is only available on Android API 29 and above.
+///
+class SurfaceControl {


I audited all calls, and beyond the completion callback that I have already documented, there is little guidance from the Android docs about the threading restrictions of any of the calls. I'm just going to be safe and call these as needing to be externally synchronized.

chinmaygarde · 2024-03-13T18:25:38Z

impeller/toolkit/android/toolkit_android_unittests.cc

+
+namespace impeller::android::testing {
+
+class ToolkitAndroidTest : public ::testing::Test {


API works and that we can allocate/destroy these objects.

CanCreateHardwareBuffer for hardware buffers. CanApplySurfaceTransaction for surface transactions. Same as the NDK helpers.

API symbols are correctly resolved.

CanCreateProcTable. You'll only get a valid proc table if the API symbols are correctly resolved and the version checks pass. For the individual subcomponents, this is also checked with IsAvailableOnPlatform.

The tests here don't have as much coverage.

I'm surprised by this statement. There are only two tests in NDK helpers related to anything in this patch. HardwareBuffer and SurfaceTransaction. The entire corpus is only 5 tests in total (unless I am missing something). The two tests I mentioned above more than cover the the assertions in NDK helpers.

I want to use the toolkit for everything and remove the NDK helpers after porting all existing tests. But, at least till I get to the choreographer, there isn't going to be anything to port as it should already be covered.

impeller/toolkit/android/toolkit_android_unittests.cc

chinmaygarde · 2024-03-13T18:28:16Z

impeller/toolkit/android/toolkit_android_unittests.cc

+    // tests everywhere else.
+    if (__builtin_available(android 29, *)) {
+    } else {
+      GTEST_SKIP() << "Platform too old for this test.";


Should I just do a GTEST_FAIL instead?

chinmaygarde · 2024-03-13T19:42:15Z

I believe I have addressed all open questions. PTAL. Past this, there are two patches to actually add the ABH swapchain implemnetation, and migrate existing users of NDK helpers. Then John should be able to wire up the Chroeographer VSync IDs.

dnfield

See https://github.com/flutter/engine/pull/51334/files#r1523948232

chinmaygarde · 2024-03-14T18:33:00Z

okie dokie. Killing the FML utils in this patch.

Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch. Part of flutter#51213 being chopped up.

dnfield · 2024-03-15T04:10:21Z

impeller/toolkit/android/proc_table.h

+#define FOR_EACH_ANDROID_PROC(INVOKE)            \
+  INVOKE(ATrace_isEnabled, 23)                   \
+  INVOKE(AChoreographer_getInstance, 24)         \
+  INVOKE(AChoreographer_postFrameCallback, 24)   \


We must not use this if sizeof(uint64_t) != sizeof(long).

On 32 bit platforms we have to use the Java based API. But we still should try to resolve it on a 64 bit platform that is < API 29. I made the same mistake when migrating things to the NDKHelpers class :)

(That's why the #if FML_ARCH_64BIT guard was around this, or whatever it actually was)

dnfield · 2024-03-15T04:12:56Z

Overall this LGTM, will approve once the guard is added back to prevent undesirable usage of AChoreographer_postFrameCallback(long is back and CI is passing (looks like you missed at least one more flutter::NDKHelpers reference).

chinmaygarde · 2024-03-15T07:52:12Z

Oh, was still tinkering on this before signing off for the day. I doubt this compiles right now. Will ping again in the morning after the fixups.

chinmaygarde

This is good to go. PTAL. Some updates since the last review:

The NDK helpers are gone and existing users migrated.
The toolkit now has a choreographer that handles the Callback32/64 variants under the hood.
The previous version of the toolkit had an extra layer of protection over just the dysym. It checked not just that the API was available, but also that the platform was modern enough per the documentation. At the time, I was worried that an API may have been available as a symbol in an unstable form on older Android versions. But the way to check the Android API level is itself 29. The NDK helpers also didn't have the extra checks I had in the proc table. I have now removed the extra checks in the toolkit too. I had no reason to doubt this way isn't fine. Just need to trust the platform more.
- Subsequently, the accessors to get the API level have also been removed.

chinmaygarde · 2024-03-15T18:56:02Z

impeller/toolkit/android/proc_table.cc

+  }
+
+#if FML_ARCH_CPU_32_BITS
+  // On 32-bit platforms, the nanosecond resolution timestamp causes overflow on


@dnfield There was at least one contradictory comment in the thread you linked earlier about the non-Callback64 choreographer use on 32-bit platforms. This was my reading of the underlying issue. But please double check if I accurately described the issue.

To reiterate the intent; Use the Callback64 variant where available. And use the Callback32 variant only on 64-bit devices (and where available).

chinmaygarde · 2024-03-15T19:03:10Z

impeller/toolkit/android/toolkit_android_unittests.cc

+
+TEST_F(ToolkitAndroidTest, CanPostAndWaitForFrameCallbacks) {
+  if ((true)) {
+    GTEST_SKIP()


@dnfield @jonahwilliams This test will currently hang because the unit-test run as a naked binary don't get choreographer callbacks. I'll remove this skip once I have the APK based unit-tests harness we discussed yesterday.

dnfield

LGTM as long as CI is happy!

matanlurey · 2024-03-15T21:31:20Z

Reason for revert: Broke engine post-submit, see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8753367119442265873/+/u/test:_Android_Unit_Tests__API_28_/stdout.

…ded objects. (flutter/engine#51334)

…d objects. (#51334)" This reverts commit f086ec4.

…ed objects. (#51334)" (#51457) Reverts: #51334 Initiated by: matanlurey Reason for reverting: Broke engine post-submit, see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8753367119442265873/+/u/test:_Android_Unit_Tests__API_28_/stdout. Original PR Author: chinmaygarde Reviewed By: {dnfield} This change reverts the following previous change: Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch. Part of #51213 being chopped up.

…ded objects. (flutter/engine#51334)

…145237) flutter/engine@c2fd533...ba1115c 2024-03-15 [email protected] Add more explicit logging (just to `stderr`) if a try-job detects an untriaged image (flutter/engine#51454) 2024-03-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] A toolkit for managed handles to Android NDK vended objects. (#51334)" (flutter/engine#51457) 2024-03-15 [email protected] [Impeller] A toolkit for managed handles to Android NDK vended objects. (flutter/engine#51334) 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the e: impeller label Mar 11, 2024

chinmaygarde requested a review from dnfield March 11, 2024 22:29

jonahwilliams reviewed Mar 13, 2024

View reviewed changes

dnfield reviewed Mar 13, 2024

View reviewed changes

impeller/toolkit/android/README.md Outdated Show resolved Hide resolved

dnfield reviewed Mar 13, 2024

View reviewed changes

impeller/toolkit/android/toolkit_android_unittests.cc Show resolved Hide resolved

dnfield reviewed Mar 13, 2024

View reviewed changes

chinmaygarde commented Mar 13, 2024

View reviewed changes

dnfield suggested changes Mar 14, 2024

View reviewed changes

chinmaygarde added 6 commits March 14, 2024 11:44

Add support for choreographer callbacks.

ed6c3ad

Get hardware buffer IDs.

f44366e

Describe hardware buffer handles.

a4f32e8

Add some docs.

6e9e10f

Deprecate all helpers and prep for removal.

0f343f1

chinmaygarde force-pushed the toolkit_android branch from ed8ba75 to 0f343f1 Compare March 14, 2024 22:48

github-actions bot added the platform-android label Mar 14, 2024

chinmaygarde added 3 commits March 14, 2024 16:16

Port VSyncWaiterAndroid.

6d118c7

Port external textures VK.

e925912

Nuke the helpers and gauge the fallout.

11a7e64

dnfield reviewed Mar 15, 2024

View reviewed changes

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Mar 15, 2024

chinmaygarde added 4 commits March 15, 2024 10:33

Static system unique ID.

317830f

Allow resolving EGL procs.

e4d6822

Don't do any availability checks while setting up the proc table.

ca8c4f7

Patch licenses.

6423740

chinmaygarde commented Mar 15, 2024

View reviewed changes

chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label Mar 15, 2024

chinmaygarde requested a review from dnfield March 15, 2024 19:01

chinmaygarde commented Mar 15, 2024

View reviewed changes

chinmaygarde added 2 commits March 15, 2024 12:15

Lint.

994e5f6

Build the unittests on CI.

4f57204

dnfield approved these changes Mar 15, 2024

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 15, 2024

auto-submit bot merged commit f086ec4 into flutter:main Mar 15, 2024
30 checks passed

chinmaygarde deleted the toolkit_android branch March 15, 2024 20:45

matanlurey added the revert Label used to revert changes in a closed and merged pull request. label Mar 15, 2024

engine-flutter-autoroll mentioned this pull request Mar 15, 2024

Roll Flutter Engine from c2fd5333a5d2 to f086ec486044 (1 revision) flutter/flutter#145233

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2024

f086ec486 [Impeller] A toolkit for managed handles to Android NDK ven…

f11c917

…ded objects. (flutter/engine#51334)

auto-submit bot pushed a commit that referenced this pull request Mar 15, 2024

Revert "[Impeller] A toolkit for managed handles to Android NDK vende…

3464f71

…d objects. (#51334)" This reverts commit f086ec4.

auto-submit bot mentioned this pull request Mar 15, 2024

Reverts "[Impeller] A toolkit for managed handles to Android NDK vended objects. (#51334)" #51457

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Mar 15, 2024

engine-flutter-autoroll mentioned this pull request Mar 15, 2024

Roll Flutter Engine from c2fd5333a5d2 to ba1115c30381 (3 revisions) flutter/flutter#145237

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2024

f086ec486 [Impeller] A toolkit for managed handles to Android NDK ven…

09e2a24

…ded objects. (flutter/engine#51334)

		// Zero sized hardware buffers cannot be allocated.
		desc.size = size.Max(ISize{1u, 1u});


		namespace impeller::android::testing {

		class ToolkitAndroidTest : public ::testing::Test {

[Impeller] A toolkit for managed handles to Android NDK vended objects. #51334

[Impeller] A toolkit for managed handles to Android NDK vended objects. #51334

Conversation

chinmaygarde commented Mar 11, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented Mar 13, 2024 • edited Loading

dnfield left a comment

Choose a reason for hiding this comment

chinmaygarde commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Mar 15, 2024

chinmaygarde commented Mar 15, 2024

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

matanlurey commented Mar 15, 2024

dnfield Mar 13, 2024 •

edited

Loading

chinmaygarde commented Mar 13, 2024 •

edited

Loading