Last Comment Bug 771350 - Support direct texturing of gralloc buffers in ShadowThebesLayerOGL
: Support direct texturing of gralloc buffers in ShadowThebesLayerOGL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 765734 775436 775649 777094
Blocks: b2g-layers-work
  Show dependency treegraph
 
Reported: 2012-07-05 14:53 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-24 15:22 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Cody's WIP rebased on platform-demo-mc tip (22.27 KB, patch)
2012-07-05 14:55 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP v2 (22.06 KB, patch)
2012-07-05 15:43 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP, v3 (works) (23.99 KB, patch)
2012-07-06 00:52 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP, v4 (24.12 KB, patch)
2012-07-06 16:27 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP, v5 (24.75 KB, patch)
2012-07-06 16:49 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
First step of splitting up the patch (13.58 KB, patch)
2012-07-18 19:31 PDT, Cody Brocious [:Daeken]
cjones.bugs: review+
Details | Diff | Splinter Review
Initial step2 reconstruction (3.68 KB, patch)
2012-07-19 11:27 PDT, Cody Brocious [:Daeken]
no flags Details | Diff | Splinter Review
handoff (3.85 KB, patch)
2012-07-19 15:19 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Shuffled around step 2 patch and added EGLImage handling (5.85 KB, patch)
2012-07-19 16:17 PDT, Cody Brocious [:Daeken]
no flags Details | Diff | Splinter Review
patch (4.19 KB, patch)
2012-07-19 17:56 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
add DirectTextureImageEGL (7.69 KB, patch)
2012-07-19 18:36 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
remove some dead code (1.40 KB, patch)
2012-07-19 18:54 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Merged patches and fixed some bugs (4.93 KB, patch)
2012-07-19 22:58 PDT, Cody Brocious [:Daeken]
no flags Details | Diff | Splinter Review
platform-demo-mc work rebased on m-c tip (16.28 KB, patch)
2012-07-20 14:14 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Remove more dead code (5.35 KB, patch)
2012-07-20 18:49 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP of DirectTexture interface, EGL impl, and ThebesLayerOGL changes (23.25 KB, patch)
2012-07-20 18:51 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Rollup (30.17 KB, patch)
2012-07-20 18:54 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Initial changes, still not working (3.21 KB, patch)
2012-07-20 21:06 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Changes (3.45 KB, patch)
2012-07-20 21:55 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Changes, dead end I think (4.40 KB, patch)
2012-07-20 22:06 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 0: Remove some dead code to make later patches clearer (5.50 KB, patch)
2012-07-20 23:30 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
b56girard: review+
Details | Diff | Splinter Review
part 1: Add GLContext::CreateDirectTextureImage and OpenDescriptorForTexturing to more easily support direct texturing without updates (16.95 KB, patch)
2012-07-20 23:30 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible (7.60 KB, patch)
2012-07-20 23:31 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
b56girard: review-
Details | Diff | Splinter Review
part 1.1: Add GLContext::CreateDirectTextureImage and OpenDescriptorForTexturing to more easily support direct texturing without updates (14.94 KB, patch)
2012-07-20 23:36 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Splinter Review
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible, v2 (8.00 KB, patch)
2012-07-21 15:45 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible, v3 (7.71 KB, patch)
2012-07-23 14:07 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
b56girard: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 14:53:56 PDT
Bug 765734 shaves all the yaks needed to actually skip texture upload on gonk.  This bug implements the GL direct-texturing magic enabled by that in ShadowThebesLayerOGL.

(Whups, I thought this was filed already.)
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 14:55:52 PDT
Created attachment 639479 [details] [diff] [review]
Cody's WIP rebased on platform-demo-mc tip

According to Cody, this patch doesn't work, and apart from that it needs some polishing, but this is the place to start from.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 15:43:00 PDT
Created attachment 639492 [details] [diff] [review]
WIP v2

Whups, messed up and if/else block when rebasing.

This patch gets me into a crash loop with

I/Gecko   (  480): ###!!! ABORT: unexpected SurfaceDescriptor type!: file /home/cjones/mozilla/platform-demo-mc/gfx/layers/ipc/ShadowLayers.cpp, line 430
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 18:02:28 PDT
I'm now alternately crashing in

###!!! ASSERTION: Invalid program type.: 'ProgramProfileOGL::ProgramExists(aType, aMask)', file /home/cjones/mozilla/platform-demo-mc/gfx/layers/opengl/LayerManagerOGL.h, line 185

on the compositor side, with what looks like a garbled ShadowBufferOGL, and

W/GraphicBufferMapper( 7435): lock(...) failed -22 (Invalid argument)
F/MOZ_Assert( 7435): Assertion failure: status == OK, at /home/cjones/mozilla/platform-demo-mc/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp:278

on the content side when trying to map a gralloc buffer rw.  There's a memory-safety bug in this patch but I fixed it, unfortunately doesn't do away with garbled buffer.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 19:09:15 PDT
The TextureImage itself isn't garbled, it's just trying to use an uninitialized mShaderType.  The shader type is only set on upload, which we obviously never do with gralloc buffers.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 19:37:34 PDT
Fixed.  Down to the EINVAL problem now.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-05 23:05:42 PDT
[GraphicBufferMapper] registerBuffer(0x21b5e00)
[GraphicBufferMapper] lock(0x21b5e00, read|write)
[GraphicBufferMapper] unlock(0x21b5e00)
[GraphicBufferMapper] registerBuffer(0x21ab340)
[GraphicBufferMapper] lock(0x21ab340, read|write)
[GraphicBufferMapper] lock(0x21b5e00, read|write)
[GraphicBufferMapper] unlock(0x21b5e00)
[GraphicBufferMapper] unlock(0x21ab340)
[GraphicBufferMapper] lock(0x21ab340, read|write)
[GraphicBufferMapper] unlock(0x21ab340)
[GraphicBufferMapper] lock(0x21b5e00, read|write)
[GraphicBufferMapper] lock(0x21b5e00, read|write)
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 00:12:22 PDT
[ShadowableThebesLayer] lock() for MapBuffer()
[GraphicBufferMapper] lock(0x18f8b58, read|write)
[ShadowableThebesLayer] mBackBuffer.lock(rw) for SyncFrontToBack()
[GraphicBufferMapper] lock(0x18f8b58, read|write)
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 00:52:27 PDT
Created attachment 639601 [details] [diff] [review]
WIP, v3 (works)

It works, and it's fast :D.

The basic layers changes belong in bug 765734.  This patch needs a lot of cleanup before landing.
Comment 9 Sotaro Ikeda [:sotaro] 2012-07-06 08:03:25 PDT
Hi, about attachment 639601 [details] [diff] [review]. It seems that there might be a timing related problem. If following 2 each functions are called in different threads, they could conflict each other. 
 - ShadowThebesLayerOGL::Swap()
 - ShadowThebesLayerOGL::RenderLayer()
It is just my impression from reading the patch ...
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 09:46:04 PDT
Those are always called only on the "compositor thread".
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 14:57:42 PDT
Hitting another crash when showing the "task switcher"

[Child 405] ###!!! ABORT: aborting because of fatal error: file /home/cjones/mozilla/platform-demo-mc/dom/ipc/ContentChild.cpp, line 609
[Parent 339] ###!!! ABORT: __delete__()d actor: file /home/cjones/mozilla/new-b2g/objdir-gecko/ipc/ipdl/PBrowser.cpp, line 28
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 15:15:14 PDT
Also seen when opening the task switcher

[Parent 609] ###!!! ABORT: Swapped-in buffer size doesn't match old buffer's!: 'newSize == prevSize', file /home/cjones/mozilla/platform-demo-mc/gfx/layers/basic/BasicLayers.cpp, line 435
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 16:27:47 PDT
Created attachment 639845 [details] [diff] [review]
WIP, v4

Fixes the size mismatch assertion.  (Restores code to destroys old buffers when they go stale.)
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 16:49:39 PDT
Created attachment 639852 [details] [diff] [review]
WIP, v5

Fixes crash with destroyed-actor.  Turns out to be unrelated to this or gralloc.  The TabParent hunk belongs in bug 750977.

This patch isn't pretty or landable, but I'm going to go ahead and drop it on platform-demo-mc.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-09 12:28:54 PDT
This patch should be split into three parts
 1. remove the unused upload logic from ThebesLayerOGL.  This can land asap.
 2. add interfaces for direct texturing with SurfaceDescriptor.  See below.
 3. implement that interface for gralloc

We shouldn't have any ifdef's in ThebesLayerOGL.cpp.

For item (2) above, I suggest the following interface in ShadowLayers.h

 class ShadowLayerManager {
    //...
    static already_AddRefed<DirectTextureImage>
    OpenDescriptorForTexturing(GLContext*, const SurfaceDescriptor&);
 };

The implementation should follow the pattern of ShadowLayerForwarder::OpenDescriptor(): have Platform*() variants that we attempt to delegate to, then fall back on a common impl around shmem.

We should implement this in ShadowLayerUtilsGralloc.cpp.  We should assert that the GLContext* is from GLContextProviderEGL, then do all the hackery dackery needed to allocate the GL/EGL resources for the gralloc buffer.

