Closed Bug 1252405 Opened 4 years ago Closed 4 years ago

Reduce MaskLayer's memory usage

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 9 obsolete files)

5.69 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.88 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.21 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
SourceSurfaceImage::GetTextureClient() always allocates TextureClient. It seems nice to avoid it if possible.
Assignee: nobody → sotaro.ikeda.g
Summary: Create TextureClient that wrap SourceSurface → TextureClient that wraps SourceSurface
This seems like a good idea; I understand it really helps the single process scenario (e.g., Android or browser chrome)
Whiteboard: [gfx-noted]
Attachment #8725181 - Attachment is obsolete: true
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.
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.
Flags: needinfo?(nical.bugzilla)
(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.
Flags: needinfo?(nical.bugzilla)
(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.
Summary: TextureClient that wraps SourceSurface → Reduce MaskLayer's memory usage
Attachment #8725513 - Attachment is obsolete: true
Address assert failure.
Attachment #8739851 - Attachment is obsolete: true
attachment 8740256 [details] [diff] [review] expects that TextureClient's buffer is initialzed to 0. But the assumption was false.
Attachment #8740256 - Attachment is obsolete: true
TextureFlag check assert seems a remaining problem.
Update TextureFlags.
Attachment #8740297 - Attachment is obsolete: true
Attachment #8740788 - Flags: review?(nical.bugzilla)
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
Attachment #8740788 - Flags: review?(nical.bugzilla)
(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.
Apply the comment.
Attachment #8740788 - Attachment is obsolete: true
Attachment #8741228 - Flags: review?(matt.woodrow)
Attachment #8741228 - Flags: review?(matt.woodrow) → review+
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.
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().
Attachment #8742596 - Flags: review?(matt.woodrow)
Attachment #8745800 - Flags: review?(matt.woodrow)
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 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?
(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.
Attachment #8741228 - Attachment description: patch - Reduce MaskLayer's memory usage → patch part 1 - Reduce MaskLayer's memory usage
Attachment #8742596 - Attachment is obsolete: true
Attachment #8742596 - Flags: review?(matt.woodrow)
Attachment #8745800 - Flags: review?(matt.woodrow)
(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?
Flags: needinfo?(matt.woodrow)
Attachment #8746335 - Flags: review?(matt.woodrow)
(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
(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.
Flags: needinfo?(matt.woodrow)
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
Attachment #8746335 - Flags: review?(matt.woodrow) → review+
Apply the comment and add GetMaskLayerImageCache()->Sweep() in FrameLayerBuilder::~FrameLayerBuilder() based on a comment on irc.
Attachment #8746335 - Attachment is obsolete: true
Attachment #8746358 - Flags: review+
See Also: → 1066138
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.
See Also: → 1173983
:mattwoodrow, which is better to address the reftest failures? attachment 8746886 [details] [diff] [review] or attachment 8745800 [details] [diff] [review]?
Flags: needinfo?(matt.woodrow)
I think just updating the fuzz is fine.
Flags: needinfo?(matt.woodrow)
Attachment #8746886 - Attachment is obsolete: true
Attachment #8745800 - Flags: review?(matt.woodrow)
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?
Attachment #8745800 - Flags: review?(matt.woodrow) → review+
> 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.
Attachment #8745800 - Attachment description: patch - Add Fuzz → patch part 3 - Add Fuzz
https://hg.mozilla.org/mozilla-central/rev/d6eac6cc4588
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272114
You need to log in before you can comment on or make changes to this bug.