727116 - Redirect SurfaceView rendering for Flash on Froyo/Gingerbread

Status: RESOLVED FIXED

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

Description

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

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) ([e-mail]) (he/him)]

Patrick has a test app that explores this method here: https://github.com/pcwalton/test-universal-surface-texture

Comment 4

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

Attachment: Draw Flash plugins with OpenGL during pan/zoom on legacy Android

Comment 5

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

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) ([e-mail]) (he/him)]

Attachment: Draw Flash plugins with OpenGL during pan/zoom on legacy Android

Comment 7

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

Attachment: Draw Flash plugins with OpenGL during pan/zoom on legacy Android

Comment 8

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

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) ([e-mail]) (he/him)]

Attachment: Draw Flash plugins with OpenGL during pan/zoom on legacy Android

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/72e069288a7e

Comment 11

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

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?)]

Attachment: patch to add pref

Adds the pref (plugins.use_placeholder) to mobile.js so we don't thrown exceptions.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

When this lands on aurora be sure to add:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d00e58830fe

Comment 14

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/72e069288a7e
http://hg.mozilla.org/mozilla-central/rev/2afa0c4f531b

Comment 15

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

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]]

Let's find out if this is blocking fennec 1.0 before approving for Aurora 14.

Comment 17

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

(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]]

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) ([e-mail]) (he/him)]

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]]

Snorp, we're concerned about bug 751609 and bug 750963 so we're removing the approval for aurora

Comment 21

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

(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).

Comment 22

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b14c92737b6a
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b11cd229b48