We should add DirectTextureImage to GLContext.h.  The API should be the extremely small subset of the TextureImage API that we need for gralloc (and later pixmap, if we want).  I think just Bind/Release.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-09 15:33:02 PDT
Apparently this patch is tripping

F/MOZ_Assert(19699): Assertion failure: status == OK, at /Volumes/doug/platform-demo-mc/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp:278

on ICS maguro.  It works 100% fine on Nexus S.

I'm sans MSM devices for the moment, can anyone help debug?
Comment 17 Michael Vines [:m1] [:evilmachines] 2012-07-10 17:06:06 PDT
We are hitting this assert because the buffer that we are trying to lock for write was previously locked for read by this same function and never unlocked.

Immediately before this assertion fails, I also see the EGL driver returning a GL_INVALID_OPERATION from the call to       sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, 0) in TextureImageEGL::UnbindGralloc().  The NULL image causes the implementation to return immediately and it performs no useful function.  Maybe related?
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-10 17:31:31 PDT
OK that's not expected.

I have no idea about the ImageTargetTexture2DOES() but it sounds like a juicy lead.
Comment 19 Cody Brocious [:Daeken] 2012-07-10 18:55:44 PDT
Hmm, we're using the same 0 parameter in the non-crossprocess direct texture code in an attempt to unbind the texture and that seems to work there, but maybe something else is going on in that case.  Is there a good way to unset the image target texture?
Comment 20 Michael Vines [:m1] [:evilmachines] 2012-07-10 19:03:24 PDT
Can you point me to that code and how to hit it during execution?  In looking at the implementation, a null texture literally does nothing other than immediately returning an error.  So my guess is that other case is not really doing anything.  Looks like only way to satisfy it is to provide another valid texture.  But maybe there's another method that can be used to unbind the texture without needing to provide a new one, not sure off hand but I could search for that from the POV of the implementation if you'd like
Comment 21 Cody Brocious [:Daeken] 2012-07-10 19:08:49 PDT
It's currently done that way in TextureImageEGL::GetLockSurface() in GLContextProviderEGL.cpp, but I think you have the right of it; this has come up before.  If we provide it another texture, then that'll still be locked for reading by the GPU/driver presumably, so I think we need a way to either unbind the texture from the EGLImage or to unbind the gralloc buffer from the EGLImage.  Destroying and recreating the EGLImage every frame seems expensive, but that'd work too.
Comment 22 Michael Vines [:m1] [:evilmachines] 2012-07-10 20:53:31 PDT
Ah, the sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, 0) in TextureImageEGL::GetLockSurface() is almost certainly failing as well.

How about creating a 1x1 (or 0x0?) texture that we use instead of 0 in cases like this?  Unless I can't read C++ anymore that should cause the implementation to release its read lock on the original EGLImage. People who know GL will probably shoot me for suggesting something like this as there is surely a more elegant mechanism...
Comment 23 Michael Vines [:m1] [:evilmachines] 2012-07-11 10:25:41 PDT
I quickly hacked GLContextProviderEGL.cpp and replaced the two instances of sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, 0) with an "mEGLImageNull" that was created from a new GraphicBuffer(1, 1, ...) and the driver complaints go away, locking complaints go away, and I longer see "screen snow" that was manifesting before when there was really no locking happening.  We should be able to use a single mEGLImageNull per process since multiple readers are fine.
Comment 24 Cody Brocious [:Daeken] 2012-07-11 10:31:13 PDT
Ah hah, that sounds great.  Thanks for digging into it.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 02:00:14 PDT
Cody, how is this coming along?

Separately, can we land mvines's workaround on platform-demo-mc?
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 02:04:23 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> Separately, can we land mvines's workaround on platform-demo-mc?

(What's outlined in comment 23, I mean.)
Comment 27 Cody Brocious [:Daeken] 2012-07-18 19:31:36 PDT
Created attachment 643708 [details] [diff] [review]
First step of splitting up the patch

This patch is the first step in splitting up the WIP.  It simply strips out the progressive upload logic and other unnecessary upload paths.
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 19:36:23 PDT
Comment on attachment 643708 [details] [diff] [review]
First step of splitting up the patch

<3
Comment 29 Andreas Gal :gal 2012-07-19 10:43:04 PDT
Comment on attachment 643708 [details] [diff] [review]
First step of splitting up the patch

Landed separately.
Comment 30 Cody Brocious [:Daeken] 2012-07-19 11:27:02 PDT
Created attachment 643942 [details] [diff] [review]
Initial step2 reconstruction

This patch is the product of last night's rearchitecture.  I've split the DirectTextureImage up slightly so as to keep EGL-specific code out of GLContext and allow us to implement other types of direct texturing.

Next steps for this patch are:
1) Add reference counting to DirectTextureImages
2) Clean up the class defs themselves
3) Write the tie-in for the EGLContext to bridge the bind
4) Add generic texture unbind

