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.

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

NativeMedia sample: use application's assetManager #573

Merged
merged 2 commits into from
Sep 13, 2018
Merged

Conversation

ggfan
Copy link
Contributor

@ggfan ggfan commented Sep 13, 2018

This is to fix issue #571:
should use application's assetManager, not Activity's assetManager to avoid being garbage collected

@ggfan
Copy link
Contributor Author

ggfan commented Sep 13, 2018

@adamlesinski, please take a look, thank you so much!

@adamlesinski
Copy link

adamlesinski commented Sep 13, 2018

This doesn't quite solve it. You are still accessing the same AssetManager, but more importantly, a reference to the AssetManager isn't being taken.

I think what really needs to change is android_fopen_set_asset_manager. That method should change to be something like:

AAssetManager* android_asset_manager = NULL;
jobject android_java_asset_manager = NULL;
void android_fopen_set_asset_manager(JNIEnv* env, jobject assetManager) {
   android_java_asset_manager = (*env)->NewGlobalRef(env, assetManager);
   android_asset_manager = AAssetManager_fromJava(env, android_java_asset_manager);
}

@ggfan
Copy link
Contributor Author

ggfan commented Sep 13, 2018

@adamlesinski please review again:

  • like to keep the jni related logic to be on file-operation's client side
  • release it at the shutdown (the vm is the same as the one creating it)

Question: then on Java side, is it still necessary to take assetManager from application ( not activity )?
thanks!

@adamlesinski
Copy link

It would be worthwhile, yes. I commented on the commit.

@ggfan ggfan merged commit 0d09e41 into master Sep 13, 2018
@ggfan ggfan deleted the assetmgr-fix branch January 11, 2019 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants