687372 - ImageLayerOGL should not destroy surface given as argument

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

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

Attachment: Don't destroy surface in ImageLayerOGL::Init

Comment 2

[Oleg Romashin (:romaxa)]

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]

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

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]

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

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

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

roc do you have any comments on this issue?

Comment 9

[Oleg Romashin (:romaxa)]

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

Attachment: Simplify Image Layer Update API, remove Init, pass allocator in swap

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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]

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

Attachment: Simplify Image Layer Update API, remove Init, pass allocator before swap

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: 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]

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

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]

Do you have a way to access the bridge/opens-channel creator when the image layer is allocated?  (CreateImageLayer.)

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Simplify Image Layer Update API, remove Init, pass allocator before swap

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: 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 20

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/261bb33e9c53

Comment 21

[Michael Wu [:mwu]]

https://hg.mozilla.org/mozilla-central/rev/261bb33e9c53