Note: There are a few cases of duplicates in user autocompletion which are being worked on.

ImageLayerOGL should not destroy surface given as argument

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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...
(Assignee)

Comment 1

6 years ago
Created attachment 560814 [details] [diff] [review]
Don't destroy surface in ImageLayerOGL::Init
Attachment #560814 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 598868
(Assignee)

Updated

6 years ago
Attachment #560814 - Flags: review?(jones.chris.g)
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.
Attachment #560814 - Flags: review?(matt.woodrow)
Attachment #560814 - Flags: review?(jones.chris.g)
Attachment #560814 - Flags: review-
(Assignee)

Comment 4

6 years ago
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?
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.
(Assignee)

Comment 6

6 years ago
> 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
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.
(Assignee)

Comment 8

6 years ago
roc do you have any comments on this issue?
(Assignee)

Comment 9

6 years ago
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
(Assignee)

Comment 10

6 years ago
Created attachment 561388 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator in swap
Assignee: nobody → romaxa
Attachment #560814 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #561388 - Flags: review?(jones.chris.g)
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.
Attachment #561388 - Flags: review?(jones.chris.g)
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?
(Assignee)

Comment 13

6 years ago
Created attachment 562245 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap
Attachment #561388 - Attachment is obsolete: true
Attachment #562245 - Flags: review?(jones.chris.g)
(Assignee)

Comment 14

6 years ago
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..
Attachment #562245 - Attachment is obsolete: true
Attachment #562245 - Flags: review?(jones.chris.g)
Attachment #562559 - Flags: review?(jones.chris.g)
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.
Attachment #562559 - Flags: review?(jones.chris.g)
(Assignee)

Comment 16

6 years ago
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...
Do you have a way to access the bridge/opens-channel creator when the image layer is allocated?  (CreateImageLayer.)
(Assignee)

Comment 18

6 years ago
Created attachment 562662 [details] [diff] [review]
Simplify Image Layer Update API, remove Init, pass allocator before swap
Attachment #562559 - Attachment is obsolete: true
Attachment #562662 - Flags: review?(jones.chris.g)
Attachment #562662 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 19

6 years ago
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
Attachment #562662 - Attachment is obsolete: true
(Assignee)

Comment 20

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

Comment 21

6 years ago
https://hg.mozilla.org/mozilla-central/rev/261bb33e9c53
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
Blocks: 691417
(Assignee)

Updated

6 years ago
Depends on: 695406
You need to log in before you can comment on or make changes to this bug.