1252405 - Reduce MaskLayer's memory usage

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

SourceSurfaceImage::GetTextureClient() always allocates TextureClient. It seems nice to avoid it if possible.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add SourceSurfaceWrapTextureData

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

This seems like a good idea; I understand it really helps the single process scenario (e.g., Android or browser chrome)

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add SourceSurfaceWrapTextureData

Comment 4

[Nicolas Silva [:nical]]

TextureClient is meant to be what hold (and more importantly owns) data that can be shared with the compositor. If we do this it mean we add a TextureClient type that don't provide the same guarantees and behavior as the other TextureClients.
In my opinion, the actual issue is the usage of SourceSurfaceImage itself rather than the fact that it allocates a new texture each time. Instead of using SourceSufaceImage we should be using SharedRGBImage, which stores its data in a TextureClient to avoid copying.

Comment 5

[Sotaro Ikeda [:sotaro]]

SharedRGBImage does not have a capability to handle multiple CompositableForwarders that SourceSurfaceImage::GetTextureClient() is doing. How do you think to handle to it? It seems not clear from your comment.

Comment 6

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> SharedRGBImage does not have a capability to handle multiple
> CompositableForwarders that SourceSurfaceImage::GetTextureClient() is doing.
> How do you think to handle to it? It seems not clear from your comment.

Then we should add that capability to SharedRGBImage. We could have it be associated with a particular forwarder without copy (that's the current situation), and create copies for the other forwarders which would be stored in a map akin to what SourceSurfaceImage does. Using the same texture on several forwarders is, as far as I know, a very particular media use-case and we don't see it much in the web, so I assume we can live with that extra copy. If the copy turns out to be too expensive, we could add support for a texture to be referenced by several forwarders (the TextureClient/Host would be associated to one given forwarder, and other forwarders would see a "fake" proxy TextureClient/Host that refers to the real texture on each side).

I think implementing this proxy texture would be rather easy, I am not convinced we actually need it at this point since the use-case is pretty rare on the web.

I would much prefer to go this route than to add a texture type that don't own its data, because doing the latter has been a source of problems for a long while.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> SharedRGBImage does not have a capability to handle multiple
> CompositableForwarders that SourceSurfaceImage::GetTextureClient() is doing.

I misunderstand about it. For mask layer's use case, we do not need to think about it.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce MaskLayer's memory usage

Comment 9

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f297d7915a63

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce MaskLayer's memory usage

Address assert failure.

Comment 11

[Sotaro Ikeda [:sotaro]]

attachment 8740256 [details] [diff] [review] expects that TextureClient's buffer is initialzed to 0. But the assumption was false.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce MaskLayer's memory usage

Comment 13

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cf6ec82f131

Comment 14

[Sotaro Ikeda [:sotaro]]

TextureFlag check assert seems a remaining problem.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce MaskLayer's memory usage

Update TextureFlags.

Comment 16

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ac5077dc6e5

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 8740788 [details] [diff] [review]
patch - Reduce MaskLayer's memory usage

Review of attachment 8740788 [details] [diff] [review]:
-----------------------------------------------------------------

I think the logic is good but I'd like the code to be moved to a different place, see comments below.

::: gfx/layers/ImageContainer.cpp
@@ +720,5 @@
> +                                    TextureAllocationFlags::ALLOC_CLEAR_BUFFER);
> +  if (!mTextureClient) {
> +    return nullptr;
> +  }
> +  

nit trailing space

::: gfx/layers/ImageContainer.h
@@ +840,5 @@
>    nsCountedRef<nsOwningThreadSourceSurfaceRef> mSourceSurface;
>    nsDataHashtable<nsUint32HashKey, RefPtr<TextureClient> >  mTextureClients;
>  };
>  
> +class MaskImageData

