Last Comment Bug 728524 - OMTC: Implement WebGL OGL texture sharing
: OMTC: Implement WebGL OGL texture sharing
Status: RESOLVED FIXED
webgl-next [k9o:p1:fx?] [games:p1][soft]
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: ARM Android
: P3 normal with 1 vote (vote)
: mozilla16
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on: 760675 762265 765388 766366 772405
Blocks: omtc gecko-games 749062 k9o-Hardware-Accel 774058 775548 honeycomb-flash 716859 756601 763098 767484 773071 774059 775220 775222 784867
  Show dependency treegraph
 
Reported: 2012-02-17 21:54 PST by Benoit Girard (:BenWa)
Modified: 2013-03-17 05:18 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+
fixed
fixed


Attachments
Share texture WIP (9.03 KB, patch)
2012-02-17 21:55 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Share texture WIP (7.90 KB, patch)
2012-02-17 21:59 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Share texture WIP (9.13 KB, patch)
2012-02-18 15:55 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Share texture WIP (Working) (11.55 KB, patch)
2012-02-20 07:31 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
KHRimage approach (11.37 KB, patch)
2012-04-22 23:39 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Working version with KHR images (20.32 KB, patch)
2012-04-24 19:37 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
bgirard: review-
Details | Diff | Splinter Review
Minor rework for TexImage filter setup (1.67 KB, patch)
2012-05-24 11:39 PDT, Oleg Romashin (:romaxa)
bgirard: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (15.09 KB, patch)
2012-05-24 16:36 PDT, Oleg Romashin (:romaxa)
joe: feedback+
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (16.47 KB, patch)
2012-05-29 22:52 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
EGLImage thread shared implementation (SGX tested) (5.68 KB, patch)
2012-05-29 23:00 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Double buffer with direct texture sharing, threads/android only (4.21 KB, patch)
2012-05-31 00:20 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl (13.34 KB, patch)
2012-05-31 18:26 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
GLTexture recycler with expiration tracking (6.74 KB, patch)
2012-05-31 18:28 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
EGL provider share WebGL textures with copy, waitless (3.92 KB, patch)
2012-05-31 18:30 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Attempt to use EGLImage sharing, (don't work) (5.64 KB, patch)
2012-06-01 00:34 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl (20.43 KB, patch)
2012-06-03 17:28 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
glEGLImageTargetRenderbufferStorageOES API (2.57 KB, patch)
2012-06-03 17:29 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
EGL Fence sync API (7.45 KB, patch)
2012-06-03 17:33 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
EGLImage attached to mOffscreent texture impl, without double buffer (6.56 KB, patch)
2012-06-03 17:40 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Attempt to sync access to EGLImage/Context (6.34 KB, patch)
2012-06-03 17:42 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl (18.18 KB, patch)
2012-06-04 17:46 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
GLTexture recycler with expiration tracking, basic texture version (7.46 KB, patch)
2012-06-04 17:47 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
EGL backend webgl sharing implementation (7.98 KB, patch)
2012-06-04 17:56 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl (20.61 KB, patch)
2012-06-19 16:19 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
EGLImage implementation. (6.26 KB, patch)
2012-06-19 16:21 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
EGLImage implementation. (7.83 KB, patch)
2012-06-20 10:00 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (20.73 KB, patch)
2012-06-20 18:29 PDT, Oleg Romashin (:romaxa)
vladimir: review+
Details | Diff | Splinter Review
EGLImage implementation. (9.44 KB, patch)
2012-06-20 18:31 PDT, Oleg Romashin (:romaxa)
vladimir: review+
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (20.90 KB, patch)
2012-06-21 13:04 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
EGLImage implementation. (9.38 KB, patch)
2012-06-21 13:05 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl (20.49 KB, patch)
2012-06-21 13:55 PDT, Oleg Romashin (:romaxa)
bgirard: review-
Details | Diff | Splinter Review
EGLImage implementation. (9.38 KB, patch)
2012-06-21 13:57 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (22.11 KB, patch)
2012-06-22 10:18 PDT, Oleg Romashin (:romaxa)
bgirard: review-
Details | Diff | Splinter Review
EGLImage implementation. (9.19 KB, patch)
2012-06-22 10:19 PDT, Oleg Romashin (:romaxa)
vladimir: review-
Details | Diff | Splinter Review
EGLImage implementation. (9.72 KB, patch)
2012-06-23 09:59 PDT, Oleg Romashin (:romaxa)
bgirard: review+
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (22.43 KB, patch)
2012-06-25 12:50 PDT, Oleg Romashin (:romaxa)
bgirard: review-
Details | Diff | Splinter Review
EGLImage implementation. (11.39 KB, patch)
2012-06-26 15:15 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
EGLImage implementation. (11.60 KB, patch)
2012-06-26 23:07 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl (24.05 KB, patch)
2012-06-28 10:29 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
EGLImage implementation. (9.61 KB, patch)
2012-06-28 10:30 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (24.27 KB, patch)
2012-07-05 08:41 PDT, Oleg Romashin (:romaxa)
jgilbert: review-
Details | Diff | Splinter Review
EGLImage implementation. (9.06 KB, patch)
2012-07-05 08:42 PDT, Oleg Romashin (:romaxa)
jgilbert: review+
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (24.21 KB, patch)
2012-07-05 20:08 PDT, Oleg Romashin (:romaxa)
jgilbert: review+
Details | Diff | Splinter Review
Public shared texture API + Canvas impl (24.34 KB, patch)
2012-07-06 11:54 PDT, Oleg Romashin (:romaxa)
bgirard: review+
Details | Diff | Splinter Review
EGLImage implementation. (9.46 KB, patch)
2012-07-06 11:54 PDT, Oleg Romashin (:romaxa)
bgirard: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
debug slave failure (6.94 KB, patch)
2012-07-11 14:32 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
Public shared texture API + Canvas impl - rebased (24.51 KB, patch)
2012-07-12 14:26 PDT, Oleg Romashin (:romaxa)
romaxa: review+
Details | Diff | Splinter Review
Webgl error logcat (2.56 KB, text/plain)
2012-07-13 14:21 PDT, Oleg Romashin (:romaxa)
no flags Details
Invalid operation FIX (2.98 KB, patch)
2012-07-13 15:04 PDT, Oleg Romashin (:romaxa)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Public shared texture API + Canvas impl - rebased (after gralloc push) (25.02 KB, patch)
2012-07-13 18:19 PDT, Oleg Romashin (:romaxa)
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Invalid operation FIX, force RGBA (3.07 KB, patch)
2012-07-14 23:28 PDT, Oleg Romashin (:romaxa)
jgilbert: review+
Details | Diff | Splinter Review
Modified version of shared handle implementation for Beta (7.87 KB, patch)
2012-07-24 13:46 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
romaxa: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-02-17 21:54:37 PST

    
Comment 1 Benoit Girard (:BenWa) 2012-02-17 21:55:58 PST
Created attachment 598493 [details] [diff] [review]
Share texture WIP

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.
Comment 2 Benoit Girard (:BenWa) 2012-02-17 21:59:03 PST
Created attachment 598495 [details] [diff] [review]
Share texture WIP

Accidentally stole credit for cjones' work when copying headers ;)
Comment 3 Benoit Girard (:BenWa) 2012-02-18 15:55:24 PST
Created attachment 598592 [details] [diff] [review]
Share texture WIP

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.
Comment 4 Benoit Girard (:BenWa) 2012-02-20 07:31:13 PST
Created attachment 598869 [details] [diff] [review]
Share texture WIP (Working)

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.
Comment 5 Jeff Gilbert [:jgilbert] 2012-02-24 11:09:51 PST
The attachment seems to be missing the new CPP file, btw.
Comment 6 Oleg Romashin (:romaxa) 2012-04-22 23:39:28 PDT
Created attachment 617400 [details] [diff] [review]
KHRimage approach

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 7 Oleg Romashin (:romaxa) 2012-04-23 10:00:16 PDT
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 8 Ali Juma [:ajuma] 2012-04-23 10:40:58 PDT
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 9 Benoit Girard (:BenWa) 2012-04-23 11:09:10 PDT
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.
Comment 10 Jeff Gilbert [:jgilbert] 2012-04-24 03:01:23 PDT
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.
Comment 11 Oleg Romashin (:romaxa) 2012-04-24 03:18:29 PDT
EGLstream seems not widely available yet.
EGLImageKHR we could probably use fence sync extension
Comment 12 Jeff Gilbert [:jgilbert] 2012-04-24 07:49:29 PDT
(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.
Comment 13 Patrick Walton (:pcwalton) 2012-04-24 07:51:29 PDT
(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.
Comment 14 Jeff Gilbert [:jgilbert] 2012-04-24 07:56:37 PDT
(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.
Comment 15 Oleg Romashin (:romaxa) 2012-04-24 19:37:11 PDT
Created attachment 618139 [details] [diff] [review]
Working version with KHR images

Ok, this solution use simple glFinish for sync KHR image, works just perfect on my look.
Comment 16 Jeff Gilbert [:jgilbert] 2012-04-25 04:32:15 PDT
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*.
Comment 17 Jeff Gilbert [:jgilbert] 2012-04-25 04:40:41 PDT
Filed bug 748717 for cleaning up of extension suffixes.
Comment 18 Benoit Girard (:BenWa) 2012-05-01 11:06:21 PDT
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.
Comment 19 Oleg Romashin (:romaxa) 2012-05-24 11:39:39 PDT
Created attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup
Comment 20 Oleg Romashin (:romaxa) 2012-05-24 16:36:16 PDT
Created attachment 627023 [details] [diff] [review]
Public shared texture API + Canvas impl

Not sure do I need to make CreateTextureImage from Shared Handle or not
Comment 21 Benoit Girard (:BenWa) 2012-05-25 12:56:55 PDT
Comment on attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup

I like :)
Comment 22 Joe Drew (not getting mail) 2012-05-28 15:00:20 PDT
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.
Comment 23 Oleg Romashin (:romaxa) 2012-05-29 22:52:40 PDT
Created attachment 628239 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed style nits.
Comment 24 Oleg Romashin (:romaxa) 2012-05-29 23:00:51 PDT
Created attachment 628242 [details] [diff] [review]
EGLImage thread shared implementation (SGX tested)

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.
Comment 25 Oleg Romashin (:romaxa) 2012-05-31 00:20:37 PDT
Created attachment 628616 [details] [diff] [review]
Double buffer with direct texture sharing, threads/android only

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
Comment 26 Oleg Romashin (:romaxa) 2012-05-31 18:26:26 PDT
Created attachment 629021 [details] [diff] [review]
Public shared texture API + Canvas impl

Added ReleaseSharedHandle API, in order to notify that texture can be reused or destroyed
Comment 27 Oleg Romashin (:romaxa) 2012-05-31 18:28:11 PDT
Created attachment 629022 [details] [diff] [review]
GLTexture recycler with expiration tracking

Kinda texture stream implementation, which allow to track double/triple buffering case and destroy unused textures
Comment 28 Oleg Romashin (:romaxa) 2012-05-31 18:30:00 PDT
Created attachment 629023 [details] [diff] [review]
EGL provider share WebGL textures with copy, waitless

Waitless version for android, use double/triple buffering with direct textures sharing across threads
Comment 29 Oleg Romashin (:romaxa) 2012-05-31 18:34:27 PDT
Hope try build will not fail
https://tbpl.mozilla.org/?tree=Try&rev=c742601afd76
Comment 30 Oleg Romashin (:romaxa) 2012-06-01 00:09:58 PDT
Working build, works fine:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/romaxa@gmail.com-a43a102fb9d8/try-android
Comment 31 Oleg Romashin (:romaxa) 2012-06-01 00:34:24 PDT
Created attachment 629094 [details] [diff] [review]
Attempt to use EGLImage sharing, (don't work)

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
Comment 32 Benoit Jacob [:bjacob] (mostly away) 2012-06-01 05:39:15 PDT
Don't worry, given that not all Androids seem to be happy with EGLImage, I won't land Attachment 628916 [details] [diff] :-)
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2012-06-01 14:44:06 PDT
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.
Comment 34 Oleg Romashin (:romaxa) 2012-06-01 21:56:58 PDT
(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)
Comment 35 Oleg Romashin (:romaxa) 2012-06-02 14:59:53 PDT
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
Comment 36 Oleg Romashin (:romaxa) 2012-06-03 17:28:06 PDT
Created attachment 629670 [details] [diff] [review]
Public shared texture API + Canvas impl

Shared Handle API, added release,  and unbind API.
Added BindSharedRenderBuffer to GLContext in order to allow bind EGLImage to WebGL FBO using glEGLImageTargetRenderbufferStorageOES
Comment 37 Oleg Romashin (:romaxa) 2012-06-03 17:29:59 PDT
Created attachment 629671 [details] [diff] [review]
glEGLImageTargetRenderbufferStorageOES API
Comment 38 Oleg Romashin (:romaxa) 2012-06-03 17:33:16 PDT
Created attachment 629672 [details] [diff] [review]
EGL Fence sync API

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..
Comment 39 Oleg Romashin (:romaxa) 2012-06-03 17:40:48 PDT
Created attachment 629673 [details] [diff] [review]
EGLImage attached to mOffscreent texture impl, without double buffer

This basically works on Tegra3, but with flickering... no sync
Comment 40 Oleg Romashin (:romaxa) 2012-06-03 17:42:19 PDT
Created attachment 629674 [details] [diff] [review]
Attempt to sync access to EGLImage/Context

Tried to use FenceSync, mutex locking, got less flickering, but still not enough.
Comment 41 Benoit Jacob [:bjacob] (mostly away) 2012-06-04 14:48:12 PDT
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 42 Jeff Gilbert [:jgilbert] 2012-06-04 14:49:30 PDT
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.
Comment 43 Oleg Romashin (:romaxa) 2012-06-04 17:46:22 PDT
Created attachment 630015 [details] [diff] [review]
Public shared texture API + Canvas impl

Updated to latest Working version
Comment 44 Oleg Romashin (:romaxa) 2012-06-04 17:47:16 PDT
Created attachment 630016 [details] [diff] [review]
GLTexture recycler with expiration tracking, basic texture version
Comment 45 Oleg Romashin (:romaxa) 2012-06-04 17:56:38 PDT
Created attachment 630018 [details] [diff] [review]
EGL backend webgl sharing implementation

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.
Comment 46 Benoit Jacob [:bjacob] (mostly away) 2012-06-05 08:48:24 PDT
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)
Comment 47 Oleg Romashin (:romaxa) 2012-06-05 10:42:48 PDT
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)
Comment 48 Oleg Romashin (:romaxa) 2012-06-05 10:48:50 PDT
Ah, and default upstream build on that test Tegra3 device, - ~340ms
Comment 49 Oleg Romashin (:romaxa) 2012-06-06 01:42:59 PDT
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
Comment 50 Oleg Romashin (:romaxa) 2012-06-08 22:43:17 PDT
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
Comment 51 Oleg Romashin (:romaxa) 2012-06-09 02:33:25 PDT
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
Comment 52 Oleg Romashin (:romaxa) 2012-06-10 19:49:03 PDT
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
Comment 53 Oleg Romashin (:romaxa) 2012-06-12 16:47:16 PDT
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
Comment 54 Jeff Gilbert [:jgilbert] 2012-06-12 16:55:53 PDT
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.
Comment 55 Jeff Gilbert [:jgilbert] 2012-06-14 16:59:40 PDT
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.
Comment 56 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-15 05:56:33 PDT
Sounds good to me; if we stick with just the one fast path, that should simplify the patches a bit, right?
Comment 57 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-06-15 08:20:31 PDT
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.
Comment 58 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-15 08:38:32 PDT
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?)
Comment 59 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-06-15 08:41:04 PDT
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.
Comment 60 Oleg Romashin (:romaxa) 2012-06-19 16:19:33 PDT
Created attachment 634644 [details] [diff] [review]
Public shared texture API + Canvas impl

