958727 - Thebes Textures leak memory if the visible region shrinks

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P2)

Description

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree Showing Bug

Looking at the layer tree here, we see that even though the visible region is only height=20, the texture is still allocated for h=480. Clean up the memory here. 

This only occurs if a thebes layer was first a certain height, then shrunk due to another optimization.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Still on it, waiting to finish fixing bug 917416.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Have a basic patch working that can detect when the visible region is smaller than the texture region. Have to figure out how to correctly invalidate a texture.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - rcb.patch

Patch that allocates a new buffer if the visible region * 2 < mBufferRect. Problem is we have an empty visible region and don't deallocate, so we force a paint for now. We see our layers before:

I/Gecko   ( 4789):       ThebesLayerComposite (0x48ad2400) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [valid=< (x=0, y=0, w=320, h=480); >]
I/Gecko   ( 4789):         DeprecatedContentHostDoubleBuffered (0x45674900) [buffer-rect=(x=0, y=0, w=320, h=480)] [buffer-rotation=(x=0, y=0)]
I/Gecko   ( 4789):           GrallocDeprecatedTextureHostOGL (0x4649e3c0) [size=(width=320, height=480)] [format=SurfaceFormat::B8G8R8X8] [flags=TEXTURE_USE_NEAREST_FILTER]
I/Gecko   ( 4789):           GrallocDeprecatedTextureHostOGL (0x4649e440) [size=(width=320, height=480)] [format=SurfaceFormat::B8G8R8X8] [flags=TEXTURE_USE_NEAREST_FILTER]


And After:

I/Gecko   ( 4789):       ThebesLayerComposite (0x48ad2400) [shadow-visible=< (x=0, y=0, w=320, h=20); >] [visible=< (x=0, y=0, w=320, h=20); >] [opaqueContent] [valid=< (x=0, y=0, w=320, h=20); >]
I/Gecko   ( 4789):         DeprecatedContentHostDoubleBuffered (0x45674900) [buffer-rect=(x=0, y=0, w=320, h=32)] [buffer-rotation=(x=0, y=0)]
I/Gecko   ( 4789):           GrallocDeprecatedTextureHostOGL (0x45508b80) [size=(width=320, height=32)] [format=SurfaceFormat::B8G8R8X8] [flags=TEXTURE_USE_NEAREST_FILTER]
I/Gecko   ( 4789):           GrallocDeprecatedTextureHostOGL (0x45508ac0) [size=(width=0, height=0)] [format=SurfaceFormat::UNKNOWN] [flags=TEXTURE_USE_NEAREST_FILTER]

It takes a couple of composited frames before the change shows up though.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: bufferShrink.patch

Thanks for the pointer on where to look. Looks like that was the right spot and we clean up the buffer in this case. However, it looked like the region to draw was always empty in this particular case, so we still have to go through one paint to clean up the buffer. Then the next paint we should bail out. Is this the right approach? Thanks!

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8371204 [details] [diff] [review]
bufferShrink.patch

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

::: gfx/layers/RotatedBuffer.cpp
@@ +513,5 @@
>    FinalizeFrame(result.mRegionToDraw);
>  
> +  // Only bail out if we can reuse the buffer. Otherwise we can have empty
> +  // regions that still have buffers and we want to deallocate those.
> +  if (result.mRegionToDraw.IsEmpty() && canReuseBuffer) {

Can we call Clear() here (if !canReuseBuffer) rather than going through the buffer allocation path with a 0 size? I think that would be more obvious.

Also, please put this in a separate patch, since it's fairly distinct from the other change in this patch.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: bufferShrink.patch

Thanks for the feedback. Updated the patch to only include the detection of a smaller region. Will spawn off a new bug to clean 0 drawing region buffers.

Comment 7

[Mason Chang [Inactive] [:mchang]]

https://tbpl.mozilla.org/?tree=Try&rev=ba53cedfdf7b

Comment 8

[Mason Chang [Inactive] [:mchang]]

https://tbpl.mozilla.org/?tree=Try&rev=6a7bee3d855c

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: bufferShrink.patch

Carrying r+ from mattwoodrow.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: bufferShrink.patch

Carrying r+ from mattwoodrow. Formatted for checkin-needed friendly.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0569a8a2f83d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0569a8a2f83d

Comment 13

[Mason Chang [Inactive] [:mchang]]

Ryan, can you please back this out and leave closed? Thanks!

For anyone else coming here, see https://bugzilla.mozilla.org/show_bug.cgi?id=974025#c9.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/82a063f4ae39