Closed Bug 868082 Opened 11 years ago Closed 11 years ago

Clipping and offset errors with transforms on active layers

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:tef+, b2g18+ verified, b2g18-v1.0.1 verified)

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
b2g18 + verified
b2g18-v1.0.1 --- verified

People

(Reporter: cwiiis, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(1 file)

fix
10.90 KB, patch
cwiiis
: review+
ajones
: feedback+
BenWa
: feedback+
akeybl
: approval-mozilla-b2g18+
Details | Diff | Splinter Review 
There are issues with transforms on active layers that are very easy to expose in v1.0.1.

STR:

1. Load the marketplace
2. Go to an app page that has screenshots
3. Hold down on a screenshot and scroll it horizontally while also scrolling the page vertically (something that's frustratingly easy to do)
4. Note how the screenshot follows you down the page and starts getting clipped after a while

This is reproduceable in several other places, but I think this one is easy enough to run into and in such a prominent place (complete with our branding all over the place), that it ought to be a blocker.

This only affects v1.0.1, it's fixed in v1-train.
Whiteboard: [tef-triage]
Jeff - this sounds like something that has a dupe bug somewhere, with a fix already in v1.1.  Can you point us in the right direction here?
Flags: needinfo?(jmuizelaar)
(In reply to lsblakk@mozilla.com from comment #1)
> Jeff - this sounds like something that has a dupe bug somewhere, with a fix
> already in v1.1.  Can you point us in the right direction here?

I have no idea. Does this happen on Firefox 18 or is it b2g specific?

Roc did you see anything like this go by?
Flags: needinfo?(jmuizelaar) → needinfo?(roc)
Could this be bug 858550?
I guess that's on both.
Here are some bugs changing gfx/layers that are in v1-train but not 1.0.1

Bug 787812 - Add Nexus S pixels constants.
Bug 828531; fix scaling CSS pixels for animations
Bug 839981 - Adding Unagi pixels constants and color
Bug 833964 - Don't keep a dangling pointer to a task
Is this bug on all devices including Unagi? I can't reproduce on my Unagi. Is it possible people are testing 1.0.1 on high-DPI devices and this is actually bug 828531?
Flags: needinfo?(roc)
Oh, my Unagi is probably running v1-train.
Could possibly be bug 846144? Bug 846144 disables some ImageLayer optimizations.
QA please try the patch in bug 846144 on a v1.0.1 build to see if it resolves this issue?
Keywords: qawanted
blocking-b2g: tef? → tef+
Whiteboard: [tef-triage] → c=
(In reply to lsblakk@mozilla.com from comment #9)
> QA please try the patch in bug 846144 on a v1.0.1 build to see if it
> resolves this issue?

Can't. We don't have the ability to test patched builds for 1.01. This is better done by development creating a custom 1.01 build with a patch and testing to see if the issue above is fixed.
Keywords: qawanted
It is possible to do custom b2g builds in try - builds will live in http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds and will have a flashable build for unagi for testing.  

Roc can you put up a patch to v1.0.1 of the work in bug 846144 that qa can test out to see if this is the right path?
Flags: needinfo?(roc)
Keywords: qawanted
(In reply to lsblakk@mozilla.com from comment #11)
> It is possible to do custom b2g builds in try - builds will live in
> http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds 

wrong link - see https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/try-builds/.
We can test if we're given an build w/ a patch that works.
Please needinfo to me and I will test around the patch.
Er, I meant needinfo me with the link to the proper build.
I've got a build going now with the patch from bug 846144.  I'll try it on my unagi when it finishes.

FWIW, as expected, jst's inari with a v1-train build from 2013-05-06 doesn't reproduce the STR from comment #0 whereas my unagi with a v1.0.1 build from 2013-05-03 *does* exhibit the behaviour in comment #0.
Whiteboard: c= → [status: may be fixed by bug 846144, awaiting confirmation] c=
FYI - A very easy way we could also figure out if bug 846144 fixes this is a do a regression window before and after that patch was fixed on v1-train in relation to the STR given in this bug.

If it turns out bug 846144 isn't the bug that fixes this, then we might want to do a regression window instead.

I'm adding regression window wanted though to allow for analysis to be done through a second path here that can also be helpful here.
Keywords: regression, regressionwindow-wanted
I can still make the STR in comment #0 happen on my unagi with the following patch applied to my gecko tree (gecko/gaia = 6fd34438/30ad443f) and flashed to the device (it's much more pronounced now, actually):

diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp
index 96e8253..a0dbebe 100644
--- a/layout/base/nsDisplayList.cpp
+++ b/layout/base/nsDisplayList.cpp
@@ -1733,6 +1733,14 @@ nsDisplayBackgroundImage::TryOptimizeToImageLayer(nsDisplayListBuilder* aBuilder
   nsRect borderArea = nsRect(ToReferenceFrame(), mFrame->GetSize());
   const nsStyleBackground::Layer &layer = mBackgroundStyle->mLayers[mLayer];
 
+  if (layer.mClip != NS_STYLE_BG_CLIP_BORDER) {
+    return false;
+  }
+  nscoord radii[8];
+  if (mFrame->GetBorderRadii(radii)) {
+    return false;
+  }
+
   nsBackgroundLayerState state =
     nsCSSRendering::PrepareBackgroundLayer(presContext,
                                            mFrame,
Can we bisect along the v1-train branch to find the fix?

I can't debug this until next week.
Flags: needinfo?(roc)
Cutting qawanted since we know aren't sure what patch fixed this. Moving to regressionwindow-wanted purely to do a bisection to find the patch that fixed this.
Keywords: qawanted
Both Leo and Unagi are capable of hidpi  If it's possible I would recommend trying to build 1.1 for inari and testing there as well to verify the fix.
Keywords: qawanted
We've got the right keyword already here on what we need to do here. B2G QA right now follows the "one keyword model per purpose." Right now, the purpose is getting bisection to find the patch. We don't need two keywords for this.
Keywords: qawanted
Is the Video in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=856242  the same issue as this one?
Regression window results for when this issue was resolved in v1: 

b2g18 2013-04-10-07-02-09 Fail
b2g18 2013-04-10-23-02-04 Fail
b2g18 2013-04-11-07-02-05 Pass
b2g18 2013-04-11-23-02-04 Pass
Keywords: regressionwindow-wanted
Hmm...that push log isn't telling me much. I can't point out a bug from that list that looks like a good candidate.
(In reply to Angela Hubenya from comment #22)
> Is the Video in this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=856242  the same issue as this
> one?

Looks like it.
(In reply to Angela Hubenya from comment #22)
> Is the Video in this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=856242  the same issue as this
> one?

Yes, this is the same issue.
Blocks: 836911
Blocks: 853486
No patch up at this point (with only one test cycle left for some of our partners), and users are able to easily work around. Not blocking since we don't have a partner champion at this point.
Status: NEW → RESOLVED
blocking-b2g: tef+ → -
Closed: 11 years ago
Resolution: --- → WONTFIX
(In reply to Alex Keybl [:akeybl] from comment #29)
> users are able to easily work around.

Can you explain how this is easily worked around? This is causing many bugs in the Marketplace which we can't do anything about because the bugs only happen on B2G.
(In reply to Matt Basta [:basta] from comment #30)
> (In reply to Alex Keybl [:akeybl] from comment #29)
> > users are able to easily work around.
> 
> Can you explain how this is easily worked around? This is causing many bugs
> in the Marketplace which we can't do anything about because the bugs only
> happen on B2G.

It's a mostly visual (as opposed to functional) bug that will be fixed in v1.1 - we need to be able to call it. If we fix this bug after today, we'll only have a single QA test cycle for partners which will likely be insufficient for a change of this nature.

If you don't agree with this decision please email b2g-release-drivers.
Ugh, I implore people to reconsider this - there is no work-around for this and it's 'purely visual' in the same way that not being able to see anything on the screen is 'purely visual'.

I gave the marketplace as an example as it's easy to reproduce on a device out of the box, but there are plenty of examples of this if you browse around. A good one, download the most popular 'Notes' application on the market and try to use it. It triggers this bug constantly just scrolling and it makes the interface disappear/render incorrectly frequently enough to make the app almost unusable. I lost data with this because a 'delete' button got hidden by this bug.

I think broken CSS transforms on a device that's meant to be a showcase of what HTML5 is capable of should be a blocker. Especially when lots of libraries that people will be using to write apps use CSS transforms.
Flags: needinfo?(akeybl)
Email b2g-release-drivers@mozilla.org with these concerns and cc me (jsmith@mozilla.com) about this and we can talk about this more there. Let's try to get this discussion out of bugzilla for now.
Flags: needinfo?(akeybl)
For Fireplace, we tried to mitigate the impact of this bug @ https://github.com/mozilla/fireplace/commit/cf5971c
Assignee: nobody → roc
In my tests, this is not fixed on B2G18.
Status: RESOLVED → REOPENED
status-b2g18: unaffected → affected
Resolution: WONTFIX → ---
Despite appearances, this doesn't seem to be related to nsIFrame::TryUpdateTransformOnly --- I have that returning false unconditionally, and the bug still occurs.
The Marketplace screenshot is a single image that is getting optimized to an ImageLayer. Disabling that optimization fixes part of the problem --- now it's always in the right place, but it still gets clipped incorrectly.
Disabling async animations doesn't seem to make any difference.
Dumping the shadow layer tree computed by CompositorParent looks good. Now suspecting bug in GL scissor rect calculation...
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> Dumping the shadow layer tree computed by CompositorParent looks good.

No, there's a problem here.
            OGLShadowContainerLayer (0x4a508890) [shadow-transform=[ 1 0; 0 1; 0 96; ]] [shadow-visible=< (x=0, y=0, w=320, h=460); >] [visible=< (x=0, y=0, w=320, h=460); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=460.000000) viewportScroll=(x=0.000000, y=96.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=1 }]
              OGLShadowThebesLayer (0x4a581490) [shadow-transform=[ 1 0; 0 1; 0 -192; ]] [shadow-visible=< (x=0, y=146, w=320, h=410); >] [transform=[ 1 0; 0 1; 0 -96; ]] [visible=< (x=0, y=146, w=320, h=410); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=146, w=320, h=410); >]
              OGLShadowContainerLayer (0x487f3890) [shadow-clip=(x=0, y=60, w=320, h=230)] [shadow-transform=[ 1 0; 0 1; 72 60; ]] [shadow-visible=< (x=43, y=15, w=94, h=150); >] [clip=(x=0, y=60, w=320, h=230)] [transform=[ 1 0; 0 1; 72 60; ]] [visible=< (x=43, y=15, w=94, h=150); >]
                OGLShadowThebesLayer (0x489bbc90) [shadow-transform=[ 1 0; 0 1; 0 -96; ]] [shadow-visible=< (x=43, y=15, w=94, h=150); >] [visible=< (x=43, y=15, w=94, h=150); >] [isFixedPosition] [valid=< (x=43, y=15, w=94, h=150); >]

CompositorParent has translated 0x4a508890 down by 96 due to the viewportScroll, 0x489bbc90 has been translated back up by 96 to compensate, but that doesn't work since 0x487f3890 is in the middle and its cliprect is unchanged.
Looks like a regression from bug 825808. If we call SetIsFixedPosition on the transformed layer corresponding to 0x487f3890, everything works as it should. That's really the right thing to do according to the meaning of SetIsFixedPosition. We just need to fix bug 825808 another way.
Backing out bug 825808 fixes this bug, and interestingly doesn't seem to regress the homescreen swiping, or fixed-pos elements in Web pages in the browser.
There's still definitely a problem that CompositorParent::TransformFixedLayers can overwrite the shadow transform set by SampleAnimations, even though I can't reproduce a bug because of it. But that looks easy to fix.
Attached patch fix — — Splinter Review 
Seems to work well. Patch also applies quite cleanly to 1.0.1.

I think this is fairly straightforward but definitely needs QA love at this stage of the game --- everything mentioned in this bug and in bug 825808.
Attachment #749642 - Flags: review?
Attachment #749642 - Flags: feedback?(ajones)
Attachment #749642 - Flags: review? → review?(chrislord.net)
Attachment #749642 - Flags: feedback?(bugmail.mozilla)
Attachment #749642 - Flags: feedback?(bugmail.mozilla) → feedback?(bgirard)
nrc and kats are away for a few days, or I'd request feedback from them too.
I'm looking on Bug 833743 - [clock] Numbers broken in Time slector of new alarm . But not sure Bug 833743 is relative with the issue here or not. 

If we turn on "Flash repainted area" in Settings app -> More Information -> Developer, we can see sometimes rendering wrong x-position frame for font animation during scrolling time picker. Is the symptom related with the issue? Thanks.
(In reply to Ian Liu [:ianliu] from comment #47)
> I'm looking on Bug 833743 - [clock] Numbers broken in Time slector of new
> alarm . But not sure Bug 833743 is relative with the issue here or not. 
> 
> If we turn on "Flash repainted area" in Settings app -> More Information ->
> Developer, we can see sometimes rendering wrong x-position frame for font
> animation during scrolling time picker. Is the symptom related with the
> issue? Thanks.

Please reference the attached image via the link.
https://bugzilla.mozilla.org/show_bug.cgi?id=833743#c14 

Number 54, AM, PM are rendering in wrong x-position. Right Side of text 'PM' is remaining wrong frame which is over the repainting picker column slide. It won't be disappeared until the region be repainted.
Comment on attachment 749642 [details] [diff] [review]
fix

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

Nice catch - I'd like the second review comment addressed, but functionally it doesn't make a difference so r+.

::: gfx/layers/Layers.h
@@ +828,5 @@
>    /**
> +   * Returns the local transform for this layer: either mTransform or,
> +   * for shadow layers, GetShadowTransform()
> +   */
> +  const gfx3DMatrix GetLocalTransform();

It might be worth getting an f+ from a gfx peer to make sure this move is ok, but it looks fine to me.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +630,2 @@
>      shadow->SetShadowTransform(layerTransform);
> +    shadow->SetShadowTransformSetByAnimation(false);

I assume this gets set as false so if this function is called again, we don't accumulate the transformation? I don't see the point in this though, as if SampleAnimations isn't called before the next composite, we'll end up using GetTransform, which won't have the async animation transformation applied and would lead to incorrect results.

Alternatively, if this is just there to reset the flag, that doesn't seem correct either as it'll only reset the flag on some fixed layers.

As SampleAnimations is called at the beginning of TransformShadowTree, I think this can be omitted. To make extra sure, if we're going to reset the flag anywhere, we should reset it at the top of SampleAnimations, where it will iterate over every layer.
Attachment #749642 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #49)
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +630,2 @@
> >      shadow->SetShadowTransform(layerTransform);
> > +    shadow->SetShadowTransformSetByAnimation(false);
> 
> I assume this gets set as false so if this function is called again, we
> don't accumulate the transformation? I don't see the point in this though,
> as if SampleAnimations isn't called before the next composite, we'll end up
> using GetTransform, which won't have the async animation transformation
> applied and would lead to incorrect results.

I wanted to keep the invariant simple: GetShadowTransformSetByAnimation returns true if and only if the shadow transform for this layer was set by SampleAnimations. It seems better to keep that invariant simple rather than relax it for the sake of removing some of these SetShadowTransformSetByAnimation calls (which are basically free).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> (In reply to Chris Lord [:cwiiis] from comment #49)
> > ::: gfx/layers/ipc/CompositorParent.cpp
> > @@ +630,2 @@
> > >      shadow->SetShadowTransform(layerTransform);
> > > +    shadow->SetShadowTransformSetByAnimation(false);
> > 
> > I assume this gets set as false so if this function is called again, we
> > don't accumulate the transformation? I don't see the point in this though,
> > as if SampleAnimations isn't called before the next composite, we'll end up
> > using GetTransform, which won't have the async animation transformation
> > applied and would lead to incorrect results.
> 
> I wanted to keep the invariant simple: GetShadowTransformSetByAnimation
> returns true if and only if the shadow transform for this layer was set by
> SampleAnimations. It seems better to keep that invariant simple rather than
> relax it for the sake of removing some of these
> SetShadowTransformSetByAnimation calls (which are basically free).

I understand, but at the moment the flag means SampleAnimations was called at some point on the layer and TransformFixedLayers wasn't - I think resetting the flag in SampleAnimations rather than in TransformFixedLayers would be a good idea, just in case someone else decides to use this flag down the line and it doesn't behave quite as expected. But I don't have strong feelings either, so you're call :)
Blocks: 872508
(In reply to Chris Lord [:cwiiis] from comment #51)
> I understand, but at the moment the flag means SampleAnimations was called
> at some point on the layer and TransformFixedLayers wasn't

It means that the current shadow transform is the result of sampling the transform animation. That's all.

> I think resetting the flag in SampleAnimations rather than in TransformFixedLayers would be a
> good idea

I'm not sure what you mean, because it sounds incorrect.
(In reply to Chris Lord [:cwiiis] from comment #51)
> I understand, but at the moment the flag means SampleAnimations was called
> at some point on the layer and TransformFixedLayers wasn't - I think
> resetting the flag in SampleAnimations rather than in TransformFixedLayers
> would be a good idea, just in case someone else decides to use this flag
> down the line and it doesn't behave quite as expected. But I don't have
> strong feelings either, so you're call :)

mShadowTransformSetByAnimation identifies ownership of the shadow transform. In this patch the ownership gets grabbed either by TransformFixedLayers or by SampleAnimations. It would be cleaner to either have separate transforms for animation and async scrolling or to have the animation borrow the transform by setting and clearing mShadowTransformSetByAnimation.

It could use a safe lease object of a boolean.
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #53)
> mShadowTransformSetByAnimation identifies ownership of the shadow transform.
> In this patch the ownership gets grabbed either by TransformFixedLayers or
> by SampleAnimations. It would be cleaner to either have separate transforms
> for animation and async scrolling or to have the animation borrow the
> transform by setting and clearing mShadowTransformSetByAnimation.

This is correct. However I don't think the overhead of carrying around an extra transform is worth it.
Sorry I didn't get to this today. I'll be flying all day tomorrow. If you still need it I'll review it Friday.
Attachment #749642 - Flags: feedback?(ajones) → feedback+
https://hg.mozilla.org/mozilla-central/rev/9af1395c410c
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Now we've got a landed patch here. There's a lot debate on this one on release drivers. Let's do blocking triage again here to figure out what to do here.
blocking-b2g: - → tef?
Can we at least land this on 1.1 while we think about 1.0.1?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> Can we at least land this on 1.1 while we think about 1.0.1?

Good idea. leo triage is happening at 4pm PST, so let me get a leo triage on this first to see if we can land this immediately with a blocking+. Then, after landing, we can nom to tef.
blocking-b2g: tef? → leo?
Comment on attachment 749642 [details] [diff] [review]
fix

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

::: gfx/layers/Layers.h
@@ +828,5 @@
>    /**
> +   * Returns the local transform for this layer: either mTransform or,
> +   * for shadow layers, GetShadowTransform()
> +   */
> +  const gfx3DMatrix GetLocalTransform();

In some places we use GetEffectiveX and others we use GetLocalX for getting either the layer or the shadow layer version of the property. I wish we were consistent with that but this isn't the right place to clean this up :(.
We have a notion of "effective transforms" which is more than just "layer or shadowlayer transform" --- it includes snapping effects. So I think exposing this "local" concept as well still makes sense.
Please provide an uplift nomination with risk assessment so we can evaluate for v1.1 landing.
blocking-b2g: leo? → tef?
tracking-b2g18: --- → +
Comment on attachment 749642 [details] [diff] [review]
fix

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 #):  825808
User impact if declined: pretty bad rendering bugs on a variety of apps/sites
Testing completed: landed on central for some days, manual testing of various sites and apps known to be affected
Risk to taking this patch (and alternatives if risky): no alternative known. This is not a zero risk patch but it's not scary IMHO. Pretty well understood.
String or UUID changes made by this patch: none
Attachment #749642 - Flags: approval-mozilla-b2g18?
Attachment #749642 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
This needs a branch-specific patch.
Keywords: branch-patch-needed
The patch attached to this bug applies to b2g18 1.1 (and 1.0.1). I developed it there. (The patch I checked into mozilla-central was somewhat different due to the changes on central.)
Attachment #749642 - Flags: feedback?(bgirard) → feedback+
let's assume the risk, plese check-in this in 1.0.1 asap
blocking-b2g: tef? → tef+
Daniel, thank you to mark it as tef+.
Can someone help to uplift to v1.0.1 as soon as possible? Thanks!
Flags: needinfo?
(In reply to khu from comment #69)
> Can someone help to uplift to v1.0.1 as soon as possible? Thanks!

Flag checkin-needed for this.
Flags: needinfo?
Keywords: branch-patch-needed → checkin-needed
Keywords: checkin-needed
Whiteboard: [status: may be fixed by bug 846144, awaiting confirmation] c=
This issue does not repro anymore. Issue seems to be fixed. The screenshots does not follow down the page in the market place. 

Verified on 

Inari Build ID: 2013053170208
Gecko:Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/be5c2ee11d02
Gaia:  e7114bdf4078274fc127a3b2a58dad91d6884219

Unagi ID: 20130530070208
Gecko:  http://hg.mozilla.org/releases/mozilla-b2g18/rev/09ac1fd2959c
Gaia:   1cca9324d4444ad28c6fa99875e17abf7e8230be
status-b2g18: fixed → verified
status-b2g18-v1.0.1: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: