The default bug view has changed. See this FAQ.

OMTC: Implement WebGL OGL texture sharing

RESOLVED FIXED in Firefox 15

Status

()

Core
Graphics: Layers
P3
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: BenWa, Assigned: romaxa)

Tracking

(Blocks: 6 bugs)

unspecified
mozilla16
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox15+ fixed, firefox16 fixed)

Details

(Whiteboard: webgl-next [k9o:p1:fx?] [games:p1][soft])

Attachments

(7 attachments, 45 obsolete attachments)

1.67 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
9.46 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
6.94 KB, patch
Details | Diff | Splinter Review
2.56 KB, text/plain
Details
25.02 KB, patch
Details | Diff | Splinter Review
3.07 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
7.87 KB, patch
romaxa
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

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

Updated

5 years ago
Depends on: 716859
(Reporter)

Comment 2

5 years ago
Created attachment 598495 [details] [diff] [review]
Share texture WIP

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

Updated

5 years ago
Attachment #598493 - Attachment is obsolete: true
(Reporter)

Comment 3

5 years ago
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.
Assignee: nobody → bgirard
Attachment #598495 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Comment 4

5 years ago
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.
Keywords: fennecnative-betablocker
Priority: -- → P3
blocking-fennec1.0: --- → beta+
The attachment seems to be missing the new CPP file, btw.
blocking-fennec1.0: beta+ → ---
(Assignee)

Comment 6

5 years ago
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)
Attachment #617400 - Flags: feedback?(jgilbert)
Attachment #617400 - Flags: feedback?(ajuma)
(Assignee)

Comment 7

5 years ago
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
Attachment #617400 - Flags: feedback?(pwalton)

Comment 8

5 years ago
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.
Attachment #617400 - Flags: feedback?(ajuma)
(Reporter)

Comment 9

5 years ago
Comment on attachment 617400 [details] [diff] [review]
KHRimage approach

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

::: gfx/layers/opengl/CanvasLayerOGL.cpp
@@ +350,5 @@
>                             bool needYFlip,
>                             CanvasSurface* aNewBack)
>  {
>    if (!mDestroyed) {
>      nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(aNewFront);

You should check the aNewFront type rather then checking null.
EGLImage does not supply any synchronization guarantees, which it seems like is the goal of this bug. EGLStream should provides this functionality, but it is relatively new, though exciting. EGLStream even has support for A/V synchronization.

In the absence of EGLStream, if we're sharing between GL and GL, we should just be using our usual texture sharing path. If we don't want to block compositing on WebGL production, we'll need multibuffering.
(Assignee)

Comment 11

5 years ago
EGLstream seems not widely available yet.
EGLImageKHR we could probably use fence sync extension
(In reply to Oleg Romashin (:romaxa) from comment #11)
> EGLstream seems not widely available yet.
> EGLImageKHR we could probably use fence sync extension

I don't see a reason to use EGL prims if it can be done with just GL prims. EGLImage/EGLSurface+EGLSync isn't any easier than sharing textures+ARB_sync, and doesn't seem to give us anything new.
(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> I don't see a reason to use EGL prims if it can be done with just GL prims.
> EGLImage/EGLSurface+EGLSync isn't any easier than sharing textures+ARB_sync,
> and doesn't seem to give us anything new.

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

Oh, wow, gross. Ok, EGLImage and EGLSync is the way forward there, then.
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: webgl-next
(Assignee)

Comment 15

5 years ago
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.
Assignee: bgirard → romaxa
Attachment #598592 - Attachment is obsolete: true
Attachment #598869 - Attachment is obsolete: true
Attachment #617400 - Attachment is obsolete: true
Attachment #618139 - Flags: review?(jgilbert)
Attachment #618139 - Flags: review?(bgirard)
Attachment #617400 - Flags: feedback?(pwalton)
Attachment #617400 - Flags: feedback?(jgilbert)
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*.
Attachment #618139 - Flags: review?(jgilbert) → review-
Filed bug 748717 for cleaning up of extension suffixes.
(Reporter)

Updated

5 years ago
Blocks: 749062
(Reporter)

Comment 18

5 years ago
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.
Attachment #618139 - Flags: review?(bgirard) → review-

Updated

5 years ago
Blocks: 749721

Updated

5 years ago
Whiteboard: webgl-next → webgl-next [k9o:p1:fx?] [games:p1]

Updated

5 years ago
Blocks: 710398
(Reporter)

Updated

5 years ago
Blocks: 756601
No longer blocks: 598873
Blocks: 598873
(Assignee)

Comment 19

5 years ago
Created attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup
Attachment #618139 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #626900 - Flags: review?(bgirard)
(Assignee)

Comment 20

5 years ago
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
Attachment #627023 - Flags: feedback?(jones.chris.g)
Attachment #627023 - Flags: feedback?(joe)
(Reporter)

Comment 21

5 years ago
Comment on attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup

I like :)
Attachment #626900 - Flags: review?(bgirard) → review+
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.
Attachment #627023 - Flags: feedback?(joe) → feedback+
(Assignee)

Comment 23

5 years ago
Created attachment 628239 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed style nits.
(Assignee)

Comment 24

5 years ago
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.
Attachment #627023 - Attachment is obsolete: true
Attachment #627023 - Flags: feedback?(jones.chris.g)
Attachment #628242 - Flags: feedback?(jgilbert)
(Assignee)

Comment 25

5 years ago
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
Attachment #628242 - Attachment is obsolete: true
Attachment #628242 - Flags: feedback?(jgilbert)
(Assignee)

Comment 26

5 years ago
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
Attachment #628239 - Attachment is obsolete: true
Attachment #628616 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
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
(Assignee)

Comment 28

5 years ago
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
(Assignee)

Comment 29

5 years ago
Hope try build will not fail
https://tbpl.mozilla.org/?tree=Try&rev=c742601afd76
(Assignee)

Updated

5 years ago
Attachment #629021 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #629022 - Flags: feedback?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #629023 - Flags: feedback?(jgilbert)
(Assignee)

Comment 30

5 years ago
Working build, works fine:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/romaxa@gmail.com-a43a102fb9d8/try-android
(Assignee)

Comment 31

5 years ago
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
Don't worry, given that not all Androids seem to be happy with EGLImage, I won't land Attachment 628916 [details] [diff] :-)
Depends on: 760675
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.
(Assignee)

Comment 34

5 years ago
(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)
(Assignee)

Comment 35

5 years ago
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
(Assignee)

Comment 36

5 years ago
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
Attachment #629021 - Attachment is obsolete: true
Attachment #629021 - Flags: review?(jgilbert)
Attachment #629670 - Flags: review?(bjacob)
(Assignee)

Comment 37

5 years ago
Created attachment 629671 [details] [diff] [review]
glEGLImageTargetRenderbufferStorageOES API
Attachment #629671 - Flags: review?(bjacob)
(Assignee)

Comment 38

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

Comment 39

5 years ago
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
(Assignee)

Comment 40

5 years ago
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.
Heads up: Bug 760675 Comment 13 is useful information from NVIDIA about the merits of using EGLImage as opposed to a share group, even on devices and drivers that correctly support share groups.
Comment on attachment 629672 [details] [diff] [review]
EGL Fence sync API

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

I would like to throw this in, but the KHR suffixes have to go.
Attachment #629672 - Flags: review-
(Assignee)

Comment 43

5 years ago
Created attachment 630015 [details] [diff] [review]
Public shared texture API + Canvas impl

Updated to latest Working version
Attachment #629094 - Attachment is obsolete: true
Attachment #629670 - Attachment is obsolete: true
Attachment #629673 - Attachment is obsolete: true
Attachment #629674 - Attachment is obsolete: true
Attachment #629670 - Flags: review?(bjacob)
(Assignee)

Comment 44

5 years ago
Created attachment 630016 [details] [diff] [review]
GLTexture recycler with expiration tracking, basic texture version
Attachment #629022 - Attachment is obsolete: true
Attachment #629023 - Attachment is obsolete: true
Attachment #629671 - Attachment is obsolete: true
Attachment #629672 - Attachment is obsolete: true
Attachment #629022 - Flags: feedback?(jgilbert)
Attachment #629023 - Flags: feedback?(jgilbert)
Attachment #629671 - Flags: review?(bjacob)
Attachment #630016 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #630015 - Flags: review?(jgilbert)
(Assignee)

Comment 45

5 years ago
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.
Attachment #630018 - Flags: feedback?(jgilbert)
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)
(Assignee)

Comment 47

5 years ago
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)
(Assignee)

Comment 48

5 years ago
Ah, and default upstream build on that test Tegra3 device, - ~340ms
(Assignee)

Comment 49

5 years ago
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
(Assignee)

Comment 50

5 years ago
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
(Assignee)

Comment 51

5 years ago
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
Blocks: 763098
(Assignee)

Comment 52

5 years ago
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
(Assignee)

Comment 53

5 years ago
Just tested B2G on SGS2 with OMTC enabled and single buffer EGLImage sharing works fine for webgl, no fence sync required.
http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/clear-varying-color.html - does 18ms
Appearing to work without some sync mechanism is unacceptable. Things need to work for reasons. We had this problem before with ANGLE and expecting its glFlush to resolve.
I think we should move forward with the CopyTexImage-based 'copy-swap' with EGLImage for now. It should be relatively low-impact to add complexity-wise, and should give us significant improvement.

I want to do a more thorough refactoring for buffering and streaming shared 'textures' (GLTexture, EGLImage, EGLSurface, and d3d-interop textures) between content and compositor.

I'm going to reverse the dependencies for this bug and bug 716859.
Blocks: 716859
No longer depends on: 716859
Assignee: romaxa → nobody
Component: Graphics → Graphics: Layers
QA Contact: thebes → graphics-layers
Assignee: nobody → romaxa
Sounds good to me; if we stick with just the one fast path, that should simplify the patches a bit, right?
One other option here is to render to a Surface on the content side, and consume that on the compositor side with a SurfaceTexture. This is only supported on Android 4.0+, but it would be fairly simple to implement. Synchronization is automatically handled by the Surface, and I would expect performance to be stellar.
That's a pretty great idea; we should test performance, since that would be a good bit simpler.  We'd still need the other paths for pre-4.0 android (and non-android GLES), though.  I assume that we could just use GeckoAppShell.createSurface()?  That seems to create a SurfaceTextureLayer and adds to a list of plugin layers; do we care? (Or maybe we want that?)
Vlad, we don't want to use that awful plugin stuff. We can do it all in Gecko, either by invoking the Java Surface/SurfaceTexture stuff through JNI or dlopening the native versions.
(Assignee)

Updated

5 years ago
Depends on: 766366
(Assignee)

Comment 60

5 years ago
Created attachment 634644 [details] [diff] [review]
Public shared texture API + Canvas impl

Added UpdateSharedHandle, which supposed to update SharedHandle content
Attachment #630015 - Attachment is obsolete: true
Attachment #630016 - Attachment is obsolete: true
Attachment #630018 - Attachment is obsolete: true
Attachment #630015 - Flags: review?(jgilbert)
Attachment #630016 - Flags: review?(jgilbert)
Attachment #630018 - Flags: feedback?(jgilbert)
Attachment #634644 - Flags: review?(jgilbert)
(Assignee)

Comment 61

5 years ago
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.
Attachment #634647 - Flags: review?(jgilbert)
(Assignee)

Comment 62

5 years ago
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
(Assignee)

Comment 63

5 years ago
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
Attachment #634647 - Attachment is obsolete: true
Attachment #634647 - Flags: review?(jgilbert)
Attachment #634961 - Flags: review?(jgilbert)
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.
(Assignee)

Comment 65

5 years ago
yep, on Tegra 3 it definitely slower.... wonder if it is the same on other GPU's, like Mali, BCom... will check it on mali SGS2. b2g later
Note that even the CopyTexSubImage2D path needs to save/restore the active texture unit and any texture bound to that unit.

Also, why does EGLTextureWrapper need that mutex lock?

