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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

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.

Comment 2

[Markus Stange [:mstange]]

(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".

Comment 3

[Markus Stange [:mstange]]

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.

Comment 4

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - nsDisplayListBuilder doesn't need to know what blend modes it contains, just whether it contains any. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38743/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38743/

Comment 5

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - Add a constructor argument to nsDisplayMixBlendMode that lets you specify the blend mode. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38745/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38745/

Comment 6

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - Rename nsDisplayMixBlendMode to nsDisplayBlendMode. r?mattwoodrow

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/

Comment 7

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - Let nsDisplayBackgroundImage specify the background blend mode. r?mattwoodrow

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/

Comment 8

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - Build nsDisplayBlendMode items for background-blend-mode. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38751/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38751/

Comment 9

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - Remove mCanBeActive and second nsDisplayBlendContainer constructor. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38753/

Comment 10

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1248913 - Make nsDisplayBlendContainer active or inactive based on its contents. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38755/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38755/

Comment 11

[Markus Stange [:mstange]]

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 12

[Matt Woodrow (:mattwoodrow)]

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

Comment 13

[Matt Woodrow (:mattwoodrow)]

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

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8728029 [details]
MozReview Request: Bug 1248913 - Rename nsDisplayMixBlendMode to nsDisplayBlendMode. r?mattwoodrow

https://reviewboard.mozilla.org/r/38747/#review35443

Comment 15

[Matt Woodrow (:mattwoodrow)]

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?

Comment 16

[Matt Woodrow (:mattwoodrow)]

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

Comment 17

[Matt Woodrow (:mattwoodrow)]

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

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8728032 [details]
MozReview Request: Bug 1248913 - Remove mCanBeActive and second nsDisplayBlendContainer constructor. r?mattwoodrow

https://reviewboard.mozilla.org/r/38753/#review35451

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7889ac4a139
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b8780ef6f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d8b36432d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dfa8eff77b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f5e5b72fa7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cedefab75f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ec5effe35f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7889ac4a139
https://hg.mozilla.org/mozilla-central/rev/f2b8780ef6f5
https://hg.mozilla.org/mozilla-central/rev/b5d8b36432d8
https://hg.mozilla.org/mozilla-central/rev/5dfa8eff77b6
https://hg.mozilla.org/mozilla-central/rev/f6f5e5b72fa7
https://hg.mozilla.org/mozilla-central/rev/0cedefab75f6
https://hg.mozilla.org/mozilla-central/rev/a9ec5effe35f

Comment 21

[Markus Stange [: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

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 23

[Markus Stange [:mstange]]

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.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 26

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e5731e04d40
https://hg.mozilla.org/releases/mozilla-aurora/rev/201a70d6ddc7
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca238297899e
https://hg.mozilla.org/releases/mozilla-aurora/rev/b5f16ce81326
https://hg.mozilla.org/releases/mozilla-aurora/rev/cda10deb527c
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1e83e40ce0b
https://hg.mozilla.org/releases/mozilla-aurora/rev/80a726021cd0

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 32

[Carsten Book [:Tomcat]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3c679ed9bce
https://hg.mozilla.org/releases/mozilla-aurora/rev/a7ecd0799004
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8a1daa58d73
https://hg.mozilla.org/releases/mozilla-aurora/rev/defbe77d7375
https://hg.mozilla.org/releases/mozilla-aurora/rev/45236d08ae4f
https://hg.mozilla.org/releases/mozilla-aurora/rev/5cd28a4a829b
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf27bbf52e4b

Comment 34

[Markus Stange [:mstange]]

Attachment: reftest

I wrote a test. I'll land it if it passes on try.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4923398238

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e4923398238

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb4113d95f1

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fb4113d95f1