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

Put JNI functions under an interface #18903

Merged
merged 21 commits into from
Jun 11, 2020
Merged

Conversation

blasten
Copy link

@blasten blasten commented Jun 8, 2020

Description

Centralize all JNI methods under an interface that doesn't depend on the Android toolchain. This way unit tests can be compiled with the host toolchain.

Related Issues

Fixes flutter/flutter#59031

Tests

I added the following tests: This is about enabling unit tests to be written.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@blasten blasten changed the title Jni interface Put JNI functions under an interface Jun 8, 2020
const fml::jni::JavaObjectWeakGlobalRef java_object_;

// Reference to the current Surface texture object.
fml::jni::JavaObjectWeakGlobalRef surface_texture_;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this object hold another reference to the same SurfaceTexture held by an AndroidExternalTextureGL instance?

If the goal is to centralize all native->Java calls into one interface, then can the SurfaceTexture reference be passed as an argument instead of duplicating state here?

Or would it make sense to split this into separate JNI wrapper interfaces for FlutterJNI and SurfaceTexture?

Copy link
Author

Choose a reason for hiding this comment

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

The issue is that SurfaceTexture requires the Android toolchain. The idea is that the new interface doesn't require the Android toolchain. I had to downcast jni_facade_ in the constructor, so I can access the method.

Splitting into a separate wrapper is possible if this workaround doesn't look good.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer moving the SurfaceTexture methods into a separate wrapper

Copy link
Author

Choose a reason for hiding this comment

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

What about we use the texture id (which is int64_t) to lookup the SurfaceTexture in Java? This way, we can keep everything under the same interface. I'm afraid that if we create a separate wrapper, then it may diminish the value of having a centralized interface.

Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that doing the lookup would add unnecessary overhead to frequently called APIs like UpdateTexImage.

Can the SurfaceTexture JNI invocations and method ID lookups be moved into android_external_texture_gl.cc? That file is already dependent on JNI and OpenGL ES APIs.

Copy link
Author

Choose a reason for hiding this comment

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

What does 19aa4e7 look like?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM - hiding the JNI object reference behind a std::any seems like a reasonable way to go if there is a requirement to make all JNI calls through one interface but that interface can not use JNI types.

