687267 - (honeycomb-flash) support for flash on Android Fennec on honeycomb

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, enhancement, P1)

Description

[Doug Turner (:dougt)]

+++ This bug was initially created as a clone of Bug #630007 +++

In bug 63007, we support pre-honeycomb.  This bug should be about the work to suppor honeycomb

Comment 1

[Doug Turner (:dougt)]

Attachment: stub

Comment 2

[Tom Levels]

Is this bug still valid since Flash won't be supported on mobile anymore?

Comment 3

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

(In reply to Tom Levels from comment #2)
> Is this bug still valid since Flash won't be supported on mobile anymore?

yup, flash won't be going away on Android for a while

Comment 4

[ekerazha]

Is this related to Ice Cream Sandwich (Android 4.0) too? I guess so.

Comment 5

[Mardeg]

(In reply to ekerazha from comment #4)
> Is this related to Ice Cream Sandwich (Android 4.0) too? I guess so.

That's bug 721741

Comment 7

[Matt Brubeck (:mbrubeck)]

Are we targeting this for Firefox 15 or 16?

Comment 8

[Arkady Blyakher [:rkd]]

Attachment: IN PROGRESS PATCH

Comment 9

[Arkady Blyakher [:rkd]]

Attachment: IN PROGRESS PATCH 2

Comment 10

[Mozilla RelEng Bot]

Try run for 250fefba2225 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=250fefba2225
Results (out of 34 total builds):
    exception: 14
    success: 15
    warnings: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-250fefba2225

Comment 11

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

Attachment: Initial support for Flash on Honeycomb

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 638788 [details] [diff] [review]
Initial support for Flash on Honeycomb

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

Nice! A few nits to pick below, but good stuff - I'd like to see a higher density of comments, but you could very well argue that the lack of comments in this is to fit in with the code that's already there :p

BenWa will be a better reviewer for the layers/painting stuff than me, and perhaps you might want to get jgilbert to review too for the texture sharing stuff? It all looks good to me though. Also not sure who's the most appropriate to approve of changes to nsObjectFrame either, may want to get approval from someone on layout team(?)

::: dom/plugins/base/android/ANPOpenGL.cpp
@@ +44,5 @@
> +    info.height = pluginRect.height;
> +
> +    nsRefPtr<GLContext> context = pinst->GLContext();
> +
> +    info.internalFormat = LOCAL_GL_BGRA;

I assume that it always allocates in this format? Maybe add a comment(?)

::: dom/plugins/base/android/ANPVideo.cpp
@@ +39,5 @@
>  }
>  
>  static ANPNativeWindow anp_video_acquireNativeWindow(NPP instance) {
> +  if (AndroidBridge::Bridge()->IsHoneycomb())
> +    return NULL;

Is this a case of this should happen on Honeycomb, or that it isn't implemented yet? A comment somewhere might be a good idea.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +68,5 @@
>    ANPEvent mEvent;
>    bool mCanceled;
>  };
>  
> +static nsRefPtr<GLContext> sPluginContext = NULL;

nsnull

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +171,5 @@
>  already_AddRefed<ImageContainer>
>  nsPluginInstanceOwner::GetImageContainer()
>  {
> +#if MOZ_WIDGET_ANDROID
> +  if (!AndroidBridge::Bridge()->IsHoneycomb())

A brief comment on what the other paths non-honeycomb takes are might be useful here.

@@ +2966,5 @@
>      float xResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetXResolution();
>      float yResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetYResolution();
>      pluginRect.Scale(xResolution, yResolution);
>  
> +

Unnecessary new-line added here.

::: dom/plugins/base/nsPluginInstanceOwner.h
@@ +291,5 @@
>    void FixUpURLS(const nsString &name, nsAString &value);
>  #ifdef MOZ_WIDGET_ANDROID
>    void SendSize(int width, int height);
>  
> +  gfxRect GetPluginRect(bool aScaled);

Given that this is only ever called once with aScaled = true, does it make sense to add this parameter instead of just doing the extra work in that one place? (which is above, in GetVisibleRect()).

::: gfx/gl/GLContext.h
@@ +793,5 @@
>          return mIsOffscreen;
>      }
>  
>      /**
>       * Create new shared GLContext content handle, must be released by ReleaseSharedHandle.

A comment about specifying aTexture = 0 meaning a texture will be created might be nice here.

@@ +1747,5 @@
>          GLsizei samples;
>      };
>  
>      GLFormats ChooseGLFormats(ContextFormat& aCF);
> +

Another errant new-line.

::: gfx/gl/GLContextProviderEGL.cpp
@@ -774,5 @@
>      if (!tex->CreateEGLImage()) {
>          NS_ERROR("EGLImage creation for EGLTextureWrapper failed");
>          delete tex;
> -        // Stop trying to create shared image Handle
> -        mShareWithEGLImage = false;

Why has this been removed?

::: gfx/layers/ImageLayers.h
@@ +815,5 @@
>  };
>  
> +class THEBES_API SharedTextureImage : public Image {
> +public:
> +  struct Data {

Any reason these are in a struct rather than just directly accessible?

::: gfx/layers/basic/BasicLayers.cpp
@@ +2638,5 @@
>        ->Paint(aContext, nsnull);
>    }
>  
> +  if (image->GetFormat() == Image::SHARED_TEXTURE &&
> +      BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL) {

Is there any need to check the parent backend type here? Will it ever not be OpenGL if the format is SharedTexture?

@@ +2643,5 @@
> +    SharedTextureImage *sharedImage = static_cast<SharedTextureImage*>(image);
> +    const SharedTextureImage::Data *data = sharedImage->GetData();
> +
> +    SharedTextureDescriptor texture(data->mShareType, data->mHandle, data->mSize, data->mInverted);
> +    BasicManager()->PaintedImage(BasicManager()->Hold(this), texture);

What's going on here? Is it just painting to the shared texture? A comment definitely wouldn't hurt.

::: layout/generic/nsObjectFrame.cpp
@@ +1177,3 @@
>      if (aBuilder->IsPaintingToWindow() &&
>          GetLayerState(aBuilder, nsnull) == LAYER_ACTIVE &&
>          IsTransparentMode()) {

So, I guess this code reads back plugin data to draw into layers, which we don't need to do on honeycomb as we force an image layer and use a shared texture... Maybe a comment?

@@ +1490,5 @@
>  #endif
>  
> +#ifdef MOZ_WIDGET_ANDROID
> +  if (AndroidBridge::Bridge()->IsHoneycomb())
> +    return LAYER_ACTIVE;

I assume that this is done to stop the layer being flattened into a ancestor layer - maybe add a comment as such?

::: mobile/android/base/GeckoApp.java
@@ -248,5 @@
>                  }
>              }
>          }
>  
> -        // we don't support Honeycomb

whoo!

Comment 13

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

(In reply to Chris Lord [:cwiiis] from comment #12)
> Comment on attachment 638788 [details] [diff] [review]
> Initial support for Flash on Honeycomb
> 
> Review of attachment 638788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! A few nits to pick below, but good stuff - I'd like to see a higher
> density of comments, but you could very well argue that the lack of comments
> in this is to fit in with the code that's already there :p
> 
> BenWa will be a better reviewer for the layers/painting stuff than me, and
> perhaps you might want to get jgilbert to review too for the texture sharing
> stuff? It all looks good to me though. Also not sure who's the most
> appropriate to approve of changes to nsObjectFrame either, may want to get
> approval from someone on layout team(?)

Yeah, definitely needs more comments. And I'd really like to get some feedback on the EGLImage usage.

> 
> ::: dom/plugins/base/android/ANPOpenGL.cpp
> @@ +44,5 @@
> > +    info.height = pluginRect.height;
> > +
> > +    nsRefPtr<GLContext> context = pinst->GLContext();
> > +
> > +    info.internalFormat = LOCAL_GL_BGRA;
> 
> I assume that it always allocates in this format? Maybe add a comment(?)

I think we're just suggesting a format to use here. Flash can use this or not, AIUI. Added a small comment attempting to explain.

> 
> ::: dom/plugins/base/android/ANPVideo.cpp
> @@ +39,5 @@
> >  }
> >  
> >  static ANPNativeWindow anp_video_acquireNativeWindow(NPP instance) {
> > +  if (AndroidBridge::Bridge()->IsHoneycomb())
> > +    return NULL;
> 
> Is this a case of this should happen on Honeycomb, or that it isn't
> implemented yet? A comment somewhere might be a good idea.

Not implemented yet. I'll change accordingly.

> 
> ::: dom/plugins/base/nsNPAPIPluginInstance.cpp
> @@ +68,5 @@
> >    ANPEvent mEvent;
> >    bool mCanceled;
> >  };
> >  
> > +static nsRefPtr<GLContext> sPluginContext = NULL;
> 
> nsnull

Fixed

> 
> ::: dom/plugins/base/nsPluginInstanceOwner.cpp
> @@ +171,5 @@
> >  already_AddRefed<ImageContainer>
> >  nsPluginInstanceOwner::GetImageContainer()
> >  {
> > +#if MOZ_WIDGET_ANDROID
> > +  if (!AndroidBridge::Bridge()->IsHoneycomb())
> 
> A brief comment on what the other paths non-honeycomb takes are might be
> useful here.

Yup, done.

> 
> @@ +2966,5 @@
> >      float xResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetXResolution();
> >      float yResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetYResolution();
> >      pluginRect.Scale(xResolution, yResolution);
> >  
> > +
> 
> Unnecessary new-line added here.

Fixed

> 
> ::: dom/plugins/base/nsPluginInstanceOwner.h
> @@ +291,5 @@
> >    void FixUpURLS(const nsString &name, nsAString &value);
> >  #ifdef MOZ_WIDGET_ANDROID
> >    void SendSize(int width, int height);
> >  
> > +  gfxRect GetPluginRect(bool aScaled);
> 
> Given that this is only ever called once with aScaled = true, does it make
> sense to add this parameter instead of just doing the extra work in that one
> place? (which is above, in GetVisibleRect()).

Yeah, initially I wanted to do something different so it doesn't make sense anymore. I'll remove the param and do as you suggest.

> 
> ::: gfx/gl/GLContext.h
> @@ +793,5 @@
> >          return mIsOffscreen;
> >      }
> >  
> >      /**
> >       * Create new shared GLContext content handle, must be released by ReleaseSharedHandle.
> 
> A comment about specifying aTexture = 0 meaning a texture will be created
> might be nice here.

Yeah, definitely. Not in love with this API at all. I'll add romaxa to see what he thinks as well.

> 
> @@ +1747,5 @@
> >          GLsizei samples;
> >      };
> >  
> >      GLFormats ChooseGLFormats(ContextFormat& aCF);
> > +
> 
> Another errant new-line.

Eek

> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ -774,5 @@
> >      if (!tex->CreateEGLImage()) {
> >          NS_ERROR("EGLImage creation for EGLTextureWrapper failed");
> >          delete tex;
> > -        // Stop trying to create shared image Handle
> > -        mShareWithEGLImage = false;
> 
> Why has this been removed?

Sometimes, for a yet unknown reason, EGLImage creation will fail. We shouldn't give up forever in that case, IMHO.

> 
> ::: gfx/layers/ImageLayers.h
> @@ +815,5 @@
> >  };
> >  
> > +class THEBES_API SharedTextureImage : public Image {
> > +public:
> > +  struct Data {
> 
> Any reason these are in a struct rather than just directly accessible?

Just following the existing convention, not sure exactly.

> 
> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +2638,5 @@
> >        ->Paint(aContext, nsnull);
> >    }
> >  
> > +  if (image->GetFormat() == Image::SHARED_TEXTURE &&
> > +      BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL) {
> 
> Is there any need to check the parent backend type here? Will it ever not be
> OpenGL if the format is SharedTexture?

Just being careful, I suppose. I can't think of a reason in practice that it would not be the case, but who knows.

> 
> @@ +2643,5 @@
> > +    SharedTextureImage *sharedImage = static_cast<SharedTextureImage*>(image);
> > +    const SharedTextureImage::Data *data = sharedImage->GetData();
> > +
> > +    SharedTextureDescriptor texture(data->mShareType, data->mHandle, data->mSize, data->mInverted);
> > +    BasicManager()->PaintedImage(BasicManager()->Hold(this), texture);
> 
> What's going on here? Is it just painting to the shared texture? A comment
> definitely wouldn't hurt.

It's just passing on the shared handle to the parent here. It's doing the same thing as the other if/else blocks so I think in larger context it may make more sense to you.

> 
> ::: layout/generic/nsObjectFrame.cpp
> @@ +1177,3 @@
> >      if (aBuilder->IsPaintingToWindow() &&
> >          GetLayerState(aBuilder, nsnull) == LAYER_ACTIVE &&
> >          IsTransparentMode()) {
> 
> So, I guess this code reads back plugin data to draw into layers, which we
> don't need to do on honeycomb as we force an image layer and use a shared
> texture... Maybe a comment?

Indeed. Added a comment.

> 
> @@ +1490,5 @@
> >  #endif
> >  
> > +#ifdef MOZ_WIDGET_ANDROID
> > +  if (AndroidBridge::Bridge()->IsHoneycomb())
> > +    return LAYER_ACTIVE;
> 
> I assume that this is done to stop the layer being flattened into a ancestor
> layer - maybe add a comment as such?

Yup, done

> 
> ::: mobile/android/base/GeckoApp.java
> @@ -248,5 @@
> >                  }
> >              }
> >          }
> >  
> > -        // we don't support Honeycomb
> 
> whoo!

Comment 14

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

Attachment: Fixed up patch

Comment 15

[Mark Callow]

Adobe just announced that they will not provide a Flash player for Android 4.1 and existing versions will not run in Android 4.1. As previously announced, they have stopped development of mobile Flash.

Do you really want to continue fixing this bug?

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Callow from comment #15)
> Adobe just announced that they will not provide a Flash player for Android
> 4.1 and existing versions will not run in Android 4.1. As previously
> announced, they have stopped development of mobile Flash.
> 
> Do you really want to continue fixing this bug?

Since the code is already written and we have perhaps a million active installs on existing Android tablets, it's probably worth the relatively small remaining effort to review the patch and check it in.

Comment 17

[Kelsey Gilbert [:jgilbert]]

More like relatively medium. The EGL stuff this is based on is just now going in, is fresh (relatively untested) code, and is going to be hit with a refactor a few weeks later.

I do think it's probably worth it in the short term, but it is somewhat daring.

Comment 18

[Ali Juma [:ajuma]]

Comment on attachment 639354 [details] [diff] [review]
Fixed up patch

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

Looks good!

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +122,5 @@
> +    if (!mPosted)
> +      return NULL;
> +
> +    if (mNeedFlip) {
> +      Flip();

"Flip" sounds like a geometric operation. How about "Swap" instead?

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 639354 [details] [diff] [review]
Fixed up patch

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

Great work, this is very impressive.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +89,5 @@
> +    sPluginContext->fDeleteTextures(1, &mTexBack);
> +    sPluginContext->fDeleteTextures(1, &mTexFront);
> +
> +    // FIXME: ImageLayerOGL will delete the shared handle if it has it
> +    // but we can't be sure of that here

Shouldn't we fix this before landing? How likely is this?

@@ +195,5 @@
>    mUsePluginLayersPref =
>      Preferences::GetBool("plugins.use_layers", mUsePluginLayersPref);
>  
> +#if MOZ_WIDGET_ANDROID
> +  mSharedTexture = new SharedPluginTexture();

Are you guaranteed this will be needed (excluding catastrophic failures)? This object is very expensive to create (creates a GLContext). Will it be used right away or a bit later during page load?

It may make sense to make this object lazy init.

@@ +211,5 @@
>      mMIMEType = nsnull;
>    }
>  
>  #if MOZ_WIDGET_ANDROID
> +  delete mSharedTexture;

nsRefPtr would make only the next line required.

@@ +928,5 @@
>  }
>  
> +GLContext* nsNPAPIPluginInstance::GLContext()
> +{
> +  sPluginContext->MakeCurrent();

Do you always want to make the context current when getting it? If so it would be worth documenting why.

@@ +940,5 @@
> +
> +void nsNPAPIPluginInstance::ReleaseTexture()
> +{
> +  mSharedTexture->Release();
> +  RedrawPlugin();

Not a fan of having a release trigger a redraw, this is very unusual. You're doing this intentionally so perhaps you should just couple ReleaseTexture with this call at the call site?

@@ +954,5 @@
> +  if (aInverted == mInverted)
> +    return;
> +
> +  mInverted = aInverted;
> +  RedrawPlugin();  

Again I don't like having this call RedrawPlugin. But if you never need to change this properly and no redraw immediately then I guess this is acceptable.

::: dom/plugins/base/nsNPAPIPluginInstance.h
@@ +5,5 @@
>  
>  #ifndef nsNPAPIPluginInstance_h_
>  #define nsNPAPIPluginInstance_h_
>  
> +#include "nsAutoPtr.h"

NIT: This could move to WIDGET_ANDROID I think.

@@ +242,5 @@
>    bool mWakeLocked;
>    bool mFullScreen;
> +  bool mInverted;
> +
> +  SharedPluginTexture* mSharedTexture;

nsRefPtr would make this easier

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +872,5 @@
> +    }
> +
> +    gl()->ApplyFilterToBoundTexture(mFilter);
> +    program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), mSize));
> +    mOGLManager->BindAndDrawQuad(program, !mInverted);

!mInverted? Really?

::: widget/android/AndroidBridge.h
@@ +356,5 @@
>      void PumpMessageLoop();
>  
>      void NotifyWakeLockChanged(const nsAString& topic, const nsAString& state);
>  
> +    int GetAPIVersion() { return mAPIVersion; }

This isn't used. I don't mind keeping it for later if you do plan on using it. Otherwise remove it.

Comment 20

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

(In reply to Benoit Girard (:BenWa) from comment #19)
> Comment on attachment 639354 [details] [diff] [review]
> Fixed up patch
> 
> Review of attachment 639354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work, this is very impressive.
> 
> ::: dom/plugins/base/nsNPAPIPluginInstance.cpp
> @@ +89,5 @@
> > +    sPluginContext->fDeleteTextures(1, &mTexBack);
> > +    sPluginContext->fDeleteTextures(1, &mTexFront);
> > +
> > +    // FIXME: ImageLayerOGL will delete the shared handle if it has it
> > +    // but we can't be sure of that here
> 
> Shouldn't we fix this before landing? How likely is this?

We can be pretty sure I think. At least in testing it never happened.

> 
> @@ +195,5 @@
> >    mUsePluginLayersPref =
> >      Preferences::GetBool("plugins.use_layers", mUsePluginLayersPref);
> >  
> > +#if MOZ_WIDGET_ANDROID
> > +  mSharedTexture = new SharedPluginTexture();
> 
> Are you guaranteed this will be needed (excluding catastrophic failures)?
> This object is very expensive to create (creates a GLContext). Will it be
> used right away or a bit later during page load?
> 
> It may make sense to make this object lazy init.

Yeah, I made it lazy init now.

> 
> @@ +211,5 @@
> >      mMIMEType = nsnull;
> >    }
> >  
> >  #if MOZ_WIDGET_ANDROID
> > +  delete mSharedTexture;
> 
> nsRefPtr would make only the next line required.

Yeah, I've refized it now.

> 
> @@ +928,5 @@
> >  }
> >  
> > +GLContext* nsNPAPIPluginInstance::GLContext()
> > +{
> > +  sPluginContext->MakeCurrent();
> 
> Do you always want to make the context current when getting it? If so it
> would be worth documenting why.

Nope. Moved the MakeCurrent call to ANPOpenGL::acquireContext

> 
> @@ +940,5 @@
> > +
> > +void nsNPAPIPluginInstance::ReleaseTexture()
> > +{
> > +  mSharedTexture->Release();
> > +  RedrawPlugin();
> 
> Not a fan of having a release trigger a redraw, this is very unusual. You're
> doing this intentionally so perhaps you should just couple ReleaseTexture
> with this call at the call site?

Sure, fixed

> 
> @@ +954,5 @@
> > +  if (aInverted == mInverted)
> > +    return;
> > +
> > +  mInverted = aInverted;
> > +  RedrawPlugin();  
> 
> Again I don't like having this call RedrawPlugin. But if you never need to
> change this properly and no redraw immediately then I guess this is
> acceptable.

Moved it to call site here as well

> 
> ::: dom/plugins/base/nsNPAPIPluginInstance.h
> @@ +5,5 @@
> >  
> >  #ifndef nsNPAPIPluginInstance_h_
> >  #define nsNPAPIPluginInstance_h_
> >  
> > +#include "nsAutoPtr.h"
> 
> NIT: This could move to WIDGET_ANDROID I think.

Yup, done

> 
> @@ +242,5 @@
> >    bool mWakeLocked;
> >    bool mFullScreen;
> > +  bool mInverted;
> > +
> > +  SharedPluginTexture* mSharedTexture;
> 
> nsRefPtr would make this easier

Done

> 
> ::: gfx/layers/opengl/ImageLayerOGL.cpp
> @@ +872,5 @@
> > +    }
> > +
> > +    gl()->ApplyFilterToBoundTexture(mFilter);
> > +    program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), mSize));
> > +    mOGLManager->BindAndDrawQuad(program, !mInverted);
> 
> !mInverted? Really?

Yeah, I guess the plugin's version of inverted is opposite from ours. I've changed it to do that flip when we call nsNPAPIPluginInstance::SetInverted instead.

> 
> ::: widget/android/AndroidBridge.h
> @@ +356,5 @@
> >      void PumpMessageLoop();
> >  
> >      void NotifyWakeLockChanged(const nsAString& topic, const nsAString& state);
> >  
> > +    int GetAPIVersion() { return mAPIVersion; }
> 
> This isn't used. I don't mind keeping it for later if you do plan on using
> it. Otherwise remove it.

Yeah, I had to store the API version for IsHoneycomb(), so just added that since it seems useful. I'll probably use it in the next iteration for SurfaceTexture stuff.

Comment 21

[Benoit Girard (:BenWa)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #20)
> > !mInverted? Really?
> 
> Yeah, I guess the plugin's version of inverted is opposite from ours. I've
> changed it to do that flip when we call nsNPAPIPluginInstance::SetInverted
> instead.
> 

Alright, can you at least comment that it's intentional, otherwise every one who reads the code will lose time.

Comment 22

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

Attachment: More fixes

Comment 23

[Benoit Girard (:BenWa)]

Comment on attachment 640659 [details] [diff] [review]
More fixes

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

r+ with these fixes.

::: dom/plugins/base/android/ANPOpenGL.cpp
@@ +59,5 @@
>  }
>  
>  static void anp_opengl_invertPluginContent(NPP instance, bool isContentInverted) {
> +    nsNPAPIPluginInstance* pinst = static_cast<nsNPAPIPluginInstance*>(instance->ndata);
> +    pinst->SetInverted(!isContentInverted /* our definition of inverted is the opposite of the plugin's */);

