Closed Bug 788409 Opened 12 years ago Closed 12 years ago

Lock screen can be seen briefly when it is being faded out

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: timdream, Assigned: cjones)

References

Details

(Whiteboard: [LOE:S] [soft])

Attachments

(3 files, 3 obsolete files)

Patch
3.46 KB, patch
dbaron
: review-
cjones
: feedback+
Details | Diff | Splinter Review
patch
5.72 KB, patch
Details | Diff | Splinter Review
above, with fix applied
5.68 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
STR: 1. Unlock the phone, observe the unlock transition Expected: 1. Lock screen exits with "fly out" effect. Actual: 1. Lock screen exits with "fly out" effect, but flashes a bit when transition ends. Note: Happen on B2G/Desktop the first time I updated the lock screen transition, but it is no longer there.
Note that lock screen currently generates slow animation warning, although no slowness observed with human eye: Performance warning: Async animation disabled because frame was not marked active for opacity animation [div with id 'lockscreen']
Probably a v1 blocker at this point due to the UI issue.
blocking-basecamp: ? → +
Tim, are you working on the lock screen?
Assignee: nobody → timdream+bugs
(In reply to Andrew Overholt [:overholt] from comment #3) > Tim, are you working on the lock screen? I work on the Gaia part of the lock screen implementation, and this is a Gecko graphics issue introduced by my implementation. I reported the bug.
Assignee: timdream+bugs → nobody
Summary: [Otoro] Lock screen can be seem briefly when it is being fade out → [Otoro] Lock screen can be seen briefly when it is being faded out
Chris Jones, who should own this? Someone on Layout? You?
Assignee: nobody → jones.chris.g
I can't reproduce this.
Tim, we need to find a way to reliably reproduce this so we can find someone who works on Gecko that can fix it. Is there something different in your phone/environment from cjones'? Starting from a clean ./flash.sh of a nightly build, can you outline the steps you use to reproduce this? Can you take a video of your Otoro phone when this happens? Better still, the entire process to reproduce it. Thanks very much!
Assignee: jones.chris.g → nobody
(In reply to Andrew Overholt [:overholt] from comment #7) > Tim, we need to find a way to reliably reproduce this so we can find someone > who works on Gecko that can fix it. Is there something different in your > phone/environment from cjones'? Starting from a clean ./flash.sh of a > nightly build, can you outline the steps you use to reproduce this? Can you > take a video of your Otoro phone when this happens? Better still, the > entire process to reproduce it. Thanks very much! I can assure you my build is as clean as possible, and STR stated in comment 0 does reproduce the issue. The flash is very brief. If you cannot see it, turn off paint flashing and find an app with bright color backgrounds (I recommend Settings app).
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #8) > (In reply to Andrew Overholt [:overholt] from comment #7) > > Tim, we need to find a way to reliably reproduce this so we can find someone > > who works on Gecko that can fix it. Is there something different in your > > phone/environment from cjones'? Starting from a clean ./flash.sh of a > > nightly build, can you outline the steps you use to reproduce this? Can you > > take a video of your Otoro phone when this happens? Better still, the > > entire process to reproduce it. Thanks very much! > > I can assure you my build is as clean as possible, and STR stated in comment > 0 does reproduce the issue. The flash is very brief. If you cannot see it, > turn off paint flashing and find an app with bright color backgrounds (I > recommend Settings app). Anyhow, I suspect this will automatically go away when bug 790508 is resolved.
OK, I can reproduce this with the settings app. Are you sure the lock screen is being hidden atomically with the end of the transition? The effect here looks kind of like what might happen if the lock screen's frame has a CSS class change after the transition, but it being set hidden (or at a lower z-index) than the settings app's frame in a separate event-loop iteration after the class change.
That was probably a little hard to follow :). What I'm asking is, are you sure this sequence of events isn't happening 1. settings app made visible 2. lock screen transition starts 3. lock screen transition ends 4. lock screen frame has a CSS class change 5. lock screen set hidden or lower z-index than settings app where (3)/(4) and (5) happen in different event-loop iterations?
I can only reproduce this with in-process apps, which is why settings works well. That makes a gecko bug more likely.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > That was probably a little hard to follow :). What I'm asking is, are you > sure this sequence of events isn't happening > 1. settings app made visible > 2. lock screen transition starts > 3. lock screen transition ends > 4. lock screen frame has a CSS class change > 5. lock screen set hidden or lower z-index than settings app > > where (3)/(4) and (5) happen in different event-loop iterations? There were no (4) and (5). I am transiting the "visibility" property, suggested by dzbarsky, which will simply hide the div when the transition ends at step 3.
Chris Jones, I'm going to put this back to you but feel free to re-assign as you see fit.
Assignee: nobody → jones.chris.g
Whiteboard: [LOE:S]
I can occasionally reproduce something like this with the browser app (only in-process app for v1), but it's not all that noticeable. I wouldn't hold the release for this. Kan-Ru, if you have cycles, want to take a look? STR (1) Unlock phone (2) Load browser app (3) Lock phone (power button) (4) Press power button, slide arrow to unlock Sometimes a brief flash can be seen when displaying the browser app, in the content area only.
blocking-basecamp: + → ?
Yeah, a very brief flash. I'll take a look.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > I can occasionally reproduce something like this with the browser app (only > in-process app for v1), but it's not all that noticeable. I wouldn't hold > the release for this. Since it's unlikely to happen, let's explicitly take this off the blocker list. That doesn't prevent a fix, of course, Kan-Ru :)
blocking-basecamp: ? → -
This seems to be caused by the scale() transform. Cannot reproduce anymore when I remove this line: https://github.com/ttaubert/gaia/blob/42233ce90d22ea3b38ac42bb13793607d95eed8f/apps/system/style/lockscreen/lockscreen.css#L27
Tim, thats very useful diagnosis. Chris, dz, looks like there is a bug in the OMTA code.
I just remembered running into something similar with OMTA. Is this maybe bug 790582?
Tim, can you reproduce this with OMTA disabled?
Slow as hell (on the nexus s at least) but I cannot reproduce it anymore with OMTA disabled.
After enabling PIN access for the lock screen (everyone with partner data in their moco email accounts has this on, right? right?), I found that (i) that transition is butter smooth to homescreen, no idea why; (ii) the artifact here is *much* worse. I think it's worse enough we should re-triage for PIN unlock. I don't know what UR says about how commonly it's expected to be used. Nick, have you fixed this bug incidentally as part of bug 780692?
Blocks: b2g-layers-work
blocking-basecamp: - → ?
Depends on: 780692
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23) > After enabling PIN access for the lock screen (everyone with partner data in > their moco email accounts has this on, right? right?), I found that (i) that > transition is butter smooth to homescreen, no idea why; (ii) the artifact > here is *much* worse. > > I think it's worse enough we should re-triage for PIN unlock. I don't know > what UR says about how commonly it's expected to be used. > > Nick, have you fixed this bug incidentally as part of bug 780692? Can I test this on desktop? I don't have an Otaro to test on. The WIP patches on 780692 are up to date if anyone wants to test for me :-)
The patch in bug 780692 doesn't seem to fix this bug, or rather make it worse, the last frame of homescreen dragging animation could be seen twice now.
If bug 780692 doesn't fix that and spreads the problem to the homescreen, then we need to fix it here and this needs to block.
Blocking based on comment #26.
blocking-basecamp: ? → +
Whiteboard: [LOE:S] → [LOE:S] [soft]
Found another clue: if I use the passcode unlock when useless blank homescreen page is the one in view, I don't see this artifact. If I unlock over a page that has actual content, then the artifact is really bad. This seems to imply that the artifact depends on content being rendered, which immediately makes me think that we're waiting longer for a buffer resolve forced by bug 805689.
Depends on: 805689
#if 0'ing out the omgslow code doesn't reliably fix this. Poo.
No longer depends on: 805689
Based on comment 25, my current hypothesis is that we 1. finish the async animation on the compositor thread 2. have just flushed layout on the main thread for the sake of transitionend etc, and the last frame is retained in the layer tree. Or in this bug, some frame near the last. 3. have begun repainting the first frame after the transitionend, but it takes a while So my guess is that this "artifact" is the time between ~last frame appearing on screen before we repaint the first post-transition frame. When the optimization in bug 780692 is successful (i.e. no flushes were forced during the animation), we should be able to suppress painting for the flush we do on the last frame since we know the compositor has drawn it already. However, if bug 780692 doesn't kick in, we'll still be vulnerable to this artifact and I can't think of anything we can do about it in general :/.
Another possibility is that what we're seeing is the last frame the *compositor* has sampled, and that frame continues to stay onscreen until we can repaint the first post-animation frame on the main thread.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #30) > > When the optimization in bug 780692 is successful (i.e. no flushes were > forced during the animation), we should be able to suppress painting for the > flush we do on the last frame since we know the compositor has drawn it > already. However, if bug 780692 doesn't kick in, we'll still be vulnerable > to this artifact and I can't think of anything we can do about it in general > :/. I think we will still need to flush on the last tick of an animation, at least until we handle visibility changes on the compositor. I have a patch to fix that, but I think there may be other properties that necessitate flushing.
Note, I said "suppress painting for the flush we did on the last frame". Basically we would coalesce two refresh driver ticks, two flushes: one for the last frame and then immediately afterward the one for the first post-animation frame.
FTR, I've seen the same artifacts happening with the transition that switches between apps where we have a simple translateX() animation. Same with the keyboard in e.g. the browser app - a simple translateY() transition. Especially when I'm on a web page like bing and focus the search field, opening the keyboard flickers back and forth until it's displayed. Disabling OMTA fixes those two occurences as well.
Summary: [Otoro] Lock screen can be seen briefly when it is being faded out → Lock screen can be seen briefly when it is being faded out
I'm seeing the same thing now when switching from homescreen to a running app. Easy to reproduce with the Settings app.
No longer depends on: 780692
The problem lies somewhere in between comment 30 and comment 32. Here's what's happening 1. content creates animation of property P from value v_0 to v_n 2. we offload this animation to the compositor thread 3. the compositor samples the last frame of the animation and schedules a recomposite 4. on the next sample, the compositor * realizes it's past the end of the animation * removes the animation spec from the shadow layer * ends up "sampling" v_0 (in simple cases) 5. around that time the main thread samples the last frame of the animation and fires transitionend 6. eventually the main thread paints the first post-animation frame and sends a layers transaction that clears the async animation spec So the artifact we're seeing here is what's on the framebuffer for the duration between (4) and (5) above: we "jerk" from property v_n to v_0 and then back to ~v_n again. A key invariant we have to maintain in this code is that only the main thread affects the async animation specs set on Layers. Otherwise we have synchronization bugs like this one. This patch restores that invariant by having the compositor keep around the last interpolated value for a layer until the main thread pushes a transaction to clear the spec and reset the value. The patch isn't correct because it doesn't properly compute the "last position" for animation specs. I need to modify ElementAnimations::GetPositionInIteration() to do that but I'm not 100% sure how yet. This patch fixes both the "frame flash" artifact and "bouncing keyboard" bugs.
Attachment #679993 - Flags: feedback?(dbaron)
The |start + duration - 1us| doesn't make me very happy. Better suggestions welcome.
Attachment #679993 - Attachment is obsolete: true
Attachment #679993 - Flags: feedback?(dbaron)
Attachment #681435 - Flags: review?(dbaron)
Comment on attachment 681435 [details] [diff] [review] Only the main thread is allowed to change animation specs set on layers. Continue sampling the end-of-animation frame on the compositor thread until the main thread catches up. Why not just set positionInIteration = 1 to render the last frame?
(In reply to David Zbarsky (:dzbarsky) from comment #38) > Comment on attachment 681435 [details] [diff] [review] > Only the main thread is allowed to change animation specs set on layers. > Continue sampling the end-of-animation frame on the compositor thread until > the main thread catches up. > > Why not just set positionInIteration = 1 to render the last frame? That's what my hacky PoC patch did. But can't the position be something other than 1.0 at the end of an animation or transition, if there's a phase or something like that used?
Attached patch Patch (obsolete) — — Splinter Review
Chris, does this work for you? (I can't reproduce this on my device)
Attachment #682989 - Flags: feedback?(jones.chris.g)
Attached patch Patch — — Splinter Review
Attachment #682989 - Attachment is obsolete: true
Attachment #682989 - Flags: feedback?(jones.chris.g)
Attachment #682990 - Flags: feedback?(jones.chris.g)
Comment on attachment 682990 [details] [diff] [review] Patch lgtm, and works the same as my buggy patch.
Attachment #682990 - Flags: review?(dbaron)
Attachment #682990 - Flags: feedback?(jones.chris.g)
Attachment #682990 - Flags: feedback+
Attachment #681435 - Attachment is obsolete: true
Attachment #681435 - Flags: review?(dbaron)
It might be a little cleaner to have GetPositionInIteration return a bool outparam for whether the animation is done and always return the actual position (instead of -1).
Comment on attachment 682990 [details] [diff] [review] Patch > static bool > SampleAnimations(Layer* aLayer, TimeStamp aPoint) > { > AnimationArray& animations = aLayer->GetAnimations(); > InfallibleTArray<AnimData>& animationData = aLayer->GetAnimationData(); > > bool activeAnimations = false; >+ bool pastLastFrame = false; > > for (uint32_t i = animations.Length(); i-- !=0; ) { > Animation& animation = animations[i]; > AnimData& animData = animationData[i]; > > double numIterations = animation.numIterations() != -1 ? > animation.numIterations() : NS_IEEEPositiveInfinity(); > double positionInIteration = > ElementAnimations::GetPositionInIteration(animation.startTime(), > aPoint, > animation.duration(), > numIterations, > animation.direction()); > > if (positionInIteration == -1) { >- animations.RemoveElementAt(i); >- animationData.RemoveElementAt(i); >- continue; >+ // We went past the end of the animation spec. Back up in time >+ // to the last frame so that we render a consistent layer tree >+ // until the main thread catches up. It's important that only >+ // the main thread changes async animation specs set on layers. >+ NS_ASSERTION(animation.numIterations() != -1, >+ "Animation with infinite iterations should not claim to be done"); >+ >+ // If we have a fractional number of iterations, the last position will >+ // not be 1.0 >+ positionInIteration = numIterations - floor(numIterations); >+ if (positionInIteration == 0) { >+ positionInIteration = 1.0; >+ } >+ pastLastFrame = true; This isn't actually right, because GetPositionInIteration does account for animation-direction, but your code doesn't. For example, if you have: animation-direction: alternate; animation-iteration-count: 2; then the graph of positionInIteration over time will look like this, where A represents a value produced by GetPositionInIteration and B represents a value produced by your added code here: 1.0 | A BBBBB | A A 0.5 | A A | A A 0.0 |A A ------------- 0 1 2 3 whereas you need to produce: 1.0 | A | A A 0.5 | A A | A A 0.0 |A ABBBB ------------- 0 1 2 3 The simplest fix might be either (1) to call GetPositionInIteration a second time with fake data if it returns -1 the first ime or (2) to massage aPoint instead (before the call to GetPositionInIteration). >- activeAnimations = true; >+ // If we are past the last frame, we are already at the correct rendering >+ // and don't need to be composited. >+ if (!pastLastFrame) { >+ activeAnimations = true; >+ } Do you need to do this the first time you hit the last frame? (I don't know, but it seems like something you might need.)
Attachment #682990 - Flags: review?(dbaron) → review-
Oh, but what cjones did with subtracting one microsecond doesn't actually work right either, since it breaks step-end timing functions. I suspect this requires changing GetPositionInIteration in some way.
(In reply to David Baron [:dbaron] from comment #46) > Oh, but what cjones did with subtracting one microsecond doesn't actually > work right either, since it breaks step-end timing functions. Right, that patch was plain wrong. dzbarsky set me straight on that.
(In reply to David Zbarsky (:dzbarsky) from comment #44) > It might be a little cleaner to have GetPositionInIteration return a bool > outparam for whether the animation is done and always return the actual > position (instead of -1). Is the position it would compute in that case always guaranteed to the the "end" position?
No, it would need to be modified to compute a useful position (but that shouldn't be too hard).
What would be required to do that? I can poke around, but maybe you or dz can take this or point me in the right direction?
Attached patch patch — — Splinter Review
I think this ought to work, but I don't have a working setup for testing it right now.
With this small change, it works great diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp --- a/layout/style/nsAnimationManager.cpp +++ b/layout/style/nsAnimationManager.cpp @@ -74,17 +74,17 @@ ElementAnimations::GetPositionInIteratio } } else { // If aAnimation is null, that means we're on the compositor // thread. We want to just keep filling forwards until the main // thread gets around to updating the compositor thread (which // might take a little while). So just assume we fill fowards and // move on. } - currentIterationCount = double(aAnimation->mIterationCount); + currentIterationCount = aIterationCount; } else { if (aAnimation && !aAnimation->IsPaused()) { aEa->mNeedsRefreshes = true; } if (currentIterationCount < 0.0) { NS_ASSERTION(aAnimation, "Should not run animation that hasn't started yet on the compositor"); if (!aAnimation->FillsBackwards()) { // No animation data. Except for that change, I think I understand this patch, if you can't find another reviewer.
Comment on attachment 684339 [details] [diff] [review] above, with fix applied This bug is pretty inbred already, so why not ;).
Attachment #684339 - Flags: review?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/a2858a64fe77
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: