If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Simplify FrameLayerBuilder by removing draw region & draw above region

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8566129 [details] [diff] [review]
v1

PaintedLayerData tracks both a "draw region" and a "visible region". The draw region is the union of the clipped bounds of the display items in the PaintedLayer, and the visible region is the union of the clipped bounds intersected with the "visible rect" of the display items. Ever since bug 1022612, this visible rect is just the dirty rect that the display item has been initialized with.

I think that after bug 1022612 we can reduce the duplication here by ignoring the draw region and always using the visible region instead. This would make things slightly faster (fewer region operations), easier to read, and would avoid bugs like bug 1120431.

There are two places that make use of the draw region: FindPaintedLayerFor and AdjustLayerDataForFixedPositioning.

FindPaintedLayerFor checks the intersection of the visible rect of the to-be-added display item with both the "draw region" and the "draw above region" of the candidate layers. This comment in ProcessDisplayItems explains why:

          // Add the entire bounds rect to the mDrawAboveRegion.
          // The visible region may be excluding opaque content above the
          // item, and we need to ensure that that content is not placed
          // in a PaintedLayer below the item!
          data->AddDrawAboveRegion(itemDrawRect);

So it's basically working around display list occlusion culling that we did before bug 1022612. Now that we're no longer doing that, we should just be able to use the visible region, right?

AdjustLayerDataForFixedPositioning is more tricky, or at least confusing to me. It attempts to expand the visible region, and then intersects the enlarged region with the draw region. Is this also working around overeager occlusion culling (which we're no longer doing)? If it's still making a difference, aren't we painting bad content in the parts that we've added to the visible region, since there might be display items in those parts which we haven't added to the display list?
(I also don't understand why that function sets the visible region of background-attachment:fixed items to be the displayport, and not the viewport.)

Please check my reasoning. This patch passes reftests.
Attachment #8566129 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Blocks: 961887
(In reply to Markus Stange [:mstange] from comment #0)
> AdjustLayerDataForFixedPositioning is more tricky, or at least confusing to
> me. It attempts to expand the visible region, and then intersects the
> enlarged region with the draw region. Is this also working around overeager
> occlusion culling (which we're no longer doing)?

I think it's trying to work around display item visible-rects being restricted to the viewport when additional content might be visible in the displayport --- which was true, but no longer is.

> If it's still making a
> difference, aren't we painting bad content in the parts that we've added to
> the visible region, since there might be display items in those parts which
> we haven't added to the display list?

Right. I think nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay handles this properly now.

> (I also don't understand why that function sets the visible region of
> background-attachment:fixed items to be the displayport, and not the
> viewport.)
> 
> Please check my reasoning. This patch passes reftests.

I think you're doing the right thing.
Comment on attachment 8566129 [details] [diff] [review]
v1

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

This is excellent!
Attachment #8566129 - Flags: review?(roc) → review+
(Assignee)

Comment 3

3 years ago
Yay :-)
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/21771a04f183
https://hg.mozilla.org/mozilla-central/rev/21771a04f183
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

3 years ago
Depends on: 1151581
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
You need to log in before you can comment on or make changes to this bug.