In playing with this, I just noticed that we're choosing an 8/8/8/8 rendering format for WebGL (default is alpha: true, and that benchmark doesn't specify).  Even if I set alpha:false, we end up choosing 8/8/8/0!  For the texture format we end up choosing GL_BGRA/GL_UNSIGNED_BYTE and GL_RGB/GL_UNSIGNED_BYTE respectively.  We really want 5/6/5 here...
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #64)
> Tested this with Fennec on a HTC One X (Tegra 3, ICS), using the clear
> benchmark: Nightly without any of these patches - 243ms; with
> CopyTexSubImage - 45 ms; with programmatic quad render - 20 ms.  Definitely
> scratching my head here as to why CopyTexSubImage is so much slower.

I'm guessing they just are overly careful about it, or something. It's not that common of a function, and it's even less common for it to be copying the whole texture, since usually we'd just use CopyTexImage for that. 

As to the 565 stuff, really we should follow bug 743182 and use the native depth of the display. 565, even if on many mobile devices, is not on all.

We shouldn't be using RGB if we have BGRA, since we should also have BGR. If we're not doing readback, that shouldn't matter though.
(Reporter)

Updated

5 years ago
Attachment #634644 - Flags: review?(bgirard)
(Reporter)

Updated

5 years ago
Attachment #634961 - Flags: review?(bgirard)
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?
Attachment #634644 - Flags: review?(jgilbert)
Attachment #634644 - Flags: review?(bgirard)
Attachment #634644 - Flags: review-
Attachment #634644 - Flags: review?(bgirard)
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.
Attachment #634961 - Flags: review?(jgilbert) → review-
Depends on: 762265, 765388
(Assignee)

Comment 70

5 years ago
> 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

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

Comment 72

5 years ago
Created attachment 635160 [details] [diff] [review]
Public shared texture API + Canvas impl
Attachment #634644 - Attachment is obsolete: true
Attachment #634644 - Flags: review?(bgirard)
Attachment #635160 - Flags: review?(jgilbert)
Attachment #635160 - Flags: review?(bgirard)
(Assignee)

Comment 73

5 years ago
Created attachment 635162 [details] [diff] [review]
EGLImage implementation.

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

Unfortunately, in order to be able to use CopyTexImage instead of CopyTexSubImage, we would need to recreate the EGLImage each frame, with preserve, which is not guaranteed to work. Again, though, this is to tide us over until we get no-copy-swap streaming. Simple is more important than absolute performance, for this segment.
(In reply to Mark Callow from comment #71)
> I have it on good authority (a.k.a. Qualcomm) that TexSubImage* can be very
> slow in tiled architectures. IIRC it has something to do with the way they
> layout the textures in memory; finding and changing the bits that need to
> change with TexSubImage* is time consuming. They told me it is often quicker
> to re-upload the whole texture with TexImage*. Presumably the same applies
> to CopyTex* as well.
> 
> That being said, as far as I know Tegra does not have a tiling architecture.

Right; I knew that was the case on tiling architectures, but I also thought that they optimized whole-image TexSubImage.  CopyTexImage would orphan the previous mipmap level, which we can't do here (since it's from an EGLImage).  And yeah, Tegra isn't tiled, which is what led me to start looking at formats -- my suspicion was that we're hitting some kind of unoptimized format conversion path that we manage to avoid/optimize if we do the quad render.  I'll look more into format stuff tomorrow...
If I modify things so that a true 5/6/5 buffer gets created, and modify the testcase to explicitly say alpha: false, then with CopyTexSubImage I get 16ms -- basically requestAnimationFrame resolution.  With the quad render I still get 16ms as well, because we're now hitting rAF resolution.  I changed the test to use postMessage with no delay; I get about 5ms with both.  (The quad render might have a slight edge, but hard to tell; the numbers basically range from 3-7ms for both.)

The changed testcases are at:

http://conduit.bitops.com/~vladimir/misc/clear-varying-color-noalpha.html
http://conduit.bitops.com/~vladimir/misc/clear-varying-color-noalpha-pm.html

(alpha: true by default is a huge performance trap on mobile, even after we fix the 565 business... since it'll force us to an 8888 context!  We should make sure we document this somewhere.)

However, I'm pretty excited about these patches, and we should land them as soon as we can -- we can fix the 565 stuff in a followup.
Assignee: romaxa → nobody
Component: Graphics: Layers → Graphics
QA Contact: graphics-layers → thebes
Uh, what, why did that stuff get reset?
Assignee: nobody → romaxa
Component: Graphics → Graphics: Layers
QA Contact: thebes → graphics-layers
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
Attachment #635160 - Flags: review+
Comment on attachment 635160 [details] [diff] [review]
Public shared texture API + Canvas impl

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +2882,5 @@
> +    }
> +    if (handle) {
> +      mGLContext->UpdateSharedHandle(flags, handle);
> +      // Make Shared Handle fully painted
> +      mGLContext->GuaranteeResolve();

Actually, one more comment -- why don't we make this part of UpdateSharedHandle?  That is, if it needs to do any kind of resolve, let it handle it directly, and have the docs just say that it'll be fully resolved and ready for use after it returns.  That way callers don't have to worry about calling GuaranteeResolve, and there may be sharing cases where a glFinish/glFlush isn't needed.
Comment on attachment 635162 [details] [diff] [review]
EGLImage implementation.

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

Looks good with one bug and some nits... but same as above applies for review from jglibert/benwa.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +711,5 @@
> +bool
> +GLContextEGL::UpdateSharedHandle(TextureImage::TextureShareType aType,
> +                                 SharedTextureHandle aSharedHandle)
> +{
> +    if (aType == TextureImage::ThreadShared) {

Here and elsewhere, cleaner to write:

if (aType != TextureImage::ThreadShared)
    return false;

instead of indenting the entire block

@@ +716,5 @@
> +        EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
> +        GLuint prevRead = GetUserBoundReadFBO();
> +        GLint oldtex, activetex;
> +        fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex);
> +        fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex);

Bug -- GL_TEXTURE_BINDING_2D needs to be queried after the ActiveTexture() call (otherwise oldtex is going to get the texture bound to the current target, which isn't necessarily TEXTURE0).

Note for a followup bug: any params that we're going to be modifying/querying often (e.g. the active texture, and the texture bound to 0) we should cache in GLContext so that we don't actually have to query it from GL.
Attachment #635162 - Flags: review+
(Assignee)

Comment 81

5 years ago
> > +      mNeedsYFlip = needYFlip;
> > +      return;
> > +    } else {
> 
> Nit: no else after return

without it we will override *aNewBack with aNewFront at the end of function
(Assignee)

Comment 82

5 years ago
Created attachment 635429 [details] [diff] [review]
Public shared texture API + Canvas impl

Address vlad comments
Attachment #635160 - Attachment is obsolete: true
Attachment #635160 - Flags: review?(jgilbert)
Attachment #635160 - Flags: review?(bgirard)
Attachment #635429 - Flags: review?(jgilbert)
Attachment #635429 - Flags: review?(bgirard)
(Assignee)

Comment 83

5 years ago
Created attachment 635430 [details] [diff] [review]
EGLImage implementation.

Addressed vlad's comments
Attachment #635162 - Attachment is obsolete: true
Attachment #635162 - Flags: review?(jgilbert)
Attachment #635162 - Flags: review?(bgirard)
Attachment #635430 - Flags: review?(jgilbert)
Attachment #635430 - Flags: review?(bgirard)
(In reply to Oleg Romashin (:romaxa) from comment #81)
> > > +      mNeedsYFlip = needYFlip;
> > > +      return;
> > > +    } else {
> > 
> > Nit: no else after return
> 
> without it we will override *aNewBack with aNewFront at the end of function

I mean:

if (foo) {
   ...
   return;
}
...morestuff...

instead of:
if (foo) {
   ...
   return;
} else {
   ...morestuff...
}

they're functionally equivalent; the former is cleaner and easier to read, though.
Comment on attachment 635430 [details] [diff] [review]
EGLImage implementation.


>+bool
>+GLContextEGL::UpdateSharedHandle(TextureImage::TextureShareType aType,
>+                                 SharedTextureHandle aSharedHandle)
>+{
>+    if (aType != TextureImage::ThreadShared)
>+        return false;
>+
>+    EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
>+    GLuint prevRead = GetUserBoundReadFBO();
>+    GLint oldtex, activetex;
>+    fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex);
>+    fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex);
>+    BindUserReadFBO(0);
>+    fActiveTexture(LOCAL_GL_TEXTURE0);
>+    fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID());

This bug wasn't addressed; this needs to be:

    GLint oldtex, activetex;
    BindUserReadFBO(0);
    fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex);
    fActiveTexture(LOCAL_GL_TEXTURE0);
    fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex);
    fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID());
(Assignee)

Comment 86

5 years ago
Created attachment 635447 [details] [diff] [review]
Public shared texture API + Canvas impl
Attachment #635429 - Attachment is obsolete: true
Attachment #635429 - Flags: review?(jgilbert)
Attachment #635429 - Flags: review?(bgirard)
Attachment #635447 - Flags: review?(jgilbert)
Attachment #635447 - Flags: review?(bgirard)
(Assignee)

Comment 87

5 years ago
Created attachment 635448 [details] [diff] [review]
EGLImage implementation.
Attachment #635430 - Attachment is obsolete: true
Attachment #635430 - Flags: review?(jgilbert)
Attachment #635430 - Flags: review?(bgirard)
Attachment #635448 - Flags: review?(jgilbert)
Attachment #635448 - Flags: review?(bgirard)
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).
(Assignee)

Comment 89

5 years ago
That is confused me too a little bit... but ok, I'll fix it in last version, let's get jgilbert and BenWa output first, so I can combine changes at the end
Comment on attachment 635448 [details] [diff] [review]
EGLImage implementation.

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +254,5 @@
>          , mBound(false)
>          , mIsPBuffer(false)
>          , mIsDoubleBuffered(false)
>          , mCanBindToTexture(false)
> +        , mShareWithEGLImage(sEGLLibrary.HasKHRImagePixmap() && sEGLLibrary.HasKHRImageTexture2D())

This is complicated and should probably be done outside the initializer list.
To know we support EGLImage stuff, we need:
EGL_KHR_image_base, EGL_KHR_gl_texture_2D_image, and GL_OES_EGL_image.
If EGL_IMAGE_PRESERVED is always false, then we can do:
(EGL_KHR_image_base || EGL_KHR_image), EGL_KHR_gl_texture_2D_image, and GL_OES_EGL_image.

Please use the extension query system added with bug 762265.

Also, please add a MOZ_ASSERT if mShareWithEGLImage is true if we're not on android or b2g.

@@ +595,5 @@
> +                                    SharedTextureHandle aSharedHandle);
> +    virtual bool ReleaseSharedHandle(TextureImage::TextureShareType aType,
> +                                     SharedTextureHandle aSharedHandle);
> +    virtual bool AttachSharedHandle(TextureImage::TextureShareType aType,
> +                                  SharedTextureHandle aSharedHandle);

Misaligned indent.

@@ +659,5 @@
> +class EGLTextureWrapper
> +{
> +public:
> +    EGLTextureWrapper(GLContext* aContext, GLuint aTexture)
> +      : mContext(aContext)

4-space indents.

@@ +665,5 @@
> +      , mEGLImage(nsnull)
> +    {
> +    }
> +    bool CreateEGLImage() {
> +        MOZ_ASSERT(!mEGLImage && mTexture && sEGLLibrary.HasKHRImagePixmap() && sEGLLibrary.HasKHRImageTexture2D());

See the list of extensions we need above. (though this function doesn't technically need GL_OES_EGL_image)

Also, you'll need to use the GLLibraryEGL::IsExtensionSupported for extension checks.

@@ +672,5 @@
> +            LOCAL_EGL_NONE
> +        };
> +        mContext->MakeCurrent();
> +        mContext->fActiveTexture(LOCAL_GL_TEXTURE0);
> +        mContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);

You shouldn't need to bind the texture, since we specify both |target| and |name| in eglCreateImage. You don't need to set the active texture, either.

@@ +677,5 @@
> +        GLContextEGL* ctx = static_cast<GLContextEGL*>(mContext);
> +        mEGLImage = sEGLLibrary.fCreateImage(EGL_DISPLAY(), ctx->Context(), LOCAL_EGL_GL_TEXTURE_2D,
> +                                             (EGLClientBuffer)mTexture, eglAttributes);
> +        if (!mEGLImage) {
> +            printf_stderr("Could not create EGL images: ERROR (0x%04x)\n", sEGLLibrary.fGetError());

Hide this printf behind if DebugMode().

@@ +713,5 @@
> +        return false;
> +
> +    EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
> +    GLuint prevRead = GetUserBoundReadFBO();
> +    GLint oldtex, activetex;

Please set these to -1 and MOZ_ASSERT that they aren't -1 after our calls to set them.

@@ +716,5 @@
> +    GLuint prevRead = GetUserBoundReadFBO();
> +    GLint oldtex, activetex;
> +    BindUserReadFBO(0);
> +    fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex);
> +    fActiveTexture(LOCAL_GL_TEXTURE0);

Since we don't need to use a particular texture unit (because we're not rendering), we can just use the current texture unit and restore it like normal. We shouldn't need to touch ActiveTexture at all.