I think this doesn't belong in ImageContainer.h/cpp since MaskImageData doesn't interact with ImageContainer at all (and There's already too much loosely-related stuff in ImageContainer.h/cpp to my taste). If this is only used once in FrameLayerBuilder, why not inline the logic in there (unless you are planning to reuse MaskImageData in other places)?

Even if you feel like this would make ContainerState::CreateMaskLayer too long, I would still much prefer this code to be in FrameLayerBuilder.cpp since it's the only place where it is used, so that we don't get the overhead of having to look for where it is implemented when reading the code (and ImageContainer.cpp is definitely not the first place I'd check when looking for mask related things).

::: layout/base/FrameLayerBuilder.cpp
@@ +6091,5 @@
>                                               newData.mAppUnitsPerDevPixel,
>                                               0,
>                                               aRoundedRectClipCount);
>  
> +    //RefPtr<SourceSurface> surface = dt->Snapshot();

nit: remove the commented out line

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #17)
> Comment on attachment 8740788 [details] [diff] [review]
> patch - Reduce MaskLayer's memory usage
> 
> Review of attachment 8740788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the logic is good but I'd like the code to be moved to a different
> place, see comments below.
> 
> ::: gfx/layers/ImageContainer.cpp
> @@ +720,5 @@
> > +                                    TextureAllocationFlags::ALLOC_CLEAR_BUFFER);
> > +  if (!mTextureClient) {
> > +    return nullptr;
> > +  }
> > +  
> 
> nit trailing space
> 
> ::: gfx/layers/ImageContainer.h
> @@ +840,5 @@
> >    nsCountedRef<nsOwningThreadSourceSurfaceRef> mSourceSurface;
> >    nsDataHashtable<nsUint32HashKey, RefPtr<TextureClient> >  mTextureClients;
> >  };
> >  
> > +class MaskImageData
> 
> I think this doesn't belong in ImageContainer.h/cpp since MaskImageData
> doesn't interact with ImageContainer at all (and There's already too much
> loosely-related stuff in ImageContainer.h/cpp to my taste). If this is only
> used once in FrameLayerBuilder, why not inline the logic in there (unless
> you are planning to reuse MaskImageData in other places)?

No, it is going to be used only in FrameLayerBuilder. I agree, I am going to move it to FrameLayerBuilder.cpp.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Reduce MaskLayer's memory usage

Apply the comment.

Comment 20

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a05a8a9a858b

Comment 21

[Sotaro Ikeda [:sotaro]]

Some test failures on the following assert.

> Assertion failure: !mActor || mActor->mDestroyed (Cannot use a texture on several IPC channels.)

I could not reproduce locally, but it seems to be caused by gMaskLayerImageCache. It reuse MaskLayerImage within same process. But with the patch, when TextureClient is used for the MaskLayer data, the TextureClient is restricted to CompositableForwarder(ClientLayerManager).

There seems several way to bypass the problem. One way is wait Bug 1264764 fix.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update MaskLayerImageCache

Comment 23

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=792da0037f9b&selectedJob=19537086

Some test failures because of one DrawTargetCairo leak detection. There seemed a case that GetMaskLayerImageCache()->Sweep() is not called since MaskLayerUserData destruction. GetMaskLayerImageCache()->Sweep() is called only in FrameLayerBuilder::DidEndTransaction().

Comment 24

[Sotaro Ikeda [:sotaro]]

From the following, Comment 23 is wrong.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82bc33e646bb

Comment 25

[Sotaro Ikeda [:sotaro]]

gfx ipc close fix(Bug 1262898) seemed to address the leak.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=457f9404bd89&selectedJob=19993975

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Add Fuzz

Comment 27

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1d12b4e2b0d

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8742596 [details] [diff] [review]
patch - Update MaskLayerImageCache

Review of attachment 8742596 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/MaskLayerImageCache.h
@@ +165,4 @@
>      }
>  
>      nsTArray<PixelRoundedRect> mRoundedClipRects;
> +    ShadowLayerForwarder* mForwarder;

Couldn't we run into issues with pointer re-use since we're not holding a strong reference?

Seems like we should hold a ref here, or use a unique id number instead (to avoid needing to worry about lifetime issues).

Comment 29

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8745800 [details] [diff] [review]
patch part 3 - Add Fuzz

Review of attachment 8745800 [details] [diff] [review]:
-----------------------------------------------------------------