Added UpdateSharedHandle, which supposed to update SharedHandle content
Comment 61 Oleg Romashin (:romaxa) 2012-06-19 16:21:45 PDT
Created attachment 634647 [details] [diff] [review]
EGLImage implementation.

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.
Comment 62 Oleg Romashin (:romaxa) 2012-06-19 20:07:06 PDT
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
Comment 63 Oleg Romashin (:romaxa) 2012-06-20 10:00:47 PDT
Created attachment 634961 [details] [diff] [review]
EGLImage implementation.

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
Comment 64 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-20 13:10:21 PDT
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.
Comment 65 Oleg Romashin (:romaxa) 2012-06-20 13:49:36 PDT
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
Comment 66 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-20 13:56:07 PDT
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...
Comment 67 Jeff Gilbert [:jgilbert] 2012-06-20 15:02:51 PDT
(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 68 Jeff Gilbert [:jgilbert] 2012-06-20 15:52:30 PDT
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 69 Jeff Gilbert [:jgilbert] 2012-06-20 16:18:58 PDT
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.
Comment 70 Oleg Romashin (:romaxa) 2012-06-20 18:27:49 PDT
> 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
Comment 71 Mark Callow 2012-06-20 18:28:49 PDT
(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.
Comment 72 Oleg Romashin (:romaxa) 2012-06-20 18:29:46 PDT
Created attachment 635160 [details] [diff] [review]
Public shared texture API + Canvas impl
Comment 73 Oleg Romashin (:romaxa) 2012-06-20 18:31:03 PDT
Created attachment 635162 [details] [diff] [review]
EGLImage implementation.

Hope I did not miss anything
Comment 74 Jeff Gilbert [:jgilbert] 2012-06-20 18:49:37 PDT
(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.
Comment 75 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-20 20:00:09 PDT
(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...
Comment 76 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 10:48:02 PDT
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.
Comment 77 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 10:49:18 PDT
Uh, what, why did that stuff get reset?
Comment 78 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 12:21:49 PDT
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 79 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 12:23:39 PDT
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 80 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 12:34:14 PDT
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.
Comment 81 Oleg Romashin (:romaxa) 2012-06-21 12:54:49 PDT
> > +      mNeedsYFlip = needYFlip;
> > +      return;
> > +    } else {
> 
> Nit: no else after return

without it we will override *aNewBack with aNewFront at the end of function
Comment 82 Oleg Romashin (:romaxa) 2012-06-21 13:04:24 PDT
Created attachment 635429 [details] [diff] [review]
Public shared texture API + Canvas impl

Address vlad comments
Comment 83 Oleg Romashin (:romaxa) 2012-06-21 13:05:02 PDT
Created attachment 635430 [details] [diff] [review]
EGLImage implementation.

Addressed vlad's comments
Comment 84 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 13:18:02 PDT
(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 85 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 13:20:46 PDT
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 86 Oleg Romashin (:romaxa) 2012-06-21 13:55:11 PDT
Created attachment 635447 [details] [diff] [review]
Public shared texture API + Canvas impl
Comment 87 Oleg Romashin (:romaxa) 2012-06-21 13:57:01 PDT
Created attachment 635448 [details] [diff] [review]
EGLImage implementation.
Comment 88 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-21 17:54:27 PDT
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).
Comment 89 Oleg Romashin (:romaxa) 2012-06-21 19:33:55 PDT
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 90 Jeff Gilbert [:jgilbert] 2012-06-21 20:48:47 PDT
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 91 Benoit Girard (:BenWa) 2012-06-22 08:56:45 PDT
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 92 Benoit Girard (:BenWa) 2012-06-22 09:00:21 PDT
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
Comment 93 Oleg Romashin (:romaxa) 2012-06-22 09:24:22 PDT
jgilbert told that we should get rid of all postfixes...
Comment 94 Oleg Romashin (:romaxa) 2012-06-22 09:48:16 PDT
> > +    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
Comment 95 Benoit Girard (:BenWa) 2012-06-22 09:49:27 PDT
(In reply to Oleg Romashin (:romaxa) from comment #93)
> jgilbert told that we should get rid of all postfixes...

Fair enough
Comment 96 Oleg Romashin (:romaxa) 2012-06-22 10:18:39 PDT
Created attachment 635797 [details] [diff] [review]
Public shared texture API + Canvas impl
Comment 97 Oleg Romashin (:romaxa) 2012-06-22 10:19:09 PDT
Created attachment 635798 [details] [diff] [review]
EGLImage implementation.
Comment 98 Jeff Gilbert [:jgilbert] 2012-06-22 13:50:18 PDT
(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.
Comment 99 Jeff Gilbert [:jgilbert] 2012-06-22 21:05:01 PDT
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.
Comment 100 Jeff Gilbert [:jgilbert] 2012-06-22 21:05:29 PDT
(Ugh, really bugzilla?)
Comment 101 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-23 06:12:39 PDT
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).
Comment 102 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-23 06:13:20 PDT
(or just init mShareWithEGLImage to false, and move this assignment to Init)
Comment 103 Oleg Romashin (:romaxa) 2012-06-23 09:59:50 PDT
Created attachment 636096 [details] [diff] [review]
EGLImage implementation.

Oh, you right, I haven't had time yesterday to test it properly.
this one works, tested
Comment 104 Benoit Girard (:BenWa) 2012-06-25 08:09:19 PDT
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.
Comment 105 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-25 08:12:25 PDT
> ::: 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.
Comment 106 Benoit Girard (:BenWa) 2012-06-25 09:54:43 PDT
(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 107 Benoit Girard (:BenWa) 2012-06-25 10:06:46 PDT
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.
Comment 108 Benoit Girard (:BenWa) 2012-06-25 12:29:58 PDT
(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.
Comment 109 Oleg Romashin (:romaxa) 2012-06-25 12:50:22 PDT
Created attachment 636449 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed benwa comments, EGLBackend part will update after jgilbert review
Comment 110 Benoit Girard (:BenWa) 2012-06-25 13:16:49 PDT
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.
Comment 111 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-25 13:54:42 PDT
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
Comment 112 Oleg Romashin (:romaxa) 2012-06-25 17:35:26 PDT
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.
Comment 113 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-25 19:48:14 PDT
Interesting.. I'll grab a stack trace tomorrow, couldn't do it when I had the crash.
Comment 114 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-26 09:30:49 PDT
(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.
Comment 115 Oleg Romashin (:romaxa) 2012-06-26 10:10:41 PDT
Ok, I see, more looks like swap is not really sync.
Comment 116 Oleg Romashin (:romaxa) 2012-06-26 13:21:05 PDT
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...
Comment 117 Oleg Romashin (:romaxa) 2012-06-26 15:15:40 PDT
Created attachment 636895 [details] [diff] [review]
EGLImage implementation.

Added lost change for extensions init (ctor -> Init)
Added reference to GLContext in TextureWrapper.
updated comment on ReleaseSharedhandle
Comment 118 Jeff Gilbert [:jgilbert] 2012-06-26 15:32:36 PDT
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 119 Jeff Gilbert [:jgilbert] 2012-06-26 17:58:57 PDT
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.
Comment 120 Oleg Romashin (:romaxa) 2012-06-26 23:07:20 PDT
Created attachment 637005 [details] [diff] [review]
EGLImage implementation.
Comment 121 Oleg Romashin (:romaxa) 2012-06-28 10:29:44 PDT
Created attachment 637577 [details] [diff] [review]
Public shared texture API + Canvas impl

Added Detach Shared Handle for API consistency. also moved API doc in this patch
Comment 122 Oleg Romashin (:romaxa) 2012-06-28 10:30:57 PDT
Created attachment 637579 [details] [diff] [review]
EGLImage implementation.

Added detach api with fImageTargetTexture2DOES(TARGET, 0) implementation
Comment 123 Jeff Gilbert [:jgilbert] 2012-07-02 15:19:20 PDT
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 124 Jeff Gilbert [:jgilbert] 2012-07-02 15:21:06 PDT
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().
Comment 125 Oleg Romashin (:romaxa) 2012-07-02 17:43:58 PDT
> > +    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)
Comment 126 Oleg Romashin (:romaxa) 2012-07-05 08:41:12 PDT
Created attachment 639357 [details] [diff] [review]
Public shared texture API + Canvas impl

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...
Comment 127 Oleg Romashin (:romaxa) 2012-07-05 08:42:01 PDT
Created attachment 639358 [details] [diff] [review]
EGLImage implementation.

Fixed last comments, and removed DetachHandle implementation
Comment 128 Jeff Gilbert [:jgilbert] 2012-07-05 17:54:13 PDT
Comment on attachment 639358 [details] [diff] [review]
EGLImage implementation.

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

I think we're actually good. :)
Comment 129 Jeff Gilbert [:jgilbert] 2012-07-05 19:19:03 PDT
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.
Comment 130 Oleg Romashin (:romaxa) 2012-07-05 20:08:31 PDT
Created attachment 639566 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed last jgilbert comments
Comment 131 Jeff Gilbert [:jgilbert] 2012-07-05 20:27:03 PDT
(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?
Comment 132 Jeff Gilbert [:jgilbert] 2012-07-05 20:45:19 PDT
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 133 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-06 07:36:47 PDT
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 134 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-06 07:46:44 PDT
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.)
Comment 135 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-06 07:47:37 PDT
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.
Comment 136 Oleg Romashin (:romaxa) 2012-07-06 11:54:08 PDT
Created attachment 639749 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed some of vlad comments
Comment 137 Oleg Romashin (:romaxa) 2012-07-06 11:54:48 PDT
Created attachment 639750 [details] [diff] [review]
EGLImage implementation.

Fixed some vlad comments
Comment 138 Jeff Gilbert [:jgilbert] 2012-07-06 16:47:51 PDT
(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 139 Benoit Girard (:BenWa) 2012-07-09 13:37:50 PDT
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 140 Benoit Girard (:BenWa) 2012-07-09 13:49:42 PDT
Comment on attachment 639749 [details] [diff] [review]
Public shared texture API + Canvas impl

\o/
Comment 142 Benoit Jacob [:bjacob] (mostly away) 2012-07-09 21:58:34 PDT
\o/ \o/ \o/
Comment 143 Ed Morley [:emorley] 2012-07-10 08:10:10 PDT
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
Comment 144 Oleg Romashin (:romaxa) 2012-07-10 08:23:56 PDT
I did try build before inbound but not sure did that hit 767064 also
https://tbpl.mozilla.org/?tree=Try&rev=c8b2e62dcdcd
Comment 145 Oleg Romashin (:romaxa) 2012-07-10 09:05:13 PDT
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
Comment 146 Oleg Romashin (:romaxa) 2012-07-10 23:15:18 PDT
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?
Comment 147 Benoit Jacob [:bjacob] (mostly away) 2012-07-11 14:29:32 PDT
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.
Comment 148 Benoit Jacob [:bjacob] (mostly away) 2012-07-11 14:32:56 PDT
Created attachment 641203 [details] [diff] [review]
debug slave failure

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
Comment 149 Benoit Jacob [:bjacob] (mostly away) 2012-07-11 15:19:56 PDT
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.
Comment 150 Benoit Jacob [:bjacob] (mostly away) 2012-07-11 21:55:07 PDT
The previous try push had -u none </brownpaperbag>

new try:
https://tbpl.mozilla.org/?tree=Try&rev=053fa8c9e421
Comment 151 Oleg Romashin (:romaxa) 2012-07-12 02:06:51 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=13448893&tree=Try&full=1 - test run is orange but no test failures reported
Comment 152 Benoit Jacob [:bjacob] (mostly away) 2012-07-12 06:36:31 PDT
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.
Comment 153 Benoit Jacob [:bjacob] (mostly away) 2012-07-12 08:47:11 PDT
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.
Comment 154 Benoit Jacob [:bjacob] (mostly away) 2012-07-12 08:55:19 PDT
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
Comment 155 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-12 10:13:21 PDT
Are we sure that we haven't seen the same failures even before these patches?
Comment 156 Benoit Jacob [:bjacob] (mostly away) 2012-07-12 12:53:36 PDT
"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.
Comment 157 Oleg Romashin (:romaxa) 2012-07-12 14:26:24 PDT
Created attachment 641601 [details] [diff] [review]
Public shared texture API + Canvas impl - rebased
Comment 158 Oleg Romashin (:romaxa) 2012-07-12 14:29:04 PDT
pushed one more try with rebased version
https://tbpl.mozilla.org/?tree=Try&rev=09aa2750f9fb
Comment 159 Oleg Romashin (:romaxa) 2012-07-13 14:21:19 PDT
Created attachment 642052 [details]
Webgl error logcat
Comment 160 Oleg Romashin (:romaxa) 2012-07-13 14:25:36 PDT
If I add gl.GetError() right after this line:  http://mxr.mozilla.org/mozilla-central/source/content/canvas/test/webgl/conformance/textures/tex-image-and-sub-image-2d-with-image.html?force=1#70

then error is not visible on page test results, and output looks like this one:
http://pastebin.mozilla.org/1702891

some eerror 1282 error visible
Comment 161 Oleg Romashin (:romaxa) 2012-07-13 15:04:11 PDT
Created attachment 642079 [details] [diff] [review]
Invalid operation FIX
Comment 162 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-13 15:20:47 PDT
Can you describe what the error actually is?
Comment 163 Oleg Romashin (:romaxa) 2012-07-13 15:25:34 PDT
Invalid error is coming from CopyTexSubImage2D which does not understand BGRA format we are using for Offscreen Textures. IIUC
Comment 164 Oleg Romashin (:romaxa) 2012-07-13 18:19:56 PDT
Created attachment 642159 [details] [diff] [review]
Public shared texture API + Canvas impl - rebased (after gralloc push)
Comment 165 Oleg Romashin (:romaxa) 2012-07-14 12:59:10 PDT
Ok, with couple of re-triggers I got fully green try build
https://tbpl.mozilla.org/?tree=Try&rev=a14c70e6a894
Comment 166 Benoit Jacob [:bjacob] (mostly away) 2012-07-14 15:05:14 PDT
Comment on attachment 642079 [details] [diff] [review]
Invalid operation FIX

r- for the bool argument: ChooseGLFormats(fmt, true) is very non-explicit.

Given that this gets fixed, you only need review from jgilbert.
Comment 167 Oleg Romashin (:romaxa) 2012-07-14 16:58:06 PDT
passing format itself would be also not very nice... because it is only for alpha case.
Comment 168 Oleg Romashin (:romaxa) 2012-07-14 16:58:38 PDT
also this will go away with 565 fix.
Comment 169 Benoit Jacob [:bjacob] (mostly away) 2012-07-14 21:33:03 PDT
A generic way to fix this is to replace

  void MakeSandwich(bool WithMayo);

by

  enum MayoStatus {
    WithoutMayo,
    WithMayo
  };
  void MakeSandwich(MayoStatus mayoStatus);

Now, if you're confident that this will go away very soon, then OK for a bool arg if Jeff says r+.
Comment 170 Oleg Romashin (:romaxa) 2012-07-14 23:28:07 PDT
Created attachment 642339 [details] [diff] [review]
Invalid operation FIX, force RGBA
Comment 171 Jeff Gilbert [:jgilbert] 2012-07-15 03:15:37 PDT
Comment on attachment 642339 [details] [diff] [review]
Invalid operation FIX, force RGBA

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

The semantics of enabling/disabling forcing of RGBA (implicitly disabling our BGRA semi-hack) is a bit muddy, but getting this in is higher priority than a readability quibble. r=me if this fixes it, which it sounds like it did.

I would feel safer to default to RGBA, and only do BGRA when we need to eat readbacks. (follow-up bug forthcoming)

The relevant bits for why this fix is necessary:
We are trying to CopyTexSubImage from a framebuffer backed by a BGRA texture, into another BGRA texture. However, we're getting INVALID_OPERATION for this on (some?) platforms. The extension for BGRA doesn't add support for BGRA textures to CopyTexImage (non-Sub), so the suspect is that CopyTexSubImage is refusing to write into BGRA format, even though it has the requisite color channels, albeit in the wrong order.

Since this read-from-FB operation should function similarly (more or less what the spec says) to TexSubImage(destTex, ReadPixels(fb)), we should (with fairly high likelyhood) expect ReadPixels(bgra-texture-backed-fb) to work fine. CopyTex(Sub)Image will generate an INVALID_OPERATION if you do something like copy from an RGB FB to an RGBA tex, but work fine for RGBA-to-RGB 'truncation'. I think the driver in question is refusing to copy its RGBA (or BGRA) framebuffer data to a BGRA texture, because it thinks the types don't (or "shouldn't") align.

We should be on the lookout for the possibility of a driver which refuses to CopyTexSubImage from a BGRA-backed FB to an RGBA tex, though.


tl;dr: r=me
Comment 172 Jeff Gilbert [:jgilbert] 2012-07-15 04:27:56 PDT
Comment on attachment 639750 [details] [diff] [review]
EGLImage implementation.

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +805,5 @@
> +
> +    NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime");
> +
> +    EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
> +    sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, wrap->GetEGLImage());

|sEGLLibrary.fImageTargetTexture2DOES| should be |fImageTargetTexture2D|, see bug 774059. I thought this was just persistent bitrot for some reason.

Just a nit. If you don't fix it here, it'll be fixed in that bug. r+ from me still.
Comment 174 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 15:13:16 PDT
This is disabled for cross-process shadow layers, right?
Comment 175 Oleg Romashin (:romaxa) 2012-07-15 17:07:57 PDT
yep, it has ThreadShared check
Comment 176 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 11:14:03 PDT
Comment on attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup

[Approval Request Comment]
Required for bug 728524
Comment 177 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 11:14:26 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #176)
> Comment on attachment 626900 [details] [diff] [review]
> Minor rework for TexImage filter setup
> 
> [Approval Request Comment]
> Required for bug 728524

I mean bug 687267 of course
Comment 178 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 11:14:49 PDT
Comment on attachment 639750 [details] [diff] [review]
EGLImage implementation.

[Approval Request Comment]
Required by bug 687267
Comment 179 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 11:15:18 PDT
Comment on attachment 642159 [details] [diff] [review]
Public shared texture API + Canvas impl - rebased (after gralloc push)

[Approval Request Comment]
Required by bug 687267
Comment 180 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 11:16:01 PDT
Comment on attachment 642339 [details] [diff] [review]
Invalid operation FIX, force RGBA

Required by bug 687267
Comment 181 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 11:19:04 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #180)
> Comment on attachment 642339 [details] [diff] [review]
> Invalid operation FIX, force RGBA
> 
> Required by bug 687267

Looks like we actually want 02c4bf15eb59, not the patch attached here
Comment 182 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 12:03:54 PDT
Comment on attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup

Approving supporting patches for bug 687267
Comment 183 Dietrich Ayala (:dietrich) 2012-07-24 13:15:41 PDT
P1 for Games so soft-blocking.
Comment 184 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-07-24 13:46:35 PDT
Created attachment 645476 [details] [diff] [review]
Modified version of shared handle implementation for Beta

This is a modified version of the public shared texture API patch that removes the canvas changes. We are only including this for plugin support, and don't want to carry additional risk with the webgl changes.
Comment 185 Oleg Romashin (:romaxa) 2012-07-24 13:57:41 PDT
Comment on attachment 645476 [details] [diff] [review]
Modified version of shared handle implementation for Beta

Looks ok for me, as API structure should work

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