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

According to Cody, this patch doesn't work, and apart from that it needs some polishing, but this is the place to start from.

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

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.

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.

Fixed. Down to the EINVAL problem now.

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

[ShadowableThebesLayer] lock() for MapBuffer() [GraphicBufferMapper] lock(0x18f8b58, read|write) [ShadowableThebesLayer] mBackBuffer.lock(rw) for SyncFrontToBack() [GraphicBufferMapper] lock(0x18f8b58, read|write)

It works, and it's fast :D. The basic layers changes belong in bug 765734. This patch needs a lot of cleanup before landing.

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

Those are always called only on the "compositor thread".

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

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

Fixes the size mismatch assertion. (Restores code to destroys old buffers when they go stale.)

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.

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.

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?

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?

OK that's not expected. I have no idea about the ImageTargetTexture2DOES() but it sounds like a juicy lead.

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?

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

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.

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

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.

Ah hah, that sounds great. Thanks for digging into it.

Cody, how is this coming along? Separately, can we land mvines's workaround on platform-demo-mc?

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

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 on attachment 643708 [details] [diff] [review] First step of splitting up the patch <3

Comment on attachment 643708 [details] [diff] [review] First step of splitting up the patch Landed separately.

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.

4) just landed

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

For reference purposes.

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.

Halp!

Based on 99985:139a8f2a8538

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.

This gets things drawing, but we're drawing the wrong textures ...

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.

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.

There were some vestigial hunks in the previous patch that would have been confusing.

Guys, we'd like to get this landed asap, so let me know if you think you'll have a high review lag.

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.

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.

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.

I had to add a friend class ShadowLayerManager to make this compile otherwise GetFrom can't be accessed from GrallocUtils.

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

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

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

I'm not against what you suggest, stylistically. The problem is that it doesn't work. Not sure what to do here.

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.

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.

The suggested change is different semantically, but ends up working incidentally.

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.

Removed.

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)

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d472eab26a31 https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd1560c17a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/32a3b6dde987

https://hg.mozilla.org/mozilla-central/rev/d472eab26a31 https://hg.mozilla.org/mozilla-central/rev/8bd1560c17a3 https://hg.mozilla.org/mozilla-central/rev/32a3b6dde987