Closed Bug 1248913 Opened 8 years ago Closed 8 years ago

background-attachment:fixed ignored by APZ in this testcase with background-blend-mode

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Depends on 1 open bug)

Details

Attachments

(8 files)

MozReview Request: Bug 1248913 - nsDisplayListBuilder doesn't need to know what blend modes it contains, just whether it contains any. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta-
Details
MozReview Request: Bug 1248913 - Add a constructor argument to nsDisplayMixBlendMode that lets you specify the blend mode. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1248913 - Rename nsDisplayMixBlendMode to nsDisplayBlendMode. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1248913 - Let nsDisplayBackgroundImage specify the background blend mode. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1248913 - Build nsDisplayBlendMode items for background-blend-mode. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1248913 - Remove mCanBeActive and second nsDisplayBlendContainer constructor. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1248913 - Make nsDisplayBlendContainer active or inactive based on its contents. r?mattwoodrow
(deleted), text/x-review-board-request
mattwoodrow
: review+
ritu
: approval-mozilla-aurora+
Details
reftest
(deleted), patch
Details | Diff | Splinter Review 
APZ scrolling on http://codepen.io/azureowl/pen/MKLpEB moves the background image until the next (slow) main thread paint puts it back to the intended fixed position.
This happens because the nsDisplayBlendContainer for the background has mCanBeActive == false. It should be true, because it contains items with two different animated geometry roots.
I'll wait for bug 1209278 before fixing this.