@@ +719,5 @@
> +    fGetIntegerv(LOCAL_GL_ACTIVE_TEXTURE, &activetex);
> +    fActiveTexture(LOCAL_GL_TEXTURE0);
> +    fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex);
> +    fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID());
> +    // CopyTexSubImage2D, is ~2x slower than simple FBO render to texture with draw quads

Put a whitespace line between this comment and the preceeding code, to break up this block visually.

@@ +726,5 @@
> +                       0, 0, mOffscreenActualSize.width, mOffscreenActualSize.height);
> +    fActiveTexture(activetex);
> +    fBindTexture(LOCAL_GL_TEXTURE_2D, oldtex);
> +    BindUserReadFBO(prevRead);
> +    // Make Shared Handle fully painted

Another empty line before this comment too.

@@ +746,5 @@
> +    ContextFormat fmt = ActualFormat();
> +    CreateTextureForOffscreen(ChooseGLFormats(fmt), mOffscreenSize, texture);
> +    EGLTextureWrapper* tex = new EGLTextureWrapper(this, texture);
> +    if (!tex->CreateEGLImage()) {
> +        NS_WARNING("Unexpected error, EGLImage wrapper creation failed");

This should probably never happen, since we're checking for all the requisite exts and such. I'd make this an NS_ERROR.
Attachment #635448 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 91

5 years ago
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.
Attachment #635447 - Flags: review?(bgirard) → review-
(Reporter)

Comment 92

5 years ago
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
Attachment #635448 - Flags: review?(bgirard)
(Assignee)

Comment 93

5 years ago
jgilbert told that we should get rid of all postfixes...
(Assignee)

Comment 94

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

Comment 95

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

Fair enough
(Assignee)

Comment 96

5 years ago
Created attachment 635797 [details] [diff] [review]
Public shared texture API + Canvas impl
Attachment #635447 - Attachment is obsolete: true
Attachment #635447 - Flags: review?(jgilbert)
Attachment #635797 - Flags: review?(jgilbert)
Attachment #635797 - Flags: review?(bgirard)
(Assignee)

Comment 97

5 years ago
Created attachment 635798 [details] [diff] [review]
EGLImage implementation.
Attachment #635448 - Attachment is obsolete: true
Attachment #635798 - Flags: review?(jgilbert)
Attachment #635798 - Flags: review?(bgirard)
blocking-basecamp: --- → ?
Blocks: 767484
(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.
blocking-basecamp: ? → ---
blocking-basecamp: --- → ?
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.
blocking-basecamp: ? → ---
(Ugh, really bugzilla?)
blocking-basecamp: --- → ?
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).
Attachment #635798 - Flags: review-
(or just init mShareWithEGLImage to false, and move this assignment to Init)
(Assignee)

Comment 103

5 years ago
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
Attachment #635798 - Attachment is obsolete: true
Attachment #635798 - Flags: review?(jgilbert)
Attachment #635798 - Flags: review?(bgirard)
Attachment #636096 - Flags: review?(jgilbert)
Attachment #636096 - Flags: review?(bgirard)
(Reporter)

Comment 104

5 years ago
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.
Attachment #635797 - Flags: review?(bgirard) → review-
> ::: 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.
(Reporter)

Comment 106

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

Comment 107

5 years ago
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.
Attachment #636096 - Flags: review?(bgirard) → review+
(Reporter)

Comment 108

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

Comment 109

5 years ago
Created attachment 636449 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed benwa comments, EGLBackend part will update after jgilbert review
Attachment #635797 - Attachment is obsolete: true
Attachment #635797 - Flags: review?(jgilbert)
Attachment #636449 - Flags: review?(jgilbert)
Attachment #636449 - Flags: review?(bgirard)
(Reporter)

Comment 110

5 years ago
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.
Attachment #636449 - Flags: review?(bgirard) → review-
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
(Assignee)

Comment 112

5 years ago
Hmm, interesting, I guess I did that test, and did not have any crashes...
But thank you I'll try perform these steps, and make sure that it does not crash.
Interesting.. I'll grab a stack trace tomorrow, couldn't do it when I had the crash.
(In reply to Benoit Girard (:BenWa) from comment #107)
> @@ +736,5 @@
> > +    fBindTexture(LOCAL_GL_TEXTURE_2D, oldtex);
> > +    BindUserReadFBO(prevRead);
> > +
> > +    // Make Shared Handle fully painted
> > +    GuaranteeResolve();
> 
> Yuk! Is this mandatory or do we have a path forward to remove this? May be
> worth adding a comment in the code base to explain why this is required and
> if it can be removed later a short description of what needs to be done.

I just tested removing this on my One X; I get about 25-30% faster framerates (was actually hitting 35fps at one point!) with a 512x512 fishtank.  I don't think I see any visual artifacts, but every once in a while I see the fix appear to go backwards for a frame or so.. not sure if that's caused by this.

But, after about a minute, we got killed off with a SEGV due to low memory; might have some kind of a leak somewhere as well.
blocking-basecamp: ? → ---
(Assignee)

Comment 115

5 years ago
Ok, I see, more looks like swap is not really sync.
(Assignee)

Comment 116

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

Comment 117

5 years ago
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
Attachment #636096 - Attachment is obsolete: true
Attachment #636096 - Flags: review?(jgilbert)
Attachment #636895 - Flags: review?(jgilbert)
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.
blocking-basecamp: --- → ?
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.
Attachment #636895 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 120

5 years ago
Created attachment 637005 [details] [diff] [review]
EGLImage implementation.
Attachment #636895 - Attachment is obsolete: true
Attachment #637005 - Flags: review?(jgilbert)
(Assignee)

Comment 121

5 years ago
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
Attachment #636449 - Attachment is obsolete: true
Attachment #636449 - Flags: review?(jgilbert)
Attachment #637577 - Flags: review?(jgilbert)
Attachment #637577 - Flags: review?(bgirard)
(Assignee)

Comment 122

5 years ago
Created attachment 637579 [details] [diff] [review]
EGLImage implementation.

Added detach api with fImageTargetTexture2DOES(TARGET, 0) implementation
Attachment #637005 - Attachment is obsolete: true
Attachment #637005 - Flags: review?(jgilbert)
Attachment #637579 - Flags: review?(jgilbert)
Attachment #637579 - Flags: review?(bgirard)
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.
Attachment #637577 - Flags: review?(jgilbert) → review-
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().
Attachment #637579 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 125

5 years ago
> > +    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)
Blocks: 687267
(Assignee)

Comment 126

5 years ago
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...
Attachment #637577 - Attachment is obsolete: true
Attachment #637577 - Flags: review?(bgirard)
Attachment #639357 - Flags: review?(jgilbert)
(Assignee)

Comment 127

5 years ago
Created attachment 639358 [details] [diff] [review]
EGLImage implementation.

Fixed last comments, and removed DetachHandle implementation
Attachment #637579 - Attachment is obsolete: true
Attachment #637579 - Flags: review?(bgirard)
Attachment #639358 - Flags: review?(jgilbert)
Comment on attachment 639358 [details] [diff] [review]
EGLImage implementation.

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

I think we're actually good. :)
Attachment #639358 - Flags: review?(jgilbert) → review+
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.
Attachment #639357 - Flags: review?(jgilbert)
Attachment #639357 - Flags: review?(bgirard)
Attachment #639357 - Flags: review-
(Assignee)

Comment 130

5 years ago
Created attachment 639566 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed last jgilbert comments
Attachment #639357 - Attachment is obsolete: true
Attachment #639357 - Flags: review?(bgirard)
Attachment #639566 - Flags: review?(bgirard)
(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?
Attachment #639566 - Flags: review+
A warning: Right now, it looks like it's possible for an Update to happen content-side while the texture is being sampled for compositing. This is unlikely, but when it happens, it could cause artifacts. I think these artifacts will be limitted to temporal aliasing, though, which is probably ok, for now.
Comment on attachment 639358 [details] [diff] [review]
EGLImage implementation.

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

Looks fine to me, except for one issue -- the onwership of mTexture in EGLTextureWrap.  I think you should just get rid of the fDeleteTextures bit and document that the ownership of the texture should be managed by whoever creates the texture wrapper, and also explicitly call DeleteTextures() if wrapper creation fails.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +672,5 @@
> +        };
> +        mContext->MakeCurrent();
> +        GLContextEGL* ctx = static_cast<GLContextEGL*>(mContext.get());
> +        mEGLImage = sEGLLibrary.fCreateImage(EGL_DISPLAY(), ctx->Context(), LOCAL_EGL_GL_TEXTURE_2D,
> +                                             (EGLClientBuffer)mTexture, eglAttributes);

Do we need this MakeCurrent()?  eglCreateImage doesn't depend on a current context; one is explicitly passed in to identify the owner of mTexture.  It probably doesn't hurt, since that context is likely current anyway, but still.

@@ +693,5 @@
> +        // then they went away when the contex was deleted, because it
> +        // was the only one that had access to it.
> +        if (ctx && !ctx->IsDestroyed() && ctx->MakeCurrent()) {
> +            ctx->fDeleteTextures(1, &mTexture);
> +        }

OK, I admit I'm still confused about the ownership of mTexture.  The texture ID is passed in to EGLTextureWrapper -- why is it doing the deletion at all?  It seems like the API should either require the lifetime of the texture to be managed externally to this (it's safe to delete it whenever, since if the EGLImage is alive it'll stay alive internally until the EGLImage and any attachements are deleted -- see EGL_KHR_image_base section 2.5.2) OR create its own texture in CreateEGLImage, after which it can be queried by GetTextureID(), and do the deletion here.  I think the first option is much cleaner, and also means we don't have to do any complicated ctx munging here.

@@ +727,5 @@
> +
> +    NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime");
> +
> +    EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
> +    MakeCurrent();

Do we need this MakeCurrent?  The other Shared Handle API calls assume that the context is current (e.g. AttachSharedHandle).  We should probably just remove the MakeCurrent and update the docs in GLContext.h to say that the expectation is that the context is current when these calls are made.

@@ +733,5 @@
> +    GLint oldtex = -1;
> +    BindUserReadFBO(0);
> +    fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex);
> +    MOZ_ASSERT(oldtex != -1);
> +    fBindTexture(LOCAL_GL_TEXTURE_2D, wrap->GetTextureID());

