Closed Bug 958727 Opened 6 years ago Closed 6 years ago

Thebes Textures leak memory if the visible region shrinks

Categories

(Core :: Graphics, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Keywords: memory-leak, perf, Whiteboard: [c=memory p=5 s= u=])

Attachments

(2 files, 4 obsolete files)

Attached file 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.
Whiteboard: [c=memory p=5 s= u=] → [c=memory p=5 s= u=][mlk]
Still on it, waiting to finish fixing bug 917416.
Keywords: mlk
Whiteboard: [c=memory p=5 s= u=][mlk] → [c=memory p=5 s= u=]
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.
Attached patch WIP - rcb.patch (obsolete) — Splinter Review
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.
Attached patch bufferShrink.patch (obsolete) — Splinter Review
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!
Attachment #8371204 - Flags: review?(matt.woodrow)
Attachment #8370545 - Attachment is obsolete: true
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.
Attached patch bufferShrink.patch (obsolete) — Splinter Review
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.
Attachment #8371204 - Attachment is obsolete: true
Attachment #8371204 - Flags: review?(matt.woodrow)
Attachment #8372527 - Flags: review?(matt.woodrow)
Blocks: 969591
Attachment #8372527 - Flags: review?(matt.woodrow) → review+
Attached patch bufferShrink.patch (obsolete) — Splinter Review
Carrying r+ from mattwoodrow.
Attachment #8372527 - Attachment is obsolete: true
Attachment #8374747 - Flags: review+
Carrying r+ from mattwoodrow. Formatted for checkin-needed friendly.
Attachment #8374747 - Attachment is obsolete: true
Attachment #8374748 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0569a8a2f83d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/integration/b2g-inbound/rev/82a063f4ae39
Flags: needinfo?(ryanvm)
Resolution: FIXED → WONTFIX
Target Milestone: mozilla30 → ---
You need to log in before you can comment on or make changes to this bug.