Here's some progress towards sharing the texture to the compositor over a PLayers update. We could get this working quickly by copying the texture but the proper way of doing it is to implement webgl double buffering.

Accidentally stole credit for cjones' work when copying headers ;)

More progress, code for sending copying the texture and sending it accross IPC is all there, need to add the code in OGL to handle OGL texture surface descriptors.

This works on mac now, but still needs to release the texture. It should work with any OGL implementation where we can share texture between GLContext in the same process.

The attachment seems to be missing the new CPP file, btw.

What about KHR image approach? I tried to make it works this way, it almost work, but I see it is blinking a lot with white color (not sure where is the problem yet)

Comment on attachment 617400 [details] [diff] [review] KHRimage approach I guess there are should be some sync performed, but I'm not sure if this supposed to work in general

Comment on attachment 617400 [details] [diff] [review] KHRimage approach The general approach looks ok to me, but I'd defer to jgilbert and pcwalton on the details.

Comment on attachment 617400 [details] [diff] [review] KHRimage approach Review of attachment 617400 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +350,5 @@ > bool needYFlip, > CanvasSurface* aNewBack) > { > if (!mDestroyed) { > nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(aNewFront); You should check the aNewFront type rather then checking null.

EGLImage does not supply any synchronization guarantees, which it seems like is the goal of this bug. EGLStream should provides this functionality, but it is relatively new, though exciting. EGLStream even has support for A/V synchronization. In the absence of EGLStream, if we're sharing between GL and GL, we should just be using our usual texture sharing path. If we don't want to block compositing on WebGL production, we'll need multibuffering.

EGLstream seems not widely available yet. EGLImageKHR we could probably use fence sync extension

(In reply to Oleg Romashin (:romaxa) from comment #11) > EGLstream seems not widely available yet. > EGLImageKHR we could probably use fence sync extension I don't see a reason to use EGL prims if it can be done with just GL prims. EGLImage/EGLSurface+EGLSync isn't any easier than sharing textures+ARB_sync, and doesn't seem to give us anything new.

(In reply to Jeff Gilbert [:jgilbert] from comment #12) > I don't see a reason to use EGL prims if it can be done with just GL prims. > EGLImage/EGLSurface+EGLSync isn't any easier than sharing textures+ARB_sync, > and doesn't seem to give us anything new. Not on the Android drivers I've seen. If you do standard GL texture sharing, the driver serializes all the commands and the contexts will not be able to execute in parallel. (This includes texture upload.) However, if you use separate contexts and use EGLImageKHR to share textures, then the contexts will execute in parallel.

(In reply to Patrick Walton (:pcwalton) from comment #13) > Not on the Android drivers I've seen. If you do standard GL texture sharing, > the driver serializes all the commands and the contexts will not be able to > execute in parallel. (This includes texture upload.) However, if you use > separate contexts and use EGLImageKHR to share textures, then the contexts > will execute in parallel. Oh, wow, gross. Ok, EGLImage and EGLSync is the way forward there, then.

Ok, this solution use simple glFinish for sync KHR image, works just perfect on my look.

Comment on attachment 618139 [details] [diff] [review] Working version with KHR images Review of attachment 618139 [details] [diff] [review]: ----------------------------------------------------------------- A bunch of extension types (KHR/OES/etc) snuck into our internals, so I'll file a bug about getting most of those out. In general, they don't add anything except verbosity. We should also file a follow-up bug to use EGLStream for this. EGLStream sounds like it is basically universal in new hardware, so we should take advantage of it. ::: gfx/gl/GLContext.h @@ +879,5 @@ > virtual bool SupportsOffscreenSplit() { > return IsExtensionSupported(EXT_framebuffer_blit) || IsExtensionSupported(ANGLE_framebuffer_blit); > } > > + virtual uintptr_t GetOffscreenKHRImage() { return 0; } EGLImage not uintptr_t, and nsnull instead of 0. ::: gfx/gl/GLContextProviderEGL.cpp @@ +600,5 @@ > } > > + uintptr_t GetOffscreenKHRImage() > + { > + if (!mImageKHR && sEGLLibrary.HasKHRImageBase()) { We can't cache the image quite this simply, since we create a new offscreen texture when we resize. @@ +607,5 @@ > + gfxXlibSurface* xsurf = static_cast<gfxXlibSurface*>(mThebesSurface.get()); > + mImageKHR = > + sEGLLibrary.fCreateImageKHR(EGL_DISPLAY(), > + EGL_NO_CONTEXT, > + LOCAL_EGL_NATIVE_PIXMAP_KHR, EGL_NATIVE_PIXMAP requires EGL ext EGL_KHR_image_pixmap. @@ +620,5 @@ > + }; > + EGLContext context = sEGLLibrary.fGetCurrentContext(); > + NS_ABORT_IF_FALSE(context != EGL_NO_CONTEXT, "Couldn't get the current context!"); > + EGLClientBuffer buffer = reinterpret_cast<EGLClientBuffer>(mOffscreenTexture); > + mImageKHR = sEGLLibrary.fCreateImageKHR(EGL_DISPLAY(), context, LOCAL_EGL_GL_TEXTURE_2D_KHR, EGL_GL_TEXTURE_2D requires the EGL ext EGL_KHR_gl_image. @@ +643,5 @@ > > bool mIsPBuffer; > bool mIsDoubleBuffered; > bool mCanBindToTexture; > + EGLImageKHR mImageKHR; Just 'EGLImage mImage/mEGLImage/mImageEGL;'. @@ +690,5 @@ > } > }; > > bool > +GLContextEGL::BindTex2DKHRImage(uintptr_t aKHRImage) We don't need this function. fImageTargetTexture2D should be moved into GLContext, and used directly if the extension is supported. @@ +693,5 @@ > bool > +GLContextEGL::BindTex2DKHRImage(uintptr_t aKHRImage) > +{ > + if (sEGLLibrary.HasKHRImageBase()) { > + sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, (EGLImageKHR)aKHRImage); This requires the GLES ext OES_EGL_image, not the EGL ext EGL_KHR_image_base. ::: gfx/gl/GLDefs.h @@ +3270,5 @@ > #define LOCAL_EGL_TEXTURE_2D 0x305F > #define LOCAL_EGL_NATIVE_PIXMAP_KHR 0x30B0 > #define LOCAL_EGL_IMAGE_PRESERVED_KHR 0x30D2 > +#define LOCAL_EGL_GL_TEXTURE_2D_KHR 0x30B1 > +#define LOCAL_EGL_GL_TEXTURE_LEVEL_KHR 0x30BC We should prefer to drop the KHR/EXT/OES suffixes for our internal defines, except in cases of name/value collisions. Also, please add a comment specifying which extension these are from. (EGL_KHR_gl_image, in this case) ::: gfx/layers/ipc/PLayers.ipdl @@ +78,5 @@ > struct SurfaceDescriptorD3D10 { > WindowsHandle handle; > }; > > +struct SurfaceDescriptorOGLImage { SurfaceDescriptorEGL(Image?) would probably be better. ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +430,5 @@ > program->SetLayerOpacity(GetEffectiveOpacity()); > program->SetRenderOffset(aOffset); > program->SetTextureUnit(0); > > + if (mTextureImage.handle()) { We should either test for IsExtensionSupported(OES_EGL_image) here, or have an NS_ABORT_IF_FALSE(IsExtensionSupported(OES_EGL_image)) in the block below. @@ +433,5 @@ > > + if (mTextureImage.handle()) { > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + gl()->BindTex2DKHRImage(mTextureImage.handle()); It would be clearer to just call fImageTargetTexture2D here. This means we need to add fImageTargetTexture2D to GLContext instead of EGLContext, and track its relevant extension there. ::: ipc/glue/IPCMessageUtils.h @@ +83,5 @@ > // This is a cross-platform approximation to HANDLE, which we expect > // to be typedef'd to void* or thereabouts. > typedef uintptr_t WindowsHandle; > > +typedef uintptr_t OGLImageHandle; EGLImageHandle. Also, please add a comment that this should approximate EGLImage, which is typedef'd from void*.

Filed bug 748717 for cleaning up of extension suffixes.

Comment on attachment 618139 [details] [diff] [review] Working version with KHR images Review of attachment 618139 [details] [diff] [review]: ----------------------------------------------------------------- This patch is a great step towards making WebGL better. We should nail down the texture sharing case otherwise all future work will be based on this and be broken. ::: ipc/glue/IPCMessageUtils.h @@ +83,5 @@ > // This is a cross-platform approximation to HANDLE, which we expect > // to be typedef'd to void* or thereabouts. > typedef uintptr_t WindowsHandle; > > +typedef uintptr_t OGLImageHandle; We can't expose this across process. We need a better solution for cross-thread only sharing instead of just mixing them. It would be great if we can solve that in a separate bug and have it block this. At the very we would need a runtime assert like we added temporary to the tiling layers until bug 747811 lands shortly. Ultimately you'll need to convince cjones with landing anything that breaks cross-process.

Not sure do I need to make CreateTextureImage from Shared Handle or not

Comment on attachment 626900 [details] [diff] [review] Minor rework for TexImage filter setup I like :)

Comment on attachment 627023 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 627023 [details] [diff] [review]: ----------------------------------------------------------------- Er, I guess I did more of a review than a feedback. Looks good! ::: gfx/layers/basic/BasicLayers.cpp @@ +2873,5 @@ > return; > } > > + if (mGLContext && > + BasicManager()->GetParentBackendType() != LayerManager::LAYERS_BASIC) { This should probably be == LayerManager::LAYERS_OPENGL @@ +2877,5 @@ > + BasicManager()->GetParentBackendType() != LayerManager::LAYERS_BASIC) { > + mozilla::gl::TextureImage::TextureShareType flags = > + XRE_GetProcessType() == GeckoProcessType_Default ? > + mozilla::gl::TextureImage::ThreadShared : > + mozilla::gl::TextureImage::ProcessShared; Just use an if - it's easier to read. @@ +2884,5 @@ > + if (handle) { > + FireDidTransactionCallback(); > + mBackBuffer = SharedTextureDescriptor(flags, handle, mBounds.Size()); > + BasicManager()->PaintedCanvas(BasicManager()->Hold(this), > + mNeedsYFlip ? true : false, Just pass mNeedsYFlip here - or is there a reason to use the ternary operator? @@ +407,5 @@ > > + ShaderProgramOGL *program = > + mTexture ? > + mOGLManager->GetBasicLayerProgram(CanUseOpaqueSurface(), true, GetMaskLayer() ? Mask2d : MaskNone) : > + mOGLManager->GetProgram(mTexImage->GetShaderProgramType(), GetMaskLayer()); Just use an if here too.

Fixed style nits.

Version of Shared Handle implementation which works on SGX/PVR drivers. Tested on beagle board SGX SDK 4.6.0.1. Maemo drivers can share simple textures via eglCreateSharedImageNOK http://gitorious.org/meego-graphics/meego-graphics/commit/25f81bf89166e6bc2f43313ac36a83b580824546/diffs But it does not like FBO offscreen texture. on NVIDIA tegra, - this does not work, see only black screen instead of texture content... possibly different EGLImage create API.

Wasted whole day on experimenting with different combinations of EGLImage,EGLImage from texture copy, and most of them just don't work. also EGLImageKHR seems does not work with dynamic textures... after all that I tried just simple CopyTexImage and share texture as it is, and it works on android (maemo/linux ubuntu beagleboard sgx530 don't). EGLImageKHR sharing without copies works only with latest TI SDK 4.6.0.1 and we can use it only with strict conditions. Will try next EGL image for doubled texture wrapped by EGLImage, hope it will work that way Current patch works good on Tegra3 (prime), and P1000 SGST., possibly worse to add proper resize handling https://tbpl.mozilla.org/?tree=Try&rev=855817ae2dd8

Added ReleaseSharedHandle API, in order to notify that texture can be reused or destroyed

Kinda texture stream implementation, which allow to track double/triple buffering case and destroy unused textures

Waitless version for android, use double/triple buffering with direct textures sharing across threads

Hope try build will not fail https://tbpl.mozilla.org/?tree=Try&rev=c742601afd76

Working build, works fine: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/romaxa@gmail.com-a43a102fb9d8/try-android

Tried to extedn GLTextureWrapper to wrap Texture + EGLImage. On Tegra3 (Asus Prime) - partially works, shared texture rendered on screen, but copy of shared texture dublicated in bottom-right corner - weird. and whole thing crashed after ~1 minute On P1000 - shared texture via EGL image not rendered at all (only weird yellow square painted), and whole device dying after some time. (also possible that I'm doing something wrong, but this way it works just great and perfect on beagleboard XM 4.6.0.1) And current approach with simple textures sharing of course stop working at all with patch https://bug759225.bugzilla.mozilla.org/attachment.cgi?id=628916

Don't worry, given that not all Androids seem to be happy with EGLImage, I won't land Attachment 628916 [details] [diff] :-)

Please have a look at Bug 760675. The global context may be disabled in the very short term, but there is a plan to re-enable it on devices where it's needed. All of that should happen very soon.

(In reply to Oleg Romashin (:romaxa) from comment #31) > Created attachment 629094 [details] [diff] [review] > Attempt to use EGLImage sharing, (don't work) Actually good news for maemo/N9, seems there both implementations (direct texture ID and EGLImage version) sharing works just great with GLStream version, then only thing does not work is dynamic changing texture wrapped by EGLImage and rendering at the same time (no internal driver sync implementation)

ok, sounds like in order to render properly into KHR image, we wrap offscreen texture with EGLImage and should use glEGLImageTargetRenderbufferStorageOES for binding with framebuffer

Shared Handle API, added release, and unbind API. Added BindSharedRenderBuffer to GLContext in order to allow bind EGLImage to WebGL FBO using glEGLImageTargetRenderbufferStorageOES

Practically we can use this for EGL sync, but I don't understand how it would help to prevent WEBGL/JS rendering at the same time when we render EGLImage in Compositor Thread..

This basically works on Tegra3, but with flickering... no sync

Tried to use FenceSync, mutex locking, got less flickering, but still not enough.

Heads up: Bug 760675 Comment 13 is useful information from NVIDIA about the merits of using EGLImage as opposed to a share group, even on devices and drivers that correctly support share groups.

Comment on attachment 629672 [details] [diff] [review] EGL Fence sync API Review of attachment 629672 [details] [diff] [review]: ----------------------------------------------------------------- I would like to throw this in, but the KHR suffixes have to go.

Updated to latest Working version

This version has 2 implementations (direct texture rendering and EGLImage based). Both versions work on tegra 3, will test it on other devices, but approximately it may look like this one. For EGLImage path, I do Create/Destroy EGLImages and CopyTexImage2D between them, in some cases it is a bit slower, and ImageTargetRenderbufferStorageOES works faster in that case... but ImageTargetRenderbufferStorageOES rendering require Full GLContext state Save/Restore and a bit more complicated, but also works.

Played with this try build: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/romaxa@gmail.com-afd45f8febbe/try-android/ Used this benchmark: http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/clear-varying-color.html This basically measures 1024x1024 WebGL canvas compositing performance with minimal nontrivial drawing. For reference, on the desktop we normally get 17 ms which means that it's capable of 60 FPS. With this build, on the Tegra 2 board, I get: default: 314 ms (varying, up to 380 ms) with USE_STREAM_EGL_IMAGE=1: 242 ms (varying, up to 280 ms) with USE_STREAM_EGL_IMAGE=1 and COPY_FROM_TEXTURE=1: 232 ms (quite stable) with USE_EGL_SHARED_IMAGE=1: 150 ms (quite stable)

Tested this particular test on Tegra3 Asus Prime ICS: default: Does not work (texture sharing seems disabled, no direct textures ID sharing between threads) with USE_STREAM_EGL_IMAGE=1: 35 ms (varying, up to 50 ms) with USE_STREAM_EGL_IMAGE=1 and COPY_FROM_TEXTURE=1: 29 ms (quite stable) with USE_EGL_SHARED_IMAGE=1: 29 ms (quite stable, but flickers)

Ah, and default upstream build on that test Tegra3 device, - ~340ms

Tested same thing on Tegra2 L4T (harmony), with eglimage i have 68ms, and readpixels(default version) - 233ms. i think there are something going wrong on bjacob's tegra2 device. btw, on tegra2 we have no neon, and tex upload/readpixels is very slow, that is shutting down perf a lot

Tried to call FenceSyncNV - also does not have effect. Also tried to use LOCAL_EGL_SYNC_FLUSH_COMMANDS_BIT, LOCAL_EGL_SYNC_PRIOR_COMMANDS_COMPLETE, and just 0, - also not any changes. giving up on this for now... will play with same thing on raspberry pi device bcom

On raspberry pi it also works fine with single buffer EGLImage, and no any additional sync needed, no flickering, lesson05 - 60FPS, clear-varying-color.html - 33ms

Ok, so here is the try build with 3 options of webgl sharing (managed by prefs or setenv) https://tbpl.mozilla.org/?tree=Try&rev=d119a030b8f1 https://hg.mozilla.org/try/file/bf7f533cb3f1/gfx/gl/GLLibraryEGL.cpp#l92 - possible evn and pref options (texture stream, eglImage stream, and shared single EGL image). Now question is what is the right way to enable it properly on different androids/SoC-GPU's/vendors... because HC/GB/ICS -androids, SGX/Nvidia/Broadcom/Qualcomm vendors, providing different support for all these versions

Just tested B2G on SGS2 with OMTC enabled and single buffer EGLImage sharing works fine for webgl, no fence sync required. http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/clear-varying-color.html - does 18ms

Appearing to work without some sync mechanism is unacceptable. Things need to work for reasons. We had this problem before with ANGLE and expecting its glFlush to resolve.

I think we should move forward with the CopyTexImage-based 'copy-swap' with EGLImage for now. It should be relatively low-impact to add complexity-wise, and should give us significant improvement. I want to do a more thorough refactoring for buffering and streaming shared 'textures' (GLTexture, EGLImage, EGLSurface, and d3d-interop textures) between content and compositor. I'm going to reverse the dependencies for this bug and bug 716859.

Sounds good to me; if we stick with just the one fast path, that should simplify the patches a bit, right?

One other option here is to render to a Surface on the content side, and consume that on the compositor side with a SurfaceTexture. This is only supported on Android 4.0+, but it would be fairly simple to implement. Synchronization is automatically handled by the Surface, and I would expect performance to be stellar.

That's a pretty great idea; we should test performance, since that would be a good bit simpler. We'd still need the other paths for pre-4.0 android (and non-android GLES), though. I assume that we could just use GeckoAppShell.createSurface()? That seems to create a SurfaceTextureLayer and adds to a list of plugin layers; do we care? (Or maybe we want that?)

Vlad, we don't want to use that awful plugin stuff. We can do it all in Gecko, either by invoking the Java Surface/SurfaceTexture stuff through JNI or dlopening the native versions.

Added UpdateSharedHandle, which supposed to update SharedHandle content

Simple version with raw EGLImage wrapper pointer sharing, only ThreadSharing implementation. Tested on Tegra, http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/clear-varying-color.html - 240ms -> 23ms imporvement.

Tested these patches on try: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/romaxa@gmail.com-e97fd67c2eab/try-android-debug/ with P1000 (EGLImage broken) and see I/Gonk (25910): Could not create EGL images: ERROR (0x300c) I/Gecko (25910): WARNING: Unexpected error, EGLImage wrapper creation failed: file GLContextProviderEGL.cpp, line 734 And rendering still works (with fallback SW path), in next version will add variable which would stop trying to create EGLImage shared handle after first fail

Here is the version which using fCopyTexSubImage2D, it is 2x slower on http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/clear-varying-color.html, comparing to draw quads texture copy code, but more simple. probably we can just address this perf issues to vendor (nvidia for example) this pass try, and have runtime blacklist variable for disabling EGLImage usage if first attempt has failed

Tested this with Fennec on a HTC One X (Tegra 3, ICS), using the clear benchmark: Nightly without any of these patches - 243ms; with CopyTexSubImage - 45 ms; with programmatic quad render - 20 ms. Definitely scratching my head here as to why CopyTexSubImage is so much slower.

yep, on Tegra 3 it definitely slower.... wonder if it is the same on other GPU's, like Mali, BCom... will check it on mali SGS2. b2g later

Note that even the CopyTexSubImage2D path needs to save/restore the active texture unit and any texture bound to that unit. Also, why does EGLTextureWrapper need that mutex lock? In playing with this, I just noticed that we're choosing an 8/8/8/8 rendering format for WebGL (default is alpha: true, and that benchmark doesn't specify). Even if I set alpha:false, we end up choosing 8/8/8/0! For the texture format we end up choosing GL_BGRA/GL_UNSIGNED_BYTE and GL_RGB/GL_UNSIGNED_BYTE respectively. We really want 5/6/5 here...

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #64) > Tested this with Fennec on a HTC One X (Tegra 3, ICS), using the clear > benchmark: Nightly without any of these patches - 243ms; with > CopyTexSubImage - 45 ms; with programmatic quad render - 20 ms. Definitely > scratching my head here as to why CopyTexSubImage is so much slower. I'm guessing they just are overly careful about it, or something. It's not that common of a function, and it's even less common for it to be copying the whole texture, since usually we'd just use CopyTexImage for that. As to the 565 stuff, really we should follow bug 743182 and use the native depth of the display. 565, even if on many mobile devices, is not on all. We shouldn't be using RGB if we have BGRA, since we should also have BGR. If we're not doing readback, that shouldn't matter though.

Comment on attachment 634644 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 634644 [details] [diff] [review]: ----------------------------------------------------------------- Layers stuff needs a review from BenWa. ::: gfx/gl/GLContext.h @@ +796,5 @@ > + * Create new shared GLContext content handle, must be released by ReleaseSharedHandle > + */ > + virtual SharedTextureHandle GetSharedHandle(TextureImage::TextureShareType aType) { return nsnull; } > + /** > + * Sync SharedHandle Storage with GLContext render storage In GL stuff, sync is always time-based, so let's rephrase this comment. Really, this is the step where the contents of the GLContext are published to the intermediate buffer for later compositing. ::: gfx/layers/basic/BasicLayers.cpp @@ +2763,5 @@ > public: > BasicShadowableCanvasLayer(BasicShadowLayerManager* aManager) : > BasicCanvasLayer(aManager), > mBufferIsOpaque(false) > + , mSwapInProcess(false) Please take the format of the local code with comma-last. @@ +2787,5 @@ > > virtual void SetBackBuffer(const SurfaceDescriptor& aBuffer) > { > mBackBuffer = aBuffer; > + mSwapInProcess = false; Please explain why this is here, even if it's just because SetBackBuffer() is called at the conclusion of a swap. @@ +2803,5 @@ > + return mBackBuffer.get_SharedTextureDescriptor().handle(); > + return nsnull; > + } > + > + void ReleaseBackBuffer() Is this called anywhere? Is it going to do something eventually? @@ +2811,3 @@ > void DestroyBackBuffer() > { > + if (GetSharedBackBufferHandle()) { While it should be cheap, this is already returning the handle. Let's grab it then and use it if it's valid, instead of calling get_SharedTexture... ourselves again. @@ +2829,5 @@ > } > > SurfaceDescriptor mBackBuffer; > bool mBufferIsOpaque; > + bool mSwapInProcess; Since we're in the land of threads and *process*es, we should use a different word here. 'Progress' would be fine. Also, is this variable ever accessed by any other other threads? @@ +2862,5 @@ > > + if (mGLContext && > + BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL) { > + mozilla::gl::TextureImage::TextureShareType flags; > + if (XRE_GetProcessType() == GeckoProcessType_Default) If default is threads, can you add a comment to that effect? @@ +2883,5 @@ > + } > + if (handle) { > + mGLContext->MakeCurrent(); > + // Make mGLContext finish all pending calls > + mGLContext->GuaranteeResolve(); You don't need this first one...probably. It's hard to tell since I don't know what UpdateSharedHandle() does before reviewing the next patch. We should need at most one resolve point, and it should be after all work in the current context has been issued, to assure it's completed. In the case of copying to an intermediate buffer, the Finish goes after the copy, and is not necessary before it. ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +293,5 @@ > ShadowCanvasLayerOGL::ShadowCanvasLayerOGL(LayerManagerOGL* aManager) > : ShadowCanvasLayer(aManager, nsnull) > , LayerOGL(aManager) > , mNeedsYFlip(false) > + , mTexture(0) You don't seem to initialize mFrontBufferDescriptor. @@ +426,5 @@ > + if (IsValidSharedTexDescriptor(mFrontBufferDescriptor)) { > + SharedTextureDescriptor texDescriptor = mFrontBufferDescriptor.get_SharedTextureDescriptor(); > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + if (!gl()->BindSharedHandle(texDescriptor.shareType(), texDescriptor.handle())) { This will always fail with the default implementation BindSharedHandle(). Also, if we're binding it here, why do we bindTexture on the line above?

Comment on attachment 634961 [details] [diff] [review] EGLImage implementation. Review of attachment 634961 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderEGL.cpp @@ +254,5 @@ > , mBound(false) > , mIsPBuffer(false) > , mIsDoubleBuffered(false) > , mCanBindToTexture(false) > + , mShareWithEGLImage(true) This should only be true if we support all the requisite extensions. @@ +662,5 @@ > + EGLTextureWrapper(GLContext* aContext, GLuint aTexture) > + : mContext(aContext) > + , mTexture(aTexture) > + , mEGLImage(nsnull) > + , mAccessLock("EGLTextureWrapper.mAccessLock") I don't believe it's here where you need the mutex. @@ +665,5 @@ > + , mEGLImage(nsnull) > + , mAccessLock("EGLTextureWrapper.mAccessLock") > + { > + } > + bool CreateEGLImage() { These functions which use EGLImage should abort if they're used when the extension they need are not supported. @@ +682,5 @@ > + if (!mEGLImage) { > + printf_stderr("Could not create EGL images: ERROR (0x%04x)\n", sEGLLibrary.fGetError()); > + return false; > + } > + sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, mEGLImage); We shouldn't need this step. When you create an EGLImage from a GL texture, that texture becomes the first EGLImage sibling. If we need it because Drivers Are Bad At Things, then we need to put it behind a WorkAroundDriverBugs(). @@ +689,5 @@ > + > + virtual ~EGLTextureWrapper() { > + MutexAutoLock lock(mAccessLock); > + mContext->MakeCurrent(); > + mContext->fDeleteTextures(1, &mTexture); During destruction, if MakeCurrent fails, don't run any more commands. @@ +717,5 @@ > +{ > + if (aType == TextureImage::ThreadShared) { > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + GLuint prevRead = GetUserBoundReadFBO(); > + BindInternalReadFBO(mOffscreenDrawFBO); No, we should be reading from what GLContext pretends is 0, which for reading is the read buffer. For mobile, this should be the same, but it's not correct. All you need is to BindUserReadFBO(0), then just CopyTexSubImage normally. The GLContext internals take care of the rest. @@ +723,5 @@ > + fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID()); > + // CopyTexSubImage2D, is ~2x slower than simple FBO render to texture with draw quads > + // But render with draw quads require complex and had maintainable context save/restore code > + fCopyTexSubImage2D(LOCAL_GL_TEXTURE_2D, 0, 0, 0, > + 0, 0, mOffscreenSize.width, mOffscreenSize.height); This should be mOffscreenActualSize. mOffscreenSize is just how big we're pretending it is. @@ +724,5 @@ > + // CopyTexSubImage2D, is ~2x slower than simple FBO render to texture with draw quads > + // But render with draw quads require complex and had maintainable context save/restore code > + fCopyTexSubImage2D(LOCAL_GL_TEXTURE_2D, 0, 0, 0, > + 0, 0, mOffscreenSize.width, mOffscreenSize.height); > + fBindTexture(LOCAL_GL_TEXTURE_2D, 0); Revert this binding to what it was previously. We also need to revert which texture unit is active. @@ +732,5 @@ > + return false; > +} > + > +SharedTextureHandle > +GLContextEGL::GetSharedHandle(TextureImage::TextureShareType aType) It seems like this is actually CreateSharedHandle(), because it seems to be creating a new EGLImage every time it's called. @@ +740,5 @@ > + GLuint texture = 0; > + ContextFormat fmt = ActualFormat(); > + CreateTextureForOffscreen(ChooseGLFormats(fmt), mOffscreenSize, texture); > + EGLTextureWrapper* tex = new EGLTextureWrapper(this, texture); > + if (!tex->CreateEGLImage()) { So we're creating a new EGLImage every frame? This is OK if it's the simplest way to do it. @@ +759,5 @@ > +{ > + if (aType == TextureImage::ThreadShared) { > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + delete wrap; > + } This should return true when it succeeds, shouldn't it? @@ +764,5 @@ > + > + return false; > +} > + > +bool GLContextEGL::BindSharedHandle(TextureImage::TextureShareType aType, This should be AttachSharedHandle, based on what it's doing. Bind just sets which texture handle is being operated on/with. This is actually overwriting the currently-bound texture's source/data. @@ +769,5 @@ > + SharedTextureHandle aSharedHandle) > +{ > + if (aType == TextureImage::ThreadShared) { > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, wrap->GetEGLImage()); This is just bitrot, but this lives in GLContext now. ::: gfx/gl/GLDefs.h @@ +3251,5 @@ > #define LOCAL_EGL_CORE_NATIVE_ENGINE 0x305B > #define LOCAL_EGL_READ 0x305A > #define LOCAL_EGL_DRAW 0x3059 > #define LOCAL_EGL_CONTEXT_LOST 0x300E > +#define LOCAL_EGL_GL_TEXTURE_2D 0x30B1 Please note that this is from EGL_KHR_gl_texture_2D_image, similar to defs from other extensions below.

> You don't seem to initialize mFrontBufferDescriptor. It is initialized by default with Tnone type > So we're creating a new EGLImage every frame? This is OK if it's the simplest way to do it. GetSharedHandle is not called on every frame. and yeah , it should be CreateSharedHandle

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #64) > Tested this with Fennec on a HTC One X (Tegra 3, ICS), using the clear > benchmark: Nightly without any of these patches - 243ms; with > CopyTexSubImage - 45 ms; with programmatic quad render - 20 ms. Definitely > scratching my head here as to why CopyTexSubImage is so much slower. I have it on good authority (a.k.a. Qualcomm) that TexSubImage* can be very slow in tiled architectures. IIRC it has something to do with the way they layout the textures in memory; finding and changing the bits that need to change with TexSubImage* is time consuming. They told me it is often quicker to re-upload the whole texture with TexImage*. Presumably the same applies to CopyTex* as well. That being said, as far as I know Tegra does not have a tiling architecture.

Hope I did not miss anything

(In reply to Mark Callow from comment #71) > (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #64) > > Tested this with Fennec on a HTC One X (Tegra 3, ICS), using the clear > > benchmark: Nightly without any of these patches - 243ms; with > > CopyTexSubImage - 45 ms; with programmatic quad render - 20 ms. Definitely > > scratching my head here as to why CopyTexSubImage is so much slower. > > I have it on good authority (a.k.a. Qualcomm) that TexSubImage* can be very > slow in tiled architectures. IIRC it has something to do with the way they > layout the textures in memory; finding and changing the bits that need to > change with TexSubImage* is time consuming. They told me it is often quicker > to re-upload the whole texture with TexImage*. Presumably the same applies > to CopyTex* as well. > > That being said, as far as I know Tegra does not have a tiling architecture. Unfortunately, in order to be able to use CopyTexImage instead of CopyTexSubImage, we would need to recreate the EGLImage each frame, with preserve, which is not guaranteed to work. Again, though, this is to tide us over until we get no-copy-swap streaming. Simple is more important than absolute performance, for this segment.

(In reply to Mark Callow from comment #71) > I have it on good authority (a.k.a. Qualcomm) that TexSubImage* can be very > slow in tiled architectures. IIRC it has something to do with the way they > layout the textures in memory; finding and changing the bits that need to > change with TexSubImage* is time consuming. They told me it is often quicker > to re-upload the whole texture with TexImage*. Presumably the same applies > to CopyTex* as well. > > That being said, as far as I know Tegra does not have a tiling architecture. Right; I knew that was the case on tiling architectures, but I also thought that they optimized whole-image TexSubImage. CopyTexImage would orphan the previous mipmap level, which we can't do here (since it's from an EGLImage). And yeah, Tegra isn't tiled, which is what led me to start looking at formats -- my suspicion was that we're hitting some kind of unoptimized format conversion path that we manage to avoid/optimize if we do the quad render. I'll look more into format stuff tomorrow...

If I modify things so that a true 5/6/5 buffer gets created, and modify the testcase to explicitly say alpha: false, then with CopyTexSubImage I get 16ms -- basically requestAnimationFrame resolution. With the quad render I still get 16ms as well, because we're now hitting rAF resolution. I changed the test to use postMessage with no delay; I get about 5ms with both. (The quad render might have a slight edge, but hard to tell; the numbers basically range from 3-7ms for both.) The changed testcases are at: http://conduit.bitops.com/~vladimir/misc/clear-varying-color-noalpha.html http://conduit.bitops.com/~vladimir/misc/clear-varying-color-noalpha-pm.html (alpha: true by default is a huge performance trap on mobile, even after we fix the 565 business... since it'll force us to an 8888 context! We should make sure we document this somewhere.) However, I'm pretty excited about these patches, and we should land them as soon as we can -- we can fix the 565 stuff in a followup.

Uh, what, why did that stuff get reset?

Comment on attachment 635160 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 635160 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, though jgilbert/benwa should still review since I'm not as familiar with the code as I used to be. ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +35,5 @@ > +static void > +MakeTexture(GLContext* gl, GLuint& aTexture) > +{ > + if (aTexture != 0) > + return; given the aTexture != 0 check/behaviour, this should probably be called "EnsureTexture" or "MakeTextureIfNeeded". @@ +335,5 @@ > + *aNewBack = mFrontBufferDescriptor; > + mFrontBufferDescriptor = aNewFront; > + mNeedsYFlip = needYFlip; > + return; > + } else { Nit: no else after return

Comment on attachment 635160 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 635160 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicLayers.cpp @@ +2882,5 @@ > + } > + if (handle) { > + mGLContext->UpdateSharedHandle(flags, handle); > + // Make Shared Handle fully painted > + mGLContext->GuaranteeResolve(); Actually, one more comment -- why don't we make this part of UpdateSharedHandle? That is, if it needs to do any kind of resolve, let it handle it directly, and have the docs just say that it'll be fully resolved and ready for use after it returns. That way callers don't have to worry about calling GuaranteeResolve, and there may be sharing cases where a glFinish/glFlush isn't needed.

Comment on attachment 635162 [details] [diff] [review] EGLImage implementation. Review of attachment 635162 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with one bug and some nits... but same as above applies for review from jglibert/benwa. ::: gfx/gl/GLContextProviderEGL.cpp @@ +711,5 @@ > +bool > +GLContextEGL::UpdateSharedHandle(TextureImage::TextureShareType aType, > + SharedTextureHandle aSharedHandle) > +{ > + if (aType == TextureImage::ThreadShared) { Here and elsewhere, cleaner to write: if (aType != TextureImage::ThreadShared) return false; instead of indenting the entire block @@ +716,5 @@ > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + GLuint prevRead = GetUserBoundReadFBO(); > + GLint oldtex, activetex; > + fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex); > + fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex); Bug -- GL_TEXTURE_BINDING_2D needs to be queried after the ActiveTexture() call (otherwise oldtex is going to get the texture bound to the current target, which isn't necessarily TEXTURE0). Note for a followup bug: any params that we're going to be modifying/querying often (e.g. the active texture, and the texture bound to 0) we should cache in GLContext so that we don't actually have to query it from GL.

> > + mNeedsYFlip = needYFlip; > > + return; > > + } else { > > Nit: no else after return without it we will override *aNewBack with aNewFront at the end of function

Address vlad comments

Addressed vlad's comments

(In reply to Oleg Romashin (:romaxa) from comment #81) > > > + mNeedsYFlip = needYFlip; > > > + return; > > > + } else { > > > > Nit: no else after return > > without it we will override *aNewBack with aNewFront at the end of function I mean: if (foo) { ... return; } ...morestuff... instead of: if (foo) { ... return; } else { ...morestuff... } they're functionally equivalent; the former is cleaner and easier to read, though.

Comment on attachment 635430 [details] [diff] [review] EGLImage implementation. >+bool >+GLContextEGL::UpdateSharedHandle(TextureImage::TextureShareType aType, >+ SharedTextureHandle aSharedHandle) >+{ >+ if (aType != TextureImage::ThreadShared) >+ return false; >+ >+ EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; >+ GLuint prevRead = GetUserBoundReadFBO(); >+ GLint oldtex, activetex; >+ fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex); >+ fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex); >+ BindUserReadFBO(0); >+ fActiveTexture(LOCAL_GL_TEXTURE0); >+ fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID()); This bug wasn't addressed; this needs to be: GLint oldtex, activetex; BindUserReadFBO(0); fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex); fActiveTexture(LOCAL_GL_TEXTURE0); fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex); fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID());

Comment on attachment 635448 [details] [diff] [review] EGLImage implementation. >+ EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; >+ GLuint prevRead = GetUserBoundReadFBO(); >+ GLint oldtex, activetex; >+ BindUserReadFBO(0); >+ fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex); >+ fActiveTexture(LOCAL_GL_TEXTURE0); >+ fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex); >+ fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID()); >+ // CopyTexSubImage2D, is ~2x slower than simple FBO render to texture with draw quads >+ // But render with draw quads require complex and had maintainable context save/restore code >+ fCopyTexSubImage2D(LOCAL_GL_TEXTURE_2D, 0, 0, 0, >+ 0, 0, mOffscreenActualSize.width, mOffscreenActualSize.height); >+ fActiveTexture(activetex); >+ fBindTexture(LOCAL_GL_TEXTURE_2D, oldtex); I missed this -- the order of these two calls need to be swapped. You need to restore the old texture 0 binding, not that texture to whatever the old active texture binding was! (fBindTexture first, then fActiveTexture here).

That is confused me too a little bit... but ok, I'll fix it in last version, let's get jgilbert and BenWa output first, so I can combine changes at the end

Comment on attachment 635448 [details] [diff] [review] EGLImage implementation. Review of attachment 635448 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderEGL.cpp @@ +254,5 @@ > , mBound(false) > , mIsPBuffer(false) > , mIsDoubleBuffered(false) > , mCanBindToTexture(false) > + , mShareWithEGLImage(sEGLLibrary.HasKHRImagePixmap() && sEGLLibrary.HasKHRImageTexture2D()) This is complicated and should probably be done outside the initializer list. To know we support EGLImage stuff, we need: EGL_KHR_image_base, EGL_KHR_gl_texture_2D_image, and GL_OES_EGL_image. If EGL_IMAGE_PRESERVED is always false, then we can do: (EGL_KHR_image_base || EGL_KHR_image), EGL_KHR_gl_texture_2D_image, and GL_OES_EGL_image. Please use the extension query system added with bug 762265. Also, please add a MOZ_ASSERT if mShareWithEGLImage is true if we're not on android or b2g. @@ +595,5 @@ > + SharedTextureHandle aSharedHandle); > + virtual bool ReleaseSharedHandle(TextureImage::TextureShareType aType, > + SharedTextureHandle aSharedHandle); > + virtual bool AttachSharedHandle(TextureImage::TextureShareType aType, > + SharedTextureHandle aSharedHandle); Misaligned indent. @@ +659,5 @@ > +class EGLTextureWrapper > +{ > +public: > + EGLTextureWrapper(GLContext* aContext, GLuint aTexture) > + : mContext(aContext) 4-space indents. @@ +665,5 @@ > + , mEGLImage(nsnull) > + { > + } > + bool CreateEGLImage() { > + MOZ_ASSERT(!mEGLImage && mTexture && sEGLLibrary.HasKHRImagePixmap() && sEGLLibrary.HasKHRImageTexture2D()); See the list of extensions we need above. (though this function doesn't technically need GL_OES_EGL_image) Also, you'll need to use the GLLibraryEGL::IsExtensionSupported for extension checks. @@ +672,5 @@ > + LOCAL_EGL_NONE > + }; > + mContext->MakeCurrent(); > + mContext->fActiveTexture(LOCAL_GL_TEXTURE0); > + mContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); You shouldn't need to bind the texture, since we specify both |target| and |name| in eglCreateImage. You don't need to set the active texture, either. @@ +677,5 @@ > + GLContextEGL* ctx = static_cast<GLContextEGL*>(mContext); > + mEGLImage = sEGLLibrary.fCreateImage(EGL_DISPLAY(), ctx->Context(), LOCAL_EGL_GL_TEXTURE_2D, > + (EGLClientBuffer)mTexture, eglAttributes); > + if (!mEGLImage) { > + printf_stderr("Could not create EGL images: ERROR (0x%04x)\n", sEGLLibrary.fGetError()); Hide this printf behind if DebugMode(). @@ +713,5 @@ > + return false; > + > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + GLuint prevRead = GetUserBoundReadFBO(); > + GLint oldtex, activetex; Please set these to -1 and MOZ_ASSERT that they aren't -1 after our calls to set them. @@ +716,5 @@ > + GLuint prevRead = GetUserBoundReadFBO(); > + GLint oldtex, activetex; > + BindUserReadFBO(0); > + fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex); > + fActiveTexture(LOCAL_GL_TEXTURE0); Since we don't need to use a particular texture unit (because we're not rendering), we can just use the current texture unit and restore it like normal. We shouldn't need to touch ActiveTexture at all. @@ +719,5 @@ > + fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex); > + fActiveTexture(LOCAL_GL_TEXTURE0); > + fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex); > + fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID()); > + // CopyTexSubImage2D, is ~2x slower than simple FBO render to texture with draw quads Put a whitespace line between this comment and the preceeding code, to break up this block visually. @@ +726,5 @@ > + 0, 0, mOffscreenActualSize.width, mOffscreenActualSize.height); > + fActiveTexture(activetex); > + fBindTexture(LOCAL_GL_TEXTURE_2D, oldtex); > + BindUserReadFBO(prevRead); > + // Make Shared Handle fully painted Another empty line before this comment too. @@ +746,5 @@ > + ContextFormat fmt = ActualFormat(); > + CreateTextureForOffscreen(ChooseGLFormats(fmt), mOffscreenSize, texture); > + EGLTextureWrapper* tex = new EGLTextureWrapper(this, texture); > + if (!tex->CreateEGLImage()) { > + NS_WARNING("Unexpected error, EGLImage wrapper creation failed"); This should probably never happen, since we're checking for all the requisite exts and such. I'd make this an NS_ERROR.

Comment on attachment 635447 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 635447 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicLayers.cpp @@ +2797,5 @@ > mBackBuffer = SurfaceDescriptor(); > BasicShadowableLayer::Disconnect(); > } > > + mozilla::gl::SharedTextureHandle GetSharedBackBufferHandle() Make this private @@ +2860,5 @@ > + if (mGLContext && > + BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL) { > + mozilla::gl::TextureImage::TextureShareType flags; > + // Shadowable layers are available in IPC and OMTC mode, > + // if process type is default, then it is OMTC mode, othwrwise IPC otherwise*. The terminology is confusing here. OMTC means compositing off the main content thread which is a bit ambiguous if you mean e10s or not. We also use the IPC framework to communicate within the same process. I recommend just using 'e10s' to specify whether we composite in a different process or not. @@ +2866,5 @@ > + flags = mozilla::gl::TextureImage::ThreadShared; > + else > + flags = mozilla::gl::TextureImage::ProcessShared; > + > + if (mSwapInProgress) { The transaction and swap will happen synchronously. Why is this needed? ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +435,5 @@ > + gl()->ApplyFilterToBoundTexture(filter); > + program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), texDescriptor.size())); > + mOGLManager->BindAndDrawQuad(program, mNeedsYFlip); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, 0); > + return; You're adding a second drawing path here. Can we get something cleaner then: if (...) { // Draw path 2 return; } // Draw path 1 Either use if+else or break the two drawing paths into a helper method.

Comment on attachment 635448 [details] [diff] [review] EGLImage implementation. Review of attachment 635448 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLDefs.h @@ +3253,5 @@ > #define LOCAL_EGL_DRAW 0x3059 > #define LOCAL_EGL_CONTEXT_LOST 0x300E > > +// EGL_KHR_gl_texture_2D_image > +#define LOCAL_EGL_GL_TEXTURE_2D 0x30B1 This needs a _KHR postfix: http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt

jgilbert told that we should get rid of all postfixes...

> > + if (!tex->CreateEGLImage()) { > > + NS_WARNING("Unexpected error, EGLImage wrapper creation failed"); > > This should probably never happen, since we're checking for all the > requisite exts and such. I'd make this an NS_ERROR. this actually just always happen on ginger bread android P1000

(In reply to Oleg Romashin (:romaxa) from comment #93) > jgilbert told that we should get rid of all postfixes... Fair enough

(In reply to Oleg Romashin (:romaxa) from comment #94) > > > + if (!tex->CreateEGLImage()) { > > > + NS_WARNING("Unexpected error, EGLImage wrapper creation failed"); > > > > This should probably never happen, since we're checking for all the > > requisite exts and such. I'd make this an NS_ERROR. > this actually just always happen on ginger bread android P1000 Let's change the wording then, since we're are expecting this on some platforms. Just "EGLImage creation for EGLTextureWrapper failed" is fine.

While I will be on PTO next week, I'll still be able to do these reviews, since we want to push this out sooner rather than later. I'll see about getting the next round of review in this weekend.

(Ugh, really bugzilla?)

Comment on attachment 635798 [details] [diff] [review] EGLImage implementation. >diff --git a/gfx/gl/GLContextProviderEGL.cpp b/gfx/gl/GLContextProviderEGL.cpp >--- a/gfx/gl/GLContextProviderEGL.cpp >+++ b/gfx/gl/GLContextProviderEGL.cpp >@@ -257,16 +257,20 @@ public: > , mBound(false) > , mIsPBuffer(false) > , mIsDoubleBuffered(false) > , mCanBindToTexture(false) > { > // any EGL contexts will always be GLESv2 > SetIsGLES2(true); > >+ mShareWithEGLImage = sEGLLibrary.HasKHRImageBase() && >+ sEGLLibrary.HasKHRImageTexture2D() && >+ IsExtensionSupported(OES_EGL_image); This can't ever work (did you test?) -- IsExtensionSupported will always return false here, because it's called before the context is initialized. Initialize this with sEGLLibrary, and then in Init(), after the InitWithPrefix call, do mShareWithEGLImage = mShareWithEGLImage && IsExtensionSupported(OES_EGL_image).

(or just init mShareWithEGLImage to false, and move this assignment to Init)

Oh, you right, I haven't had time yesterday to test it properly. this one works, tested

Comment on attachment 635797 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 635797 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Just a few things to fix up then we're good to go. ::: gfx/gl/GLContext.h @@ +796,5 @@ > + */ > + virtual SharedTextureHandle CreateSharedHandle(TextureImage::TextureShareType aType) { return nsnull; } > + /** > + * Publish GLContext content to intermediate buffer attached to shared handle > + * Shared handle content is ready to use after call finished, and no need extra Flush/Finish Shared handle content is ready to be used after call returns, and no need extra Flush/Finish are required* @@ +801,5 @@ > + */ > + virtual bool UpdateSharedHandle(TextureImage::TextureShareType aType, > + SharedTextureHandle aSharedHandle) { return false; } > + /** > + * Delete Handle created by CreateSharedHandle This comment repeats information provided in the function name. Can you elaborate on the requirements of this function in the comment: - Does it have to be called on the context where the shared handle is created? Does it have to be called on the context that's receiving the shared handle? Both? - Can this release be called when the object is actively bound to the GL_TEXTURE_2D target? - Can this release be called when the texture is still being used by the context but the operation isn't finished on the GPU (no glFinish have been called). ::: gfx/layers/basic/BasicLayers.cpp @@ +2859,5 @@ > + BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL) { > + TextureImage::TextureShareType flags = TextureImage::ProcessShared; > + // if process type is default, then it is e10s withing same process > + if (XRE_GetProcessType() == GeckoProcessType_Default) > + flags = TextureImage::ThreadShared; else flags = TextureImage::ProcessShared ::: gfx/layers/ipc/ShadowLayerUtils.h @@ +71,5 @@ > + typedef mozilla::gl::TextureImage::TextureShareType paramType; > + > + static void Write(Message* msg, const paramType& param) > + { > + WriteParam(msg, int32(param)); I can't find confirmation that an enum will always fit in int32. ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +428,5 @@ > + // Shared texture handle rendering path > + SharedTextureDescriptor texDescriptor = mFrontBufferDescriptor.get_SharedTextureDescriptor(); > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + if (!gl()->AttachSharedHandle(texDescriptor.shareType(), texDescriptor.handle())) { Is this operation cheap? We're doing it every frame. Typically we prepare the texture on swap only once. @@ +440,2 @@ > } else { > + // Tiled texture image rendering path You're changing the behavior on this rendering path but it's not clear to me why this belongs in this bug. I'd rather see this change in a different bug so we can discuss why we're making these changes if NPOT.

> ::: gfx/layers/ipc/ShadowLayerUtils.h > @@ +71,5 @@ > > + typedef mozilla::gl::TextureImage::TextureShareType paramType; > > + > > + static void Write(Message* msg, const paramType& param) > > + { > > + WriteParam(msg, int32(param)); > > I can't find confirmation that an enum will always fit in int32. C++ defines an implicit conversion from enum -> int, so it should be safe.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #105) > > ::: gfx/layers/ipc/ShadowLayerUtils.h > > @@ +71,5 @@ > > > + typedef mozilla::gl::TextureImage::TextureShareType paramType; > > > + > > > + static void Write(Message* msg, const paramType& param) > > > + { > > > + WriteParam(msg, int32(param)); > > > > I can't find confirmation that an enum will always fit in int32. > > C++ defines an implicit conversion from enum -> int, so it should be safe. Discussed this with vlad on IRC. Just to be on the safe size we should assert that enum -> int32 is safe since we support so many different platforms: vlad> sure, I mean a static assert that sizeof(enumtype) <= sizeof(int32) should be enough

Comment on attachment 636096 [details] [diff] [review] EGLImage implementation. Review of attachment 636096 [details] [diff] [review]: ----------------------------------------------------------------- I'm not familiar with EGLImage so I'll leave those details to Jeff. The patch looks good to me. ::: gfx/gl/GLContextProviderEGL.cpp @@ +727,5 @@ > + MOZ_ASSERT(oldtex != -1); > + fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID()); > + > + // CopyTexSubImage2D, is ~2x slower than simple FBO render to texture with draw quads > + // But render with draw quads require complex and had maintainable context save/restore code Do you mean hard to maintain? @@ +736,5 @@ > + fBindTexture(LOCAL_GL_TEXTURE_2D, oldtex); > + BindUserReadFBO(prevRead); > + > + // Make Shared Handle fully painted > + GuaranteeResolve(); Yuk! Is this mandatory or do we have a path forward to remove this? May be worth adding a comment in the code base to explain why this is required and if it can be removed later a short description of what needs to be done.

(In reply to Benoit Girard (:BenWa) from comment #104) > @@ +440,2 @@ > > } else { > > + // Tiled texture image rendering path > > You're changing the behavior on this rendering path but it's not clear to me > why this belongs in this bug. I'd rather see this change in a different bug > so we can discuss why we're making these changes if NPOT. Nevermind this comment, the interdiff mislead me.

Fixed benwa comments, EGLBackend part will update after jgilbert review

Comment on attachment 636449 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 636449 [details] [diff] [review]: ----------------------------------------------------------------- Sharing resources on GL is very error prone and inconsistent. I really want our wrapper API to be very clear on how you can use it. ::: gfx/gl/GLContext.h @@ +801,5 @@ > + */ > + virtual bool UpdateSharedHandle(TextureImage::TextureShareType aType, > + SharedTextureHandle aSharedHandle) { return false; } > + /** > + * after call returns true SharedHandle may not be used. Please specify the things I had asked to clarify about this call and that we discussed on IRC. If you're unsure about some rules then state the assumption that you're making about the usage of this call. Please follow the comment voice and capitalization similar to the other comments.

As much as I love the performance wins here, I think this needs more bulletproofing before landing, or at least needs to be hidden behind a pref. Things to test: - fire up Fennec on Android, open up WebGL content, then hit the Home button... go back to Fennec. (In my build here, Fennec crashes when it's brought back up) - open up multiple WebGL content pages, switching back and forth between them

Hmm, interesting, I guess I did that test, and did not have any crashes... But thank you I'll try perform these steps, and make sure that it does not crash.

Interesting.. I'll grab a stack trace tomorrow, couldn't do it when I had the crash.

(In reply to Benoit Girard (:BenWa) from comment #107) > @@ +736,5 @@ > > + fBindTexture(LOCAL_GL_TEXTURE_2D, oldtex); > > + BindUserReadFBO(prevRead); > > + > > + // Make Shared Handle fully painted > > + GuaranteeResolve(); > > Yuk! Is this mandatory or do we have a path forward to remove this? May be > worth adding a comment in the code base to explain why this is required and > if it can be removed later a short description of what needs to be done. I just tested removing this on my One X; I get about 25-30% faster framerates (was actually hitting 35fps at one point!) with a 512x512 fishtank. I don't think I see any visual artifacts, but every once in a while I see the fix appear to go backwards for a frame or so.. not sure if that's caused by this. But, after about a minute, we got killed off with a SEGV due to low memory; might have some kind of a leak somewhere as well.

Ok, I see, more looks like swap is not really sync.

Ok, found one problem here: I'm pushing Shared EGLImages with textures to Parent compositor... for painting, and on destroy I'm trying to kill Front buffer using mContext from child thread. and problem is that Child thread GLContext already destroyed so I see 3 options here: 1) kill mTexture after EGLImage created, and instead of copy Texture to Texture(wrapped to EGL Image) do Render Texture to EGLImage directly... (so we don't have texture which is need to be deleted using original context) Cons: require going back to Save/Restore context option, 2) In ~BasicShadowableCanvasLayer or BasicShadowableCanvasLayer::DestroyBackBuffer, make sync call to Parent thread, and make sure that all Front buffers are destroyed before Childthread original glContext get's killed Cons: option require special IPDL Child->Parent hook, which will do cleanup ParentThread stuff 3) or we can delay Child thread GLContext destruction until all Front buffers are cleaned up properly...

Added lost change for extensions init (ctor -> Init) Added reference to GLContext in TextureWrapper. updated comment on ReleaseSharedhandle

We need guarantee resolve because it's not possible to be sure we don't need it. We thought we didn't need it on ANGLE w/d3d, but then we found a demo which exhibited artifacts from not finishing rendering before the 'result' is displayed. Also, don't be too worried about the maintainability of these new bindings, as there's going to be a refactoring which hits this code when I get our generalized sharing for multiple backends. Primarily, what we should worry about here is simplicity and correctness, and secondarily what speed we can get at minimal cost.

Comment on attachment 636895 [details] [diff] [review] EGLImage implementation. Review of attachment 636895 [details] [diff] [review]: ----------------------------------------------------------------- Close! ::: gfx/gl/GLContextProviderEGL.cpp @@ +671,5 @@ > + , mTexture(aTexture) > + , mEGLImage(nsnull) > + { > + } > + bool CreateEGLImage() { Add newline before function declaration. @@ +674,5 @@ > + } > + bool CreateEGLImage() { > + MOZ_ASSERT(!mEGLImage && mTexture && sEGLLibrary.HasKHRImageBase()); > + static const EGLint eglAttributes[] = { > + LOCAL_EGL_IMAGE_PRESERVED_KHR, LOCAL_EGL_FALSE, HasKHRImageBase() doesn't guarantee that LOCAL_EGL_IMAGE_PRESERVED (remove the _KHR) is supported. "if EGL_KHR_image is supported and EGL_KHR_image_base is not, the attribute EGL_IMAGE_PRESERVED_KHR is not accepted in <attrib_list>" Since the default is false, just don't specify it. @@ +710,5 @@ > + mEGLImage = nsnull; > + } > + mContext = nsnull; > + } > + GLuint GetTextureID() { More newlines, here and for the function below. @@ +728,5 @@ > + SharedTextureHandle aSharedHandle) > +{ > + if (aType != TextureImage::ThreadShared) > + return false; > + I recommend adding an assert in these functions that assume mShareWithEGLImage is true.

Added Detach Shared Handle for API consistency. also moved API doc in this patch

Added detach api with fImageTargetTexture2DOES(TARGET, 0) implementation

Comment on attachment 637577 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 637577 [details] [diff] [review]: ----------------------------------------------------------------- If there's no real reason for a function to fail (except for where everything is crazy/dead), don't bother returning a bool. However, if we should be returning a bool, we should always be checking it. UpdateSH and ReleaseSH seems like they shouldn't return bools, since there's no reason they should fail. For the cases where we currently return false, we should probably be asserting in DEBUG builds instead. ::: gfx/layers/basic/BasicLayers.cpp @@ +2808,5 @@ > { > + if (mBackBuffer.type() == SurfaceDescriptor::TSharedTextureDescriptor) { > + SharedTextureDescriptor handle = mBackBuffer.get_SharedTextureDescriptor(); > + if (mGLContext && handle.handle()) { > + mGLContext->ReleaseSharedHandle(handle.shareType(), handle.handle()); ReleaseSH without checking its return value again? @@ +2866,5 @@ > > + if (mGLContext && > + BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL) { > + TextureImage::TextureShareType flags; > + // if process type is default, then it is e10s withing same process Shouldn't this be ", then it is single-process (non-e10s)"? @@ +2880,5 @@ > + mBackBuffer = SharedTextureDescriptor(flags, handle, mBounds.Size()); > + } > + } > + if (handle) { > + mGLContext->UpdateSharedHandle(flags, handle); UpdateSH without checking the return value? ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +361,5 @@ > + gl()->fDeleteTextures(1, &mTexture); > + } > + if (IsValidSharedTexDescriptor(mFrontBufferDescriptor)) { > + SharedTextureDescriptor texDescriptor = mFrontBufferDescriptor.get_SharedTextureDescriptor(); > + gl()->ReleaseSharedHandle(texDescriptor.shareType(), texDescriptor.handle()); Why does ReleaseSH return a bool if we don't test it? @@ +428,5 @@ > + // Shared texture handle rendering path, single texture rendering > + SharedTextureDescriptor texDescriptor = mFrontBufferDescriptor.get_SharedTextureDescriptor(); > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + if (!gl()->AttachSharedHandle(texDescriptor.shareType(), texDescriptor.handle())) { If there's no performance hit to attach/detach every frame, this is fine. Otherwise, shouldn't it be relatively simple to only reattach when necessary? @@ +436,5 @@ > + gl()->ApplyFilterToBoundTexture(filter); > + program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), texDescriptor.size())); > + mOGLManager->BindAndDrawQuad(program, mNeedsYFlip); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, 0); > + if (!gl()->DetachSharedHandle(texDescriptor.shareType(), texDescriptor.handle())) { You're trying to detach from texture ID 0 here, since you bound GL_TEXTURE_2D back to 0 directly above. For EGLImage, you shouldn't even need to detach ever, I think. Just attach when you get an update, and leave it attached.

Comment on attachment 637579 [details] [diff] [review] EGLImage implementation. Review of attachment 637579 [details] [diff] [review]: ----------------------------------------------------------------- Should be the last r- for this from me. :) ::: gfx/gl/GLContextProviderEGL.cpp @@ +702,5 @@ > + // if we don't have a context (either real or shared), > + // then they went away when the contex was deleted, because it > + // was the only one that had access to it. > + if (ctx && !ctx->IsDestroyed() && ctx->MakeCurrent()) { > + ctx->MakeCurrent(); Surely we shouldn't need to MakeCurrent() twice. @@ +721,5 @@ > + const EGLImage GetEGLImage() { > + return mEGLImage; > + } > + > + nsRefPtr<GLContext> mContext; Is RefPtr really necessary/useful here? @@ +736,5 @@ > + > + NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime"); > + > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + GLuint prevRead = GetUserBoundReadFBO(); Needs a MakeCurrent().

> > + nsRefPtr<GLContext> mContext; > > Is RefPtr really necessary/useful here? yep, without it we have situation when GLContext last reference destroyed before last EGLWrapper... (Parent/child compositor threads, child destroying all refs and GLContext and only after that Parent thread wrapper destruction started)

Ok, I decided to remove return value from DetachSharedHandle and remove EGL implementation. We cannot cache sEGLLibrary.fImageTargetTexture2DOES, because we don't know if some other place did same thing, like flash binder...

Fixed last comments, and removed DetachHandle implementation

Comment on attachment 639358 [details] [diff] [review] EGLImage implementation. Review of attachment 639358 [details] [diff] [review]: ----------------------------------------------------------------- I think we're actually good. :)

Comment on attachment 639357 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 639357 [details] [diff] [review]: ----------------------------------------------------------------- Only one logic problem and one comment issue, but otherwise would have been r+ from me. Don't upload the fix until bgirard has done his review, because I marked the beginning of parts I cannot review myself with 'bgirard?'. That is, the whole change block after the 'bgirard?' comments need bgirard review. :) As much of the rest as you can, too, BenWa, but especially those parts. ::: gfx/gl/GLContext.h @@ +801,5 @@ > + */ > + virtual void UpdateSharedHandle(TextureImage::TextureShareType aType, > + SharedTextureHandle aSharedHandle) { } > + /** > + * After call returns true SharedHandle may not be used. This no longer applies after removing the retval. ::: gfx/layers/basic/BasicCanvasLayer.cpp @@ +312,5 @@ > } > > void DestroyBackBuffer() > { > + if (mBackBuffer.type() == SurfaceDescriptor::TSharedTextureDescriptor) { bgirard? @@ +370,5 @@ > BasicCanvasLayer::Paint(aContext, aMaskLayer); > return; > } > > + if (mGLContext && bgirard? ::: gfx/layers/ipc/ShadowLayerUtils.h @@ +65,5 @@ > }; > #endif // !defined(MOZ_HAVE_XSURFACEDESCRIPTOR) > > +template<> > +struct ParamTraits<mozilla::gl::TextureImage::TextureShareType> bgirard? ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +423,5 @@ > program->SetRenderOffset(aOffset); > program->SetTextureUnit(0); > program->LoadMask(GetMaskLayer()); > > + if (IsValidSharedTexDescriptor(mFrontBufferDescriptor)) { bgirard? @@ +426,5 @@ > > + if (IsValidSharedTexDescriptor(mFrontBufferDescriptor)) { > + // Shared texture handle rendering path, single texture rendering > + SharedTextureDescriptor texDescriptor = mFrontBufferDescriptor.get_SharedTextureDescriptor(); > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); This doesn't get reset, but I presume it doesn't matter, since this is in the compositor. bgirard? @@ +436,5 @@ > + gl()->ApplyFilterToBoundTexture(filter); > + program->SetLayerQuadRect(nsIntRect(nsIntPoint(0, 0), texDescriptor.size())); > + mOGLManager->BindAndDrawQuad(program, mNeedsYFlip); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, 0); > + gl()->DetachSharedHandle(texDescriptor.shareType(), texDescriptor.handle()); Reverse the above two lines.

Fixed last jgilbert comments

(In reply to Oleg Romashin (:romaxa) from comment #130) > Created attachment 639566 [details] [diff] [review] > Public shared texture API + Canvas impl > > Fixed last jgilbert comments Ugh, I asked you not to so yet. Oh well. BenWa, are you able to understand what I meant without the markings on the splinter review UI?

A warning: Right now, it looks like it's possible for an Update to happen content-side while the texture is being sampled for compositing. This is unlikely, but when it happens, it could cause artifacts. I think these artifacts will be limitted to temporal aliasing, though, which is probably ok, for now.

Comment on attachment 639358 [details] [diff] [review] EGLImage implementation. Review of attachment 639358 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, except for one issue -- the onwership of mTexture in EGLTextureWrap. I think you should just get rid of the fDeleteTextures bit and document that the ownership of the texture should be managed by whoever creates the texture wrapper, and also explicitly call DeleteTextures() if wrapper creation fails. ::: gfx/gl/GLContextProviderEGL.cpp @@ +672,5 @@ > + }; > + mContext->MakeCurrent(); > + GLContextEGL* ctx = static_cast<GLContextEGL*>(mContext.get()); > + mEGLImage = sEGLLibrary.fCreateImage(EGL_DISPLAY(), ctx->Context(), LOCAL_EGL_GL_TEXTURE_2D, > + (EGLClientBuffer)mTexture, eglAttributes); Do we need this MakeCurrent()? eglCreateImage doesn't depend on a current context; one is explicitly passed in to identify the owner of mTexture. It probably doesn't hurt, since that context is likely current anyway, but still. @@ +693,5 @@ > + // then they went away when the contex was deleted, because it > + // was the only one that had access to it. > + if (ctx && !ctx->IsDestroyed() && ctx->MakeCurrent()) { > + ctx->fDeleteTextures(1, &mTexture); > + } OK, I admit I'm still confused about the ownership of mTexture. The texture ID is passed in to EGLTextureWrapper -- why is it doing the deletion at all? It seems like the API should either require the lifetime of the texture to be managed externally to this (it's safe to delete it whenever, since if the EGLImage is alive it'll stay alive internally until the EGLImage and any attachements are deleted -- see EGL_KHR_image_base section 2.5.2) OR create its own texture in CreateEGLImage, after which it can be queried by GetTextureID(), and do the deletion here. I think the first option is much cleaner, and also means we don't have to do any complicated ctx munging here. @@ +727,5 @@ > + > + NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime"); > + > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + MakeCurrent(); Do we need this MakeCurrent? The other Shared Handle API calls assume that the context is current (e.g. AttachSharedHandle). We should probably just remove the MakeCurrent and update the docs in GLContext.h to say that the expectation is that the context is current when these calls are made. @@ +733,5 @@ > + GLint oldtex = -1; > + BindUserReadFBO(0); > + fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex); > + MOZ_ASSERT(oldtex != -1); > + fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID()); I'd like some documentation to say what's happening here.. something like: // We need to copy the current GLContext drawing buffer to the texture // exported by the EGLImage. Need to save both the read FBO and the texture // binding, because we're going to munge them to do this. Also note that there's another implementation possibility -- we create *two* EGLImages, for two textures, and bounce between them as long as the WebGL preserve backbuffer context flag is false. That gives us double buffering with no copying, and should be a pretty big perf improvement... it'll require changing the framebuffer attachments on each frame, but I think that should be cheaper than doing a copy. We should file a bug on doing something like this, unless it's already covered in bug 716859. @@ +746,5 @@ > + BindUserReadFBO(prevRead); > + > + // Make Shared Handle fully resolved in order to > + // guarantee content ready to draw in different thread GLContext > + GuaranteeResolve(); Gah, I wish we could avoid this GuaranteeResolve(), but we can't yet. @@ +768,5 @@ > + NS_ERROR("EGLImage creation for EGLTextureWrapper failed"); > + delete tex; > + // Stop trying to create shared image Handle > + mShareWithEGLImage = false; > + return nsnull; More lifetime weirdness with 'texture'. That "delete tex" will end up implicitly deleting the texture that was passed in, which is just weird -- it looks like a texture leak here at first inspection; we should stop doing the deletion in there and just call fDeleteTextures() here if EGL image creation fails. @@ +797,5 @@ > + > + NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime"); > + > + EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle; > + sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, wrap->GetEGLImage()); This is assuming that the context is current -- totally fine, but the docs for AttachSharedHandle should mention this.

Comment on attachment 639566 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 639566 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +430,5 @@ > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); > + gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + if (!gl()->AttachSharedHandle(texDescriptor.shareType(), texDescriptor.handle())) { > + NS_ERROR("Failed to attach shared texture handle"); > + return; Hmm. This AttachSharedHandle only needs to be done once to attach the EGLImage to mTexture -- the attachment to mTexture stays valid, for future calls we only need to call BindTexture(TEXTURE_2D, mTexture). I'm not sure how expensive the attachment itself is, but is there any reason why we Attach/Detach here constantly per draw? We can simply check if the texDescriptor handle is the same as what we had before, and if so, skip attaching. We shouldn't ever need to detach, either. Seems like we should work that in to the API somehow, since skipping this attach call is likely to be a perf win unless we get lucky and hit an internal optimization where GL sees that the right EGLImage is already attached to mTexture. (I know detach is a no-op with EGL Image, but attach isn't.)

Note: I'm happy to have the fixes/investigations for the above issues happen in future bugs.. no need to block landing on m-c.

Fixed some of vlad comments

Fixed some vlad comments

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #133) > // We need to copy the current GLContext drawing buffer to the texture > // exported by the EGLImage. Need to save both the read FBO and the texture > // binding, because we're going to munge them to do this. > > Also note that there's another implementation possibility -- we create *two* > EGLImages, for two textures, and bounce between them as long as the WebGL > preserve backbuffer context flag is false. That gives us double buffering > with no copying, and should be a pretty big perf improvement... it'll > require changing the framebuffer attachments on each frame, but I think that > should be cheaper than doing a copy. We should file a bug on doing > something like this, unless it's already covered in bug 716859. This is bug 716859. It's not going to hit the 16 train, so we're doing this bug for now. Double-buffering will hit 17 though.

Comment on attachment 639357 [details] [diff] [review] Public shared texture API + Canvas impl Review of attachment 639357 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicCanvasLayer.cpp @@ +393,5 @@ > + BasicManager()->PaintedCanvas(BasicManager()->Hold(this), > + mNeedsYFlip, > + mBackBuffer); > + // Move SharedTextureHandle ownership to ShadowLayer > + mBackBuffer = SurfaceDescriptor(); Why is this not using the new back buffer from the transaction swap? ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +426,5 @@ > > + if (IsValidSharedTexDescriptor(mFrontBufferDescriptor)) { > + // Shared texture handle rendering path, single texture rendering > + SharedTextureDescriptor texDescriptor = mFrontBufferDescriptor.get_SharedTextureDescriptor(); > + gl()->fActiveTexture(LOCAL_GL_TEXTURE0); Not sure what you mean by reset? It set the texture unit to 0.

Comment on attachment 639749 [details] [diff] [review] Public shared texture API + Canvas impl \o/

https://hg.mozilla.org/integration/mozilla-inbound/rev/255445a0a851 https://hg.mozilla.org/integration/mozilla-inbound/rev/153e82923805 https://hg.mozilla.org/integration/mozilla-inbound/rev/6087689a0745

\o/ \o/ \o/

Backed out for being the likely cause of bug 772405. This landed on top of bug 767064's webgl bustage, which made things harder to diagnose. To help in the future, please can you paste try run URLs in-bug (along the lines of https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound) - thank you :-) https://hg.mozilla.org/integration/mozilla-inbound/rev/04dc0717dd53

I did try build before inbound but not sure did that hit 767064 also https://tbpl.mozilla.org/?tree=Try&rev=c8b2e62dcdcd

Ok, I've found some old try https://tbpl.mozilla.org/?tree=Try&rev=1519d7af7e29 where I see the same error https://tbpl.mozilla.org/php/getParsedLog.php?id=13032494&tree=Try&full=1#error2

Launched this page, full tests round https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html on Tegra3 Asus, 4.0.3, and video/image tex-image-and-sub-image-2d passes tests... Also launched these tests standalone - also pass.. sounds like it device specific and broken on device which is running tests... weird. Any ideas?

The tests all pass on the tegra board that we have in the toronto office as well. It's supposed to be exactly the same thing as the test slaves. So my only idea now is to do tryserver runs with more logging. Notice that in android mochitests, the ONLY output that gets logged is the strings passed to mochitest ok() and todo() functions. Otherwise we'd have plenty of info already.

On try: https://tbpl.mozilla.org/?tree=Try&rev=467cccb7f0d2 this tryserver run is based off 02b26fb307b4 which has Romaxa's patches but does not have Vlad's 16bit patches: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=02b26fb307b4

New try push: https://tbpl.mozilla.org/?tree=Try&rev=1c7405e72c01 The previous one failed locally due to ok() not being defined... hope this one works.

The previous try push had -u none </brownpaperbag> new try: https://tbpl.mozilla.org/?tree=Try&rev=053fa8c9e421

https://tbpl.mozilla.org/php/getParsedLog.php?id=13448893&tree=Try&full=1 - test run is orange but no test failures reported

So, I re-triggered a few times, and this is really weird. 5 runs: 1. https://tbpl.mozilla.org/php/getParsedLog.php?id=13448893&tree=Try&full=1 (Is the one mentioned in comment 151) this run reached this point in the WebGL tests: 46491 INFO TEST-INFO | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/programs/gl-get-active-attribute.html] (WebGL mochitest) Starting test page INFO | automation.py | Application ran for: 0:18:01.369796 INFO | automation.py | Reading PID log: /tmp/tmpPf77P1pidlog getting files in '/mnt/sdcard/tests/profile/minidumps/' WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected! Then the next log entries suggest that several system process have restarted, in particular there is this: D/AndroidRuntime( 939): >>>>>>>>>>>>>> AndroidRuntime START <<<<<<<<<<<<<< which suggests that the whole system got restarted (??) and there is no subsequent Mozilla-related lines, so this looks like a bad OOM. 2. https://tbpl.mozilla.org/php/getParsedLog.php?id=13450523&tree=Try&full=1 This one dies much earlier (doesn't get to the WebGL tests) on what looks like a typical infrastructure issue, so we can ignore it: 7900 INFO TEST-START | /tests/content/base/test/test_bug410229.html failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! Error receiving data from socket. cmd=ps ; err=[Errno 54] Connection reset by peer reconnecting socket unable to connect socket: [Errno 61] Connection refused 3. https://tbpl.mozilla.org/php/getParsedLog.php?id=13450328&tree=Try&full=1 This one passed! So I get to see all my logging, but as it passed, it's not interesting... 4. https://tbpl.mozilla.org/php/getParsedLog.php?id=13451042&tree=Try&full=1#error0 This one is a sort of timeout, but not the usual test timeout, it's a buildbot thing killing the browser: 49526 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/more/functions/drawArraysOutOfBounds.html] Test passed - testDrawArraysVBOMultiOutOfBounds 49527 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/more/functions/drawArraysOutOfBounds.html] Test command timed out: 2400 seconds without output, killing pid 48170 process killed by signal 9 program finished with exit code -1 elapsedTime=3478.961889 TinderboxPrint: mochitest-plain<br/><em class="testfail">T-FAIL</em> buildbot.slave.commands.TimeoutError: command timed out: 2400 seconds without output, killing pid 48170 Earlier, we have several instances of: failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log! This looks like an infrastructure issue... 5. https://tbpl.mozilla.org/php/getParsedLog.php?id=13453471&tree=Try&full=1 this one passed, like 3. ******************************************************************************** ******************************************************************************** So I haven't been able to reproduce at all the conformance/textures that I was trying to debug :-/ The other question is why is this patch apparently so prone to OOM or infrastructure issue; maybe there is a real issue here, like a memory leak.

I suggest that we re-review these patches to see if there might be a leak or otherwise abnormal memory usage. Are these 3 changesets keeping the tree in a green state? If yes, we could push to try with only the first, and with only the first two, to see where the trouble starts.

Try with 255445a0a851 (first patch only): https://tbpl.mozilla.org/?tree=Try&rev=d8cfb5b1a4bb Try with 153e82923805 (first and second patch only): https://tbpl.mozilla.org/?tree=Try&rev=4a1ee4c311e0 Try with 6087689a0745 (all three patches): https://tbpl.mozilla.org/?tree=Try&rev=b604b0ed474a

Are we sure that we haven't seen the same failures even before these patches?

"Better" than that: all 3 try runs in comment 154 are successful, after a few retriggers, aside from clearly unrelated infra issues in the 2nd one. -> I believe in witches -> Oleg: please rebase your patches against the latest green cset on inbound (they dont apply cleanly anymore), and push again to try, and retrigger M1 a few times.