702883 - Avoid JNI for lock/unlock of ANPSurface

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Right now we use JNI and a ByteBuffer for allocating and passing around bitmap data when a plugin surface is locked and unlocked. This is inefficient and causes some memory management issues (sometimes crashes due to buffers being too large). It would be better to use a native solution similar to what the stock WebKit implementation does.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a native solution for locking/unlocking plugin surfaces

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a native solution for locking/unlocking plugin surfaces

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 574795 [details] [diff] [review]
Use a native solution for locking/unlocking plugin surfaces

Review of attachment 574795 [details] [diff] [review]:
-----------------------------------------------------------------

I'll land this on birch, but we should figure out what to do about these struct definitions before merging to m-c. Please file a follow up bug

::: dom/plugins/base/android/ANPSurface.cpp
@@ +42,5 @@
>  
>  #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GeckoPlugins" , ## args)
>  #define ASSIGN(obj, name)   (obj)->name = anp_surface_##name
>  
> +// Copied from Android headers

I'm not sure how this should work license-wise. It might need to be a header in other-licenses that you #include

@@ +157,3 @@
>  
> +static bool anp_surface_lock(JNIEnv* env, jobject surfaceView, ANPBitmap* bitmap, ANPRectI* dirtyRect) {
> +  if (!bitmap || !surfaceView) {

no curly braces (here and elsewhere)

@@ +173,5 @@
> +  SurfaceInfo info;
> +  int err = gSurfaceFunctions.lock(surface, &info, NULL);
> +  if (err < 0) {
> +    return false;
> +  }

if (gSurfaceFunctions.lock(surface, &info, NULL) < 0)
   return false;

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 574795 [details] [diff] [review]
Use a native solution for locking/unlocking plugin surfaces

this crashes: 

I/GeckoAppShell( 5660): load "com.adobe.flashplayer.FlashPaintSurface" from "com.adobe.flashplayer" for "/datadata/com.adobe.flashplayer/lib/libflashplayer.so"
I/GeckoJNI( 5660): leaving void Java_org_mozilla_gecko_GeckoAppShell_executeNextRunnable(JNIEnv*, _jclass*)
I/GeckoAppShell( 5660): addPluginView:com.adobe.flashplayer.FlashPaintSurface@40618340 @ x:34.0 y:152.0 w:952.0 h:442.0
D/dalvikvm( 5660): GetFieldID: unable to find field Landroid/view/Surface;.mSurfacePointer:I
D/dalvikvm( 5660): GetFieldID: unable to find field Landroid/view/Surface;.mSurface:I
I/ActivityManager(  203): Process org.mozilla.fennec_blassey (pid 5660) has died.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ok, looks like there is yet another field name I need to handle. What version of Android is this?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use a native solution for locking/unlocking plugin surfaces

OK, this should work on 2.3+ now.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

http://hg.mozilla.org/projects/birch/rev/bcb7930881c9