Redirect SurfaceView rendering for Flash on Froyo/Gingerbread

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

Trunk
Firefox 15
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 +, fennec+)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Patrick has a test app that explores this method here: https://github.com/pcwalton/test-universal-surface-texture
tracking-fennec: --- → +
Priority: -- → P3
Duplicate of this bug: 719865
Duplicate of this bug: 702992
Created attachment 618665 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Attachment #618665 - Flags: review?(blassey.bugs)
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
Attachment #618665 - Flags: review?(blassey.bugs) → review-
Created attachment 619116 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Attachment #619116 - Flags: review?(blassey.bugs)
Attachment #618665 - Attachment is obsolete: true
Created attachment 619128 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Attachment #619128 - Flags: review?(blassey.bugs)
Attachment #619116 - Attachment is obsolete: true
Attachment #619116 - Flags: review?(blassey.bugs)
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/
Attachment #619128 - Flags: review?(blassey.bugs) → review+
Created attachment 619151 [details] [diff] [review]
Draw Flash plugins with OpenGL during pan/zoom on legacy Android
Attachment #619151 - Flags: review?(snorp)
Attachment #619151 - Flags: review?(snorp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e069288a7e
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.
Attachment #619151 - Flags: approval-mozilla-aurora?
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.
Attachment #619245 - Flags: review?(blassey.bugs)
Attachment #619245 - Flags: review?(blassey.bugs) → review+
Attachment #619245 - Attachment is patch: true
When this lands on aurora be sure to add:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d00e58830fe
http://hg.mozilla.org/mozilla-central/rev/72e069288a7e
http://hg.mozilla.org/mozilla-central/rev/2afa0c4f531b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
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
Attachment #619245 - Flags: approval-mozilla-aurora?
Let's find out if this is blocking fennec 1.0 before approving for Aurora 14.
blocking-fennec1.0: --- → ?
(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.
blocking-fennec1.0: ? → +
Depends on: 743247

Updated

5 years ago
Depends on: 750778
Depends on: 750760
Target Milestone: --- → Firefox 15
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.
Attachment #619151 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #619245 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.

Updated

5 years ago
Depends on: 751609

Comment 20

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

Updated

5 years ago
Attachment #619151 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-

Updated

5 years ago
Attachment #619245 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
(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).
Blocks: 719851
Attachment #619151 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
Attachment #619245 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
Depends on: 746633
https://hg.mozilla.org/releases/mozilla-aurora/rev/b14c92737b6a
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b11cd229b48
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.