We should check whether we can use RequiredLayerStateForChildren instead of mCanBeActive in nsDisplayBlendContainer::GetLayerState.
(In reply to Markus Stange [:mstange] from comment #1)
> We should check whether we can use RequiredLayerStateForChildren instead of
> mCanBeActive in nsDisplayBlendContainer::GetLayerState.

Oh, we can't, because nsDisplayMixBlendMode::GetLayerState always returns LAYER_ACTIVE, so this would make all blend containers active.

Not sure how to work around this. I guess we could add a new layer state LAYER_INACTIVE_BLENDMODE, which is treated as LAYER_INACTIVE by RequiredLayerStateForChildren and as LAYER_ACTIVE everywhere else. Either way, nsDisplayMixBlendMode::GetLayerState will need some way to indicate the difference between "LAYER_ACTIVE because otherwise FrameLayerBuilder will give me my own BasicLayerManager and put the result in a PaintedLayer, and that won't blend properly" and "LAYER_ACTIVE because I have animated descendants so we really need to make all my ancestors LAYER_ACTIVE".
And the other problem is that even if the background-blend-mode nsDisplayBlendContainer ContainerLayer is active, we won't get correct blending in the compositor because the individual background-image layers are not wrapped into nsDisplayMixBlendMode items. background-blend-mode rendering relies on the fact that all background items are drawing into the same buffer on the main thread, and does blending during the rendering of the background images.
We're going to use it both for background-blend-mode and for mix-blend-mode.

Review commit: https://reviewboard.mozilla.org/r/38747/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38747/
Attachment #8728029 - Flags: review?(matt.woodrow)
This is needed because blending for nsDisplayBackgroundImage items will soon
happen outside of nsDisplayBackgroundImage::Paint, it will be done by an
nsDisplayBlendMode item that wraps the nsDisplayBackgroundImage item.

Review commit: https://reviewboard.mozilla.org/r/38749/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38749/
Attachment #8728030 - Flags: review?(matt.woodrow)
I ended up not including any AppleBlendMode optimizations in this patch stack, because it wouldn't help in many cases. When we draw background images, we eventually go through DrawImageInternal in nsLayoutUtils.cpp (except for gradients), which creates an intermediate surface if the composition op is not OP_OVER. So we're using an intermediate surface during background-blend-mode drawing either way. With these patches, the intermediate surface we're using is the one installed by the PushGroupForLayer call in BasicPaintedLayer::PaintThebes, and then we're carrying along OP_OVER inside of that so DrawImageInternal won't create an extra intermediate surface.
Comment on attachment 8728027 [details]
MozReview Request: Bug 1248913 - nsDisplayListBuilder doesn't need to know what blend modes it contains, just whether it contains any. r?mattwoodrow

https://reviewboard.mozilla.org/r/38743/#review35439
Attachment #8728027 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8728028 [details]
MozReview Request: Bug 1248913 - Add a constructor argument to nsDisplayMixBlendMode that lets you specify the blend mode. r?mattwoodrow

https://reviewboard.mozilla.org/r/38745/#review35441
Attachment #8728028 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8728029 [details]
MozReview Request: Bug 1248913 - Rename nsDisplayMixBlendMode to nsDisplayBlendMode. r?mattwoodrow

https://reviewboard.mozilla.org/r/38747/#review35443
Attachment #8728029 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8728030 [details]
MozReview Request: Bug 1248913 - Let nsDisplayBackgroundImage specify the background blend mode. r?mattwoodrow

https://reviewboard.mozilla.org/r/38749/#review35445

::: layout/base/nsCSSRendering.h:614
(Diff revision 1)
> +   * layer's composition mode) will be used.

Can we assert this?
Attachment #8728030 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8728031 [details]
MozReview Request: Bug 1248913 - Build nsDisplayBlendMode items for background-blend-mode. r?mattwoodrow

https://reviewboard.mozilla.org/r/38751/#review35447
Attachment #8728031 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8728033 [details]
MozReview Request: Bug 1248913 - Make nsDisplayBlendContainer active or inactive based on its contents. r?mattwoodrow

https://reviewboard.mozilla.org/r/38755/#review35449
Attachment #8728033 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8728032 [details]
MozReview Request: Bug 1248913 - Remove mCanBeActive and second nsDisplayBlendContainer constructor. r?mattwoodrow

https://reviewboard.mozilla.org/r/38753/#review35451
Attachment #8728032 - Flags: review?(matt.woodrow) → review+
Assignee: nobody → mstange
Comment on attachment 8728027 [details]
MozReview Request: Bug 1248913 - nsDisplayListBuilder doesn't need to know what blend modes it contains, just whether it contains any. r?mattwoodrow

Requesting approval for 46 and 47 on all patches in this bug. This is not a particularly critical bug, because background-blend-mode is still a very new technology and not many people are using it yet, but the patches are simple and low risk and they fix a real bug.

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: when APZ is on, the combination of background-blend-mode and background-attachment:fixed does not work correctly. Images move when they should stay fixed.
[Describe test coverage new/current, TreeHerder]: We have lots of background-blend-mode tests but not one that tests this particular case.
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8728027 - Flags: approval-mozilla-beta?
Attachment #8728027 - Flags: approval-mozilla-aurora?
Hi Markus, Milan, the amount of code change here is making me nervous. The good thing is it's been in Nightly for a week without any known regressions. The other thing that is bothersome is that we are not adding any automated test to ensure we don't regress this again. How difficult would it be to add an automated test case for this? Also, is APZ getting tested enough in Nightly to consider uplifting it to Aurora/Beta now or should we let it stabilize for some more time? Thanks!
Flags: needinfo?(mstange)
Flags: needinfo?(milan)
You're right, I'll need to write a test for this. Shouldn't be too hard.
We can let this stabilize for some more time; since APZ won't be getting turned on by default on 46 Release we don't really need it on Beta.
Flags: needinfo?(mstange)
Comment on attachment 8728027 [details]
MozReview Request: Bug 1248913 - nsDisplayListBuilder doesn't need to know what blend modes it contains, just whether it contains any. r?mattwoodrow

Agreed, while I want to support the apz/e10s experiment on beta, this doesn't seem crucial for now for 46 beta or release.
Attachment #8728027 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Hi Kats, please see comment 22. Given the number of quality issues we've hit recently right after we go live, I am trying to limit the code change not just in Beta but also in Aurora cycle if possible. Do you consider the fixes here safe for uplift to Aurora 47? Thanks!
Flags: needinfo?(bugmail.mozilla)
Hi Ritu, I looked through the patches and 4 of the 7 patches are really just refactoring of code and very low risk. The other 3 patches seem to implement the actual changes, and they look relatively straightforward to me, even though I don't know that part of the code very well. On the flip side this bug probably won't get hit very often in the wild so it's not immediately urgent to uplift either.

However I can think of one compelling reason to uplift this - it touches a bunch of code that will likely get touched again in the near future to fix other possibly-more-important bugs. If we don't uplift this now, then uplifting those other fixes will become harder and riskier because of rebasing. It might be that we'll end up having to uplift this later anyway in order to uplift other fixes. If that's the case then I'd rather uplift this sooner rather than later, so it gets more bake time on aurora.

So yeah, it's a bit of a coin toss, but I'm leaning in favour of uplifting to aurora 47.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> Hi Ritu, I looked through the patches and 4 of the 7 patches are really just
> refactoring of code and very low risk. The other 3 patches seem to implement
> the actual changes, and they look relatively straightforward to me, even
> though I don't know that part of the code very well. On the flip side this
> bug probably won't get hit very often in the wild so it's not immediately
> urgent to uplift either.
> 
> However I can think of one compelling reason to uplift this - it touches a
> bunch of code that will likely get touched again in the near future to fix
> other possibly-more-important bugs. If we don't uplift this now, then
> uplifting those other fixes will become harder and riskier because of
> rebasing. It might be that we'll end up having to uplift this later anyway
> in order to uplift other fixes. If that's the case then I'd rather uplift
> this sooner rather than later, so it gets more bake time on aurora.
> 
> So yeah, it's a bit of a coin toss, but I'm leaning in favour of uplifting
> to aurora 47.

Thanks Kats. I have mixed feelings approving this uplift but I will do it if it is needed for bug fixes coming down the pipeline. I hope we do carve out time to beef up automation and add automated tests when requesting uplift of this much code (7 patches!).
Comment on attachment 8728027 [details]
MozReview Request: Bug 1248913 - nsDisplayListBuilder doesn't need to know what blend modes it contains, just whether it contains any. r?mattwoodrow

APZ, Aurora47+
Attachment #8728027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
this got backed out because it seems this caused android assertion/crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=2210742&repo=mozilla-aurora
Flags: needinfo?(mstange)
This is probably because APZ is not enabled on android beyond nightly. We should add a guard for null scroll ids in that new empty-transform code you added.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> This is probably because APZ is not enabled on android beyond nightly. We
> should add a guard for null scroll ids in that new empty-transform code you
> added.

turned out this patch was innocent and so relanded in 
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f3c679ed9bce
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a7ecd0799004
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e8a1daa58d73
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/defbe77d7375
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/45236d08ae4f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5cd28a4a829b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/bf27bbf52e4b
status-firefox47: affected → fixed
Flags: needinfo?(mstange)
Attached patch reftest (deleted) — — Splinter Review 
I wrote a test. I'll land it if it passes on try.
As a heads-up, the new reftest needs a bit of fuzzing on Windows D2D w/ e10s enabled. I'll take care of that.
https://treeherder.mozilla.org/logviewer.html#?job_id=161883&repo=ash
Flags: needinfo?(milan)
Depends on: 1293586
Depends on: 1532749
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: