Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 9 years ago

When we paint tiled layers, the heuristics in ClientTiledPaintedLayer::RenderLayer use a number of heuristics to paint the high-precision and low-precision areas progressively so that we don't waste time painting unnecessary areas. This code works fine now that bug 1134762 has landed but it doesn't account for layers with OMTA.

Consider a scenario like this:

ClientContainerLayer (scrollable, has framemetrics with a critical displayport)
 - ClientTiledPaintedLayer1
 - ClientTiledPaintedLayer2

In this case we only paint in high resolution the parts of ClientTiledPaintedLayer1 and ClientTiledPaintedLayer2 that are within the container layer's critical displayport. This is because these layers might be arbitarily large and there's no point in painting the whole layer because they will move in sync with the container layer's scrolling and so only the parts of them that are in the displayport need to be painted.

However, if ClientTiledPaintedLayer1 for example has an OMTA on it, then it can move out of sync with ClientContainerLayer in ways that we cannot easily predict on the client side. In order to prevent the layer from checkerboarding during OMTA we need to make sure the entire visible region of the layer gets painted in high-res.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 9 years ago

This is what I had in mind (mostly resurrected from my old patch on bug 1132741). Needs a bit more testing to make sure it fixes all the things.

	:gerard-majax
	Comment 2 • 9 years ago

Comment on attachment 8569869 [details] [diff] [review]
Patch

At least that now fixes the case of pulling down the notification tray on Z3 devices.

	:gerard-majax
	Comment 3 • 9 years ago

If I pull very fast the notification tray on Z3, I still see some glitches just on the statusbar itself.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

Comment on attachment 8569869 [details] [diff] [review]
Patch

Yeah let me fix that up too, there's still something missing here.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 9 years ago

The problem with that is even though we skip the displayport clipping with my patch, we still draw the layer progressively. We always draw tiles starting from the top, going across and in the case of the notification tray it animates so that the bottom becomes visible first. This is simply a case of the animation racing the tile-by-tile painting and making the bottom visible before it's done painting. I think we should just use the fast path in cases where we're not using a displayport.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 9 years ago

BenWa, as an alternative we could use the v1 patch but make UseProgressiveDraw() return false on OMTA layers. That should also fix it and might be a little safer. Let me know if you'd rather do that.

	:gerard-majax
	Comment 8 • 9 years ago

Looks better.

	:gerard-majax
	Comment 9 • 9 years ago

Comment on attachment 8569886 [details] [diff] [review]
Part 1 - Patch v2

I could not find defect testing this on Z3 and Z3 Compact devices :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 9 years ago

This does what I described in comment 7.

	Benoit Girard (:BenWa)
	Comment 11 • 9 years ago

Comment on attachment 8569886 [details] [diff] [review]
Part 1 - Patch v2

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

I'd like to have a better condition in UseFastPath before we land this, but I could be convinced that we should take this later. And actually since we will need this on a branch I might of just convinced myself. Probably worth a quick discussion before we land this.

::: gfx/layers/Layers.cpp
@@ +765,5 @@
> +bool
> +Layer::HasTransformAnimation() const
> +{
> +  for (uint32_t i = 0; i < mAnimations.Length(); i++) {
> +    if (mAnimations[i].property() == eCSSProperty_transform) {

I saw mPendingAnimations on Layers when I was doing my local testing. Do we need to check that one too?

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +169,5 @@
> +  // LayoutDevice space of this layer, but only if there is no OMT animation
> +  // on this layer. If there is an OMT animation then we need to draw the whole
> +  // visible region of this layer as determined by layout, because we don't know
> +  // what parts of it might move into view in the compositor.
> +  if (!hasTransformAnimation) {

Cool, I was hoping we patch it by not setting a critical display port.

@@ +239,5 @@
> +  // entire visible region of the layer, so we can use the fast-path and be
> +  // done with it. In particular we don't want to be drawing OMTA layers
> +  // progressively as they might already be animating before we're done drawing.
> +  if (!scrollAncestor || hasTransformAnimation) {
> +    return true;

Assuming that you didn't recreate the OOM bug from taking the fast path when we computed a displayport set for the low precession buffer, then you're leaving this to be really fragile for someone else to make the same mistake. If we have hasTransformAnimation but end up with a displayport set for low resolution then we're going to get into trouble.

Perhaps we need a good check here for a low precision displayport. Maybe that thing should be a low precision displayport? Then we could decide to take UseFastPath and paint the full displayport and not paint the low res display port.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 9 years ago

(In reply to Benoit Girard (:BenWa) from comment #11)
> I saw mPendingAnimations on Layers when I was doing my local testing. Do we
> need to check that one too?

Good question, I'm not sure. Looking through the code it looks like stuff is put into mPendingAnimations and then it's swapped into mAnimations as part of ApplyPendingUpdatesToSubtree which is called on the client side during the end-transaction. But I'm not sure if that happens before or after this tiling code is run.

> > +  // done with it. In particular we don't want to be drawing OMTA layers
> > +  // progressively as they might already be animating before we're done drawing.
> > +  if (!scrollAncestor || hasTransformAnimation) {
> > +    return true;
> 
> Assuming that you didn't recreate the OOM bug from taking the fast path when
> we computed a displayport set for the low precession buffer, then you're
> leaving this to be really fragile for someone else to make the same mistake.
> If we have hasTransformAnimation but end up with a displayport set for low
> resolution then we're going to get into trouble.
> 
> Perhaps we need a good check here for a low precision displayport. Maybe
> that thing should be a low precision displayport? Then we could decide to
> take UseFastPath and paint the full displayport and not paint the low res
> display port.

So I think that if we're recreating the OOM bug it will likely happen regardless of this UseFastPath() change. Consider the case where we have a OMTA layer which is a child of a scrollable layer with a very large low-res displayport. Then the OMTA layer's visible region is going to be the entire low-res displayport (or the size of the layer, whichever is smaller). Now, since we are ignoring the critical displayport on this layer (regardless of the change to UseFastPath) we will draw that entire area in high-precision and retain it. So if it's going to OOM, that will happen because we are ignoring the critical displayport and not because of the change to UseFastPath.

However, keep in mind that layers with OMTA should be smaller than viewport size - if they are large then layout will skip doing OMTA on them anyway because nsDisplayTransform::ShouldPrerenderTransformedContent will return false. I would argue that if we are not able to fully render an OMTA layer in high-precision, then we shouldn't be doing OMTA on it to begin with. i.e. if we OOM because of this patch then the fix for that is to make the size constraint in nsDisplayTransform::ShouldPrerenderTransformedContent tighter.

	Benoit Girard (:BenWa)
	Comment 13 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> I would argue that if we are not able to fully render an OMTA layer
> in high-precision, then we shouldn't be doing OMTA on it to begin with.

I would argue that as well.

Alright I'm reasonably convinced. I just don't like that we have conditions which are a proxy for what we really want here which is 'don't take the fast path if we have a low res display port'.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f9a1b92cdd5

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 9 years ago

This gets rid of the fast path and moves the logic into UseProgressiveDraw. I think this makes it much cleaner because we have access to mPaintData stuff in here which we didn't have in UseFastPath(), so the checks are simpler and easier to follow.

	Benoit Girard (:BenWa)
	Comment 16 • 9 years ago

Comment on attachment 8570037 [details] [diff] [review]
Part 2 - Followup to make code cleaner

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

Looks good! I'm hoping this will leads to less intermittent bugs.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 9 years ago

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/305e676779f8
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/ddf965a90c07

	Benoit Girard (:BenWa)
	Comment 20 • 9 years ago

2.2 blocker since bug 1133966 was a 2.2 blocker (and this is a fairly serious problem).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 33 • 9 years ago

[Blocking Requested - why for this release]: Various 2.1+ bugs were duped into this one, as this bug fixes their symptoms on master. Bug 1114731 and bug 1119726 are two examples of bugs that were duped to this one.

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/305e676779f8
https://hg.mozilla.org/mozilla-central/rev/ddf965a90c07

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 35 • 9 years ago

Try push with these patches on 2.2:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58216f01c0c

Try push with these patches on 2.1:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7437a67581b0

In both cases I applied the patches on top of the patches from bug 1135677 and bug 1134762.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 36 • 9 years ago

Updated 2.1 push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9f5352a82f5. There is a M-12 failure, but that's the same non-problem as what I described at bug 1134762 comment 25.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 37 • 9 years ago

Comment on attachment 8569886 [details] [diff] [review]
Part 1 - Patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Tiling code (it's been around for a while), but turning on APZ in the parent process made it more obvious
User impact if declined: during OMT animations the content being moved around may not be fully painted until the animation is complete. Visually this looks pretty bad, see the long list of dupes on this bug.
Testing completed: locally. We should get QA to verify this (and the dupes) are fixed on master.
Risk to taking this patch (and alternatives if risky): Slight risk because we've fiddled with this code a bunch lately but I'm pretty confident that this (applied on top of bug 1134762 and bug 1135677) is the right fix.
String or UUID changes made by this patch: none

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 9 years ago

Backed out for being the likely cause of bug 1137952.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec45286064aa

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 40 • 9 years ago

Since this was backed out on inbound, do we reopen this bug?

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 9 years ago

Yup, sorry.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 42 • 9 years ago

The patch I landed for bug 1137952 fixes the M-2 test, so I'm relanding this. The try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca7df4e9aae includes both these patches and that one and is green.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2054e4cb26ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/da60b4e6c98b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 43 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/2054e4cb26ff
https://hg.mozilla.org/mozilla-central/rev/da60b4e6c98b

	bhavana bajaj [:bajaj]
	Comment 44 • 9 years ago

Given the number of DUP's on this, inclined on taking. I am NI'ing njpark to help test/verify this on nightly before we uplift to branches.

	Peter Bylenga [:PBylenga]
	Comment 45 • 9 years ago

Kats, we unduped bug 1099268 and bug 1099327 as they still occur on 3.0.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 46 • 9 years ago

Thanks. Those two look like unrelated issues and probably need to be fixed in Gaia.

	No-Jun Park [:njpark]
	Comment 47 • 9 years ago

tested the patch on 2.1 and 2.2 and checked that DUP's are no longer reproducing (except the bugs in Comment 45).  Also did graphics sanity checks on both revision, and we didn't find any issue specific to this patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4c7732b81d84
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b2e4589e553f

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/877c6966a307

	Martin Shuman [:Marty]
	Comment 49 • 9 years ago

This issue is verified fixed in in the latest 3.0 build.
Flickering and Redraw issues with UI transitions are not observed.

Environmental Variables:
Device: Flame 3.0 (319MB)(Full Flash)
Build ID: 20150305010212
Gaia: eff3321ab4e65da3f906688ebb55ddf1e93d9452
Gecko: 56492f7244a9
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

	Vincent Liu[:vliu]
	Comment 50 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/877c6966a307

	Pi Wei Cheng [:piwei] (inactive)
	Comment 52 • 9 years ago

This issue has been verified fixed on Flame 2.2 and on Flame 2.1. No graphical issues were found when transitioning on various pages (checked all scenarios on dupes of this bug)

Device: Flame 2.2 (full flash 319MB)
BuildID: 20150309002506
Gaia: 166491b92278dc9e648f8d49ab02d9ca00d74421
Gecko: 91b7aa6a3243
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.1 (full flash 319MB)
BuildID: 20150309001219
Gaia: ea97a87048a4c1e2a479bbea1d75e0a182b2c4c9
Gecko: 0443f2e951dc
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Leaving verifyme tag for 2.1S verification. We don't have this device in the lab.