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

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

13 Branch
Firefox 13
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 fixed, fennec11+)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 2 obsolete attachments)

We need it.
Depends on: 707824, 719872
Created attachment 592127 [details] [diff] [review]
Add support for Flash on Android 4.0+
Attachment #592127 - Flags: review?(blassey.bugs)
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!!!
Attachment #592127 - Flags: review?(blassey.bugs) → review-
(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.
Created attachment 592220 [details] [diff] [review]
Add support for Flash on Android 4.0+
Attachment #592220 - Flags: review?(blassey.bugs)
Attachment #592127 - Attachment is obsolete: true
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
Attachment #592220 - Flags: review?(blassey.bugs) → review+
Created attachment 592899 [details] [diff] [review]
Add support for Flash on Android 4.0+

Updated patch that implements the ANPVideo interface
Attachment #592899 - Flags: review?(blassey.bugs)
Attachment #592220 - Attachment is obsolete: true
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?
Attachment #592899 - Flags: review?(blassey.bugs) → review+
(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.
Version: Trunk → Firefox 13
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87113ff33ff
Backed out for build failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b87113ff33ff

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6aa3112c2e6
Yeah, we need the mozconfig to point to SDK 14. Bug 722719
Depends on: 722719
Relanded after the mozconfig change

http://hg.mozilla.org/integration/mozilla-inbound/rev/e57ddd9fdd5e
Target Milestone: --- → Firefox 12
tracking-fennec: --- → ?
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → fixed
Target Milestone: Firefox 12 → Firefox 13
Backed out from inbound, because it broke the build.
status-firefox13: fixed → affected
The breakage seems to be identical to that linked from comment 10.
Created attachment 593317 [details] [diff] [review]
patch to use reflection and not require ICS SDK
Attachment #593317 - Flags: review?(doug.turner)
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?
Attachment #593317 - Flags: review?(doug.turner) → review+
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
tracking-fennec: ? → 11+
Whiteboard: [inbound]

Comment 18

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

Updated

5 years ago
Blocks: 718998
https://hg.mozilla.org/mozilla-central/rev/9e88718a9469
https://hg.mozilla.org/mozilla-central/rev/587eea31cfa1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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:
Attachment #592899 - Flags: approval-mozilla-beta?
Attachment #592899 - Flags: approval-mozilla-aurora?
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:
Attachment #593317 - Flags: approval-mozilla-beta?
Attachment #593317 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox13: affected → ---
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.
Attachment #592899 - Flags: approval-mozilla-beta?
Attachment #592899 - Flags: approval-mozilla-beta+
Attachment #592899 - Flags: approval-mozilla-aurora?
Attachment #592899 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Attachment #593317 - Flags: approval-mozilla-beta?
Attachment #593317 - Flags: approval-mozilla-beta+
Attachment #593317 - Flags: approval-mozilla-aurora?
Attachment #593317 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d39937f28182
https://hg.mozilla.org/releases/mozilla-aurora/rev/976580a0c9d7
status-firefox12: affected → fixed
status-firefox13: --- → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/adad0b40879f
https://hg.mozilla.org/releases/mozilla-beta/rev/6f91168863bf
status-firefox11: affected → fixed
Duplicate of this bug: 721840
You need to log in before you can comment on or make changes to this bug.