Last Comment Bug 687267 - (honeycomb-flash) support for flash on Android Fennec on honeycomb
(honeycomb-flash)
: support for flash on Android Fennec on honeycomb
Status: VERIFIED FIXED
: compat, flashplayer, meta, mobile, pp
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All Android
: P1 enhancement with 3 votes (vote)
: mozilla17
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
: Catalin Suciu [:csuciu]
Mentors:
: 759134 (view as bug list)
Depends on: 778445 778983 787685 630007 728524 776142 776329 776331 776334 778442 783754 863490
Blocks: 695826 774059
  Show dependency treegraph
 
Reported: 2011-09-17 11:41 PDT by Doug Turner (:dougt)
Modified: 2013-04-18 16:32 PDT (History)
81 users (show)
catalin.suciu: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
fixed
-
15+


Attachments
stub (32.54 KB, patch)
2011-09-17 11:43 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
IN PROGRESS PATCH (40.69 KB, patch)
2012-06-27 09:31 PDT, Arkady Blyakher [:rkd]
no flags Details | Diff | Splinter Review
IN PROGRESS PATCH 2 (41.91 KB, patch)
2012-06-28 14:18 PDT, Arkady Blyakher [:rkd]
no flags Details | Diff | Splinter Review
Initial support for Flash on Honeycomb (50.36 KB, patch)
2012-07-03 10:26 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
Fixed up patch (48.02 KB, patch)
2012-07-05 08:22 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
More fixes (55.72 KB, patch)
2012-07-10 10:19 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review+
Details | Diff | Splinter Review
Now with video support (181.88 KB, patch)
2012-07-14 18:54 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Oops, this time without debug messages (181.64 KB, patch)
2012-07-14 19:02 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Rebased, more fixes (143.87 KB, patch)
2012-07-16 09:24 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review-
vladimir: review+
Details | Diff | Splinter Review
Yet more fixes (167.43 KB, patch)
2012-07-18 13:38 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review+
Details | Diff | Splinter Review
One more time (167.69 KB, patch)
2012-07-18 14:10 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
bgirard: review+
jgilbert: review-
Details | Diff | Splinter Review
More review fixes (145.77 KB, patch)
2012-07-19 19:08 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
More review fixes (164.50 KB, patch)
2012-07-19 19:29 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review-
Details | Diff | Splinter Review
Initial support for Flash on Honeycomb (166.94 KB, patch)
2012-07-20 11:34 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review+
Details | Diff | Splinter Review
Initial support for Flash on Honeycomb (168.23 KB, patch)
2012-07-22 17:51 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2011-09-17 11:41:51 PDT
+++ 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) 2011-09-17 11:43:20 PDT
Created attachment 560720 [details] [diff] [review]
stub
Comment 2 Tom Levels 2011-12-26 13:45:05 PST
Is this bug still valid since Flash won't be supported on mobile anymore?
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-12-26 13:49:16 PST
(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 2012-01-08 08:42:54 PST
Is this related to Ice Cream Sandwich (Android 4.0) too? I guess so.
Comment 5 Mardeg 2012-02-01 21:51:50 PST
(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 6 Aaron Train [:aaronmt] 2012-05-28 09:54:21 PDT
*** Bug 759134 has been marked as a duplicate of this bug. ***
Comment 7 Matt Brubeck (:mbrubeck) 2012-06-26 22:24:39 PDT
Are we targeting this for Firefox 15 or 16?
Comment 8 Arkady Blyakher [:rkd] 2012-06-27 09:31:17 PDT
Created attachment 637161 [details] [diff] [review]
IN PROGRESS PATCH
Comment 9 Arkady Blyakher [:rkd] 2012-06-28 14:18:41 PDT
Created attachment 637668 [details] [diff] [review]
IN PROGRESS PATCH 2
Comment 10 Mozilla RelEng Bot 2012-07-02 15:45:23 PDT
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/jwillcox@mozilla.com-250fefba2225
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-03 10:26:34 PDT
Created attachment 638788 [details] [diff] [review]
Initial support for Flash on Honeycomb
Comment 12 Chris Lord [:cwiiis] 2012-07-04 07:43:04 PDT
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) (jwillcox@mozilla.com) 2012-07-05 08:05:32 PDT
(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) (jwillcox@mozilla.com) 2012-07-05 08:22:30 PDT
Created attachment 639354 [details] [diff] [review]
Fixed up patch
Comment 15 Mark Callow 2012-07-05 18:31:21 PDT
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) 2012-07-05 19:04:56 PDT
(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 Jeff Gilbert [:jgilbert] 2012-07-05 20:35:50 PDT
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] 2012-07-06 08:36:18 PDT
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) 2012-07-06 15:17:19 PDT
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) (jwillcox@mozilla.com) 2012-07-10 10:01:37 PDT
(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) 2012-07-10 10:05:00 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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) (jwillcox@mozilla.com) 2012-07-10 10:19:02 PDT
Created attachment 640659 [details] [diff] [review]
More fixes
Comment 23 Benoit Girard (:BenWa) 2012-07-10 10:52:23 PDT
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 2012-07-11 03:33:50 PDT
(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) (jwillcox@mozilla.com) 2012-07-14 18:54:46 PDT
Created attachment 642305 [details] [diff] [review]
Now with video support
Comment 26 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-14 18:59:31 PDT
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) (jwillcox@mozilla.com) 2012-07-14 19:02:54 PDT
Created attachment 642306 [details] [diff] [review]
Oops, this time without debug messages
Comment 28 Jeff Gilbert [:jgilbert] 2012-07-15 04:52:25 PDT
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) (jwillcox@mozilla.com) 2012-07-16 09:24:40 PDT
Created attachment 642613 [details] [diff] [review]
Rebased, more fixes
Comment 30 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-16 09:40:13 PDT
(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) 2012-07-16 10:17:43 PDT
Would it make sense instead of Inverted use YFlip word, in order to keep inline with previous code
Comment 32 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-17 10:36:23 PDT
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) 2012-07-17 14:35:14 PDT
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) (jwillcox@mozilla.com) 2012-07-18 07:07:16 PDT
(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) (jwillcox@mozilla.com) 2012-07-18 13:38:53 PDT
Created attachment 643562 [details] [diff] [review]
Yet more fixes
Comment 36 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-18 13:40:37 PDT
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) (jwillcox@mozilla.com) 2012-07-18 13:45:01 PDT
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) 2012-07-18 13:47:39 PDT
Comment on attachment 643562 [details] [diff] [review]
Yet more fixes