Steps 3 and 4 I'm taking care of now as I already have the code, just had to move it around.  I could use help/advice on 1 and 2.
Comment 31 Andreas Gal :gal 2012-07-19 11:49:33 PDT
4) just landed
Comment 32 Andreas Gal :gal 2012-07-19 15:19:41 PDT
Created attachment 644036 [details] [diff] [review]
handoff
Comment 33 Cody Brocious [:Daeken] 2012-07-19 16:17:26 PDT
Created attachment 644060 [details] [diff] [review]
Shuffled around step 2 patch and added EGLImage handling

This patch moves the DirectTextureImage implementations around and cleans up the GL code significantly.  Everything is there except actually binding them on the Thebes side, and some little issues (marked with XXX comments in the patch).
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-19 17:10:03 PDT
Comment on attachment 644060 [details] [diff] [review]
Shuffled around step 2 patch and added EGLImage handling

>diff --git a/gfx/gl/GLContext.h b/gfx/gl/GLContext.h

>+class DirectTextureImage

Needs a comment of course.

>+{

NS_INLINE_DECL_REFCOUNTING(DirectTextureImage)

Nit: { } on same like as decl.

>+
>+    virtual void Bind() {

Leave this pure virtual, I don't believe there's a sensible
default impl.

Follow the API of TextureImage

  virtual void BindTexture(GLenum aTextureUnit)

>+
>+    }

Unbind() ?

How do we know the format and size of the texture image so we
know which shader program to select?  It's probably best to
refactor TextureImage into TextureImage : TextureImageBase, put
the format/size/texture name in TextureImageBase, and then have
DirectTextureImage derive from TextureImageBase.

>+    virtual DirectTextureImage *CreateDirectTextureImage(void* buffer) { return nsnull; }

Return already_AddRefed<DirectTextureImage>.

Not a big fan of void* APIs, but that battle has already been
lost in GLContext, so whatev.

>diff --git a/gfx/gl/GLContextProviderEGL.cpp b/gfx/gl/GLContextProviderEGL.cpp

>+class GrallocDirectTextureImage
>+    : public DirectTextureImage

Presumably needs to be ifdef ANDROID or ifdef MOZ_WIDGET_GONK.

>+    GrallocDirectTextureImage(GLContextEGL *context, void *buffer)
>+        : mGLContext(context)
>+    {

>+            return; // XXX: No way to determine if this failed

Check for failure in your factory method,
CreateDirectTextureImage(), and return null from there.

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp

>+/*static*/ DirectTextureImage *
>+ShadowLayerManager::OpenDescriptorForTexturing(GLContext* context, const SurfaceDescriptor& descriptor) {

>+  DirectTextureImage *image = PlatformOpenDescriptorForTexturing(context, descriptor);
>+  return image; // XXX: This needs a fallback

return Platform...();

This is a start but it's not going to work for
ShadowThebesLayerOGL.
Comment 35 Andreas Gal :gal 2012-07-19 17:56:34 PDT
Created attachment 644105 [details] [diff] [review]
patch
Comment 36 Andreas Gal :gal 2012-07-19 18:36:41 PDT
Created attachment 644115 [details] [diff] [review]
add DirectTextureImageEGL
Comment 37 Andreas Gal :gal 2012-07-19 18:54:57 PDT
Created attachment 644124 [details] [diff] [review]
remove some dead code
Comment 38 Cody Brocious [:Daeken] 2012-07-19 22:58:33 PDT
Created attachment 644183 [details] [diff] [review]
Merged patches and fixed some bugs
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 14:14:00 PDT
Created attachment 644469 [details] [diff] [review]
platform-demo-mc work rebased on m-c tip

For reference purposes.
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 18:49:41 PDT
Created attachment 644555 [details] [diff] [review]
Remove more dead code
Comment 41 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 18:51:07 PDT
Created attachment 644557 [details] [diff] [review]
WIP of DirectTexture interface, EGL impl, and ThebesLayerOGL changes

I cargo culted the GL parts of this from Cody's patches and platform-demo-mc, and it should work.  No crashing or apparently gralloc leakage.

However, all I see is black screen.  Would really appreciate help debugging this.
Comment 42 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 18:54:56 PDT
Created attachment 644559 [details] [diff] [review]
Rollup

Halp!
Comment 43 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 19:01:03 PDT
Based on 99985:139a8f2a8538
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 21:06:02 PDT
Created attachment 644586 [details] [diff] [review]
Initial changes, still not working

The sematnics of BindGralloc() from platform-demo-mc and BindExternalImage() in tip are totally different.  Moving towards a more unified impl makes sense, but the previous patch was using the latter but expecting the semantics of the former.

This changes that, but I don't see my new DirectTextureImageEGL::BindTexImage being called.  Probably TiledTextureImage screwing us over somehow.  Will poke more after food.
Comment 45 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 21:55:34 PDT
Created attachment 644591 [details] [diff] [review]
Changes

This gets things drawing, but we're drawing the wrong textures ...
Comment 46 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 22:06:18 PDT
Created attachment 644593 [details] [diff] [review]
Changes, dead end I think

Will go back to approach of code on platform-demo-mc, which is known to work.

BindExternalImage() may not like our flavor of gralloc buffer, for some reason.
Comment 47 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 22:36:43 PDT
DirectTextureImageEGL was shadowing two members from its base class.  That's probably why gdb was telling me some nonsensical things.  One of the previous patches may be functional.
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 23:30:34 PDT
Created attachment 644605 [details] [diff] [review]
part 0: Remove some dead code to make later patches clearer
Comment 49 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 23:30:57 PDT
Created attachment 644606 [details] [diff] [review]
part 1: Add GLContext::CreateDirectTextureImage and OpenDescriptorForTexturing to more easily support direct texturing without updates
Comment 50 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 23:31:25 PDT
Created attachment 644607 [details] [diff] [review]
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible
Comment 51 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 23:36:04 PDT
Created attachment 644608 [details] [diff] [review]
part 1.1: Add GLContext::CreateDirectTextureImage and OpenDescriptorForTexturing to more easily support direct texturing without updates

There were some vestigial hunks in the previous patch that would have been confusing.
Comment 52 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 23:53:48 PDT
Guys, we'd like to get this landed asap, so let me know if you think you'll have a high review lag.
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 00:14:05 PDT
I should note that this patch doesn't implement the dummy 1x1 EGLImage hack that we landed on platform-demo-mc, because that iteration of the code was trying to use the same texture with multiple EGLImages / gralloc buffers, but the iteration here doesn't do that.
Comment 54 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 00:16:19 PDT
One more note: I wanted to use an interface other than TextureImage to expose direct texturing, but to do so would have required a pretty nontrivial refactoring of TextureImage and, more frighteningly, TextureImageEGL, so we decided to punt on that.
Comment 55 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 00:39:30 PDT
Should also add, this patch fixes a bug in Swap() where we returned an inaccurate (too small, but not incorrect) valid region to the child.  Fixing that allows our buffer rotation optimization to kick in and we get noticeably better perf.
Comment 56 Andreas Gal :gal 2012-07-21 02:23:06 PDT
I had to add a friend class ShadowLayerManager to make this compile otherwise GetFrom can't be accessed from GrallocUtils.
Comment 57 Benoit Girard (:BenWa) 2012-07-21 10:00:50 PDT
Comment on attachment 644607 [details] [diff] [review]
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible

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

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +29,5 @@
>  
> +GLenum
> +WrapMode(GLContext *aGl, PRUint32 aFlags)
> +{
> +  return ((aFlags & ALLOW_REPEAT) &&

This expression is unnecessarily hard to read, let's turn this into if(EXP) { return X; } else { return Y; }

@@ +959,5 @@
> +  mTexImage = aNewBackBuffer;
> +  mBufferRect = aRect;
> +  mBufferRotation = aRotation;
> +
> +  mInitialised = !!mTexImage;   // sic

This comment isn't useful

@@ +996,5 @@
> +  if (IsSurfaceDescriptorValid(mBufferDescriptor)) {
> +    AutoOpenSurface currentFront(OPEN_READ_ONLY, mBufferDescriptor);
> +    AutoOpenSurface newFront(OPEN_READ_ONLY, aNewFront.buffer());
> +    if (currentFront.Size() != newFront.Size()) {
> +      // Current front buffer is obsolete

It's not information to say something is obsolete before deleting it. Try:
The buffer changed size making it obsolete.

@@ +1005,5 @@
>    if (!mBuffer) {
>      mBuffer = new ShadowBufferOGL(this);
>    }
> +  
> +  if (nsRefPtr<TextureImage> texImage =

Let's not initialize the variable in a conditional statement, I don't see a good reason why this is used here at the expense of readability.

@@ +1013,5 @@
> +    // front buffer, and return our previous directly-textured surface
> +    // to the renderer.
> +    ThebesBuffer newBack;
> +    {
> +      nsRefPtr<TextureImage> destroy = mBuffer->Swap(

I find it very confusing that we're doing a swap and instead deleting the old TextureImage and using a scope to delete it early. This seems to indicate that this code wants to live within swap. Lets make this more clear by renaming the method and/or documenting why we don't want to recycle this texture image.

::: gfx/layers/opengl/ThebesLayerOGL.h
@@ +124,5 @@
>    virtual void Disconnect();
>  
>    virtual void SetValidRegion(const nsIntRegion& aRegion)
>    {
> +    mLastValidRegion = mValidRegion;

The definition of mLastValidRegion is not well defined and this is incredibly fragile. The definition of mLastValidRegion is 'the valid region before the last call to SetValidRegion'. This makes assumptions on how SetValidRegion is used and we don't enforce any rules on that. It just happens to work by accident because shadow layers are only updated from ShadowLayersParent.cpp in pratice.

Let's document and rename mLastValidRegion to mBackBufferValidRegion and update it within 'ShadowThebesLayerOGL::Swap'.
Comment 58 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 15:45:10 PDT
(In reply to Benoit Girard (:BenWa) from comment #57)
> Comment on attachment 644607 [details] [diff] [review]
> part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where
> possible
> 
> Review of attachment 644607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> @@ +29,5 @@
> >  
> > +GLenum
> > +WrapMode(GLContext *aGl, PRUint32 aFlags)
> > +{
> > +  return ((aFlags & ALLOW_REPEAT) &&
> 
> This expression is unnecessarily hard to read, let's turn this into if(EXP)
> { return X; } else { return Y; }
> 

Fine.

> @@ +959,5 @@
> > +  mTexImage = aNewBackBuffer;
> > +  mBufferRect = aRect;
> > +  mBufferRotation = aRotation;
> > +
> > +  mInitialised = !!mTexImage;   // sic
> 
> This comment isn't useful
> 

"mInitialised" is misspelled.  "sic" means, "I'm using this even though I know it's incorrect". ;)

> @@ +996,5 @@
> > +  if (IsSurfaceDescriptorValid(mBufferDescriptor)) {
> > +    AutoOpenSurface currentFront(OPEN_READ_ONLY, mBufferDescriptor);
> > +    AutoOpenSurface newFront(OPEN_READ_ONLY, aNewFront.buffer());
> > +    if (currentFront.Size() != newFront.Size()) {
> > +      // Current front buffer is obsolete
> 
> It's not information to say something is obsolete before deleting it. Try:
> The buffer changed size making it obsolete.
> 

Updated.

> @@ +1005,5 @@
> >    if (!mBuffer) {
> >      mBuffer = new ShadowBufferOGL(this);
> >    }
> > +  
> > +  if (nsRefPtr<TextureImage> texImage =
> 
> Let's not initialize the variable in a conditional statement, I don't see a
> good reason why this is used here at the expense of readability.
> 

I don't understand the objection here.  It's good C++ style: it keeps the variable scoped to the block it's used, and only executes the block if the variable is nonnull.

> @@ +1013,5 @@
> > +    // front buffer, and return our previous directly-textured surface
> > +    // to the renderer.
> > +    ThebesBuffer newBack;
> > +    {
> > +      nsRefPtr<TextureImage> destroy = mBuffer->Swap(
> 
> I find it very confusing that we're doing a swap and instead deleting the
> old TextureImage and using a scope to delete it early. This seems to
> indicate that this code wants to live within swap. Lets make this more clear
> by renaming the method and/or documenting why we don't want to recycle this
> texture image.
> 

Also not quite understanding.  Swap() means, take the new buffer and attributes and give me back the old buffer and attributes.  In this case, the caller wants to delete the old buffer.

> ::: gfx/layers/opengl/ThebesLayerOGL.h
> @@ +124,5 @@
> >    virtual void Disconnect();
> >  
> >    virtual void SetValidRegion(const nsIntRegion& aRegion)
> >    {
> > +    mLastValidRegion = mValidRegion;
> 
> The definition of mLastValidRegion is not well defined and this is
> incredibly fragile. The definition of mLastValidRegion is 'the valid region
> before the last call to SetValidRegion'. This makes assumptions on how
> SetValidRegion is used and we don't enforce any rules on that. It just
> happens to work by accident because shadow layers are only updated from
> ShadowLayersParent.cpp in pratice.
> 

The layers protocol enforces this rule.  That ShadowLayersParent (the agent of the content renderer) only updates the semantic copy of the "real layers" is the core of the shadow layers protocol.  There's nothing in the compositor thread that ever tries to draw on shadow layers because (i) it can't, since it has no access to DOM/frame tree; (ii) if it did, lots of things break, including probably most IPC reftests on tinderbox.

So I feel like we're not quite on the same page somehow.

> Let's document and rename mLastValidRegion to mBackBufferValidRegion and
> update it within 'ShadowThebesLayerOGL::Swap'.

How does, mValidRegionForNextBackBuffer suit you?  We can't update it within Swap() because the new valid region has already been set.  (I.e., we've lost the information we needed.)  I also added a comment.
Comment 59 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 15:45:56 PDT
Created attachment 644694 [details] [diff] [review]
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible, v2
Comment 60 Benoit Girard (:BenWa) 2012-07-23 08:31:10 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #58)
> (In reply to Benoit Girard (:BenWa) from comment #57)
> > Comment on attachment 644607 [details] [diff] [review]
> > part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where
> > possible
> > 
> > Review of attachment 644607 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> > @@ +29,5 @@
> > >  
> > > +GLenum
> > > +WrapMode(GLContext *aGl, PRUint32 aFlags)
> > > +{
> > > +  return ((aFlags & ALLOW_REPEAT) &&
> > 
> > This expression is unnecessarily hard to read, let's turn this into if(EXP)
> > { return X; } else { return Y; }
> > 
> 
> Fine.
> 
> > @@ +959,5 @@
> > > +  mTexImage = aNewBackBuffer;
> > > +  mBufferRect = aRect;
> > > +  mBufferRotation = aRotation;
> > > +
> > > +  mInitialised = !!mTexImage;   // sic
> > 
> > This comment isn't useful
> > 
> 
> "mInitialised" is misspelled.  "sic" means, "I'm using this even though I
> know it's incorrect". ;)
> 
> > @@ +996,5 @@
> > > +  if (IsSurfaceDescriptorValid(mBufferDescriptor)) {
> > > +    AutoOpenSurface currentFront(OPEN_READ_ONLY, mBufferDescriptor);
> > > +    AutoOpenSurface newFront(OPEN_READ_ONLY, aNewFront.buffer());
> > > +    if (currentFront.Size() != newFront.Size()) {
> > > +      // Current front buffer is obsolete
> > 
> > It's not information to say something is obsolete before deleting it. Try:
> > The buffer changed size making it obsolete.
> > 
> 
> Updated.
> 
> > @@ +1005,5 @@
> > >    if (!mBuffer) {
> > >      mBuffer = new ShadowBufferOGL(this);
> > >    }
> > > +  
> > > +  if (nsRefPtr<TextureImage> texImage =
> > 
> > Let's not initialize the variable in a conditional statement, I don't see a
> > good reason why this is used here at the expense of readability.
> > 
> 
> I don't understand the objection here.  It's good C++ style: it keeps the
> variable scoped to the block it's used, and only executes the block if the
> variable is nonnull.
> 

Right but I feel it hurt readability to get a tighter scope which isn't necessary here. A quick (likely buggy) regex shows that there is at least 240 instances of this in our code base. Most of which is in the JS engine. The instances in gfx looks like they're almost all from your patches. I'm not thrilled about using these where it's not required but that's fine I guess.

find . -name "*.cpp" | xargs grep --color 'if ([^ )]* [^ ]* =[^=]' | wc -l

> > @@ +1013,5 @@
> > > +    // front buffer, and return our previous directly-textured surface
> > > +    // to the renderer.
> > > +    ThebesBuffer newBack;
> > > +    {
> > > +      nsRefPtr<TextureImage> destroy = mBuffer->Swap(
> > 
> > I find it very confusing that we're doing a swap and instead deleting the
> > old TextureImage and using a scope to delete it early. This seems to
> > indicate that this code wants to live within swap. Lets make this more clear
> > by renaming the method and/or documenting why we don't want to recycle this
> > texture image.
> > 
> 
> Also not quite understanding.  Swap() means, take the new buffer and
> attributes and give me back the old buffer and attributes.  In this case,
> the caller wants to delete the old buffer.
> 
> > ::: gfx/layers/opengl/ThebesLayerOGL.h
> > @@ +124,5 @@
> > >    virtual void Disconnect();
> > >  
> > >    virtual void SetValidRegion(const nsIntRegion& aRegion)
> > >    {
> > > +    mLastValidRegion = mValidRegion;
> > 
> > The definition of mLastValidRegion is not well defined and this is
> > incredibly fragile. The definition of mLastValidRegion is 'the valid region
> > before the last call to SetValidRegion'. This makes assumptions on how
> > SetValidRegion is used and we don't enforce any rules on that. It just
> > happens to work by accident because shadow layers are only updated from
> > ShadowLayersParent.cpp in pratice.
> > 
> 
> The layers protocol enforces this rule.  That ShadowLayersParent (the agent
> of the content renderer) only updates the semantic copy of the "real layers"
> is the core of the shadow layers protocol.  There's nothing in the
> compositor thread that ever tries to draw on shadow layers because (i) it
> can't, since it has no access to DOM/frame tree; (ii) if it did, lots of
> things break, including probably most IPC reftests on tinderbox.
> 
> So I feel like we're not quite on the same page somehow.

I think we are on the same page. I'm NOT thinking of something trying to draw in the compositor. You're just relying on the invalid region being what you want at the last time SetValidRegion is called. You know that's true because you're assuming a property of the user of the API (ShadowLayersParent) but that's an assumption that only us know about and is going to very frustrating to others 6 months down the road.

Why not set mValidRegionForNextBackBuffer for the next swap at the end of ShadowThebesLayerOGL::Swap?

> 
> > Let's document and rename mLastValidRegion to mBackBufferValidRegion and
> > update it within 'ShadowThebesLayerOGL::Swap'.
> 
> How does, mValidRegionForNextBackBuffer suit you?  We can't update it within
> Swap() because the new valid region has already been set.  (I.e., we've lost
> the information we needed.)  I also added a comment.
Comment 61 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 10:53:36 PDT
I'm not against what you suggest, stylistically.  The problem is that it doesn't work.

Not sure what to do here.
Comment 62 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 11:22:41 PDT
Comment on attachment 644608 [details] [diff] [review]
part 1.1: Add GLContext::CreateDirectTextureImage and OpenDescriptorForTexturing to more easily support direct texturing without updates

Sorry to pass this off to you Rob, but I can't get ahold of Vlad and we very crucially need this work for b2g.  Let me know if you're not comfortable reviewing these changes.
Comment 63 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 13:43:11 PDT
I had to change the part 1.1 patch to pull in GLDefs.h instead of GLContext.h, because GLContext.h brings in windows.h which breaks a *lot* of things.  But it was a trivial change.
Comment 64 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 14:07:56 PDT
Created attachment 645061 [details] [diff] [review]
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible, v3

The suggested change is different semantically, but ends up working incidentally.
Comment 65 Benoit Girard (:BenWa) 2012-07-23 14:15:57 PDT
Comment on attachment 645061 [details] [diff] [review]
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible, v3

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

Looks better, optinal nit for 'sic'

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +961,5 @@
> +  mTexImage = aNewBackBuffer;
> +  mBufferRect = aRect;
> +  mBufferRotation = aRotation;
> +
> +  mInitialised = !!mTexImage;   // sic

I would rather see 'sic' be clarified, fixed or removed. Looking at this code other people will wonder by 'sic' is written and lose time. The spelling is just a matter of locale.
Comment 66 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 14:19:47 PDT
Removed.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-23 16:38:27 PDT
Comment on attachment 644608 [details] [diff] [review]
part 1.1: Add GLContext::CreateDirectTextureImage and OpenDescriptorForTexturing to more easily support direct texturing without updates

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

r+ with that fixed

::: gfx/layers/ipc/ShadowLayerUtilsX11.cpp
@@ +195,5 @@
> +ShadowLayerManager::OpenDescriptorForDirectTexturing(GLContext*,
> +                                                     const SurfaceDescriptor&,
> +                                                     GLenum)
> +{
> +  // FIXME/bug XXXXXX: implement this using texture-from-pixmap

Return null?

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +621,5 @@
> +/*static*/ already_AddRefed<TextureImage>
> +ShadowLayerManager::OpenDescriptorForDirectTexturing(GLContext*,
> +                                                     const SurfaceDescriptor&,
> +                                                     GLenum)
> +{

How does this even compile? I guess it should return null?

::: gfx/layers/ipc/ShadowLayers.h
@@ +407,5 @@
>    virtual already_AddRefed<ShadowRefLayer> CreateShadowRefLayer() { return nsnull; }
>  
> +  static already_AddRefed<TextureImage>
> +  OpenDescriptorForDirectTexturing(GLContext* aContext,
> +                                   const SurfaceDescriptor& aDescriptor,

Document this? Especially document that it returns null on failure (I assume)
Comment 68 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 16:57:05 PDT
Yes, this blew up on tryserver.  I fixed those locally.

> ::: gfx/layers/ipc/ShadowLayers.h
> @@ +407,5 @@
> >    virtual already_AddRefed<ShadowRefLayer> CreateShadowRefLayer() { return nsnull; }
> >  
> > +  static already_AddRefed<TextureImage>
> > +  OpenDescriptorForDirectTexturing(GLContext* aContext,
> > +                                   const SurfaceDescriptor& aDescriptor,
> 
> Document this? Especially document that it returns null on failure (I assume)

Whups, totally forgot that, thanks:

  /**
   * Try to open |aDescriptor| for direct texturing.  If the
   * underlying surface supports direct texturing, a non-null
   * TextureImage is returned.  Otherwise null is returned.
   */

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