I'd like some documentation to say what's happening here.. something like:

// We need to copy the current GLContext drawing buffer to the texture
// exported by the EGLImage.  Need to save both the read FBO and the texture
// binding, because we're going to munge them to do this.

Also note that there's another implementation possibility -- we create *two* EGLImages, for two textures, and bounce between them as long as the WebGL preserve backbuffer context flag is false.  That gives us double buffering with no copying, and should be a pretty big perf improvement... it'll require changing the framebuffer attachments on each frame, but I think that should be cheaper than doing a copy.  We should file a bug on doing something like this, unless it's already covered in bug 716859.

@@ +746,5 @@
> +    BindUserReadFBO(prevRead);
> +
> +    // Make Shared Handle fully resolved in order to
> +    // guarantee content ready to draw in different thread GLContext
> +    GuaranteeResolve();

Gah, I wish we could avoid this GuaranteeResolve(), but we can't yet.

@@ +768,5 @@
> +        NS_ERROR("EGLImage creation for EGLTextureWrapper failed");
> +        delete tex;
> +        // Stop trying to create shared image Handle
> +        mShareWithEGLImage = false;
> +        return nsnull;

More lifetime weirdness with 'texture'.  That "delete tex" will end up implicitly deleting the texture that was passed in, which is just weird -- it looks like a texture leak here at first inspection; we should stop doing the deletion in there and just call fDeleteTextures() here if EGL image creation fails.