Why does avoiding a copy causes differences in rendering?

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #28)
> Comment on attachment 8742596 [details] [diff] [review]
> patch - Update MaskLayerImageCache
> 
> Review of attachment 8742596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/MaskLayerImageCache.h
> @@ +165,4 @@
> >      }
> >  
> >      nsTArray<PixelRoundedRect> mRoundedClipRects;
> > +    ShadowLayerForwarder* mForwarder;
> 
> Couldn't we run into issues with pointer re-use since we're not holding a
> strong reference?
> 
> Seems like we should hold a ref here, or use a unique id number instead (to
> avoid needing to worry about lifetime issues).

Yea, it should hold a ref.

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Update MaskLayerImageCache

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #29)
> 
> Why does avoiding a copy causes differences in rendering?

In current gecko, DrawTargetD2D1 is always created as DrawTarget. But when the patches are applied, DrawTargetCairo for MaskLayer, since DXGITextureData::Create() does not support SurfaceFormat::A8 and BufferTextureData is created for TextureClient.
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp#338

Is there a reason why DXGITextureData does not support SurfaceFormat::A8?

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro ouf of office 29/Apr - 8/May] from comment #32)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #29)
> > 
> > Why does avoiding a copy causes differences in rendering?
> 
> In current gecko, DrawTargetD2D1 is always created as DrawTarget. But when
> the patches are applied, DrawTargetCairo for MaskLayer,

BufferTextureData::BorrowDrawTarget() created DrawTargetCairo.
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/BufferTexture.cpp#261

Comment 34

[Matt Woodrow (:mattwoodrow)]

(In reply to Sotaro Ikeda [:sotaro ouf of office 29/Apr - 8/May] from comment #32)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #29)
> > 
> > Why does avoiding a copy causes differences in rendering?
> 
> In current gecko, DrawTargetD2D1 is always created as DrawTarget. But when
> the patches are applied, DrawTargetCairo for MaskLayer, since
> DXGITextureData::Create() does not support SurfaceFormat::A8 and
> BufferTextureData is created for TextureClient.
>  
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.
> cpp#338
> 
> Is there a reason why DXGITextureData does not support SurfaceFormat::A8?

See bug 1066138.

Comment 35

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8746335 [details] [diff] [review]
patch part 2 - Update MaskLayerImageCache

Review of attachment 8746335 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/MaskLayerImageCache.cpp
@@ +49,5 @@
>  }
>  
>  MaskLayerImageCache::MaskLayerImageKey::MaskLayerImageKey()
>    : mRoundedClipRects()
> +  , mForwarder(nullptr)

No need to initialise a RefPtr

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Update MaskLayerImageCache

Apply the comment and add GetMaskLayerImageCache()->Sweep() in FrameLayerBuilder::~FrameLayerBuilder() based on a comment on irc.

Comment 37

[Sotaro Ikeda [:sotaro]]

Then, I tried to enable DXGITextureData's SurfaceFormat::A8 support, to address reftest failures. But it seems not simple. It seems better to put off from this bug. Bug 1173983 made R8 as SurfaceFormat::A8 in d3d11, but moz2d still use A8 for SurfaceFormat::A8. CreateBitmapFromDxgiSurface to DXGI_FORMAT_R8_UNORM was also failed with 0x88982f80.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Disable texture for mask data on d3d11

Comment 39

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4598e5c4f685&selectedJob=20101178

Comment 40

[Sotaro Ikeda [:sotaro]]

:mattwoodrow, which is better to address the reftest failures? attachment 8746886 [details] [diff] [review] or attachment 8745800 [details] [diff] [review]?

Comment 41

[Matt Woodrow (:mattwoodrow)]

I think just updating the fuzz is fine.

Comment 42

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8745800 [details] [diff] [review]
patch part 3 - Add Fuzz

Review of attachment 8745800 [details] [diff] [review]:
-----------------------------------------------------------------

Why does avoiding a copy causes differences in rendering?

Comment 43

[Sotaro Ikeda [:sotaro]]

> Review of attachment 8745800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why does avoiding a copy causes differences in rendering?

Just avoiding the difference should not cause the difference. As in comment 32, with attachment 8741228 [details] [diff] [review], different draw target is created, it seems to cause the difference.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6eac6cc4588

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6eac6cc4588

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12b3ef0b79d0

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12b3ef0b79d0