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

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

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

(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

Are we targeting this for Firefox 15 or 16?

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

(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!

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

(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.

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.

(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.

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

(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.

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

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 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 :(

(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.

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.

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.

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?

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.

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.

(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.

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

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

Latest patch is looking good so far on Try, but not everything is built yet. https://tbpl.mozilla.org/?tree=Try&rev=fb5b18d8ed90

Comment on attachment 644406 [details] [diff] [review] Initial support for Flash on Honeycomb Review of attachment 644406 [details] [diff] [review]: ----------------------------------------------------------------- Alright, go for it. :)

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.

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.

Backed out for likely causing bug 776142. https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfd941c49a6

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

This patch doesn't break robocop/mochitest

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).

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

Comment 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.

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

(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.

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

This issue is fixed in all current builds. Verifying. Testcases available here: https://moztrap.mozilla.org/manage/cases/?filter-suite=65