Closed Bug 1442190 Opened 7 years ago Closed 7 years ago

Flatten inactive nsDisplayOpacity display items

Categories

(Core :: Web Painting, enhancement, P2)

Product:
Core
Component:
Web Painting
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mikokm, Assigned: mikokm)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 3 obsolete files)

Bug 1442190 - Part 1: Add nsDisplayOpacityGeometry
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1442190 - Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1442190 - Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1442190 - Part 4: Add FLBDisplayItemIterator
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1442190 - Part 6: Make three reftests fuzzy
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
This bug tracks the work of implementing flattening, as described in bug 1439292, for nsDisplayOpacity items.
Attached patch wip.diff (obsolete) — — Splinter Review
Current work in progress.
Priority: -- → P2
Attachment #8955080 - Attachment is patch: true
Attachment #8955080 - Attachment mime type: text/x-patch → text/plain
Attached patch wip-v2.diff (obsolete) — — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de7d856e3e4b36f2267e5a395fb1c3fdeef42191 There are still a couple of problems with clipping and invalidation.
Attachment #8955080 - Attachment is obsolete: true
Attached patch flatten-opacity.patch (obsolete) — — Splinter Review
Attachment #8957207 - Attachment is obsolete: true
Attachment #8960823 - Attachment is obsolete: true
These patches enable flattening of inactive nsDisplayOpacity items in FrameLayerBuilder. This is done by tracking when the opacity items are flattened, and emitting PUSH_OPACITY and POP_OPACITY markers around the children. Brief summary: -------------- Part 1: In order to properly invalidate the area painted by nsDisplayOpacity, nsDisplayOpacityGeometry is added to track opacity value changes. Part 2: Adds unimplemented virtual methods StartNested(nsDisplayItem* aItem) and EndNested(nsDisplayItem* aItem) to FlattenedDisplayItemIterator. These are called when a display item is flattened. Part 3: Adds functionality to distinguish whether nsDisplayOpacity has applied opacity to the children. This is needed to avoid applying opacity twice, and to support (old) flattening with WebRender. An alternative way to implement this would have been to replace return value for all display items that implement ShouldFlatten(). This approach might be nicer solution in the future if/when other effects are flattened. Part 4: Adds FLBDisplayItemIterator that implements StartNested() and EndNested() functions and uses them to emit PUSH_OPACITY and POP_OPACITY markers when entering/leaving nested list. ShouldFlattenNextItem() is overridden to check whether nsDisplayOpacity will use active layers, in which case we do not flatten it. Part 5: Replaces FlattenedDisplayItemIterator with FLBDisplayItemIterator in ContainerState::ProcessDisplayItems(). The display items are tagged with either ITEM, PUSH_OPACITY, or POP_OPACITY markers, that are stored in AssignedDisplayItem struct along with the item. Most of the changes are to ensure that subpixel aa, invalidation, and visibility calculations work correctly with the markers. The actual use of the opacity markers happens in FrameLayerBuilder::PaintItems(). The complexity there comes mainly from having to track multiple nested clips for and within temporary surfaces. Part 6: Three reftests had to be fuzzed. - 654950-1-ref.html: There is an existing comment about blending. Off by one in one color component sounds reasonable. - grid-abspos-items-002.html: Looks like it could be caused by missing layer clip[1]. - anonymous-block.html: Subpixel AA for the text has a slightly different intensity. [1]: https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/layout/generic/nsTextFrame.cpp#5139
Comment on attachment 8961032 [details] Bug 1442190 - Part 1: Add nsDisplayOpacityGeometry https://reviewboard.mozilla.org/r/229764/#review235680
Attachment #8961032 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8961033 [details] Bug 1442190 - Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place https://reviewboard.mozilla.org/r/229766/#review235682 ::: layout/painting/nsDisplayList.h:3212 (Diff revision 1) > +class FlattenedDisplayItemIterator > +{ > +public: > + FlattenedDisplayItemIterator(nsDisplayListBuilder* aBuilder, > + nsDisplayList* aList, > + const bool aResolveFlattening) Make this default to true so that the existing callers don't need to care about it.
Attachment #8961033 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8961033 [details] Bug 1442190 - Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place https://reviewboard.mozilla.org/r/229766/#review235684 I don't think we need to make things virtual here. We don't have any code that refers to a FLBDisplayItemIterator via a FlattenedDisplayItemIterator, so we shouldn't need virtual function to resolve to the derived class. Instead we can just make FLBDisplayItemIterator use protected inheritance (and add an implementation forwarding PeekNext), and then everything will be resolved at compile-time. r=me with the 'virtual' additions removed.
Comment on attachment 8961034 [details] Bug 1442190 - Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children https://reviewboard.mozilla.org/r/229768/#review235686 ::: layout/painting/nsDisplayList.h:5224 (Diff revision 1) > nsDisplayOpacity(nsDisplayListBuilder* aBuilder, > const nsDisplayOpacity& aOther) > : nsDisplayWrapList(aBuilder, aOther) > , mOpacity(aOther.mOpacity) > , mForEventsAndPluginsOnly(aOther.mForEventsAndPluginsOnly) > + , mOpacityAppliedToChildren(aOther.mOpacityAppliedToChildren) We block flattening if we might need to merge, so aOther.mOpacityAppliedToChildren must be false. We should just set our version to false, and assert that the aOther's is false too. ::: layout/painting/nsDisplayList.cpp:6709 (Diff revision 1) > } > > +bool > +nsDisplayOpacity::ShouldFlattenAway(nsDisplayListBuilder* aBuilder) > +{ > + if (mOpacityAppliedToChildren) { If this ever succeeds, then it means we've called ShouldFlattenAway twice, which is probably a bad thing (especially since we're only caching the result if it was true). Does that happen? Can we instead just assert that it doesn't?
Attachment #8961034 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8961033 [details] Bug 1442190 - Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place https://reviewboard.mozilla.org/r/229766/#review235684 Sorry, this wasn't very clear. The public interface doesn't need to be virtual, ShouldFlattenNextItem/StartNested/EndNested obviously do.
Comment on attachment 8961035 [details] Bug 1442190 - Part 4: Add FLBDisplayItemIterator https://reviewboard.mozilla.org/r/229770/#review235692 ::: layout/painting/FrameLayerBuilder.cpp:181 (Diff revision 1) > + void StartNested(nsDisplayItem* aItem) override > + { > + if (aItem->GetType() == DisplayItemType::TYPE_OPACITY) { > + nsDisplayOpacity* opacity = static_cast<nsDisplayOpacity*> (aItem); > + > + if (opacity->OpacityAppliedToChildren()) { We could probably combine these two checks into a single call (a virtual on nsDisplayItem), something like bool NeedsMarkerForFlattening(DisplayItemEntryType* aOutMarkerType). Sets you up better for supporting types other than opacity in the future. ::: layout/painting/FrameLayerBuilder.cpp:1597 (Diff revision 1) > + return false; > + } > + > + const bool shouldFlatten = mNext->ShouldFlattenAway(mBuilder); > + > + if (!shouldFlatten) { Can just do if (!mNext->ShouldFlattenAway()) { return false;} We use shouldFlatten again at the end, but it's guaranteed to be true.
Attachment #8961035 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8961037 [details] Bug 1442190 - Part 6: Make three reftests fuzzy https://reviewboard.mozilla.org/r/229774/#review235700
Attachment #8961037 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8961036 [details] Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases https://reviewboard.mozilla.org/r/229772/#review235702 ::: layout/painting/FrameLayerBuilder.cpp:1743 (Diff revision 1) > bool mDisabledAlpha; > > + /** > + * True if the painted layer has text items. > + */ > + bool mHasTextItems; I think this should be a property of the AssignedDisplayItem(mType == PUSH_OPACITY). We don't want to copy the background into our temporaries if text exists outside of the opacity. I think you'll need to keep a stack of currently applied PUSH_OPACITYs (and replace opacityNesting with opacityStack.Length()), which contain a pointer to the AssignedDisplayItem entry we created for that marker. When we encounter text, set mHasTextItems = true on all entries in the opacityStack. It's possible that you could share the existing stack in FLBDisplayItemIterator, but it might be hard to find an understandable abstraction around that.
Comment on attachment 8961036 [details] Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases https://reviewboard.mozilla.org/r/229772/#review235702 > I think this should be a property of the AssignedDisplayItem(mType == PUSH_OPACITY). > > We don't want to copy the background into our temporaries if text exists outside of the opacity. > > I think you'll need to keep a stack of currently applied PUSH_OPACITYs (and replace opacityNesting with opacityStack.Length()), which contain a pointer to the AssignedDisplayItem entry we created for that marker. > > When we encounter text, set mHasTextItems = true on all entries in the opacityStack. > > It's possible that you could share the existing stack in FLBDisplayItemIterator, but it might be hard to find an understandable abstraction around that. This is now fixed. I solved it by using an array containing indices to mAssignedDisplayItems and changing PUSH_OPACITY type to PUSH_OPACITY_WITH_BG when items needing subpixel AA are encountered.
Comment on attachment 8961036 [details] Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases https://reviewboard.mozilla.org/r/229772/#review240790 ::: layout/painting/FrameLayerBuilder.cpp:3704 (Diff revision 3) > > bool isFirstVisibleItem = mVisibleRegion.IsEmpty(); > > - Maybe<nscolor> uniformColor = aItem->IsUniform(aState->mBuilder); > + Maybe<nscolor> uniformColor; > + if (!aHasOpacity) { > + uniformColor = aItem->IsUniform(aState->mBuilder); Probably a TODO, but we could still be a uniform color with opacity couldn't we? ::: layout/painting/FrameLayerBuilder.cpp:4854 (Diff revision 3) > + > + if (marker == DisplayItemEntryType::POP_OPACITY) { > + MOZ_ASSERT(selectedPLD && opacityNesting > 0); > + opacityNesting--; > + > + if (opacityNesting == 0) { Seems like we could remove opacityNesting and just use selectedPLD->mOpacityIndices.Length(). ::: layout/painting/FrameLayerBuilder.cpp:6154 (Diff revision 3) > } > + > + if (cdi->mType == DisplayItemEntryType::POP_OPACITY || > + (cdi->mType == DisplayItemEntryType::ITEM && cdi->mHasOpacity)) { > + // The visibility calculations are skipped when the item is an effect end > + // marker, or when the display item is within a flattened opacity group. Maybe mention that the reason we skip items within a flattened group is that we should have already called RecomputeVisibility on the group display item, which includes recursing into this child. ::: layout/painting/FrameLayerBuilder.cpp:6216 (Diff revision 3) > + > + nsDisplayOpacity* opacityItem = static_cast<nsDisplayOpacity*>(aItem.mItem); > + const float opacity = opacityItem->GetOpacity(); > + > + if (aItem.mType == DisplayItemEntryType::PUSH_OPACITY_WITH_BG) { > + aContext->PushGroupAndCopyBackground(gfxContentType::COLOR_ALPHA, opacity); I believe this is only valid if the destination is going to be opaque (either a gfxContentType::COLOR surface, or has opaque pixels drawn to it). We don't compute the latter, so I think you just want to check the former in the if condition. The condition might also need DrawTarget::SetPermitSubpixelAA(false), since I'm pretty sure PaintedLayerData::Accumulate isn't calling nsDisplayItem::DisableComponentAlpha() for us in this case.
Attachment #8961036 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8961036 [details] Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases https://reviewboard.mozilla.org/r/229772/#review241032 ::: layout/painting/FrameLayerBuilder.cpp:6154 (Diff revision 3) > } > + > + if (cdi->mType == DisplayItemEntryType::POP_OPACITY || > + (cdi->mType == DisplayItemEntryType::ITEM && cdi->mHasOpacity)) { > + // The visibility calculations are skipped when the item is an effect end > + // marker, or when the display item is within a flattened opacity group. Done.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s da0c1b74a1f0a1918b8b84bdf66ce8a0b95410a7 -d 964dbe5e2afe: rebasing 457209:da0c1b74a1f0 "Bug 1442190 - Part 1: Add nsDisplayOpacityGeometry r=mattwoodrow" merging layout/painting/nsDisplayList.cpp merging layout/painting/nsDisplayList.h rebasing 457210:59b2e143ab28 "Bug 1442190 - Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place r=mattwoodrow" merging layout/painting/nsDisplayList.h rebasing 457211:7ba6014ca3b9 "Bug 1442190 - Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children r=mattwoodrow" merging layout/painting/nsDisplayList.cpp merging layout/painting/nsDisplayList.h rebasing 457212:03cf17f1a860 "Bug 1442190 - Part 4: Add FLBDisplayItemIterator r=mattwoodrow" merging layout/painting/FrameLayerBuilder.cpp rebasing 457213:c3b07cdd589f "Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases r=mattwoodrow" merging layout/painting/FrameLayerBuilder.cpp merging layout/painting/nsDisplayList.cpp rebasing 457214:56d8c5259cb6 "Bug 1442190 - Part 6: Make three reftests fuzzy r=mattwoodrow" (tip) merging layout/reftests/bugs/reftest.list merging layout/reftests/css-grid/reftest.list merging layout/reftests/text-overflow/reftest.list warning: conflicts while merging layout/reftests/text-overflow/reftest.list! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mikokm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7f82627c75fa Part 1: Add nsDisplayOpacityGeometry r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/8018f83c161b Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/9503cb9d5fc1 Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/c992deef1ec5 Part 4: Add FLBDisplayItemIterator r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/1ca2a0266f20 Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/db65cdb10a34 Part 6: Make three reftests fuzzy r=mattwoodrow
(In reply to Andreea Pavel [:apavel] from comment #40) > Backed out for failing reftest text-overflow/anonymous-block.html, at least > on OS X > > Push that caused the failures: > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=db65cdb10a345cabb8e8c714285edfd0dff0cd1a > > Failure log: > https://treeherder.mozilla.org/logviewer. > html#?job_id=173087505&repo=autoland&lineNumber=5835 > > Backout: > https://hg.mozilla.org/integration/autoland/rev/ > 3cd75c11b4ff40527bf347fef1a69eec1ad5f2c4 This reftest was supposed to be marked fuzzy, but a chunk was lost during rebasing.
Flags: needinfo?(mikokm)
Pushed by mikokm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f8c99ac6bf0d Part 1: Add nsDisplayOpacityGeometry r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/7a1185d616dc Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/cebcdd61afcb Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/ce317da6a8fc Part 4: Add FLBDisplayItemIterator r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/e406251cf5ea Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/e8dc919d0f42 Part 6: Make three reftests fuzzy r=mattwoodrow
(In reply to Noemi Erli[:noemi_erli] from comment #44) > Backed out 6 changesets (bug 1442190) for reftest failures on > /anonymous-block.html on a CLOSED TREE > > Push with failures: > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=e8dc919d0f426ae2f4a0f4d7f87115a3beb08294&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&filter-resultStatus=retry&filter- > resultStatus=usercancel&filter-resultStatus=runnable&filter- > resultStatus=pending&filter-resultStatus=running&selectedJob=173131388 > > Failure log: > https://treeherder.mozilla.org/logviewer. > html#?job_id=173131388&repo=autoland&lineNumber=1824 > > Backout: > https://hg.mozilla.org/integration/autoland/rev/ > 4fd59fe7f0f847701f154e03e463757baa021837 The cause for this failure is a slight brightness difference in subpixel aa text, caused by flattening. It seems that Windows 10 and Windows 7 need a different fuzzing for this. I have fixed this, and I am pushing the changes again, after the (this time exhaustive) try run is finished.
Flags: needinfo?(mikokm)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s ccc6a2f531541b0d6dfce79bf0d1685cb9298f01 -d 93c851a45ec7: rebasing 457666:ccc6a2f53154 "Bug 1442190 - Part 1: Add nsDisplayOpacityGeometry r=mattwoodrow" merging layout/painting/nsDisplayList.cpp merging layout/painting/nsDisplayList.h merging layout/painting/nsDisplayListInvalidation.h rebasing 457667:aed465eaaa5d "Bug 1442190 - Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place r=mattwoodrow" merging layout/painting/nsDisplayList.h rebasing 457668:c8da49ae5ac2 "Bug 1442190 - Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children r=mattwoodrow" merging layout/painting/nsDisplayList.cpp merging layout/painting/nsDisplayList.h rebasing 457669:830b45b588ec "Bug 1442190 - Part 4: Add FLBDisplayItemIterator r=mattwoodrow" merging layout/painting/FrameLayerBuilder.cpp merging layout/painting/FrameLayerBuilder.h rebasing 457670:558bc2277602 "Bug 1442190 - Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases r=mattwoodrow" merging layout/painting/FrameLayerBuilder.cpp merging layout/painting/FrameLayerBuilder.h merging layout/painting/nsDisplayList.cpp warning: conflicts while merging layout/painting/FrameLayerBuilder.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mikokm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/df7030abb8e1 Part 1: Add nsDisplayOpacityGeometry r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/f4a9c1f73c1d Part 2: Virtualize FlattenedDisplayItemIterator and move it to a more appropriate place r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/faf8093a7fbc Part 3: Add functionality to know whether nsDisplayOpacity::ShouldFlattenAway() applied opacity to children r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/3bb3af9020e1 Part 4: Add FLBDisplayItemIterator r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/7c3ec4cce489 Part 5: Use FLBDisplayItemIterator and flatten inactive nsDisplayOpacity in more cases r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/665843c6cfd1 Part 6: Make three reftests fuzzy r=mattwoodrow
Depends on: 1453944
Depends on: 1454653
Depends on: 1455908
Depends on: 1455944
Depends on: 1454105
Depends on: 1461177
No longer depends on: 1461177
Depends on: 1471181
Depends on: 1472465
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: