969591 - Cleanup Thebes Layer if mRegionToDraw is Empty and we still have a buffer allocated

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P3)

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: clearBuffers.patch

Hey Matt, just a question. Why would we have a case where the visible region is not empty, but the region to draw is empty? Is that a separate bug that we should be investigating instead? Thanks! Otherwise, with this patch, we clean up the memory leak original reported in bug 958727.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8372654 [details] [diff] [review]
clearBuffers.patch

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

::: gfx/layers/RotatedBuffer.cpp
@@ +516,5 @@
> +  // a clean buffer for those cases. If everything is empty, then clear out
> +  // the buffers.
> +  if (result.mRegionToDraw.IsEmpty() && !canReuseBuffer &&
> +      validRegion.IsEmpty()) {
> +    Clear();

If think this should be a nested if() only testing !canReuseBuffer. We always want to return here if mRegionToDraw is empty, and sometimes clear the buffers as well.

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Mason Chang [:mchang] from comment #1)
> 
> Hey Matt, just a question. Why would we have a case where the visible region
> is not empty, but the region to draw is empty? Is that a separate bug that
> we should be investigating instead? Thanks! Otherwise, with this patch, we
> clean up the memory leak original reported in bug 958727.

This happens when there has been no invalidation since the last paint. It should be a fairly common, and valid case.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: clearBuffers.patch

Thanks for the pointer Matt, that did it. It also shrinks the buffer the targeted buffer:

ThebesLayerComposite (0x43e55000) [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); >]
  DeprecatedContentHostDoubleBuffered (0x4656c2b0) [buffer-rect=(x=0, y=0, w=320, h=32)] [buffer-rotation=(x=0, y=0)]
    GrallocDeprecatedTextureHostOGL (0x4530b400) [size=(width=320, height=32)] [format=SurfaceFormat::B8G8R8X8] [flags=TEXTURE_USE_NEAREST_FILTER]
    GrallocDeprecatedTextureHostOGL (0x45308f00) [size=(width=0, height=0)] [format=SurfaceFormat::UNKNOWN] [flags=TEXTURE_USE_NEAREST_FILTER]


Last question, any idea why the last buffer is empty while the other buffer has [320x32]? This seems like a new issue with this patch. Is that a separate bug or that makes sense? I've never seen double buffered thebes layers have different sizes. Thanks!

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8373117 [details] [diff] [review]
clearBuffers.patch

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

::: gfx/layers/RotatedBuffer.cpp
@@ +515,5 @@
> +  // but the valid Region is not empty, so we still need to allocate
> +  // a clean buffer for those cases. If everything is empty, then clear out
> +  // the buffers.
> +  if (result.mRegionToDraw.IsEmpty() && canReuseBuffer) {
> +    if (validRegion.IsEmpty()) {

actually, this should be 'neededRegion'.

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Mason Chang [:mchang] from comment #4)
> Created attachment 8373117 [details] [diff] [review]
> clearBuffers.patch
> 
> Thanks for the pointer Matt, that did it. It also shrinks the buffer the
> targeted buffer:
> 
> ThebesLayerComposite (0x43e55000) [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); >]
>   DeprecatedContentHostDoubleBuffered (0x4656c2b0) [buffer-rect=(x=0, y=0,
> w=320, h=32)] [buffer-rotation=(x=0, y=0)]
>     GrallocDeprecatedTextureHostOGL (0x4530b400) [size=(width=320,
> height=32)] [format=SurfaceFormat::B8G8R8X8]
> [flags=TEXTURE_USE_NEAREST_FILTER]
>     GrallocDeprecatedTextureHostOGL (0x45308f00) [size=(width=0, height=0)]
> [format=SurfaceFormat::UNKNOWN] [flags=TEXTURE_USE_NEAREST_FILTER]
> 
> 
> Last question, any idea why the last buffer is empty while the other buffer
> has [320x32]? This seems like a new issue with this patch. Is that a
> separate bug or that makes sense? I've never seen double buffered thebes
> layers have different sizes. Thanks!

Does it stay this way? I think we only modify the back-buffer, since the compositor 'owns' the front buffer. We won't make the same changes to the front buffer until we swap them.

Comment 7

[Mason Chang [Inactive] [:mchang]]

For some layers, it stays this way, while in other layers it settles. For example, in the settings app, the last layer that is the scrollable layer has an [0x0] layer buffer until we stop scrolling. The settings Header that says 'Settings', never settles and always has an empty buffer:

ThebesLayerComposite (0x4735bc00) [shadow-visible=< (x=0, y=0, w=320, h=50); >] [visible=< (x=0, y=0, w=320, h=50); >] [opaqueContent] [valid=< (x=0, y=0, w=320, h=50); >]
  DeprecatedContentHostDoubleBuffered (0x47393080) [buffer-rect=(x=0, y=0, w=320, h=50)] [buffer-rotation=(x=0, y=0)]
    GrallocDeprecatedTextureHostOGL (0x458028c0) [size=(width=320, height=50)] [format=SurfaceFormat::B8G8R8X8] [flags=NoFlags]
    GrallocDeprecatedTextureHostOGL (0x458026c0) [size=(width=0, height=0)] [format=SurfaceFormat::UNKNOWN] [flags=NoFlags]

Visually, I don't see any issues.

Comment 8

[Matt Woodrow (:mattwoodrow)]

I think this is ok.

If we're not drawing into the Settings header, then this makes sense. We draw one, allocate the sized buffer, and give it to the compositor.

If we never do any invalidation, then we'll never need to allocate the back buffer.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: clearBuffers.patch

Carrying r+ from mattwoodrow.

Comment 10

[Mason Chang [Inactive] [:mchang]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/68de70a3d9f8

Please make sure that future patches include commit information when requesting checkin. Thanks :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for assertions.
https://hg.mozilla.org/integration/b2g-inbound/rev/67ea2ad5b39e

https://tbpl.mozilla.org/php/getParsedLog.php?id=34421493&tree=B2g-Inbound

Comment 13

[Mason Chang [Inactive] [:mchang]]

From backout, still have to investigate if still an issue. Reopen if still an issue.