Last Comment Bug 721741 - Make Flash plugins work on Ice Cream Sandwich (Android 4.0)
: Make Flash plugins work on Ice Cream Sandwich (Android 4.0)
Status: RESOLVED FIXED
[inbound]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 13 Branch
: ARM Android
: P1 critical (vote)
: Firefox 13
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
: 721840 (view as bug list)
Depends on: 707824 719872 722719
Blocks: 718998
  Show dependency treegraph
 
Reported: 2012-01-27 07:03 PST by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2012-02-08 14:05 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
11+


Attachments
Add support for Flash on Android 4.0+ (100.30 KB, patch)
2012-01-27 07:05 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review-
Details | Diff | Splinter Review
Add support for Flash on Android 4.0+ (101.74 KB, patch)
2012-01-27 12:47 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
Add support for Flash on Android 4.0+ (116.34 KB, patch)
2012-01-30 15:47 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch to use reflection and not require ICS SDK (2.36 KB, patch)
2012-01-31 22:06 PST, Brad Lassey [:blassey] (use needinfo?)
dougt: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-27 07:03:26 PST
We need it.
Comment 1 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-27 07:05:54 PST
Created attachment 592127 [details] [diff] [review]
Add support for Flash on Android 4.0+
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-01-27 12:09:04 PST
Comment on attachment 592127 [details] [diff] [review]
Add support for Flash on Android 4.0+

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

r- for the Looper on the gecko thread. Everything else can be taken care of in follow up bugs.

