When a tiled layer is subject to OMTA we shouldn't clip it to the displayport

VERIFIED FIXED in Firefox 39

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
mozilla39
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.1 verified, b2g-v2.1S fixed, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Posted patch Patch (obsolete) — Splinter Review
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.
Comment on attachment 8569869 [details] [diff] [review]
Patch

At least that now fixes the case of pulling down the notification tray on Z3 devices.
Attachment #8569869 - Flags: feedback+
Attachment #8569869 - Flags: review?(bgirard)
If I pull very fast the notification tray on Z3, I still see some glitches just on the statusbar itself.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8569869 [details] [diff] [review]
Patch

Yeah let me fix that up too, there's still something missing here.
Flags: needinfo?(bugmail.mozilla)
Attachment #8569869 - Flags: review?(bgirard)
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.
Attachment #8569869 - Attachment is obsolete: true
Attachment #8569886 - Flags: review?(bgirard)
Attachment #8569886 - Flags: feedback?(lissyx+mozillians)
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.
Looks better.
Comment on attachment 8569886 [details] [diff] [review]
Part 1 - Patch v2

I could not find defect testing this on Z3 and Z3 Compact devices :)
Attachment #8569886 - Flags: feedback?(lissyx+mozillians) → feedback+
Posted patch Alternative patch (obsolete) — Splinter Review
This does what I described in comment 7.
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.
Attachment #8569886 - Flags: review?(bgirard) → review-
(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.
(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'.
Attachment #8569886 - Flags: review- → review+
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.
Attachment #8569907 - Attachment is obsolete: true
Attachment #8570037 - Flags: review?(bgirard)
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.
Attachment #8570037 - Flags: review?(bgirard) → review+
Duplicate of this bug: 1133961
Duplicate of this bug: 1133966
2.2 blocker since bug 1133966 was a 2.2 blocker (and this is a fairly serious problem).
blocking-b2g: --- → 2.2?
Duplicate of this bug: 1098476
Duplicate of this bug: 1099268
Duplicate of this bug: 1099327
Duplicate of this bug: 1099353
Duplicate of this bug: 1101767
Duplicate of this bug: 1127877
Duplicate of this bug: 1132741
Duplicate of this bug: 1136519
Duplicate of this bug: 1136760
Duplicate of this bug: 1128810
Duplicate of this bug: 1119726
Duplicate of this bug: 1114731
[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.
blocking-b2g: 2.2? → 2.1?
https://hg.mozilla.org/mozilla-central/rev/305e676779f8
https://hg.mozilla.org/mozilla-central/rev/ddf965a90c07
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
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.
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
Attachment #8569886 - Attachment description: Patch v2 → Part 1 - Patch v2
Attachment #8569886 - Flags: approval-mozilla-b2g37?
Attachment #8570037 - Flags: approval-mozilla-b2g37?
Backed out for being the likely cause of bug 1137952.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec45286064aa
Depends on: 1137952
Target Milestone: mozilla39 → ---
Since this was backed out on inbound, do we reopen this bug?
Flags: needinfo?(ryanvm)
Flags: needinfo?(bugmail.mozilla)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yup, sorry.
Flags: needinfo?(ryanvm)
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
Flags: needinfo?(bugmail.mozilla)
Depends on: 1139033
https://hg.mozilla.org/mozilla-central/rev/2054e4cb26ff
https://hg.mozilla.org/mozilla-central/rev/da60b4e6c98b
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
Flags: needinfo?(npark)
Kats, we unduped bug 1099268 and bug 1099327 as they still occur on 3.0.
Flags: needinfo?(bugmail.mozilla)
Thanks. Those two look like unrelated issues and probably need to be fixed in Gaia.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(npark)
blocking-b2g: 2.1? → 2.1+
Attachment #8569886 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8570037 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8571358 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
Keywords: qawanted, verifyme
No longer blocks: 1109518
Duplicate of this bug: 1109518
QA Whiteboard: [QAnalyst-Triage+]
QA Contact: pcheng
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Contact: pcheng
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.