Closed Bug 771350 Opened 12 years ago Closed 12 years ago

Support direct texturing of gralloc buffers in ShadowThebesLayerOGL

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(3 files, 23 obsolete files)

part 0: Remove some dead code to make later patches clearer
5.50 KB, patch
BenWa
: 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
roc
: review+
Details | Diff | Splinter Review
part 2: Use OpenDescriptorForTexturing() in ShadowThebesLayerOGL, where possible, v3
7.71 KB, patch
BenWa
: review+
Details | Diff | Splinter Review 
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.
Attached patch WIP v2 (obsolete) — — Splinter Review 
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
Attachment #639479 - Attachment is obsolete: true
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)
Attached patch WIP, v3 (works) (obsolete) — — Splinter Review 
It works, and it's fast :D.

The basic layers changes belong in bug 765734.  This patch needs a lot of cleanup before landing.
Attachment #639492 - Attachment is obsolete: true
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
Attached patch WIP, v4 (obsolete) — — Splinter Review 
Fixes the size mismatch assertion.  (Restores code to destroys old buffers when they go stale.)
Attachment #639601 - Attachment is obsolete: true
Attached patch WIP, v5 (obsolete) — — Splinter Review 
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.
Attachment #639845 - Attachment is obsolete: true
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?
Assignee: nobody → cbrocious
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.
Attachment #639852 - Attachment is obsolete: true
Attachment #643708 - Flags: review?(jones.chris.g)
Comment on attachment 643708 [details] [diff] [review]
First step of splitting up the patch

<3
Attachment #643708 - Flags: review?(jones.chris.g) → review+
Blocks: 775436
No longer blocks: 775436
Depends on: 775436
Comment on attachment 643708 [details] [diff] [review]
First step of splitting up the patch

Landed separately.
Attachment #643708 - Attachment is obsolete: true
Blocks: 775649
No longer blocks: 775649
Depends on: 775649
Attached patch Initial step2 reconstruction (obsolete) — — Splinter Review 
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
Attached patch handoff (obsolete) — — Splinter Review 

    
  

        Attachment #644036 -
        Attachment is patch: true

    
    

    
  


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

        Attachment #643942 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #644036 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        add DirectTextureImageEGL (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #644105 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        remove some dead code (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  

        Attachment #644060 -
        Attachment is obsolete: true

        Attachment #644115 -
        Attachment is obsolete: true

    
    

    
  


  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rollup (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Changes (obsolete)
        — 
          — Splinter Review
      

    


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

        Attachment #644586 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Changes, dead end I think (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #644591 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  


  
Assignee: cbrocious → jones.chris.g

        Attachment #644124 -
        Attachment is obsolete: true

        Attachment #644183 -
        Attachment is obsolete: true

        Attachment #644469 -
        Attachment is obsolete: true

        Attachment #644555 -
        Attachment is obsolete: true

        Attachment #644557 -
        Attachment is obsolete: true

        Attachment #644559 -
        Attachment is obsolete: true

        Attachment #644593 -
        Attachment is obsolete: true

        Attachment #644605 -
        Flags: review?(bgirard)

        Attachment #644606 -
        Flags: review? → review?(vladimir)

    
    

    
  


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

        Attachment #644606 -
        Attachment is obsolete: true

        Attachment #644606 -
        Flags: review?(vladimir)

        Attachment #644608 -
        Flags: review?(vladimir)

    
    

    
  
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.

        Attachment #644605 -
        Flags: review?(bgirard) → review+

    
    

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

        Attachment #644607 -
        Flags: review?(bgirard) → review-

    
    

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

        Attachment #644608 -
        Flags: review?(vladimir) → review?(roc)

    
    

    
  
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.

        Attachment #644694 -
        Attachment is obsolete: true

        Attachment #644694 -
        Flags: review?(bgirard)

        Attachment #645061 -
        Flags: review?(bgirard)

    
    

    
  
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.

        Attachment #645061 -
        Flags: review?(bgirard) → review+

    
    

    
  
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)

        Attachment #644608 -
        Flags: review?(roc) → review+

    
    

    
  
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.
   */
Depends on: 777094

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: