721741 - Make Flash plugins work on Ice Cream Sandwich (Android 4.0)

Status: RESOLVED FIXED

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

Description

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

We need it.

Comment 1

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

Attachment: Add support for Flash on Android 4.0+

Comment 2

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

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

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

Attachment: Add support for Flash on Android 4.0+

Comment 5

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

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

Attachment: Add support for Flash on Android 4.0+

Updated patch that implements the ANPVideo interface

Comment 7

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

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b87113ff33ff

Comment 10

[Ed Morley [:emorley]]

Backed out for build failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b87113ff33ff

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6aa3112c2e6

Comment 11

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

Yeah, we need the mozconfig to point to SDK 14. Bug 722719

Comment 12

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

Relanded after the mozconfig change

http://hg.mozilla.org/integration/mozilla-inbound/rev/e57ddd9fdd5e

Comment 13

[Boris Zbarsky [:bzbarsky]]

Backed out from inbound, because it broke the build.

Comment 14

[Boris Zbarsky [:bzbarsky]]

The breakage seems to be identical to that linked from comment 10.

Comment 15

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

Attachment: patch to use reflection and not require ICS SDK

Comment 16

[Doug Turner (:dougt)]

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

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]

Is this also easy to implement for Android 3 (see https://bugzilla.mozilla.org/show_bug.cgi?id=687267)?

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9e88718a9469
https://hg.mozilla.org/mozilla-central/rev/587eea31cfa1

Comment 20

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

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

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

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 23

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d39937f28182
https://hg.mozilla.org/releases/mozilla-aurora/rev/976580a0c9d7

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/adad0b40879f
https://hg.mozilla.org/releases/mozilla-beta/rev/6f91168863bf