Nit: Don't really like /* */ within a code statement.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +75,5 @@
> +
> +static bool EnsureGLContext()
> +{
> +  if (!sPluginContext) {
> +    sPluginContext = GLContextProvider::CreateOffscreen(gfxIntSize(16, 16));

Why 16x16? Maybe add a comment saying that's the best way to get a place holder context.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1794,5 @@
> +{
> +  gfxRect r = nsIntRect(0, 0, mPluginWindow->width, mPluginWindow->height);
> +
> +  float xResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetXResolution();
> +  float yResolution = mObjectFrame->PresContext()->GetRootPresContext()->PresShell()->GetYResolution();

I have no idea if this code is valid or not and if it handles all cases correctly.

::: gfx/gl/GLContext.h
@@ +576,5 @@
>      virtual GLContextType GetContextType() { return ContextTypeUnknown; }
>  
>      virtual bool MakeCurrentImpl(bool aForce = false) = 0;
>  
> +    virtual void* GetNativeContext() { return NULL; }

You should use GetNativeData instead. See CreateCompositor for an example:
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#1758

::: gfx/gl/GLContextProviderEGL.cpp
@@ +530,5 @@
>      void SetPlatformContext(void *context) {
>          mPlatformContext = context;
>      }
>  
> +    void* GetNativeContext() {

Remove, see above.

Comment 24

[ekerazha]

(In reply to Mark Callow from comment #15)
> Adobe just announced that they will not provide a Flash player for Android
> 4.1 and existing versions will not run in Android 4.1. As previously
> announced, they have stopped development of mobile Flash.
> 
> Do you really want to continue fixing this bug?

Flash Player isn't *officially* supported on Android 4.1.x, but it still works.

Comment 25

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

Attachment: Now with video support

Comment 26

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

The latest patch is a doozy. The most important part is that it implements the ANPVideoInterface for Honeycomb. In order to do this, it was necessary to add a way to draw a SurfaceTexture with our layer system. I've added nsSurfaceTexture, as well as a new Image type and associated stuff for using it in an ImageLayer.

There are a lot of things I'm not totally happy with, but we're on a tight deadline so I thought I would get this up as soon as I could. I guess what really bothers me the most are the changes to GLContext::CreateSharedHandle and the addition of GLContext::GetSharedTextureDetails, so feedback on that specifically would be great. Also the more eyes on this we can get the better I will feel, so anyone should feel free to driveby.

Comment 27

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

Attachment: Oops, this time without debug messages

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 642306 [details] [diff] [review]
Oops, this time without debug messages

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

This is the superficial stuff I've seen so far. I'm still reviewing the actual EGLImage/SharedTexture logic/timelines/etc.

::: gfx/gl/GLContext.cpp
@@ +31,5 @@
>  #ifdef DEBUG
>  PRUintn GLContext::sCurrentGLContextTLS = -1;
>  #endif
>  
> +PRUint32 GLContext::sDebugMode = 1;

You left DebugMode on.

::: gfx/gl/GLContext.h
@@ +50,5 @@
>  typedef uintptr_t SharedTextureHandle;
>  
>  enum ShaderProgramType {
>      RGBALayerProgramType,
> +    RGBALayerExternalOESProgramType,

Prefer to drop the vendor suffix here as well.

@@ +806,2 @@
>       */
> +    virtual SharedTextureHandle CreateSharedHandle(TextureImage::TextureShareType aType, void* aBuffer = nsnull, SharedTextureBufferType aBufferType = SharedTextureBufferType::None) { return nsnull; }

newlines

::: gfx/gl/GLContextProviderEGL.cpp
@@ +805,5 @@
>      GuaranteeResolve();
>  }
>  
>  SharedTextureHandle
> +GLContextEGL::CreateSharedHandle(TextureImage::TextureShareType aType, void* aBuffer /* = nsnull */, SharedTextureBufferType aBufferType /* = SharedTextureBufferType::None */)

Add newlines.

@@ +810,2 @@
>  {
> +    // Both EGLImage and SurfaceTexture only support ThreadShared currently

Should we not then assert?

@@ +812,5 @@
>      if (aType != TextureImage::ThreadShared)
>          return nsnull;
>  
> +    if (aBuffer && aBufferType == SharedTextureBufferType::SurfaceTexture) {
> +        // FIXME: it's possible to make nsSurfaceTexture work across processes. We should do that.

Why are we leaving a comment instead of filing a bug?

The basis for this is coming, but we should wait for New Shiny Things to get fitted in before trying to tackle it. (ETA: a few weeks)

@@ +820,5 @@
>      if (!mShareWithEGLImage)
>          return nsnull;
>  
>      MakeCurrent();
> +    GLuint texture;

Initialize this.

@@ -767,5 @@
>      if (!tex->CreateEGLImage()) {
>          NS_ERROR("EGLImage creation for EGLTextureWrapper failed");
>          delete tex;
> -        // Stop trying to create shared image Handle
> -        mShareWithEGLImage = false;

Oof, I'm gonna have to re-review some code then. :)

@@ +886,5 @@
> +
> +    if (wrapper->Type() == SharedHandleType::SurfaceTexture) {
> +        /**
> +         * NOTE: SurfaceTexture spams us if there are any existing GL errors, so we'll clear
> +         * them here in order to avoid that.

This doesn't seem like a good reason for adding a GetAndClearError() here. This should only be used when we need to clear any existing errors because we need to check if a command we send generates an error.

What spam is this causing?

@@ +894,5 @@
> +        SurfaceTextureWrapper* surfaceTextureWrapper = reinterpret_cast<SurfaceTextureWrapper*>(wrapper);
> +
> +        // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> +        // be applied to the texture coordinates. We should return that here
> +        // so we can render correctly.

So why are we not?

@@ +900,5 @@
> +    } else if (wrapper->Type() == SharedHandleType::Image) {
> +        NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime");
> +
> +        EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
> +        sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, wrap->GetEGLImage());

Wrong function. You want GLContext::fImageTargetTexture2D(). See bug 774059.

::: gfx/gl/GLDefs.h
@@ +3257,5 @@
>  // EGL_KHR_gl_texture_2D_image
>  #define LOCAL_EGL_GL_TEXTURE_2D               0x30B1
>  
> +// OES_EGL_image_external
> +#define LOCAL_GL_TEXTURE_EXTERNAL_OES         0x8D65

Drop vendor suffixes for internal use.

::: gfx/layers/basic/BasicImageLayer.cpp
@@ +10,5 @@
>  
>  using namespace mozilla::gfx;
>  
>  namespace mozilla {
> +namespace layers {

Null change?

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +668,5 @@
>  }
>  
>  ShadowImageLayerOGL::ShadowImageLayerOGL(LayerManagerOGL* aManager)
>    : ShadowImageLayer(aManager, nsnull)
> +  , LayerOGL(aManager), mSharedHandle(0), mInverted(false), mTexture(0)

newlines

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +751,5 @@
>    } else {
>      MakeCurrent();
>    }
>  
> +  TexturePoolOGL::Fill(gl());

Is there a pressing reason this pool stuff is static?

::: gfx/layers/opengl/LayerManagerOGLShaders.txt
@@ +210,5 @@
>    gl_FragColor = texture2D(uTexture, vTexCoord) * uLayerOpacity * mask;
>  }
>  @end
>  
> +// Single texture in RGBA format for use with GL_TEXTURE_EXTERNAL_OES

No _OES.

::: gfx/layers/opengl/TexturePoolOGL.cpp
@@ +32,5 @@
> +    sActiveContext->MakeCurrent();
> +
> +    sActiveContext->fGenTextures(1, &texture);
> +  } else {
> +      if (sTextures.GetSize() == 0) {

4-space tabs in 2-space file.

@@ +67,5 @@
> +  MonitorAutoLock lock(sMonitor);
> +
> +  GLuint texture = 0;
> +  while (sTextures.GetSize() < 5) {
> +    sActiveContext->fGenTextures(1, &texture);

This class just creates texture (state) objects? Is this really an observed bottleneck?

Comment 29

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

Attachment: Rebased, more fixes

Comment 30

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

(In reply to Jeff Gilbert [:jgilbert] from comment #28)
> Review of attachment 642306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is the superficial stuff I've seen so far. I'm still reviewing the
> actual EGLImage/SharedTexture logic/timelines/etc.

Thanks. I've fixed most of these, some comments below:

> @@ +810,2 @@
> >  {
> > +    // Both EGLImage and SurfaceTexture only support ThreadShared currently
> 
> Should we not then assert?

Well that only gets triggered in debug mode, no? We want it to always return null. Also, this is the same check the code already did elsewhere.

> 
> @@ +812,5 @@
> >      if (aType != TextureImage::ThreadShared)
> >          return nsnull;
> >  
> > +    if (aBuffer && aBufferType == SharedTextureBufferType::SurfaceTexture) {
> > +        // FIXME: it's possible to make nsSurfaceTexture work across processes. We should do that.
> 
> Why are we leaving a comment instead of filing a bug?
> 
> The basis for this is coming, but we should wait for New Shiny Things to get
> fitted in before trying to tackle it. (ETA: a few weeks)

I plan to file a bug, and will reference it there for the final patch.

> @@ -767,5 @@
> >      if (!tex->CreateEGLImage()) {
> >          NS_ERROR("EGLImage creation for EGLTextureWrapper failed");
> >          delete tex;
> > -        // Stop trying to create shared image Handle
> > -        mShareWithEGLImage = false;
> 
> Oof, I'm gonna have to re-review some code then. :)
> 

Yeah...I don't like this either. Unfortunately, Flash sometimes leaves the texture in a state that makes EGLImage creation fail. It's rare, but it does happen and I've been unable to figure out how to avoid it so far. Further, I think it's wrong for CreateSharedHandle to just give up. The calling code should decide itself whether it wants to stop trying to use this facility, IMHO.

> @@ +886,5 @@
> > +
> > +    if (wrapper->Type() == SharedHandleType::SurfaceTexture) {
> > +        /**
> > +         * NOTE: SurfaceTexture spams us if there are any existing GL errors, so we'll clear
> > +         * them here in order to avoid that.
> 
> This doesn't seem like a good reason for adding a GetAndClearError() here.
> This should only be used when we need to clear any existing errors because
> we need to check if a command we send generates an error.
> 
> What spam is this causing?

Yeah, I agree it is less than ideal. The message you see is this: https://github.com/TeamHorizon/platform_frameworks_native/blob/jellybean/libs/gui/SurfaceTexture.cpp#L267

It happens on every frame, so it's pretty irritating. On the HTC One X I was testing there was always an INVALID_OPERATION error there for some reason. Unrelated to my changes, AFAICT.

> 
> @@ +894,5 @@
> > +        SurfaceTextureWrapper* surfaceTextureWrapper = reinterpret_cast<SurfaceTextureWrapper*>(wrapper);
> > +
> > +        // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> > +        // be applied to the texture coordinates. We should return that here
> > +        // so we can render correctly.
> 
> So why are we not?

It involves some shader changes that I just haven't done yet. Things seem to work fine as-is, with limited testing. I'll file a bug and get a follow-up in shortly.

> ::: gfx/layers/basic/BasicImageLayer.cpp
> @@ +10,5 @@
> >  
> >  using namespace mozilla::gfx;
> >  
> >  namespace mozilla {
> > +namespace layers {
> 
> Null change?

Not sure what's going on there, I didn't make any change myself. I think maybe the line break was wrong (\r\n ?) and my editor corrected it.

> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +751,5 @@
> >    } else {
> >      MakeCurrent();
> >    }
> >  
> > +  TexturePoolOGL::Fill(gl());
> 
> Is there a pressing reason this pool stuff is static?

Well, the point is to be able to easily access a texture that has been allocated in the on-screen context from the content thread. This seemed like the best solution, but I'm open to others.

> @@ +67,5 @@
> > +  MonitorAutoLock lock(sMonitor);
> > +
> > +  GLuint texture = 0;
> > +  while (sTextures.GetSize() < 5) {
> > +    sActiveContext->fGenTextures(1, &texture);
> 
> This class just creates texture (state) objects? Is this really an observed
> bottleneck?

There isn't a bottleneck, but we need an on-screen texture for the SurfaceTexture constructor. We construct that object in the content thread, so here we are. It is a dumb API.

Comment 31

[Oleg Romashin (:romaxa)]

Would it make sense instead of Inverted use YFlip word, in order to keep inline with previous code

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 642613 [details] [diff] [review]
Rebased, more fixes

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

Despite the gargantuan size, this looks good to me, and is surprisingly (mostly) straightforward!

::: widget/android/AndroidBridge.cpp
@@ +1943,3 @@
>          return GetNativeSurface(aEnv, aSurface);
>      else
>          return nsnull;

Just a minor nit here -- no else after return, that's just gross.

if (thing)
  return foo;
if (otherthing)
  return bar;
return nsnull;

Comment 33

[Benoit Girard (:BenWa)]

Comment on attachment 642613 [details] [diff] [review]
Rebased, more fixes

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

A few fix up we discussed on Vidyo, otherwise a great patch.

::: gfx/gl/GLContext.h
@@ +856,5 @@
>  
> +    enum SharedTextureBufferType {
> +      None,
> +      TextureID,
> +      SurfaceTexture,

SurfaceTexture is specific to android. It doesn't belong in GLContext. Maybe ifdef this out.

@@ +866,5 @@
>      virtual SharedTextureHandle CreateSharedHandle(TextureImage::TextureShareType aType) { return nsnull; }
>      /**
> +     * Create a new shared GLContext content handle, using the passed buffer as a source.
> +     * Must be released by ReleaseSharedHandle. UpdateSharedHandle will have no effect
> +     * on handles created with this method (as it makes no sense)

as it makes no sense = since the caller is providing the source, it is responsible for updating the buffer.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +772,5 @@
>  
> +    SharedTextureHandleWrapper* wrapper = reinterpret_cast<SharedTextureHandleWrapper*>(aSharedHandle);
> +
> +    if (wrapper->Type() == SharedHandleType::SurfaceTexture) {
> +        // FIXME: GLContext isn't currently set up to render into nsSurfaceTexture

lets assert here

::: gfx/layers/ImageLayers.h
@@ +873,5 @@
> +  const Data* GetData() { return &mData; }
> +
> +  gfxIntSize GetSize() { return mData.mSize; }
> +
> +  virtual already_AddRefed<gfxASurface> GetAsSurface() { return NULL; }

We could implement this if we wanted. Maybe we should put an assertion.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +930,5 @@
> +                             LOCAL_GL_ONE, LOCAL_GL_ONE);
> +    gl()->ApplyFilterToBoundTexture(mFilter);
> +    program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), mSize));
> +    mOGLManager->BindAndDrawQuad(program, mInverted);
> +    gl()->fBindTexture(target, 0);

Need to call DetachSharedHandle here.

::: gfx/layers/opengl/TexturePoolOGL.cpp
@@ +13,5 @@
> +namespace gl {
> +
> +static GLContext* sActiveContext = NULL;
> +
> +static Monitor sMonitor("TexturePoolOGL.sMonitor");

This will fail leak test and will add static constructor. We should use gfx Platform or better yet use lazy init if we can.

::: gfx/thebes/nsSurfaceTexture.cpp
@@ +15,5 @@
> +
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "nsSurfaceTexture" , ## args)
> +
> +// UGH
> +static std::map<int, nsSurfaceTexture*> sInstances;

static constructor :(

Comment 34

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

(In reply to Benoit Girard (:BenWa) from comment #33)
> Comment on attachment 642613 [details] [diff] [review]
> Rebased, more fixes
> 
> Review of attachment 642613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few fix up we discussed on Vidyo, otherwise a great patch.
> 
> ::: gfx/gl/GLContext.h
> @@ +856,5 @@
> >  
> > +    enum SharedTextureBufferType {
> > +      None,
> > +      TextureID,
> > +      SurfaceTexture,
> 
> SurfaceTexture is specific to android. It doesn't belong in GLContext. Maybe
> ifdef this out.

I've added ifdefs here and around the impl in GLContextEGL

> 
> @@ +866,5 @@
> >      virtual SharedTextureHandle CreateSharedHandle(TextureImage::TextureShareType aType) { return nsnull; }
> >      /**
> > +     * Create a new shared GLContext content handle, using the passed buffer as a source.
> > +     * Must be released by ReleaseSharedHandle. UpdateSharedHandle will have no effect
> > +     * on handles created with this method (as it makes no sense)
> 
> as it makes no sense = since the caller is providing the source, it is
> responsible for updating the buffer.

Yup, done.

> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +772,5 @@
> >  
> > +    SharedTextureHandleWrapper* wrapper = reinterpret_cast<SharedTextureHandleWrapper*>(aSharedHandle);
> > +
> > +    if (wrapper->Type() == SharedHandleType::SurfaceTexture) {
> > +        // FIXME: GLContext isn't currently set up to render into nsSurfaceTexture
> 
> lets assert here

Done. (There was already an assertion below it, so I just removed the conditional)

> 
> ::: gfx/layers/ImageLayers.h
> @@ +873,5 @@
> > +  const Data* GetData() { return &mData; }
> > +
> > +  gfxIntSize GetSize() { return mData.mSize; }
> > +
> > +  virtual already_AddRefed<gfxASurface> GetAsSurface() { return NULL; }
> 
> We could implement this if we wanted. Maybe we should put an assertion.

We actually can't really implement this because the shared handle has no API for it (as it currently stands). We could implement it for nsSurfaceTexture, though.

> 
> ::: gfx/layers/opengl/ImageLayerOGL.cpp
> @@ +930,5 @@
> > +                             LOCAL_GL_ONE, LOCAL_GL_ONE);
> > +    gl()->ApplyFilterToBoundTexture(mFilter);
> > +    program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), mSize));
> > +    mOGLManager->BindAndDrawQuad(program, mInverted);
> > +    gl()->fBindTexture(target, 0);
> 
> Need to call DetachSharedHandle here.

Yup, done.

> 
> ::: gfx/layers/opengl/TexturePoolOGL.cpp
> @@ +13,5 @@
> > +namespace gl {
> > +
> > +static GLContext* sActiveContext = NULL;
> > +
> > +static Monitor sMonitor("TexturePoolOGL.sMonitor");
> 
> This will fail leak test and will add static constructor. We should use gfx
> Platform or better yet use lazy init if we can.

Yeah, ugh. What a pain. I added Init and Shutdown methods and called them from gfxPlatform.

> 
> ::: gfx/thebes/nsSurfaceTexture.cpp
> @@ +15,5 @@
> > +
> > +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "nsSurfaceTexture" , ## args)
> > +
> > +// UGH
> > +static std::map<int, nsSurfaceTexture*> sInstances;
> 
> static constructor :(

See above.

Comment 35

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

Attachment: Yet more fixes

Comment 36

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

The latest patch addresses Benoit's review comments and also applied the texture transform from SurfaceTexture in the fragment shader. I thought I could get away without this in the initial patch, but it turns out it's very necessary with Flash on Galaxy Nexus. Without the transform applied, you see a bunch of garbage around the video.

Comment 37

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

Oh, another large-ish change was the addition of AndroidBridge::AcquireNativeWindowFromSurfaceTexture and AndroidBridge::ReleaseNativeWindowForSurfaceTexture. These ended up being necessary because Google removed a critical function (ANativeWindow_fromSurfaceTexture) in Jelly Bean and I had to work around that by calling something else. Releasing the window with that method is different, hence the separate pair of acquire/release methods.

Comment 38

[Benoit Girard (:BenWa)]

Comment on attachment 643562 [details] [diff] [review]
Yet more fixes

Hoping the bugzilla interdiff is accurate.

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 643562 [details] [diff] [review]
Yet more fixes

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

::: gfx/thebes/gfxPlatform.cpp
@@ +350,5 @@
>      nsCOMPtr<nsISupports> forceReg
>          = do_CreateInstance("@mozilla.org/gfx/init;1");
> +
> +    // Texture pool init
> +    mozilla::gl::TexturePoolOGL::Init();

I think we only want to init this on Android only.

Comment 40

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

Attachment: One more time

Comment 41

[Bill Gianopoulos [:WG9s]]

Last time I tried to take your latest patch and apply it to latest trunk it failed miserably.  If this patch actually will apply that is great!  otherwise could you provide some kind of tryserver build with the patch so others can test the flash functionality?

Comment 42

[Steve Fink [:sfink] [:s:]]

Backed out with http://hg.mozilla.org/integration/mozilla-inbound/rev/e0a1d64fc929 since it looks like it broke Windows builds. Isn't this still missing a review, too? It currently has r?jgilbert.

Comment 43

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 643579 [details] [diff] [review]
One more time

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

So. We effectively can't share texture IDs across threads on android. We should be using EGLImage for this.
GL_OES_EGL_image_external does not govern using texture IDs between contexts. Rather, it allows for sampling directly from EGLImages in GLSL, and potentially allowing for on-the-fly conversions from YUV to RGB.

We should be able to relatively directly reuse the EGLImage stuff we just put in recently. The main difference appears to be that we have a matrix we have to transform the texture by.

For Flash, all we really need is to create a context with a texture for Flash to draw into. If we can get away with attaching this texture to an EGLImage directly, we should do that, since theoretically, Flash is presumedly compositing and publishing onto this texture. If we can't do this, we should keep another EGLImage-backed texture and copy Flash's texture contents into the EGLImage when needed.

Also, I think we should pass this through Gerv for the Apache stuff in StrongPointer.h. (and atomic.h, if we keep it)

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +136,5 @@
> +
> +    if (mTextureInfo.mWidth == 0 || mTextureInfo.mHeight == 0)
> +      return nsnull;
> +
> +    mCurrentHandle = sPluginContext->CreateSharedHandle(TextureImage::ThreadShared, (void*)mTextureInfo.mTexture, GLContext::TextureID);

Wait. We don't allow for sharing Texture IDs across threads on android. (Various bugs) We MUST use EGLImages for this.

::: gfx/gl/GLContext.h
@@ +857,5 @@
>          return IsExtensionSupported(EXT_framebuffer_blit) || IsExtensionSupported(ANGLE_framebuffer_blit);
>      }
>  
> +    enum SharedTextureBufferType {
> +      TextureID,

4-space not 2-space.

@@ +901,5 @@
>                                       SharedTextureHandle aSharedHandle) { }
> +
> +
> +    typedef struct {
> +      GLuint mTarget;

4-space. This should also be GLenum.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +710,5 @@
> +
> +    SharedHandleType mHandleType;
> +};
> +
> +class SurfaceTextureWrapper: public SharedTextureHandleWrapper

Do these names have to be this similar? So far we have:
SurfaceTextureWrapper
SurfaceTextureHandle
SurfaceTextureHandleWrapper

@@ +862,5 @@
> +    if (aType != TextureImage::ThreadShared)
> +        return nsnull;
> +
> +    switch (aBufferType) {
> +#ifdef MOZ_WIDGET_ANDROID

Why are we just 'ifdef'ing everything out? This makes things messier and harder to keep consistent.

@@ +864,5 @@
> +
> +    switch (aBufferType) {
> +#ifdef MOZ_WIDGET_ANDROID
> +    case SharedTextureBufferType::SurfaceTexture:
> +        // FIXME: it's possible to make nsSurfaceTexture work across processes. We should do that.

This should return null unless we support:

@@ +972,5 @@
>  
> +    SharedTextureHandleWrapper* wrapper = reinterpret_cast<SharedTextureHandleWrapper*>(aSharedHandle);
> +
> +    switch (wrapper->Type()) {
> +#ifdef MOZ_WIDGET_ANDROID

Don't ifdef things out if you don't have to, here.

@@ +977,5 @@
> +    case SharedHandleType::SurfaceTexture: {
> +        /**
> +         * NOTE: SurfaceTexture spams us if there are any existing GL errors, so we'll clear
> +         * them here in order to avoid that.
> +         */

Put this comment inside the relevant DEBUG block.

@@ +986,5 @@
> +
> +        // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> +        // be applied to the texture coordinates. We should return that here
> +        // so we can render correctly. Bug 775083
> +        surfaceTextureWrapper->SurfaceTexture()->UpdateTexImage();

This doesn't appear to actually attach the SharedTexture to anything.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +34,5 @@
> +    return;
> +
> +  gl->fGenTextures(1, &aTexture);
> +
> +  gl->fActiveTexture(LOCAL_GL_TEXTURE0);

fActiveTexture is unnecessary here.

@@ +694,5 @@
>  }
>  
>  ShadowImageLayerOGL::ShadowImageLayerOGL(LayerManagerOGL* aManager)
>    : ShadowImageLayer(aManager, nsnull)
> +  , LayerOGL(aManager), mSharedHandle(0), mInverted(false), mTexture(0)

This is already doing separate lines, so don't start stacking.

@@ +941,5 @@
> +    program->SetTextureTransform(handleDetails.mTextureTransform);
> +    program->LoadMask(GetMaskLayer());
> +
> +    MakeTextureIfNeeded(gl(), mTexture);
> +    gl()->fActiveTexture(handleDetails.mTarget);

This should be LOCAL_GL_TEXTURE0 not handleDetails.mTarget.

::: gfx/layers/opengl/TexturePoolOGL.cpp
@@ +6,5 @@
> +#include "GLContext.h"
> +#include "nsDeque.h"
> +#include "mozilla/Monitor.h"
> +
> +#define TEXTURE_POOL_SIZE 5

This seems low for someone opening a bunch of flash tabs. (flash ads, also)

@@ +34,5 @@
> +  GLuint texture = 0;
> +  if (sActiveContext->IsOwningThreadCurrent()) {
> +    sActiveContext->MakeCurrent();
> +
> +    sActiveContext->fGenTextures(1, &texture);

We should refill the pool when we get a chance. As it stands, the pool never refills until the LayerManager is recreated.
Otherwise, the pool only shrinks.

@@ +37,5 @@
> +
> +    sActiveContext->fGenTextures(1, &texture);
> +  } else {
> +    while (sTextures->GetSize() == 0)
> +      sMonitor->Wait();

NS_WARNING the first time we have to wait, so we know what's going on in DEBUG builds.

@@ +53,5 @@
> +  if (!sActiveContext)
> +    return;
> +
> +  GLuint texture;
> +  while ((texture = (GLuint)sTextures->Pop()) != 0)

Please do this in multiple steps. |while (sTextures->GetSize())| is preferrable to relying on nsDeque to return nsnull when empty, which is not documented.

@@ +54,5 @@
> +    return;
> +
> +  GLuint texture;
> +  while ((texture = (GLuint)sTextures->Pop()) != 0)
> +    sActiveContext->fDeleteTextures(1, &texture);

Needs a MakeCurrent().

@@ +71,5 @@
> +  }
> +
> +  GLuint texture = 0;
> +  while (sTextures->GetSize() < TEXTURE_POOL_SIZE) {
> +    sActiveContext->fGenTextures(1, &texture);

Needs MakeCurrent().

@@ +72,5 @@
> +
> +  GLuint texture = 0;
> +  while (sTextures->GetSize() < TEXTURE_POOL_SIZE) {
> +    sActiveContext->fGenTextures(1, &texture);
> +    sTextures->Push((void*) texture);

It would be a cleaner use of nsDeque to pass in GLuint*, instead of hiding a GLuint in a void*. This code is not performance sensitive, so there's little reason not to use it properly.

::: layout/generic/nsObjectFrame.cpp
@@ +1152,1 @@
>    // ???

???, indeed.

If you don't know why XP_MACOSX always returns false, just add a new elseif block for MOZ_WIDGET_ANDROID and say why android, at least, returns false.

::: widget/android/android/StrongPointer.h
@@ +16,5 @@
> +
> +
> +/*************************************************************************
> + *
> + * WARNING: EVERYTHING HERE IS NEUTERED

Note explicitly that Mozilla has change this file.

@@ +29,5 @@
> +
> +#ifndef ANDROID_STRONG_POINTER_H
> +#define ANDROID_STRONG_POINTER_H
> +
> +#include "atomic.h"

Do we really need this file? It doesn't look like we're using anything from it.

Comment 44

[Kelsey Gilbert [:jgilbert]]

Gerv, is the Apache stuff in StrongPointer.h alright? (and atomic.h, if we keep it)

Comment 45

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

(In reply to Jeff Gilbert [:jgilbert] from comment #43)
> Comment on attachment 643579 [details] [diff] [review]
> One more time
> 
> Review of attachment 643579 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review! Some comments inline:

> 
> So. We effectively can't share texture IDs across threads on android. We
> should be using EGLImage for this.
> GL_OES_EGL_image_external does not govern using texture IDs between
> contexts. Rather, it allows for sampling directly from EGLImages in GLSL,
> and potentially allowing for on-the-fly conversions from YUV to RGB.

The usage of GL_OES_EGL_image_external is for the most part incidental to how SurfaceTexture behaves. I'll cover that more below.

> 
> We should be able to relatively directly reuse the EGLImage stuff we just
> put in recently. The main difference appears to be that we have a matrix we
> have to transform the texture by.

I think you may be confusing things here. This patch does in fact reuse the EGLImage stuff that was landed recently. There are two major enhancements to it:

1) It adds an API (the new CreateSharedHandle overload) that allows the user to pass in the source for the shared handle. The incarnation that already landed only allows the output of the GLContext to be used as the source (e.g. the WebGL scenario).

2) It adds another sharing mechanism to the SharedTextureHandle API, based on Android SurfaceTexture (http://developer.android.com/reference/android/graphics/SurfaceTexture.html). I added a class that wraps this and takes care of the JNI wrangling, nsSurfaceTexture.

> 
> For Flash, all we really need is to create a context with a texture for
> Flash to draw into. If we can get away with attaching this texture to an
> EGLImage directly, we should do that, since theoretically, Flash is
> presumedly compositing and publishing onto this texture. If we can't do
> this, we should keep another EGLImage-backed texture and copy Flash's
> texture contents into the EGLImage when needed.

Right. This is in fact what this patch does, in part. We do create a shared handle (and corresponding EGLImage) on every update of the plugin, however, due to some lifecycle concerns.

> 
> Also, I think we should pass this through Gerv for the Apache stuff in
> StrongPointer.h. (and atomic.h, if we keep it)

According to this (https://wiki.mozilla.org/MPL_Upgrade) we should be ok, but obviously Gerv would know for sure.

> 
> ::: dom/plugins/base/nsNPAPIPluginInstance.cpp
> @@ +136,5 @@
> > +
> > +    if (mTextureInfo.mWidth == 0 || mTextureInfo.mHeight == 0)
> > +      return nsnull;
> > +
> > +    mCurrentHandle = sPluginContext->CreateSharedHandle(TextureImage::ThreadShared, (void*)mTextureInfo.mTexture, GLContext::TextureID);
> 
> Wait. We don't allow for sharing Texture IDs across threads on android.
> (Various bugs) We MUST use EGLImages for this.

The third argument, GLContext::TextureID, describes what sort of buffer we are passing as the source in the second argument. In this case it's a texture ID. When creating the SharedTextureHandle, we create the EGLImage using this source texture ID. This is exactly what was happening in the original CreateSharedHandle, only the texture was created within instead of being passed from the outside.

> 
> ::: gfx/gl/GLContext.h
> @@ +857,5 @@
> >          return IsExtensionSupported(EXT_framebuffer_blit) || IsExtensionSupported(ANGLE_framebuffer_blit);
> >      }
> >  
> > +    enum SharedTextureBufferType {
> > +      TextureID,
> 
> 4-space not 2-space.

Thanks, fixed.

> 
> @@ +901,5 @@
> >                                       SharedTextureHandle aSharedHandle) { }
> > +
> > +
> > +    typedef struct {
> > +      GLuint mTarget;
> 
> 4-space. This should also be GLenum.

Fixed. Thanks.

> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +710,5 @@
> > +
> > +    SharedHandleType mHandleType;
> > +};
> > +
> > +class SurfaceTextureWrapper: public SharedTextureHandleWrapper
> 
> Do these names have to be this similar? So far we have:
> SurfaceTextureWrapper
> SurfaceTextureHandle
> SurfaceTextureHandleWrapper

Actually, we have:

SharedTextureHandleWrapper, which is the base class for SurfaceTextureWrapper and EGLTextureWrapper.

We then have SharedTextureHandle, which is the opaque type exposed to the outside. Naming is hard. I'd appreciate suggestions :)

> 
> @@ +862,5 @@
> > +    if (aType != TextureImage::ThreadShared)
> > +        return nsnull;
> > +
> > +    switch (aBufferType) {
> > +#ifdef MOZ_WIDGET_ANDROID
> 
> Why are we just 'ifdef'ing everything out? This makes things messier and
> harder to keep consistent.

This was BenWa's suggestion, since SurfaceTexture (and associated SharedTextureBufferType) is only usable from Android and the enum does not describe a generic cross-platform thing. I don't feel strongly about it one way or another.

> 
> @@ +864,5 @@
> > +
> > +    switch (aBufferType) {
> > +#ifdef MOZ_WIDGET_ANDROID
> > +    case SharedTextureBufferType::SurfaceTexture:
> > +        // FIXME: it's possible to make nsSurfaceTexture work across processes. We should do that.
> 
> This should return null unless we support:

We do, above, when we check for ThreadShared. I guess the comment is kind of in the wrong place, fixed.

> 
> @@ +972,5 @@
> >  
> > +    SharedTextureHandleWrapper* wrapper = reinterpret_cast<SharedTextureHandleWrapper*>(aSharedHandle);
> > +
> > +    switch (wrapper->Type()) {
> > +#ifdef MOZ_WIDGET_ANDROID
> 
> Don't ifdef things out if you don't have to, here.

See above comment re: ifdef

> 
> @@ +977,5 @@
> > +    case SharedHandleType::SurfaceTexture: {
> > +        /**
> > +         * NOTE: SurfaceTexture spams us if there are any existing GL errors, so we'll clear
> > +         * them here in order to avoid that.
> > +         */
> 
> Put this comment inside the relevant DEBUG block.

Done

> 
> @@ +986,5 @@
> > +
> > +        // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> > +        // be applied to the texture coordinates. We should return that here
> > +        // so we can render correctly. Bug 775083
> > +        surfaceTextureWrapper->SurfaceTexture()->UpdateTexImage();
> 
> This doesn't appear to actually attach the SharedTexture to anything.

It actually does attach it to the GL_TEXTURE_EXTERNAL_OES target. See the documentation here http://developer.android.com/reference/android/graphics/SurfaceTexture.html and relevant implementation here https://github.com/android/platform_frameworks_base/blob/ics-mr0/libs/gui/SurfaceTexture.cpp#L665

> 
> ::: gfx/layers/opengl/ImageLayerOGL.cpp
> @@ +34,5 @@
> > +    return;
> > +
> > +  gl->fGenTextures(1, &aTexture);
> > +
> > +  gl->fActiveTexture(LOCAL_GL_TEXTURE0);
> 
> fActiveTexture is unnecessary here.

Fixed. Copy/pasted from somewhere else blindly. The fBindTexture is also unnecessary, removed both.

> 
> @@ +694,5 @@
> >  }
> >  
> >  ShadowImageLayerOGL::ShadowImageLayerOGL(LayerManagerOGL* aManager)
> >    : ShadowImageLayer(aManager, nsnull)
> > +  , LayerOGL(aManager), mSharedHandle(0), mInverted(false), mTexture(0)
> 
> This is already doing separate lines, so don't start stacking.

Fixed

> 
> @@ +941,5 @@
> > +    program->SetTextureTransform(handleDetails.mTextureTransform);
> > +    program->LoadMask(GetMaskLayer());
> > +
> > +    MakeTextureIfNeeded(gl(), mTexture);
> > +    gl()->fActiveTexture(handleDetails.mTarget);
> 
> This should be LOCAL_GL_TEXTURE0 not handleDetails.mTarget.

Oops! Fixed.

> 
> ::: gfx/layers/opengl/TexturePoolOGL.cpp
> @@ +6,5 @@
> > +#include "GLContext.h"
> > +#include "nsDeque.h"
> > +#include "mozilla/Monitor.h"
> > +
> > +#define TEXTURE_POOL_SIZE 5
> 
> This seems low for someone opening a bunch of flash tabs. (flash ads, also)

Fair enough, but it does get filled on every frame. I boosted it to 10.

> 
> @@ +34,5 @@
> > +  GLuint texture = 0;
> > +  if (sActiveContext->IsOwningThreadCurrent()) {
> > +    sActiveContext->MakeCurrent();
> > +
> > +    sActiveContext->fGenTextures(1, &texture);
> 
> We should refill the pool when we get a chance. As it stands, the pool never
> refills until the LayerManager is recreated.
> Otherwise, the pool only shrinks.

The pool is filled in LayerManagerOGL::Render, which I would assume is called on every frame.

> 
> @@ +37,5 @@
> > +
> > +    sActiveContext->fGenTextures(1, &texture);
> > +  } else {
> > +    while (sTextures->GetSize() == 0)
> > +      sMonitor->Wait();
> 
> NS_WARNING the first time we have to wait, so we know what's going on in
> DEBUG builds.

Good idea, done.

> 
> @@ +53,5 @@
> > +  if (!sActiveContext)
> > +    return;
> > +
> > +  GLuint texture;
> > +  while ((texture = (GLuint)sTextures->Pop()) != 0)
> 
> Please do this in multiple steps. |while (sTextures->GetSize())| is
> preferrable to relying on nsDeque to return nsnull when empty, which is not
> documented.

Fair enough

> 
> @@ +54,5 @@
> > +    return;
> > +
> > +  GLuint texture;
> > +  while ((texture = (GLuint)sTextures->Pop()) != 0)
> > +    sActiveContext->fDeleteTextures(1, &texture);
> 
> Needs a MakeCurrent().

Fixed. Added one prior to the loop.

> 
> @@ +71,5 @@
> > +  }
> > +
> > +  GLuint texture = 0;
> > +  while (sTextures->GetSize() < TEXTURE_POOL_SIZE) {
> > +    sActiveContext->fGenTextures(1, &texture);
> 
> Needs MakeCurrent().

Done

> 
> @@ +72,5 @@
> > +
> > +  GLuint texture = 0;
> > +  while (sTextures->GetSize() < TEXTURE_POOL_SIZE) {
> > +    sActiveContext->fGenTextures(1, &texture);
> > +    sTextures->Push((void*) texture);
> 
> It would be a cleaner use of nsDeque to pass in GLuint*, instead of hiding a
> GLuint in a void*. This code is not performance sensitive, so there's little
> reason not to use it properly.

Yeah, that hack didn't build on Linux anyway. Changed to GLuint*.

> 
> ::: layout/generic/nsObjectFrame.cpp
> @@ +1152,1 @@
> >    // ???
> 
> ???, indeed.
> 
> If you don't know why XP_MACOSX always returns false, just add a new elseif
> block for MOZ_WIDGET_ANDROID and say why android, at least, returns false.

Done

> 
> ::: widget/android/android/StrongPointer.h
> @@ +16,5 @@
> > +
> > +
> > +/*************************************************************************
> > + *
> > + * WARNING: EVERYTHING HERE IS NEUTERED
> 
> Note explicitly that Mozilla has change this file.
> 
> @@ +29,5 @@
> > +
> > +#ifndef ANDROID_STRONG_POINTER_H
> > +#define ANDROID_STRONG_POINTER_H
> > +
> > +#include "atomic.h"
> 
> Do we really need this file? It doesn't look like we're using anything from
> it.

Indeed, I think I commented all usage out. I'll confirm and remove if so.

Comment 46

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

Attachment: More review fixes

Comment 47

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

Attachment: More review fixes

Comment 48

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #45)
> (In reply to Jeff Gilbert [:jgilbert] from comment #43)
> > Comment on attachment 643579 [details] [diff] [review]
> > One more time
> > 
> > Review of attachment 643579 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thanks for the review! Some comments inline:
> 
> > 
> > So. We effectively can't share texture IDs across threads on android. We
> > should be using EGLImage for this.
> > GL_OES_EGL_image_external does not govern using texture IDs between
> > contexts. Rather, it allows for sampling directly from EGLImages in GLSL,
> > and potentially allowing for on-the-fly conversions from YUV to RGB.
> 
> The usage of GL_OES_EGL_image_external is for the most part incidental to
> how SurfaceTexture behaves. I'll cover that more below.
> 
> > 
> > We should be able to relatively directly reuse the EGLImage stuff we just
> > put in recently. The main difference appears to be that we have a matrix we
> > have to transform the texture by.
> 
> I think you may be confusing things here. This patch does in fact reuse the
> EGLImage stuff that was landed recently. There are two major enhancements to
> it:
> 
> 1) It adds an API (the new CreateSharedHandle overload) that allows the user
> to pass in the source for the shared handle. The incarnation that already
> landed only allows the output of the GLContext to be used as the source
> (e.g. the WebGL scenario).
> 
> 2) It adds another sharing mechanism to the SharedTextureHandle API, based
> on Android SurfaceTexture
> (http://developer.android.com/reference/android/graphics/SurfaceTexture.
> html). I added a class that wraps this and takes care of the JNI wrangling,
> nsSurfaceTexture.
> 
> > 
> > For Flash, all we really need is to create a context with a texture for
> > Flash to draw into. If we can get away with attaching this texture to an
> > EGLImage directly, we should do that, since theoretically, Flash is
> > presumedly compositing and publishing onto this texture. If we can't do
> > this, we should keep another EGLImage-backed texture and copy Flash's
> > texture contents into the EGLImage when needed.
> 
> Right. This is in fact what this patch does, in part. We do create a shared
> handle (and corresponding EGLImage) on every update of the plugin, however,
> due to some lifecycle concerns.

Ok, interesting. Follow-up bug for this?

> 
> > 
> > ::: dom/plugins/base/nsNPAPIPluginInstance.cpp
> > @@ +136,5 @@
> > > +
> > > +    if (mTextureInfo.mWidth == 0 || mTextureInfo.mHeight == 0)
> > > +      return nsnull;
> > > +
> > > +    mCurrentHandle = sPluginContext->CreateSharedHandle(TextureImage::ThreadShared, (void*)mTextureInfo.mTexture, GLContext::TextureID);
> > 
> > Wait. We don't allow for sharing Texture IDs across threads on android.
> > (Various bugs) We MUST use EGLImages for this.
> 
> The third argument, GLContext::TextureID, describes what sort of buffer we
> are passing as the source in the second argument. In this case it's a
> texture ID. When creating the SharedTextureHandle, we create the EGLImage
> using this source texture ID. This is exactly what was happening in the
> original CreateSharedHandle, only the texture was created within instead of
> being passed from the outside.
> 

Ok, I think I understand the structure now.

> 
> > 
> > ::: gfx/gl/GLContextProviderEGL.cpp
> > @@ +710,5 @@
> > > +
> > > +    SharedHandleType mHandleType;
> > > +};
> > > +
> > > +class SurfaceTextureWrapper: public SharedTextureHandleWrapper
> > 
> > Do these names have to be this similar? So far we have:
> > SurfaceTextureWrapper
> > SurfaceTextureHandle
> > SurfaceTextureHandleWrapper
> 
> Actually, we have:
> 
> SharedTextureHandleWrapper, which is the base class for
> SurfaceTextureWrapper and EGLTextureWrapper.
> 
> We then have SharedTextureHandle, which is the opaque type exposed to the
> outside. Naming is hard. I'd appreciate suggestions :)

Wow, I really did not notice that these were not all 'SurfaceTexture[...]'. This should be fine. I clearly just need more (or less?) caffeine.

> > 
> > @@ +986,5 @@
> > > +
> > > +        // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> > > +        // be applied to the texture coordinates. We should return that here
> > > +        // so we can render correctly. Bug 775083
> > > +        surfaceTextureWrapper->SurfaceTexture()->UpdateTexImage();
> > 
> > This doesn't appear to actually attach the SharedTexture to anything.
> 
> It actually does attach it to the GL_TEXTURE_EXTERNAL_OES target. See the
> documentation here
> http://developer.android.com/reference/android/graphics/SurfaceTexture.html
> and relevant implementation here
> https://github.com/android/platform_frameworks_base/blob/ics-mr0/libs/gui/
> SurfaceTexture.cpp#L665

Oh, so it does. :)
Please document that this is what happens here. (TEXTURE_EXTERNAL binding is set to the updated data)

Comment 49

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 644141 [details] [diff] [review]
More review fixes

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

r-, but fix this and it's r+. :)

::: gfx/gl/GLContextProviderEGL.cpp
@@ +917,5 @@
> +
> +    switch (aBufferType) {
> +#ifdef MOZ_WIDGET_ANDROID
> +    case SharedTextureBufferType::SurfaceTexture:
> +        return (SharedTextureHandle) new SurfaceTextureWrapper(reinterpret_cast<nsSurfaceTexture*>(aBuffer));

Please return null if !IsExtensionSupported(GL_OES_EGL_image_external), which should be in now if bug 774059 sticks. Since we rely on this extension, we should bail here if we won't be able to use it when drawing in Layers.

Comment 50

[Gervase Markham [:gerv]]

Yes, it's fine to import entire Apache-licensed files from elsewhere into the Mozilla codebase (and alter them if necessary). (If we wrote it, or if you want to copy Apache stuff into an MPLed file, it's more complex. But this is simple.)

Gerv

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d4c945625ca

Comment 52

[Kelsey Gilbert [:jgilbert]]

That was just part of the result from the previous attempt at landing.

Comment 53

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

Attachment: Initial support for Flash on Honeycomb

Comment 54

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

Latest patch is looking good so far on Try, but not everything is built yet.

https://tbpl.mozilla.org/?tree=Try&rev=fb5b18d8ed90

Comment 55

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb

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

Alright, go for it. :)

Comment 56

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

Relanded with fixes

https://hg.mozilla.org/integration/mozilla-inbound/rev/8571114112b2

Comment 57

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

Comment on attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb

[Approval Request Comment]

This is a pretty risky patch, but is considered critical for the tablet experience in Fx15. Without it we don't get Flash on Honeycomb.

Comment 58

[Alex Keybl [:akeybl]]

Comment on attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb

[Triage Comment]
We took tablet support in FN15, so let's also get Honeycomb Flash support in. We plan to simul-ship both features, so let's get as much testing as possible.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8571114112b2

Comment 60

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out for likely causing bug 776142.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfd941c49a6

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cfd941c49a6

Comment 62

[Justin Wood (:Callek)]

Comment on attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb

So that we don't get ourselves confused, I'm reverting the a+ to a? per c#60

Comment 63

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

Attachment: Initial support for Flash on Honeycomb

This patch doesn't break robocop/mochitest

Comment 64

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

The issue here was that the test driver was enumerating all methods in GeckoAppShell, and it couldn't resolve the SurfaceTexture type. Making that a generic Object fixed things (sigh).

Comment 65

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4987ffd173a4

Comment 66

[Ed Morley [:emorley]]

Backed out for build failures on OS X (and possibly others):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4987ffd173a4
https://tbpl.mozilla.org/php/getParsedLog.php?id=13771347&tree=Mozilla-Inbound

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

Comment 67

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

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

Comment 68

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f07aca82e80e

Comment 69

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

Comment on attachment 644809 [details] [diff] [review]
Initial support for Flash on Honeycomb

[Approval Request Comment]
Enables Flash support on Honeycomb, which is considered a critical feature for tablet. I would rate the risk as an 11 out of 10.

Comment 70

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/410a1df0d0b1

Comment 71

[Scoobidiver (away)]

Why didn't you fix the 4 regressions before uplifting it?

Comment 72

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

(In reply to Scoobidiver from comment #71)
> Why didn't you fix the 4 regressions before uplifting it?

It's not going to be perfect the first time around. My understanding is that right now it's better to have something working relatively well than nothing at all. If the powers that be disagree, that's fine, and we can back it out.

Comment 73

[Alex Keybl [:akeybl]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #72)
> It's not going to be perfect the first time around. My understanding is that
> right now it's better to have something working relatively well than nothing
> at all. If the powers that be disagree, that's fine, and we can back it out.

This is correct - we need early feedback to get this shipped alongside tablet support in FN15, which warrants cutting a couple of corners on aurora/beta.

Comment 74

[Alex Keybl [:akeybl]]

In addition, early feedback will allow us to prioritize regressions.

Comment 75

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2467864aeb9d

Comment 76

[Scoobidiver (away)]

Bug 776331 is a real regression, i.e. not the result of a new imperfect feature.

Comment 77

[Catalin Suciu [:csuciu]]

This issue is fixed in all current builds.
Verifying.

Testcases available here: https://moztrap.mozilla.org/manage/cases/?filter-suite=65