Bug 687267 (honeycomb-flash)

support for flash on Android Fennec on honeycomb

VERIFIED FIXED in Firefox 15

Status

()

Core
Plug-ins
P1
enhancement
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: dougt, Assigned: snorp)

Tracking

(5 keywords)

unspecified
mozilla17
All
Android
compat, flashplayer, meta, mobile, pp
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox14 affected, firefox15+ fixed, firefox16 fixed, blocking-fennec1.0 -, fennec15+)

Details

Attachments

(3 attachments, 12 obsolete attachments)

32.54 KB, patch
Details | Diff | Splinter Review
41.91 KB, patch
Details | Diff | Splinter Review
168.23 KB, patch
blassey
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
+++ 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
(Reporter)

Comment 1

6 years ago
Created attachment 560720 [details] [diff] [review]
stub
Assignee: nobody → doug.turner
(Reporter)

Updated

6 years ago
Assignee: doug.turner → nobody

Updated

6 years ago
Blocks: 695826

Comment 2

6 years ago
Is this bug still valid since Flash won't be supported on mobile anymore?
(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

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

Comment 5

5 years ago
(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
tracking-fennec: + → 13+

Updated

5 years ago
Duplicate of this bug: 759134

Updated

5 years ago
tracking-fennec: 13+ → ?
status-firefox14: --- → affected
status-firefox15: --- → affected
Assignee: nobody → blyakher.a
Are we targeting this for Firefox 15 or 16?
blocking-fennec1.0: --- → ?

Updated

5 years ago
Alias: honeycomb-flash
Created attachment 637161 [details] [diff] [review]
IN PROGRESS PATCH

Updated

5 years ago
Attachment #637161 - Attachment is patch: true
tracking-fennec: ? → 15+
blocking-fennec1.0: ? → -

Updated

5 years ago
tracking-firefox15: --- → +
Created attachment 637668 [details] [diff] [review]
IN PROGRESS PATCH 2
Attachment #637161 - Attachment is obsolete: true
Assignee: blyakher.a → snorp

Comment 10

5 years ago
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
Created attachment 638788 [details] [diff] [review]
Initial support for Flash on Honeycomb
Attachment #638788 - Flags: review?(chrislord.net)
Attachment #638788 - Flags: review?(bgirard)
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!
Attachment #638788 - Flags: review?(chrislord.net) → review+
(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!
Depends on: 728524
Created attachment 639354 [details] [diff] [review]
Fixed up patch
Attachment #638788 - Attachment is obsolete: true
Attachment #638788 - Flags: review?(bgirard)
Attachment #638788 - Attachment is obsolete: false
Attachment #639354 - Flags: review?(romaxa)
Attachment #639354 - Flags: review?(jgilbert)
Attachment #639354 - Flags: review?(bgirard)
Attachment #639354 - Flags: review?(ajuma)

Comment 15

5 years ago
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?
(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.
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

5 years ago
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?
Attachment #639354 - Flags: review?(ajuma) → review+
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.
Attachment #639354 - Flags: review?(bgirard)
(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.
(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.
Created attachment 640659 [details] [diff] [review]
More fixes
Attachment #638788 - Attachment is obsolete: true
Attachment #639354 - Attachment is obsolete: true
Attachment #639354 - Flags: review?(romaxa)
Attachment #639354 - Flags: review?(jgilbert)
Attachment #639354 - Attachment is obsolete: false
Attachment #640659 - Flags: review?(bgirard)
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.
Attachment #640659 - Flags: review?(bgirard) → review+

Comment 24

5 years ago
(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.
Created attachment 642305 [details] [diff] [review]
Now with video support
Attachment #639354 - Attachment is obsolete: true
Attachment #640659 - Attachment is obsolete: true
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.
Attachment #642305 - Flags: review?(jgilbert)
Attachment #642305 - Flags: review?(bgirard)
Created attachment 642306 [details] [diff] [review]
Oops, this time without debug messages
Attachment #642306 - Flags: review?(bgirard)
Attachment #642305 - Attachment is obsolete: true
Attachment #642305 - Flags: review?(jgilbert)
Attachment #642305 - Flags: review?(bgirard)
Attachment #642306 - Flags: review?(jgilbert)
Blocks: 774059
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?
Created attachment 642613 [details] [diff] [review]
Rebased, more fixes
Attachment #642613 - Flags: review?(jgilbert)
Attachment #642306 - Attachment is obsolete: true
Attachment #642306 - Flags: review?(jgilbert)
Attachment #642306 - Flags: review?(bgirard)

Updated

5 years ago
Flags: in-moztrap?(catalin.suciu)
(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.

Updated

5 years ago
QA Contact: catalin.suciu
Would it make sense instead of Inverted use YFlip word, in order to keep inline with previous code
Attachment #642613 - Flags: review?(bgirard)
Attachment #642613 - Flags: review?(vladimir)
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;
Attachment #642613 - Flags: review?(vladimir) → review+
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 :(
Attachment #642613 - Flags: review?(bgirard) → review-
(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.
Created attachment 643562 [details] [diff] [review]
Yet more fixes
Attachment #643562 - Flags: review?(bgirard)
Attachment #642613 - Attachment is obsolete: true
Attachment #642613 - Flags: review?(jgilbert)
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.
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 on attachment 643562 [details] [diff] [review]
Yet more fixes

Hoping the bugzilla interdiff is accurate.
Attachment #643562 - Flags: review?(bgirard) → review+
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.
Created attachment 643579 [details] [diff] [review]
One more time
Attachment #643579 - Flags: review?(bgirard)
Attachment #643562 - Attachment is obsolete: true
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?

Updated

5 years ago
Attachment #643579 - Flags: review?(bgirard) → review+
Attachment #643579 - Flags: review?(jgilbert)
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 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.
Attachment #643579 - Flags: review?(jgilbert) → review-
Gerv, is the Apache stuff in StrongPointer.h alright? (and atomic.h, if we keep it)
(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.
Created attachment 644134 [details] [diff] [review]
More review fixes
Attachment #644134 - Flags: review?(jgilbert)
Attachment #643579 - Attachment is obsolete: true
Created attachment 644141 [details] [diff] [review]
More review fixes
Attachment #644141 - Flags: review?(jgilbert)
Attachment #644134 - Attachment is obsolete: true
Attachment #644134 - Flags: review?(jgilbert)
(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 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.
Attachment #644141 - Flags: review?(jgilbert) → review-
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
https://hg.mozilla.org/mozilla-central/rev/7d4c945625ca
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
That was just part of the result from the previous attempt at landing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
Created attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb
Attachment #644406 - Flags: review?(jgilbert)
Attachment #644141 - Attachment is obsolete: true
Latest patch is looking good so far on Try, but not everything is built yet.

https://tbpl.mozilla.org/?tree=Try&rev=fb5b18d8ed90
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 644406 [details] [diff] [review]
Initial support for Flash on Honeycomb

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

Alright, go for it. :)
Attachment #644406 - Flags: review?(jgilbert) → review+
Relanded with fixes

https://hg.mozilla.org/integration/mozilla-inbound/rev/8571114112b2
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.
Attachment #644406 - Flags: approval-mozilla-beta?
Attachment #644406 - Flags: approval-mozilla-aurora?
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.
Attachment #644406 - Flags: approval-mozilla-beta?
Attachment #644406 - Flags: approval-mozilla-beta+
Attachment #644406 - Flags: approval-mozilla-aurora?
Attachment #644406 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/8571114112b2
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 776329

Updated

5 years ago
Depends on: 776331

Updated

5 years ago
Depends on: 776334
Backed out for likely causing bug 776142.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfd941c49a6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 776142
https://hg.mozilla.org/mozilla-central/rev/6cfd941c49a6
Target Milestone: mozilla17 → ---
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
Attachment #644406 - Flags: approval-mozilla-beta?
Attachment #644406 - Flags: approval-mozilla-beta+
Attachment #644406 - Flags: approval-mozilla-aurora?
Attachment #644406 - Flags: approval-mozilla-aurora+
Created attachment 644809 [details] [diff] [review]
Initial support for Flash on Honeycomb

This patch doesn't break robocop/mochitest
Attachment #644809 - Flags: review?(blassey.bugs)
Attachment #644406 - Attachment is obsolete: true
Attachment #644406 - Flags: approval-mozilla-beta?
Attachment #644406 - Flags: approval-mozilla-aurora?
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).
Target Milestone: --- → mozilla17
Attachment #644809 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4987ffd173a4
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07aca82e80e
https://hg.mozilla.org/mozilla-central/rev/f07aca82e80e
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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.
Attachment #644809 - Flags: approval-mozilla-beta?
Attachment #644809 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #644809 - Flags: approval-mozilla-beta?
Attachment #644809 - Flags: approval-mozilla-beta+
Attachment #644809 - Flags: approval-mozilla-aurora?
Attachment #644809 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/410a1df0d0b1

Comment 71

5 years ago
Why didn't you fix the 4 regressions before uplifting it?
status-firefox16: --- → fixed
(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.
(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.
In addition, early feedback will allow us to prioritize regressions.
https://hg.mozilla.org/releases/mozilla-beta/rev/2467864aeb9d

Comment 76

5 years ago
Bug 776331 is a real regression, i.e. not the result of a new imperfect feature.
status-firefox15: affected → fixed

Updated

5 years ago
Depends on: 778442

Updated

5 years ago
Depends on: 778445

Updated

5 years ago
Depends on: 778983

Updated

5 years ago
Depends on: 783754

Updated

5 years ago
Depends on: 787685
This issue is fixed in all current builds.
Verifying.

Testcases available here: https://moztrap.mozilla.org/manage/cases/?filter-suite=65
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(catalin.suciu) → in-moztrap+
Depends on: 863490
You need to log in before you can comment on or make changes to this bug.