@@ +797,5 @@
> +
> +    NS_ASSERTION(mShareWithEGLImage, "EGLImage not supported or disabled in runtime");
> +
> +    EGLTextureWrapper* wrap = (EGLTextureWrapper*)aSharedHandle;
> +    sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, wrap->GetEGLImage());

This is assuming that the context is current -- totally fine, but the docs for AttachSharedHandle should mention this.
Comment on attachment 639566 [details] [diff] [review]
Public shared texture API + Canvas impl

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

::: gfx/layers/opengl/CanvasLayerOGL.cpp
@@ +430,5 @@
> +    gl()->fActiveTexture(LOCAL_GL_TEXTURE0);
> +    gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);
> +    if (!gl()->AttachSharedHandle(texDescriptor.shareType(), texDescriptor.handle())) {
> +      NS_ERROR("Failed to attach shared texture handle");
> +      return;

Hmm.  This AttachSharedHandle only needs to be done once to attach the EGLImage to mTexture -- the attachment to mTexture stays valid, for future calls we only need to call BindTexture(TEXTURE_2D, mTexture).  I'm not sure how expensive the attachment itself is, but is there any reason why we Attach/Detach here constantly per draw?  We can simply check if the texDescriptor handle is the same as what we had before, and if so, skip attaching.  We shouldn't ever need to detach, either.

Seems like we should work that in to the API somehow, since skipping this attach call is likely to be a perf win unless we get lucky and hit an internal optimization where GL sees that the right EGLImage is already attached to mTexture.  (I know detach is a no-op with EGL Image, but attach isn't.)
Note: I'm happy to have the fixes/investigations for the above issues happen in future bugs.. no need to block landing on m-c.
(Assignee)

Comment 136

5 years ago
Created attachment 639749 [details] [diff] [review]
Public shared texture API + Canvas impl

Fixed some of vlad comments
Attachment #639566 - Attachment is obsolete: true
Attachment #639566 - Flags: review?(bgirard)
Attachment #639749 - Flags: review?(bgirard)
(Assignee)

Comment 137

5 years ago
Created attachment 639750 [details] [diff] [review]
EGLImage implementation.

Fixed some vlad comments
Attachment #639358 - Attachment is obsolete: true
Attachment #639750 - Flags: review?(bgirard)
(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.
(Reporter)

Comment 139

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

Comment 140

5 years ago
Comment on attachment 639749 [details] [diff] [review]
Public shared texture API + Canvas impl

\o/
Attachment #639749 - Flags: review?(bgirard) → review+
(Reporter)

Updated

5 years ago
Attachment #639750 - Flags: review?(bgirard) → review+
(Assignee)

Comment 141

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/255445a0a851
https://hg.mozilla.org/integration/mozilla-inbound/rev/153e82923805
https://hg.mozilla.org/integration/mozilla-inbound/rev/6087689a0745
\o/ \o/ \o/
Backed out for being the likely cause of bug 772405. 

This landed on top of bug 767064's webgl bustage, which made things harder to diagnose. To help in the future, please can you paste try run URLs in-bug (along the lines of https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound) - thank you :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/04dc0717dd53
Depends on: 772405
(Assignee)

Comment 144

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

Comment 145

5 years ago
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
(Assignee)

Comment 146

5 years ago
Launched this page, full tests round
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html

on Tegra3 Asus, 4.0.3,  and video/image tex-image-and-sub-image-2d passes tests...
Also launched these tests standalone - also pass..
sounds like it device specific and broken on device which is running tests... weird.

Any ideas?
The tests all pass on the tegra board that we have in the toronto office as well. It's supposed to be exactly the same thing as the test slaves.

So my only idea now is to do tryserver runs with more logging. Notice that in android mochitests, the ONLY output that gets logged is the strings passed to mochitest ok() and todo() functions. Otherwise we'd have plenty of info already.
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
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.
Blocks: 773071
The previous try push had -u none </brownpaperbag>

new try:
https://tbpl.mozilla.org/?tree=Try&rev=053fa8c9e421
(Assignee)

Comment 151

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=13448893&tree=Try&full=1 - test run is orange but no test failures reported
So, I re-triggered a few times, and this is really weird.

5 runs:

1.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13448893&tree=Try&full=1
(Is the one mentioned in comment 151)
this run reached this point in the WebGL tests:

46491 INFO TEST-INFO | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/programs/gl-get-active-attribute.html] (WebGL mochitest) Starting test page
INFO | automation.py | Application ran for: 0:18:01.369796
INFO | automation.py | Reading PID log: /tmp/tmpPf77P1pidlog
getting files in '/mnt/sdcard/tests/profile/minidumps/'
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

Then the next log entries suggest that several system process have restarted, in particular there is this:
D/AndroidRuntime(  939): >>>>>>>>>>>>>> AndroidRuntime START <<<<<<<<<<<<<<
which suggests that the whole system got restarted (??) and there is no subsequent Mozilla-related lines, so this looks like a bad OOM.

2.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13450523&tree=Try&full=1

This one dies much earlier (doesn't get to the WebGL tests) on what looks like a typical infrastructure issue, so we can ignore it:

7900 INFO TEST-START | /tests/content/base/test/test_bug410229.html
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
Error receiving data from socket. cmd=ps
; err=[Errno 54] Connection reset by peer
reconnecting socket
unable to connect socket: [Errno 61] Connection refused

3.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13450328&tree=Try&full=1

This one passed! So I get to see all my logging, but as it passed, it's not interesting...

4.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13451042&tree=Try&full=1#error0

This one is a sort of timeout, but not the usual test timeout, it's a buildbot thing killing the browser:

49526 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/more/functions/drawArraysOutOfBounds.html] Test passed - testDrawArraysVBOMultiOutOfBounds
49527 INFO TEST-PASS | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/more/functions/drawArraysOutOfBounds.html] Test 
command timed out: 2400 seconds without output, killing pid 48170
process killed by signal 9
program finished with exit code -1
elapsedTime=3478.961889
TinderboxPrint: mochitest-plain<br/><em class="testfail">T-FAIL</em>
buildbot.slave.commands.TimeoutError: command timed out: 2400 seconds without output, killing pid 48170

Earlier, we have several instances of:

failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!
failed to validate file when downloading /mnt/sdcard/tests/logs/mochitest.log!

This looks like an infrastructure issue...

5.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13453471&tree=Try&full=1

this one passed, like 3.


********************************************************************************
********************************************************************************

So I haven't been able to reproduce at all the conformance/textures that I was trying to debug :-/

The other question is why is this patch apparently so prone to OOM or infrastructure issue; maybe there is a real issue here, like a memory leak.
I suggest that we re-review these patches to see if there might be a leak or otherwise abnormal memory usage.

Are these 3 changesets keeping the tree in a green state? If yes, we could push to try with only the first, and with only the first two, to see where the trouble starts.
Try with 255445a0a851 (first patch only):
https://tbpl.mozilla.org/?tree=Try&rev=d8cfb5b1a4bb

Try with 153e82923805 (first and second patch only):
https://tbpl.mozilla.org/?tree=Try&rev=4a1ee4c311e0

Try with 6087689a0745 (all three patches):
https://tbpl.mozilla.org/?tree=Try&rev=b604b0ed474a
Are we sure that we haven't seen the same failures even before these patches?
"Better" than that: all 3 try runs in comment 154 are successful, after a few retriggers, aside from clearly unrelated infra issues in the 2nd one.

-> I believe in witches

-> Oleg: please rebase your patches against the latest green cset on inbound (they dont apply cleanly anymore), and push again to try, and retrigger M1 a few times.
(Assignee)

Comment 157

5 years ago
Created attachment 641601 [details] [diff] [review]
Public shared texture API + Canvas impl - rebased
Attachment #639749 - Attachment is obsolete: true
Attachment #641601 - Flags: review+
(Assignee)

Comment 158

5 years ago
pushed one more try with rebased version
https://tbpl.mozilla.org/?tree=Try&rev=09aa2750f9fb
(Assignee)

Comment 159

5 years ago
Created attachment 642052 [details]
Webgl error logcat
(Assignee)

Comment 160

5 years ago
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
(Assignee)

Comment 161

5 years ago
Created attachment 642079 [details] [diff] [review]
Invalid operation FIX
Attachment #642079 - Flags: review?(jgilbert)
Can you describe what the error actually is?
(Assignee)

Comment 163

5 years ago
Invalid error is coming from CopyTexSubImage2D which does not understand BGRA format we are using for Offscreen Textures. IIUC
(Assignee)

Comment 164

5 years ago
Created attachment 642159 [details] [diff] [review]
Public shared texture API + Canvas impl - rebased (after gralloc push)
Attachment #641601 - Attachment is obsolete: true
(Assignee)

Comment 165

5 years ago
Ok, with couple of re-triggers I got fully green try build
https://tbpl.mozilla.org/?tree=Try&rev=a14c70e6a894
(Assignee)

Updated

5 years ago
Attachment #642079 - Flags: review?(bjacob)
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.
Attachment #642079 - Flags: review?(bjacob) → review-
(Assignee)

Comment 167

5 years ago
passing format itself would be also not very nice... because it is only for alpha case.
(Assignee)

Comment 168

5 years ago
also this will go away with 565 fix.
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+.
(Assignee)

Comment 170

5 years ago
Created attachment 642339 [details] [diff] [review]
Invalid operation FIX, force RGBA
Attachment #642079 - Attachment is obsolete: true
Attachment #642079 - Flags: review?(jgilbert)
Attachment #642339 - Flags: review?(jgilbert)
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
Attachment #642339 - Flags: review?(jgilbert) → review+
Blocks: 774058
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.
Blocks: 774059
https://hg.mozilla.org/mozilla-central/rev/5a89db18c245
https://hg.mozilla.org/mozilla-central/rev/4f6f1a2aa64e
https://hg.mozilla.org/mozilla-central/rev/6fed555b91c3
https://hg.mozilla.org/mozilla-central/rev/02c4bf15eb59
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
This is disabled for cross-process shadow layers, right?
(Assignee)

Comment 175

5 years ago
yep, it has ThreadShared check
Blocks: 775222
Blocks: 775548
Blocks: 775220
Comment on attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup

[Approval Request Comment]
Required for bug 728524
Attachment #626900 - Flags: approval-mozilla-beta?
(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 on attachment 639750 [details] [diff] [review]
EGLImage implementation.

[Approval Request Comment]
Required by bug 687267
Attachment #639750 - Flags: approval-mozilla-beta?
Comment on attachment 642159 [details] [diff] [review]
Public shared texture API + Canvas impl - rebased (after gralloc push)

[Approval Request Comment]
Required by bug 687267
Attachment #642159 - Flags: approval-mozilla-beta?
Comment on attachment 642339 [details] [diff] [review]
Invalid operation FIX, force RGBA

Required by bug 687267
(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 on attachment 626900 [details] [diff] [review]
Minor rework for TexImage filter setup

Approving supporting patches for bug 687267
Attachment #626900 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #639750 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #642159 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox15: --- → affected
status-firefox16: --- → fixed
tracking-firefox15: --- → +
P1 for Games so soft-blocking.
blocking-basecamp: ? → +
Whiteboard: webgl-next [k9o:p1:fx?] [games:p1] → webgl-next [k9o:p1:fx?] [games:p1][soft]
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.
Attachment #645476 - Flags: review?(romaxa)
(Assignee)

Comment 185

5 years ago
Comment on attachment 645476 [details] [diff] [review]
Modified version of shared handle implementation for Beta

Looks ok for me, as API structure should work
Attachment #645476 - Flags: review?(romaxa) → review+
https://hg.mozilla.org/releases/mozilla-beta/rev/677033d5e8d2
https://hg.mozilla.org/releases/mozilla-beta/rev/4c978abd219d
https://hg.mozilla.org/releases/mozilla-beta/rev/ff28791ee7e9
https://hg.mozilla.org/releases/mozilla-beta/rev/9adaa83ecc7e

Updated

5 years ago
status-firefox15: affected → fixed
Blocks: 784867
You need to log in before you can comment on or make changes to this bug.