::: dom/plugins/base/android/ANPAudio.cpp
@@ +350,5 @@
>  }
>  
> +uint32_t
> +anp_audio_trackLatency(ANPAudioTrack* s) {
> +  return 1;

get a bug on file for this being stubbed out and put the bug number in a comment here

::: dom/plugins/base/android/ANPOpenGL.cpp
@@ +35,5 @@
> +using namespace mozilla;
> +using namespace mozilla::gl;
> +
> +static ANPEGLContext anp_opengl_acquireContext(NPP inst) {
> +    NOT_IMPLEMENTED();

is there a bug on file for the Honeycomb support? if not, file one and put the bug number in a comment here

::: dom/plugins/base/android/ANPSystem.cpp
@@ +48,5 @@
>  
>  #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GeckoPlugins" , ## args)
>  #define ASSIGN(obj, name)   (obj)->name = anp_system_##name
>  
> +

loose the extra ws

@@ +65,5 @@
>  
> +const char*
> +anp_system_getApplicationDataDirectory(NPP instance)
> +{
> +  return anp_system_getApplicationDataDirectory();

when this version of getApplicationDataDirectory() is called, I'd like to append the plugin's package name (which you can get from the NPP)

This fine to land as is, but please file a follow up

::: dom/plugins/base/android/ANPVideo.cpp
@@ +28,5 @@
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GeckoPlugins" , ## args)
> +#define ASSIGN(obj, name)   (obj)->name = anp_video_##name
> +
> +static ANPNativeWindow anp_video_acquireNativeWindow(NPP instance) {
> +    LOG("%s: not impl", __PRETTY_FUNCTION__);

file a bug for these functions and put its number in a comment
also, why aren't you using NOT_IMPLEMENTED()?

::: dom/plugins/base/android/ANPWindow.cpp
@@ +93,5 @@
> +  nsNPAPIPluginInstance* pinst = static_cast<nsNPAPIPluginInstance*>(instance->ndata);
> +
> +  nsPluginInstanceOwner* owner;
> +  if (NS_FAILED(pinst->GetOwner((nsIPluginInstanceOwner**)&owner))) {
> +    return rect;

if you're going to return that rect, initialize it to something

::: dom/plugins/base/android/android_npapi.h
@@ +961,5 @@
>                  ANPBitmap   bitmap;
> +                struct {
> +                    int32_t width;
> +                    int32_t height;
> +                } surface;

surfaceSize?

@@ +1030,5 @@
> +typedef void* ANPEGLContext;
> +
> +struct ANPOpenGLInterfaceV0 : ANPInterface {
> +    /**
> +     */

loose the empty comments

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1694,5 @@
> +
> +  mInstance->HandleEvent(&event, nsnull);
> +}
> +
> +void nsPluginInstanceOwner::SendLifecycleEvent(bool onScreen)

what about the rest of the life cycle events? Probably better to names this SendOnScreenEvent to leave room for all the others

::: mobile/android/base/GeckoApp.java
@@ +222,5 @@
>      String[] getPluginDirectories() {
> +        // we don't support Honeycomb
> +        if (Build.VERSION.SDK_INT == Build.VERSION_CODES.HONEYCOMB ||
> +            Build.VERSION.SDK_INT == Build.VERSION_CODES.HONEYCOMB_MR1 ||
> +            Build.VERSION.SDK_INT == Build.VERSION_CODES.HONEYCOMB_MR2)

let's do:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) &&
    Build.VERSION.SDK_INT < Build.VERSION_CODES.ICE_CREAM_SANDWICH)

@@ +1450,5 @@
> +
> +        if (layer == null) {
> +            layer = SurfaceTextureLayer.create(new Point(x, y), new IntSize(w, h), metrics.getZoomFactor(), inverted);
> +            if (layer == null) {
> +                Log.i(LOGTAG, "Failed to create plugin layer");

this seems more severe than just info. At least warning, maybe logged as an error

@@ +1469,5 @@
> +        return layer.getSurface();
> +    }
> +    
> +    private void hidePluginLayer(Layer layer) {
> +        LayerView layerView = mLayerController.getView();

do we need to null check mLayerController here (and else where)? I don't think so, but wanted to ask.

::: mobile/android/base/GeckoAppShell.java
@@ +462,5 @@
>      }
>  
>      public static void runGecko(String apkPath, String args, String url, boolean restoreSession) {
> +        // Needed for flash to not crash
> +        Looper.prepare();

No!!!

if anything gets posted to this looper it'll never run. This isn't just for flash, this will hide crappy errors in our own code as well.

We hit this When first implementing flash support. The solution was to run certain things with the PostToJavaThread() to get them on our main thread.

::: mobile/android/base/gfx/SurfaceTextureLayer.java
@@ +232,5 @@
> +		GLES11.glDrawArrays(GL10.GL_TRIANGLE_STRIP, 0, vertices.length / 2);
> +
> +		// Clean up
> +		GLES11.glDisableClientState(GL10.GL_VERTEX_ARRAY);
> +		GLES11.glDisableClientState(GL10.GL_TEXTURE_COORD_ARRAY);

TABS!!!
Comment 3 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-27 12:40:38 PST
(In reply to Brad Lassey [:blassey] from comment #2)
> Comment on attachment 592127 [details] [diff] [review]
> Add support for Flash on Android 4.0+
> 
> Review of attachment 592127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the Looper on the gecko thread. Everything else can be taken care of
> in follow up bugs.
> 
> ::: dom/plugins/base/android/ANPAudio.cpp
> @@ +350,5 @@
> >  }
> >  
> > +uint32_t
> > +anp_audio_trackLatency(ANPAudioTrack* s) {
> > +  return 1;
> 
> get a bug on file for this being stubbed out and put the bug number in a
> comment here

Done, bug 721835

> 
> ::: dom/plugins/base/android/ANPOpenGL.cpp
> @@ +35,5 @@
> > +using namespace mozilla;
> > +using namespace mozilla::gl;
> > +
> > +static ANPEGLContext anp_opengl_acquireContext(NPP inst) {
> > +    NOT_IMPLEMENTED();
> 
> is there a bug on file for the Honeycomb support? if not, file one and put
> the bug number in a comment here

Done, bug 687267.

> 
> ::: dom/plugins/base/android/ANPSystem.cpp
> @@ +48,5 @@
> >  
> >  #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GeckoPlugins" , ## args)
> >  #define ASSIGN(obj, name)   (obj)->name = anp_system_##name
> >  
> > +
> 
> loose the extra ws
> 
> @@ +65,5 @@
> >  
> > +const char*
> > +anp_system_getApplicationDataDirectory(NPP instance)
> > +{
> > +  return anp_system_getApplicationDataDirectory();
> 
> when this version of getApplicationDataDirectory() is called, I'd like to
> append the plugin's package name (which you can get from the NPP)
> 
> This fine to land as is, but please file a follow up

Done, bug 721838

> 
> ::: dom/plugins/base/android/ANPVideo.cpp
> @@ +28,5 @@
> > +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GeckoPlugins" , ## args)
> > +#define ASSIGN(obj, name)   (obj)->name = anp_video_##name
> > +
> > +static ANPNativeWindow anp_video_acquireNativeWindow(NPP instance) {
> > +    LOG("%s: not impl", __PRETTY_FUNCTION__);
> 
> file a bug for these functions and put its number in a comment
> also, why aren't you using NOT_IMPLEMENTED()?
> 

Done, bug 721840. Also, I inherited those stubs from you :). Fixed.

> ::: dom/plugins/base/android/ANPWindow.cpp
> @@ +93,5 @@
> > +  nsNPAPIPluginInstance* pinst = static_cast<nsNPAPIPluginInstance*>(instance->ndata);
> > +
> > +  nsPluginInstanceOwner* owner;
> > +  if (NS_FAILED(pinst->GetOwner((nsIPluginInstanceOwner**)&owner))) {
> > +    return rect;
> 
> if you're going to return that rect, initialize it to something

Done.

> 
> ::: dom/plugins/base/android/android_npapi.h
> @@ +961,5 @@
> >                  ANPBitmap   bitmap;
> > +                struct {
> > +                    int32_t width;
> > +                    int32_t height;
> > +                } surface;
> 
> surfaceSize?

I just copied whatever Android had there. I guess surfaceSize works.

> 
> @@ +1030,5 @@
> > +typedef void* ANPEGLContext;
> > +
> > +struct ANPOpenGLInterfaceV0 : ANPInterface {
> > +    /**
> > +     */
> 
> loose the empty comments
> 

Done.

> ::: dom/plugins/base/nsPluginInstanceOwner.cpp
> @@ +1694,5 @@
> > +
> > +  mInstance->HandleEvent(&event, nsnull);
> > +}
> > +
> > +void nsPluginInstanceOwner::SendLifecycleEvent(bool onScreen)
> 
> what about the rest of the life cycle events? Probably better to names this
> SendOnScreenEvent to leave room for all the others

Quite right, I'll refactor or rename.

> 
> ::: mobile/android/base/GeckoApp.java
> @@ +222,5 @@
> >      String[] getPluginDirectories() {
> > +        // we don't support Honeycomb
> > +        if (Build.VERSION.SDK_INT == Build.VERSION_CODES.HONEYCOMB ||
> > +            Build.VERSION.SDK_INT == Build.VERSION_CODES.HONEYCOMB_MR1 ||
> > +            Build.VERSION.SDK_INT == Build.VERSION_CODES.HONEYCOMB_MR2)
> 
> let's do:
> if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) &&
>     Build.VERSION.SDK_INT < Build.VERSION_CODES.ICE_CREAM_SANDWICH)

Done.

> 
> @@ +1450,5 @@
> > +
> > +        if (layer == null) {
> > +            layer = SurfaceTextureLayer.create(new Point(x, y), new IntSize(w, h), metrics.getZoomFactor(), inverted);
> > +            if (layer == null) {
> > +                Log.i(LOGTAG, "Failed to create plugin layer");
> 
> this seems more severe than just info. At least warning, maybe logged as an
> error

Well, it might actually happen more than you realize. We have to pregenerate the texture that the SurfaceTexture uses, because we can't do it here (not on the GL thread). If we run out of textures, we are sunk. Thankfully, Flash will retry later, so it normally works out. Still, I agree it should at least be a warning.

> 
> @@ +1469,5 @@
> > +        return layer.getSurface();
> > +    }
> > +    
> > +    private void hidePluginLayer(Layer layer) {
> > +        LayerView layerView = mLayerController.getView();
> 
> do we need to null check mLayerController here (and else where)? I don't
> think so, but wanted to ask.
> 

It is set in onCreate, so I don't think we need to worry about it.

> ::: mobile/android/base/GeckoAppShell.java
> @@ +462,5 @@
> >      }
> >  
> >      public static void runGecko(String apkPath, String args, String url, boolean restoreSession) {
> > +        // Needed for flash to not crash
> > +        Looper.prepare();
> 
> No!!!
> 
> if anything gets posted to this looper it'll never run. This isn't just for
> flash, this will hide crappy errors in our own code as well.
> 
> We hit this When first implementing flash support. The solution was to run
> certain things with the PostToJavaThread() to get them on our main thread.

Ah, ok. That was in your initial Honeycomb patch and I just left it. Nuked.

> 
> ::: mobile/android/base/gfx/SurfaceTextureLayer.java
> @@ +232,5 @@
> > +		GLES11.glDrawArrays(GL10.GL_TRIANGLE_STRIP, 0, vertices.length / 2);
> > +
> > +		// Clean up
> > +		GLES11.glDisableClientState(GL10.GL_VERTEX_ARRAY);
> > +		GLES11.glDisableClientState(GL10.GL_TEXTURE_COORD_ARRAY);
> 
> TABS!!!

OMG!! Fixed.
Comment 4 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-27 12:47:01 PST
Created attachment 592220 [details] [diff] [review]
Add support for Flash on Android 4.0+
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-01-27 13:30:19 PST
Comment on attachment 592220 [details] [diff] [review]
Add support for Flash on Android 4.0+

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

::: mobile/android/base/GeckoAppShell.java
@@ +461,5 @@
>          }
>      }
>  
>      public static void runGecko(String apkPath, String args, String url, boolean restoreSession) {
> +

remove the blank line
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-30 15:47:49 PST
Created attachment 592899 [details] [diff] [review]
Add support for Flash on Android 4.0+

Updated patch that implements the ANPVideo interface
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-01-30 20:24:33 PST
Comment on attachment 592899 [details] [diff] [review]
Add support for Flash on Android 4.0+

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

::: dom/plugins/base/android/ANPVideo.cpp
@@ +88,5 @@
> +  Invalidate(instance);
> +}
> +
> +static void anp_video_setFramerateCallback(NPP instance, const ANPNativeWindow window, ANPVideoFrameCallbackProc callback) {
> +    NOT_IMPLEMENTED();

file a follow up

::: mobile/android/base/gfx/LayerRenderer.java
@@ +173,5 @@
> +            mExtraLayers.add(layer);
> +        }
> +    }
> +
> +    public synchronized void removeLayer(Layer layer) {

why are you synchronizing on the LayerRendered object?
Comment 8 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-31 06:26:03 PST
(In reply to Brad Lassey [:blassey] from comment #7)
> Comment on attachment 592899 [details] [diff] [review]
> Add support for Flash on Android 4.0+
> 
> Review of attachment 592899 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/android/ANPVideo.cpp
> @@ +88,5 @@
> > +  Invalidate(instance);
> > +}
> > +
> > +static void anp_video_setFramerateCallback(NPP instance, const ANPNativeWindow window, ANPVideoFrameCallbackProc callback) {
> > +    NOT_IMPLEMENTED();
> 
> file a follow up

Bug 722682

> 
> ::: mobile/android/base/gfx/LayerRenderer.java
> @@ +173,5 @@
> > +            mExtraLayers.add(layer);
> > +        }
> > +    }
> > +
> > +    public synchronized void removeLayer(Layer layer) {
> 
> why are you synchronizing on the LayerRendered object?

Typo, fixed.
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-31 07:09:09 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87113ff33ff
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-31 08:22:46 PST
Yeah, we need the mozconfig to point to SDK 14. Bug 722719
Comment 12 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-31 19:09:06 PST
Relanded after the mozconfig change

http://hg.mozilla.org/integration/mozilla-inbound/rev/e57ddd9fdd5e
Comment 13 Boris Zbarsky [:bz] 2012-01-31 20:40:23 PST
Backed out from inbound, because it broke the build.
Comment 14 Boris Zbarsky [:bz] 2012-01-31 20:41:11 PST
The breakage seems to be identical to that linked from comment 10.
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-01-31 22:06:48 PST
Created attachment 593317 [details] [diff] [review]
patch to use reflection and not require ICS SDK
Comment 16 Doug Turner (:dougt) 2012-01-31 22:10:09 PST
Comment on attachment 593317 [details] [diff] [review]
patch to use reflection and not require ICS SDK

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

r+ w/ fixes.

::: mobile/android/base/GeckoApp.java
@@ +221,4 @@
>      String[] getPluginDirectories() {
>          // we don't support Honeycomb
>          if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB &&
> +            Build.VERSION.SDK_INT < 14)

Build.VERSION.SDK_INT < 14) // Build.VERSION_CODES.ICE_CREAM_SANDWICH

::: mobile/android/base/gfx/SurfaceTextureLayer.java
@@ +157,5 @@
>  
>      @Override
>      protected void finalize() throws Throwable {
> +        if (mSurfaceTexture != null) {
> +            try { SurfaceTexture.class.getDeclaredMethod("release").invoke(mSurfaceTexture); }

new lines are cheap.  expand this out so that it is easier to read please.

@@ +159,5 @@
>      protected void finalize() throws Throwable {
> +        if (mSurfaceTexture != null) {
> +            try { SurfaceTexture.class.getDeclaredMethod("release").invoke(mSurfaceTexture); }
> +            catch (NoSuchMethodException nsme) { Log.e(LOGTAG, "error finding release method on mSurfaceTexture", nsme); }
> +            catch (IllegalAccessException iae) { Log.e(LOGTAG, "error invoking release method on mSurfaceTexture", iae); }

Since this is really just cleanup and I really don't want to crash, can you also catch Exception and log it?
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2012-01-31 22:34:52 PST
re-landed on inbound along with my patch to use reflection
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e88718a9469
https://hg.mozilla.org/integration/mozilla-inbound/rev/587eea31cfa1

We no longer require the ICS SDK to build
Comment 18 Tom Levels 2012-02-02 00:24:27 PST
Is this also easy to implement for Android 3 (see https://bugzilla.mozilla.org/show_bug.cgi?id=687267)?
Comment 20 Brad Lassey [:blassey] (use needinfo?) 2012-02-02 14:19:29 PST
Comment on attachment 592899 [details] [diff] [review]
Add support for Flash on Android 4.0+

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
SDK 15 will be required to build
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Comment 21 Brad Lassey [:blassey] (use needinfo?) 2012-02-02 14:19:55 PST
Comment on attachment 593317 [details] [diff] [review]
patch to use reflection and not require ICS SDK

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
No plugin support on ICS
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:
Comment 22 Alex Keybl [:akeybl] 2012-02-05 13:43:09 PST
Comment on attachment 592899 [details] [diff] [review]
Add support for Flash on Android 4.0+

[Triage Comment]
\o/. Mobile only - approved for Aurora 12 and Beta 11.
Comment 25 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-02-08 14:05:49 PST
*** Bug 721840 has been marked as a duplicate of this bug. ***

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