Last Comment Bug 702883 - Avoid JNI for lock/unlock of ANPSurface
: Avoid JNI for lock/unlock of ANPSurface
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
Depends on:
Blocks: 702643
  Show dependency treegraph
 
Reported: 2011-11-15 22:00 PST by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2012-01-09 14:59 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Use a native solution for locking/unlocking plugin surfaces (15.13 KB, patch)
2011-11-15 22:01 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Use a native solution for locking/unlocking plugin surfaces (15.13 KB, patch)
2011-11-15 22:01 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review-
Details | Diff | Splinter Review
Use a native solution for locking/unlocking plugin surfaces (15.51 KB, patch)
2011-11-16 06:23 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2011-11-15 22:00:39 PST
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) (jwillcox@mozilla.com) 2011-11-15 22:01:20 PST
Created attachment 574794 [details] [diff] [review]
Use a native solution for locking/unlocking plugin surfaces
Comment 2 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-11-15 22:01:56 PST
Created attachment 574795 [details] [diff] [review]
Use a native solution for locking/unlocking plugin surfaces
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-11-15 22:35:50 PST
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?) 2011-11-15 23:25:26 PST
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) (jwillcox@mozilla.com) 2011-11-16 06:06:28 PST
Ok, looks like there is yet another field name I need to handle. What version of Android is this?
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-11-16 06:23:25 PST
Created attachment 574889 [details] [diff] [review]
Use a native solution for locking/unlocking plugin surfaces

OK, this should work on 2.3+ now.
Comment 7 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-11-17 11:38:16 PST
http://hg.mozilla.org/projects/birch/rev/bcb7930881c9

Note You need to log in before you can comment on or make changes to this bug.