Hoping the bugzilla interdiff is accurate.
Comment 39 Benoit Girard (:BenWa) 2012-07-18 13:50:43 PDT
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) (jwillcox@mozilla.com) 2012-07-18 14:10:48 PDT
Created attachment 643579 [details] [diff] [review]
One more time
Comment 41 Bill Gianopoulos [:WG9s] 2012-07-18 17:11:37 PDT
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:] 2012-07-19 14:59:20 PDT
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 Jeff Gilbert [:jgilbert] 2012-07-19 16:29:58 PDT
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 Jeff Gilbert [:jgilbert] 2012-07-19 16:32:13 PDT
Gerv, is the Apache stuff in StrongPointer.h alright? (and atomic.h, if we keep it)
Comment 45 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-19 18:42:21 PDT
(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) (jwillcox@mozilla.com) 2012-07-19 19:08:38 PDT
Created attachment 644134 [details] [diff] [review]
More review fixes
Comment 47 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-19 19:29:54 PDT
Created attachment 644141 [details] [diff] [review]
More review fixes
Comment 48 Jeff Gilbert [:jgilbert] 2012-07-19 22:19:15 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 Jeff Gilbert [:jgilbert] 2012-07-19 22:30:55 PDT
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] 2012-07-20 02:02:03 PDT
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] 2012-07-20 06:43:54 PDT
https://hg.mozilla.org/mozilla-central/rev/7d4c945625ca
Comment 52 Jeff Gilbert [:jgilbert] 2012-07-20 09:02:35 PDT
That was just part of the result from the previous attempt at landing.
Comment 53 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-20 11:34:16 PDT
Created attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb
Comment 54 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-20 11:34:59 PDT
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 Jeff Gilbert [:jgilbert] 2012-07-20 12:19:23 PDT
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) (jwillcox@mozilla.com) 2012-07-20 12:21:40 PDT
Relanded with fixes

https://hg.mozilla.org/integration/mozilla-inbound/rev/8571114112b2
Comment 57 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-20 12:29:52 PDT
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] 2012-07-20 15:08:55 PDT
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] 2012-07-20 21:03:20 PDT
https://hg.mozilla.org/mozilla-central/rev/8571114112b2
Comment 60 :Ms2ger (⌚ UTC+1/+2) 2012-07-22 05:04:28 PDT
Backed out for likely causing bug 776142.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfd941c49a6
Comment 61 Ryan VanderMeulen [:RyanVM] 2012-07-22 10:38:11 PDT
https://hg.mozilla.org/mozilla-central/rev/6cfd941c49a6
Comment 62 Justin Wood (:Callek) 2012-07-22 13:21:53 PDT
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) (jwillcox@mozilla.com) 2012-07-22 17:51:29 PDT
Created attachment 644809 [details] [diff] [review]
Initial support for Flash on Honeycomb

This patch doesn't break robocop/mochitest
Comment 64 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-22 17:52:38 PDT
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) (jwillcox@mozilla.com) 2012-07-23 06:30:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4987ffd173a4
Comment 67 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-23 11:34:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07aca82e80e
Comment 68 Ed Morley [:emorley] 2012-07-24 03:01:26 PDT
https://hg.mozilla.org/mozilla-central/rev/f07aca82e80e
Comment 69 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 07:09:25 PDT
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) (jwillcox@mozilla.com) 2012-07-24 10:47:58 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/410a1df0d0b1
Comment 71 Scoobidiver (away) 2012-07-24 13:06:10 PDT
Why didn't you fix the 4 regressions before uplifting it?
Comment 72 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 13:27:59 PDT
(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] 2012-07-24 13:45:24 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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] 2012-07-24 13:50:54 PDT
In addition, early feedback will allow us to prioritize regressions.
Comment 75 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 14:05:56 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/2467864aeb9d
Comment 76 Scoobidiver (away) 2012-07-24 14:08:04 PDT
Bug 776331 is a real regression, i.e. not the result of a new imperfect feature.
Comment 77 Catalin Suciu [:csuciu] 2013-04-01 23:54:55 PDT
This issue is fixed in all current builds.
Verifying.

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

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