Last Comment Bug 727116 - Redirect SurfaceView rendering for Flash on Froyo/Gingerbread
: Redirect SurfaceView rendering for Flash on Froyo/Gingerbread
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P3 normal (vote)
: Firefox 15
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
: 702992 719865 (view as bug list)
Depends on: 743247 746633 750760 750778 751609
Blocks: 719851
  Show dependency treegraph
 
Reported: 2012-02-14 09:30 PST by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2012-05-08 20:28 PDT (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
+
+


Attachments
Draw Flash plugins with OpenGL during pan/zoom on legacy Android (67.46 KB, patch)
2012-04-26 08:41 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review-
Details | Diff | Splinter Review
Draw Flash plugins with OpenGL during pan/zoom on legacy Android (71.38 KB, patch)
2012-04-27 11:23 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Draw Flash plugins with OpenGL during pan/zoom on legacy Android (73.10 KB, patch)
2012-04-27 11:44 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
Draw Flash plugins with OpenGL during pan/zoom on legacy Android (73.00 KB, patch)
2012-04-27 13:06 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
snorp: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
patch to add pref (805 bytes, patch)
2012-04-27 21:00 PDT, Mark Finkle (:mfinkle) (use needinfo?)
blassey.bugs: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2012-02-14 09:30:56 PST
In Ice Cream Sandwich, Flash draws into an ANativeWindow, which we can render with OpenGL using a SurfaceTexture. This is great, as we can scale and pan the surface smoothly as we do with our other OpenGL stuff.

On Froyo and Gingerbread, Flash provides a Java SurfaceView which the browser is expected to add to the view hierarchy. It is positioned and resized using the standard Android view layout mechanism. Since this is totally out of the OpenGL rendering loop, the results are normally pretty bad. It should be possible, however, to access the Surface and underlying ANativeWindow. From that we should be able to use glEGLImageTargetTexture2DOES just as we do in AndroidDirectTexture to display the surface with OpenGL. It should, in theory, work on all devices. The part that normally fails with AndroidDirectTexture and AndroidGraphicBuffer is the actual allocating/locking of the memory, but we aren't doing that here.

Also, if this works, it means we could redo AndroidGraphicBuffer so that it uses this hack as a backing (instead of trying to allocate directly). I think SurfaceView talks to SurfaceFlinger to have it create the surfaces there which is why it works.
Comment 1 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-02-15 12:36:31 PST
Patrick has a test app that explores this method here: https://github.com/pcwalton/test-universal-surface-texture
Comment 2 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-12 13:23:47 PDT
*** Bug 719865 has been marked as a duplicate of this bug. ***
Comment 3 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-25 13:48:43 PDT
*** Bug 702992 has been marked as a duplicate of this bug. ***
Comment 4 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-26 08:41:14 PDT
Created attachment 618665 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-04-26 11:33:58 PDT
Comment on attachment 618665 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android

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

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1770,2 @@
>  
>    jclass cls = env->FindClass("org/mozilla/gecko/GeckoAppShell");

get this from the bridge

@@ +1773,3 @@
>    jmethodID method = env->GetStaticMethodID(cls,
>                                              "addPluginView",
>                                              "(Landroid/view/View;DDDD)V");

move this to the bridge and get it from there

@@ +2887,5 @@
> +  nsIFrame* current = (nsIFrame*)mObjectFrame;
> +  while (current && current->GetContent() && current->GetContent()->Tag() != nsGkAtoms::html) {    
> +    offset += current->GetPosition();
> +    current = current->GetParent();
> +  }

this seems wrong

::: mobile/android/base/GeckoAppShell.java
@@ +82,4 @@
>  
>  import org.json.JSONArray;
>  import org.json.JSONObject;
> +import org.json.JSONException;

doesn't look like this is needed

::: mobile/android/base/gfx/BufferedCairoImage.java
@@ +88,5 @@
> +    public void setBitmap(Bitmap bitmap) {
> +        mFormat = CairoUtils.bitmapConfigToCairoFormat(bitmap.getConfig());
> +        mSize = new IntSize(bitmap.getWidth(), bitmap.getHeight());
> +        mNeedToFreeBuffer = true;
> +        // XXX Why is this * 4? Shouldn't it depend on mFormat?

it should, change it

::: mobile/android/base/gfx/PluginLayer.java
@@ +255,5 @@
> +    }
> +
> +    class PluginLayoutParams extends AbsoluteLayout.LayoutParams
> +    {
> +        private static final int MAX_DIMENSION = 2048;

query this from GL, it can be different between devices

::: widget/android/AndroidBridge.cpp
@@ +71,4 @@
>  #define IME_FULLSCREEN_PREF "widget.ime.android.landscape_fullscreen"
>  #define IME_FULLSCREEN_THRESHOLD_PREF "widget.ime.android.fullscreen_threshold"
>  
> +#define CLEAR_EXCEPTION(env) if (env->ExceptionOccurred()) env->ExceptionClear();

not a fan of this. Instead, add CheckForException() tothe auto jni frame that returns a bool. It should check for the exception, report it, clear it and return true if one occurred, otherwise return false.

@@ +213,5 @@
> +        if (!jSurfacePointerField) {
> +            CLEAR_EXCEPTION(jEnv);
> +
> +            // And something else in 2.3+
> +            jSurfacePointerField = jEnv->GetFieldID(jSurfaceClass, "mNativeSurface", "I");

check the android version and do the right thing, rather than querying for things that don't exist

@@ +2089,4 @@
>  #ifndef MOZ_JAVA_COMPOSITOR
>    return NULL;
>  #else
> +  AutoLocalJNIFrame frame;

init with the env

@@ +2093,2 @@
>  
>    JNIEnv* env = GetJNIForThread();

is this called on the gecko thread? if so, use GetJNIEnv()

@@ +2093,3 @@
>  
>    JNIEnv* env = GetJNIForThread();
>    jclass cls = env->FindClass("org/mozilla/gecko/GeckoAppShell");

use mGeckoAppShellClass

@@ +2131,3 @@
>  
>    JNIEnv* env = GetJNIForThread();
>    jclass cls = env->FindClass("org/mozilla/gecko/GeckoAppShell");

use mGeckoAppShellClass

@@ +2135,3 @@
>    jmethodID method = env->GetStaticMethodID(cls,
>                                              "showSurface",
> +                                            "(Landroid/view/Surface;IIIIZZ)V");

look this up on init

::: widget/android/AndroidJNI.cpp
@@ +961,5 @@
> +    jobject surfaceBits = nsnull;
> +    unsigned char* bitsCopy = nsnull;
> +    int dstWidth, dstHeight, dstSize;
> +
> +    void* window = AndroidBridge::Bridge()->AcquireNativeWindow(GetJNIForThread(), surface);

s/GetJNIForThread()/jenv/

@@ +1009,5 @@
> +
> +        jSurfaceBitsWidth = jenv->GetFieldID(jSurfaceBitsClass, "width", "I");
> +        jSurfaceBitsHeight = jenv->GetFieldID(jSurfaceBitsClass, "height", "I");
> +        jSurfaceBitsFormat = jenv->GetFieldID(jSurfaceBitsClass, "format", "I");
> +        jSurfaceBitsBuffer = jenv->GetFieldID(jSurfaceBitsClass, "buffer", "Ljava/nio/ByteBuffer;");

move to AndroidBridge
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-27 11:23:28 PDT
Created attachment 619116 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Comment 7 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-27 11:44:14 PDT
Created attachment 619128 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-04-27 12:40:08 PDT
Comment on attachment 619128 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android

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

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +2881,5 @@
> +  nsIFrame* current = (nsIFrame*)mObjectFrame;
> +  while (current && current->GetContent() && current->GetContent()->Tag() != nsGkAtoms::html) {    
> +    offset += current->GetPosition();
> +    current = current->GetParent();
> +  }

file a follow up bug to figure out a better way to do this

::: widget/android/AndroidJNI.cpp
@@ +922,5 @@
> +    jobject surfaceBits = nsnull;
> +    unsigned char* bitsCopy = nsnull;
> +    int dstWidth, dstHeight, dstSize;
> +
> +    void* window = AndroidBridge::Bridge()->AcquireNativeWindow(GetJNIForThread(), surface);

s/GetJNIForThread()/jenv/
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-27 13:06:23 PDT
Created attachment 619151 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Comment 10 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-27 13:07:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e069288a7e
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-27 13:07:54 PDT
Comment on attachment 619151 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android

[Approval Request Comment]
Very large patch, but should dramatically increase Flash usability, especially on phones without ICS. Mobile only.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-27 21:00:28 PDT
Created attachment 619245 [details] [diff] [review]
patch to add pref

Adds the pref (plugins.use_placeholder) to mobile.js so we don't thrown exceptions.
Comment 13 Jeff Muizelaar [:jrmuizel] 2012-04-27 22:14:28 PDT
When this lands on aurora be sure to add:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d00e58830fe
Comment 15 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-04-30 06:44:40 PDT
Comment on attachment 619245 [details] [diff] [review]
patch to add pref

[Approval Request Comment]
Apparently needed in order to use the other patch on this bug
Comment 16 Alex Keybl [:akeybl] 2012-04-30 10:25:00 PDT
Let's find out if this is blocking fennec 1.0 before approving for Aurora 14.
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2012-04-30 12:59:32 PDT
(In reply to Alex Keybl [:akeybl] from comment #16)
> Let's find out if this is blocking fennec 1.0 before approving for Aurora 14.

this is blocking 14, two bugs that were marked blocking were duped to it. Let's give these patches a couple days of bake time on trunk before up lifting them though.
Comment 18 Alex Keybl [:akeybl] 2012-05-03 09:04:22 PDT
Comment on attachment 619151 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android

[Triage Comment]
Bake time has passed without any incident, and this blocks Fennec 1.0. Approved for Aurora 14.
Comment 19 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-03 09:06:33 PDT
There is one bad thing occurring, actually, bug 750760. I have a patch for it, though, so we can land this along with the fix for that one.
Comment 20 JP Rosevear [:jpr] 2012-05-04 12:35:02 PDT
Snorp, we're concerned about bug 751609 and bug 750963 so we're removing the approval for aurora
Comment 21 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-04 12:45:05 PDT
(In reply to JP Rosevear [:jpr] from comment #20)
> Snorp, we're concerned about bug 751609 and bug 750963 so we're removing the
> approval for aurora

Bug 751609 is indeed concerning. I have a workaround coming.

Bug 750963 is no factor since the problem already happens on Aurora (and m.youtube.com doesn't even serve Flash AFAIK).

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