Closed Bug 1055821 Opened 6 years ago Closed 6 years ago

Add layer construction logging

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Right now it's too difficult to find out why a layer is being constructed in such a way.

This patch adds structured logging per layers that surface the decisions that lead to a layer being constructed in a particular way. Currently it focuses on thebes/color/image layers decisions but it can be extended to cover more.
Attachment #8475506 - Flags: review?(matt.woodrow)
Comment on attachment 8475506 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2387,5 @@
> +  if (mVisibleRegion.IsEqual(nsIntRegion(aVisibleRect)) &&
> +             mVisibleRegion.IsEqual(aClippedOpaqueRegion) &&
> +             clipMatches &&
> +             aItem->SupportsOptimizingToImage()) {
> +    mImage = static_cast<nsDisplayImageContainer*>(aItem);

Feel free to tell me to break this into another patch.

I don't really know if we want to be doing this or if this should never happen because covered items should be removed earlier (by what piece of code exactly if so?).

Here's an example with the b2g system background. These items go into the same layer:
I/Gecko   (16366):   CanvasBackgroundColor p=0xaf276c60 f=0xb05f77a0(Canvas(html)(-1)) bounds(0,0,19200,34160) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,34160)  uniform (opaque 0,0,19200,34160) (rgba 0,0,0,255) layer=0xaf242b80
I/Gecko   (16366):   BackgroundColor p=0xaf276cb0 f=0xaf739090(HTMLScroll(div)(1)) bounds(0,0,19200,34160) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,34160)  uniform (opaque 0,0,19200,34160) (rgba 0.000000,0.000000,0.000000,1.000000) layer=0xaf242b80
I/Gecko   (16366):   Background p=0xaf276d20 f=0xaf739090(HTMLScroll(div)(1)) bounds(0,0,19200,34160) visible(0,0,19200,34160) componentAlpha(0,0,0,0) clip(0,0,19200,34160)  (opaque 0,0,19200,34160) layer=0xaf242b80
Comment on attachment 8475506 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2387,5 @@
> +  if (mVisibleRegion.IsEqual(nsIntRegion(aVisibleRect)) &&
> +             mVisibleRegion.IsEqual(aClippedOpaqueRegion) &&
> +             clipMatches &&
> +             aItem->SupportsOptimizingToImage()) {
> +    mImage = static_cast<nsDisplayImageContainer*>(aItem);

Yes, let's do this as a separate patch :)

It seems valid though, and you can make it aClippedOpaqueRegion.Contains(mVisibleRegion) instead to cover cases where the image is bigger than the content already in the layer.

I think we can also do this in the same condition that sets mImage with an empty visible region, rather than having it separate.
Comment on attachment 8475506 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2301,4 @@
>                              const nsIntRect& aDrawRect,
>                              const DisplayItemClip& aClip)
>  {
> +  FLB_LOG_THEBES_DECISION(this, "Accumulating dp=%p, f=%p against tld=%p\n", aItem, aItem->Frame(), this);

Dump aItem->Name() here too? Like "%s(%p)", aItem->Name(), aItem maybe.

Names are much easier to read than pointers.
Attachment #8475506 - Flags: review?(matt.woodrow) → review+
I can add it, but the information is already in the log. It does save a lookup however.
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=49da09974994
Assignee: nobody → bgirard
Attachment #8475506 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8476803 - Flags: review+
Attachment #8476803 - Attachment is obsolete: true
Attachment #8476813 - Flags: review+
Blocks: 1056944
checkin-needed to coalesce with simple changes.
Keywords: checkin-needed
This is what I get for being too conservative with build requests.
Attachment #8476813 - Attachment is obsolete: true
Attachment #8477454 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/73a965cfceba
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.