@blasten blasten marked this pull request as ready for review June 9, 2020 00:09
///
/// @note `surface_texture` must be `fml::jni::JavaObjectWeakGlobalRef`
///
virtual void SurfaceTextureAttachToGLContext(std::any surface_texture,
Copy link
Member

Choose a reason for hiding this comment

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

std::any still leads to needing casts in Android code and the loss of readability of the type of the surface texture and its consequences to IDE completions. While there are very real uses of std::any, in this case, it is little more than a glorified raw pointer.

I would much prefer something like the following in a common header.

#if OS_ANDROID
using JavaWeakGlobalRef = fml::jni::JavaObjectWeakGlobalRef;
#else
using JavaWeakGlobalRef = std::nullptr_t;
#endif

This avoid casts, the type is clear and IDE completions all just work.

Copy link
Author

Choose a reason for hiding this comment

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

Done in 8d9d48a

@blasten blasten requested a review from chinmaygarde June 9, 2020 21:25
@blasten
Copy link
Author

blasten commented Jun 10, 2020

@godofredoc do you know why 6 checks are now canceled (by unknown)?

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
zljj0818 pushed a commit to zljj0818/flutter that referenced this pull request Jun 22, 2020
* 141ee78 Roll Dart SDK from 2b917f5b6a0e to 0fab083e1c2b (15 revisions) (flutter/engine#18974)

* f5ab179 Call Shell::NotifyLowMemoryWarning on Android Trim and LowMemory events (flutter/engine#18979)

* 4dabac9 Roll Fuchsia Linux SDK from 8AiUM... to ThzHh... (flutter/engine#18976)

* 50cae02 Reland: Add RAII wrapper for EGLSurface (flutter/engine#18977)

* db7f226 Include the memory header as unique_ptr is used in ASCII Trie. (flutter/engine#18978)

* b19a17d Implement an EGL resource context for the Linux shell. (flutter/engine#18918)

* ca26b75 Make Linux shell plugin constructor descriptions consistent (flutter/engine#18940)

* efd1452 Roll Skia from 9c401e7e1ace to f69e40841eb9 (11 revisions) (flutter/engine#18983)

* e5845af Put JNI functions under an interface (flutter/engine#18903)

* 1ddc6e1 Call destructor and fix check (flutter/engine#18985)

* 7f71f1d Roll Fuchsia Mac SDK from oWhyp... to gAD3P... (flutter/engine#18982)

* 74291b7 Roll Dart SDK from 0fab083e1c2b to e62b89c56d01 (10 revisions) (flutter/engine#18987)

* 2739bbf [web] Provide a hook to disable location strategy (flutter/engine#18969)

* 336d000 Roll Skia from f69e40841eb9 to 553496b66f12 (2 revisions) (flutter/engine#18992)

* b82769b Roll Skia from 553496b66f12 to 0ad37b87549b (2 revisions) (flutter/engine#18993)

* f5ca58b Roll Dart SDK from e62b89c56d01 to b0d2b97d2cd7 (4 revisions) (flutter/engine#18994)

* 2a82a08 [dart] Account for compiler api change (flutter/engine#19002)

* 369e0a9 Add ui_benchmarks (flutter/engine#18945)

* a0a92d6 Roll Skia from 0ad37b87549b to de175abede4d (1 revision) (flutter/engine#18995)

* cea5a9c Roll Dart SDK from b0d2b97d2cd7 to f043f9e5f6ea (6 revisions) (flutter/engine#18997)

* d417772 Roll Fuchsia Mac SDK from gAD3P... to Wj0yo... (flutter/engine#19001)

* 4538622 Roll Skia from de175abede4d to 32d5cfa1f35e (15 revisions) (flutter/engine#19005)

* 71fce02 Fix shift-tab not working by adding more GTK->GLFW key mappings. (flutter/engine#18988)

* 5ddc122 Fix inverted check in creating resource surface (flutter/engine#18989)

* 87d8888 Show warning if method response errors occur and error not handled. (flutter/engine#18946)

* 5171fbd Roll Skia from 32d5cfa1f35e to 21bbfc6c2dfe (5 revisions) (flutter/engine#19006)

* 4bd6aea Always send key events, even if they're used for text input. (flutter/engine#18991)

* 9f898e9 Don't process key events when the text input is not requested (flutter/engine#18990)

* 48d7091 Roll buildroot for Windows warning update (flutter/engine#19000)

* 5f37029 Roll Dart SDK from f043f9e5f6ea to f0ea02bc51f8 (22 revisions) (flutter/engine#19010)

* ac809b4 onBeginFrame JNI (flutter/engine#18866)

* e1c622b Make SKSL caching test work on Fuchsia on arm64 (flutter/engine#18572)

* 2651beb Exit before pushing a trace event when layer tree holder is empty (flutter/engine#19008)

* acf048b Remove log added for local testing (flutter/engine#19012)

* 1f3aa23 Roll Dart SDK from f0ea02bc51f8 to 0b64f5488965 (9 revisions) (flutter/engine#19013)

* 8dcc95d Roll Fuchsia Mac SDK from Wj0yo... to gR0Zc... (flutter/engine#19015)

* f6455fa Roll Dart SDK from 0b64f5488965 to 50836c171e91 (4 revisions) (flutter/engine#19017)

* b2fea9d Roll Skia from 21bbfc6c2dfe to 30212b7941d6 (6 revisions) (flutter/engine#19009)

* 0065646 Roll Skia from 30212b7941d6 to 3d6bf04366f6 (17 revisions) (flutter/engine#19020)

* 0a852d8 Revert "Call Shell::NotifyLowMemoryWarning on Android Trim and LowMemory events (flutter#18979)" (flutter/engine#19023)

* 194acdf apply null safety syntax to mobile dart:ui (flutter/engine#18933)

* b0a0e0e Roll Skia from 3d6bf04366f6 to 637838d20abd (2 revisions) (flutter/engine#19021)

* be499ab Roll Fuchsia Mac SDK from gR0Zc... to H-uAk... (flutter/engine#19022)

* 8c24c41 Roll Skia from 637838d20abd to ac16760df463 (1 revision) (flutter/engine#19025)

* 7cb7003 onEndFrame JNI (flutter/engine#18867)

* e3fdb23 [fuchsia] Add ability to configure separate data and asset dirs (flutter/engine#18858)

* 983b6e1 Call Shell::NotifyLowMemory when backgrounded/memory pressure occurs on Android (flutter/engine#19026)

* f7d241f Wire up channel for restoration data (flutter/engine#18042)

* e84d497 Fix hit testing logic in fuchsia a11y (flutter/engine#19029)

* 801559a Revert to last-known-good-rev of Dart SDK (flutter/engine#19031)
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* 141ee78 Roll Dart SDK from 2b917f5b6a0e to 0fab083e1c2b (15 revisions) (flutter/engine#18974)

* f5ab179 Call Shell::NotifyLowMemoryWarning on Android Trim and LowMemory events (flutter/engine#18979)

* 4dabac9 Roll Fuchsia Linux SDK from 8AiUM... to ThzHh... (flutter/engine#18976)

* 50cae02 Reland: Add RAII wrapper for EGLSurface (flutter/engine#18977)

* db7f226 Include the memory header as unique_ptr is used in ASCII Trie. (flutter/engine#18978)

* b19a17d Implement an EGL resource context for the Linux shell. (flutter/engine#18918)

* ca26b75 Make Linux shell plugin constructor descriptions consistent (flutter/engine#18940)

* efd1452 Roll Skia from 9c401e7e1ace to f69e40841eb9 (11 revisions) (flutter/engine#18983)

* e5845af Put JNI functions under an interface (flutter/engine#18903)

* 1ddc6e1 Call destructor and fix check (flutter/engine#18985)

* 7f71f1d Roll Fuchsia Mac SDK from oWhyp... to gAD3P... (flutter/engine#18982)

* 74291b7 Roll Dart SDK from 0fab083e1c2b to e62b89c56d01 (10 revisions) (flutter/engine#18987)

* 2739bbf [web] Provide a hook to disable location strategy (flutter/engine#18969)

* 336d000 Roll Skia from f69e40841eb9 to 553496b66f12 (2 revisions) (flutter/engine#18992)

* b82769b Roll Skia from 553496b66f12 to 0ad37b87549b (2 revisions) (flutter/engine#18993)

* f5ca58b Roll Dart SDK from e62b89c56d01 to b0d2b97d2cd7 (4 revisions) (flutter/engine#18994)

* 2a82a08 [dart] Account for compiler api change (flutter/engine#19002)

* 369e0a9 Add ui_benchmarks (flutter/engine#18945)

* a0a92d6 Roll Skia from 0ad37b87549b to de175abede4d (1 revision) (flutter/engine#18995)

* cea5a9c Roll Dart SDK from b0d2b97d2cd7 to f043f9e5f6ea (6 revisions) (flutter/engine#18997)

* d417772 Roll Fuchsia Mac SDK from gAD3P... to Wj0yo... (flutter/engine#19001)

* 4538622 Roll Skia from de175abede4d to 32d5cfa1f35e (15 revisions) (flutter/engine#19005)

* 71fce02 Fix shift-tab not working by adding more GTK->GLFW key mappings. (flutter/engine#18988)

* 5ddc122 Fix inverted check in creating resource surface (flutter/engine#18989)

* 87d8888 Show warning if method response errors occur and error not handled. (flutter/engine#18946)

* 5171fbd Roll Skia from 32d5cfa1f35e to 21bbfc6c2dfe (5 revisions) (flutter/engine#19006)

* 4bd6aea Always send key events, even if they're used for text input. (flutter/engine#18991)

* 9f898e9 Don't process key events when the text input is not requested (flutter/engine#18990)

* 48d7091 Roll buildroot for Windows warning update (flutter/engine#19000)

* 5f37029 Roll Dart SDK from f043f9e5f6ea to f0ea02bc51f8 (22 revisions) (flutter/engine#19010)

* ac809b4 onBeginFrame JNI (flutter/engine#18866)

* e1c622b Make SKSL caching test work on Fuchsia on arm64 (flutter/engine#18572)

* 2651beb Exit before pushing a trace event when layer tree holder is empty (flutter/engine#19008)

* acf048b Remove log added for local testing (flutter/engine#19012)

* 1f3aa23 Roll Dart SDK from f0ea02bc51f8 to 0b64f5488965 (9 revisions) (flutter/engine#19013)

* 8dcc95d Roll Fuchsia Mac SDK from Wj0yo... to gR0Zc... (flutter/engine#19015)

* f6455fa Roll Dart SDK from 0b64f5488965 to 50836c171e91 (4 revisions) (flutter/engine#19017)

* b2fea9d Roll Skia from 21bbfc6c2dfe to 30212b7941d6 (6 revisions) (flutter/engine#19009)

* 0065646 Roll Skia from 30212b7941d6 to 3d6bf04366f6 (17 revisions) (flutter/engine#19020)

* 0a852d8 Revert "Call Shell::NotifyLowMemoryWarning on Android Trim and LowMemory events (flutter#18979)" (flutter/engine#19023)

* 194acdf apply null safety syntax to mobile dart:ui (flutter/engine#18933)

* b0a0e0e Roll Skia from 3d6bf04366f6 to 637838d20abd (2 revisions) (flutter/engine#19021)

* be499ab Roll Fuchsia Mac SDK from gR0Zc... to H-uAk... (flutter/engine#19022)

* 8c24c41 Roll Skia from 637838d20abd to ac16760df463 (1 revision) (flutter/engine#19025)

* 7cb7003 onEndFrame JNI (flutter/engine#18867)

* e3fdb23 [fuchsia] Add ability to configure separate data and asset dirs (flutter/engine#18858)

* 983b6e1 Call Shell::NotifyLowMemory when backgrounded/memory pressure occurs on Android (flutter/engine#19026)

* f7d241f Wire up channel for restoration data (flutter/engine#18042)

* e84d497 Fix hit testing logic in fuchsia a11y (flutter/engine#19029)

* 801559a Revert to last-known-good-rev of Dart SDK (flutter/engine#19031)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow unit tests in platform/android/** C++ code
6 participants