Last Comment Bug 687372 - ImageLayerOGL should not destroy surface given as argument
: ImageLayerOGL should not destroy surface given as argument
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla10
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on: 695406
Blocks: 691417 598868
  Show dependency treegraph
 
Reported: 2011-09-18 13:39 PDT by Oleg Romashin (:romaxa)
Modified: 2011-10-18 11:25 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't destroy surface in ImageLayerOGL::Init (1.98 KB, patch)
2011-09-18 13:45 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review-
Details | Diff | Review
Simplify Image Layer Update API, remove Init, pass allocator in swap (34.54 KB, patch)
2011-09-20 22:44 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Simplify Image Layer Update API, remove Init, pass allocator before swap (36.69 KB, patch)
2011-09-24 10:57 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Simplify Image Layer Update API, remove Init, pass allocator before swap (36.74 KB, patch)
2011-09-26 15:41 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Simplify Image Layer Update API, remove Init, pass allocator before swap (36.89 KB, patch)
2011-09-26 22:42 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Review
Simplify Image Layer Update API, remove Init, pass allocator before swap. to PUSH (36.93 KB, patch)
2011-09-28 12:15 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review

Description Oleg Romashin (:romaxa) 2011-09-18 13:39:31 PDT
In Init function
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ImageLayerOGL.cpp#842
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ImageLayerOGL.cpp#873

ImageLayerOGL deallocating shared memory surface, and I think this is not good because original creator can loose reference to that surface.
Also in media bridge I'm creating that memory allocated by different protocol...
Comment 1 Oleg Romashin (:romaxa) 2011-09-18 13:45:30 PDT
Created attachment 560814 [details] [diff] [review]
Don't destroy surface in ImageLayerOGL::Init
Comment 2 Oleg Romashin (:romaxa) 2011-09-18 13:46:35 PDT
also I found that in other backend's we don't do this, so it is obviously wrong and if this surface need to be destroyed, then only in child process side.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-19 21:10:35 PDT
Comment on attachment 560814 [details] [diff] [review]
Don't destroy surface in ImageLayerOGL::Init

This code is "OK" (i.e. not crashy) currently because the content process is creating two buffers when it only needs one, with GL.  But that's a waste of resources.  What should actually be happening is that content process only creates one buffer when mManager->IsCompositingCheap().

This patch as is will cause shmem to leak in fennec with GL compositing on, so r-.  Please fix to use one buffer in the content process and then remove the hacky surface deletion here.
Comment 4 Oleg Romashin (:romaxa) 2011-09-19 22:52:23 PDT
Can we just remove Init call completely? and just call Swap on first paint?
So in that case sequence will be next:
BasicShadowLayers (Compositing not cheap)
  First ShadowableImageLayer Paint:
  1) mBackBuffer = Null - AllocBuffer
  2) paint buffer and call Swap
  3) Null surface returned, set as null mBackBuffer
  Second ShadowableImageLayer Paint:
  1) mBackBuffer = Null - AllocBuffer second
  2) paint buffer and call Swap
  3) Previous surface returned, mBackBuffer = returnedSurface
  ShadowableImageLayer Paint++:
  1) mBackBuffer != Null
  2) paint buffer and call Swap
  3) mBackBuffer = returnedSurface
  et.c.

On Destroy just call Swap with NULL buffer, will get mFrontBuffer and dealloc that.

YUV or OGL layers
  ShadowableImageLayer Paint++:
  1) mBackBuffer = Null - AllocBuffer
  2) paint buffer and call Swap
  3) mBackBuffer returned back


Is there are any problems with this swap implementation?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-20 00:07:31 PDT
Nope, sounds good to me.  On destroy though, the child should dealloc its back buffer, and the parent dealloc its front buffer if non-null (like what happens now).  Otherwise destroying layers will require a synchronous message per layer, and in addition to being fairly hard to implement in the current model, it's more overhead.
Comment 6 Oleg Romashin (:romaxa) 2011-09-20 00:09:23 PDT
> resources.  What should actually be happening is that content process only
> creates one buffer when mManager->IsCompositingCheap().
it is not good to rely on that, because we would still want to use Real swap in OGL layers, for example for PluginSurface-based on XID, which we can bind to Texture and render without any upload
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-20 00:23:03 PDT
Actually yeah, you want ShadowLayerForwarder::ShouldDoubleBuffer().  That should be fixed to return true if we're using X11 and have texture-from-pixmap.  ShadowImageLayerOGL would need a few more little tweaks to use that properly.
Comment 8 Oleg Romashin (:romaxa) 2011-09-20 01:02:00 PDT
roc do you have any comments on this issue?
Comment 9 Oleg Romashin (:romaxa) 2011-09-20 01:05:48 PDT
One suggestion is to Create common IFace for DeallocShmem API, and pass whatever parentProtocol allocator pointer behind that IFace, so Image Layerlayer can call that IFacePointer->DeallocShmem()-> which will be redirected to MEdiaBridge/PluginBridge or PLayer Parent protocol allocator
Comment 10 Oleg Romashin (:romaxa) 2011-09-20 22:44:17 PDT
Created attachment 561388 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator in swap
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-22 20:08:53 PDT
Comment on attachment 561388 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator in swap

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>+    if (mBackBufferY) {
>+      BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackBufferY);
>+      BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackBufferU);
>+      BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackBufferV);

Nice catch.  Sad that we haven't found this leak this yet :/.

We should convert mBackBufferY/U/V to SurfaceDescriptor.  Want to file
a followup?  I don't remember why we went with Shmem over
SurfaceDescriptor, and neither does Matt.

> 
>-    if (mSize != data->mYSize || mCbCrSize != data->mCbCrSize) {
>-
>+    if (mSize != data->mYSize || mCbCrSize != data->mCbCrSize || !mBackBufferY) {
>       if (mBackBufferY) {
>         BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackBufferY);
>         BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackBufferU);
>         BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackBufferV);

Factor this out and share it with the dtor.

>-        BasicManager()->DestroyedImageBuffer(BasicManager()->Hold(this));
>       }
>       mSize = data->mYSize;
>       mCbCrSize = data->mCbCrSize;
> 
>-      nsRefPtr<gfxSharedImageSurface> tmpYSurface;
>-      nsRefPtr<gfxSharedImageSurface> tmpUSurface;
>-      nsRefPtr<gfxSharedImageSurface> tmpVSurface;
>-
>-      if (!BasicManager()->AllocDoubleBuffer(
>+      if (!BasicManager()->AllocBuffer(

 if (!AllocBuffer() ||
     !AllocBuffer() ||
     !AllocBuffer())
   NS_RUNTIMEABORT()

plz.

>     if (IsSurfaceDescriptorValid(mFrontBufferDescriptor)) {
>-      BasicManager()->ShadowLayerManager::DestroySharedSurface(&mFrontBufferDescriptor, mAllocator);
>+      mAllocator->DestroySharedSurface(&mFrontBufferDescriptor);

Please change this for BasicShadowCanvasLayer while you're at it.


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

> class ShadowLayer
> {
> public:
>+  class ISurfaceDeAllocator
>+  {
>+  public:
>+    virtual void DestroySharedSurface(gfxSharedImageSurface* aSurface) = 0;
>+    virtual void DestroySharedSurface(SurfaceDescriptor* aSurface) = 0;
>+  };
>+

Make this a top-level class.  You need to hide the dtor here too, like so

 private:
   ~ISurfaceDeAllocator();

otherwise bad things will happen if code tries to delete ISurfaceDeAllocator*.

>   /**
>    * CONSTRUCTION PHASE ONLY
>    */
>-  void SetAllocator(PLayersParent* aAllocator)
>+  void SetAllocator(ISurfaceDeAllocator* aAllocator)
>   {
>-    NS_ABORT_IF_FALSE(!mAllocator, "Stomping allocator?");
>+    NS_ABORT_IF_FALSE(!mAllocator || mAllocator == aAllocator, "Stomping allocator?");

I don't like this change.  The allocator only needs to be set once,
when the layer is created.  Why are you changing this?

>-  /**
>-   * CONSTRUCTION PHASE ONLY
>-   *
>-   * Destroy the current front buffer.
>-   */
>-  virtual void DestroyFrontBuffer() = 0;
>+  virtual void Swap(const SharedImage& aFront,
>+                    ISurfaceDeAllocator* aAllocator,
>+                    SharedImage* aNewBack) = 0;

(See above re: aAllocator.)

I like this patch.  Would like to see v2.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-22 20:13:51 PDT
In general, this is a really nice approach: we should switch all the layer types over to this model.  By deferring creation of the second buffer until it's actually needed, we can save memory on layers that are totally static.  (I would imagine that's not too uncommon.  Even if it's not, it shouldn't hurt perf to defer.)  Then once we need the separate back buffer, we can allocate it and copy from the front buffer before repainting.  (Except for image layers, which don't need the readback.)

Want to hack on that?
Comment 13 Oleg Romashin (:romaxa) 2011-09-24 10:57:15 PDT
Created attachment 562245 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap
Comment 14 Oleg Romashin (:romaxa) 2011-09-26 15:41:54 PDT
Created attachment 562559 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap

Added check for mTexImage together with size checks..
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-26 17:18:11 PDT
Comment on attachment 562559 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap

>-  void SetAllocator(PLayersParent* aAllocator)
>+  virtual void SetAllocator(ISurfaceDeAllocator* aAllocator)
>   {
>-    NS_ABORT_IF_FALSE(!mAllocator, "Stomping allocator?");
>+    NS_ASSERTION(!mAllocator || mAllocator == aAllocator, "Stomping allocator?");
>     mAllocator = aAllocator;
>   }

You didn't address this.  The allocator should only be set once.  It
can't change.

Also, this method doesn't need to be virtual for this patch.
Comment 16 Oleg Romashin (:romaxa) 2011-09-26 17:30:41 PDT
I do call SetAllocator before every Swap call.... because calling SetAllocator from CreateIamgeLayer transaction will not allow to setup allocator from other protocol like media bridge...
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-26 17:34:53 PDT
Do you have a way to access the bridge/opens-channel creator when the image layer is allocated?  (CreateImageLayer.)
Comment 18 Oleg Romashin (:romaxa) 2011-09-26 22:42:14 PDT
Created attachment 562662 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap
Comment 19 Oleg Romashin (:romaxa) 2011-09-28 12:15:49 PDT
Created attachment 563139 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap. to PUSH

Fixed some windows related build problems
https://tbpl.mozilla.org/?tree=Try&rev=3c11a4c84212
Comment 21 Michael Wu [:mwu] 2011-09-29 01:32:26 PDT
https://hg.mozilla.org/mozilla-central/rev/261bb33e9c53

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