763430 - Smooth orientation transition for B2G devices

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Etienne Segonzac (:etienne)]

On popular demand, here is a patch proposition providing a smooth transition when you change the orientation of a B2G device.

It works as follow:
- draws the content before the rotation in a canvas (listening to MozBeforeResize)
- draws the content after the rotation in a second canvas
- launches a CSS transition where the "screen" is rotating while fading between the before and after content.

It was tested a GSII (crazy fast) and an otoro (still smooth).

Comment 1

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Comment 2

[[:fabrice] Fabrice Desré]

I tested a bit on the otoro: as soon as something is really running on the phone, we don't get a smooth animation, and even sometimes no animation at all.

Also, we're allocation 2 full-screen sized canvases, and I'm wondering about the memory impact of doing that at this layer.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> I tested a bit on the otoro: as soon as something is really running on the
> phone, we don't get a smooth animation, and even sometimes no animation at
> all.

I assume bug 706179 should help here?

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) from comment #3)

> I assume bug 706179 should help here?

hopefully!

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> (In reply to Vivien Nicolas (:vingtetun) from comment #3)
> 
> > I assume bug 706179 should help here?
> 
> hopefully!

Cool. In the meantime if anyone got some optimization ideas I'm listening :)

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Also, we're allocation 2 full-screen sized canvases, and I'm wondering about
> the memory impact of doing that at this layer.

BTW I also welcome any suggestion on how to make something equivalent live in Gaia.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 631852 [details] [diff] [review]
Patch proposal

Chris can you review that? (i don't think that can be done in Gaia at this point)

Comment 8

[Andreas Gal :gal]

This is the right thing to do IMO, amongst others also because with the content process lower priority this should be a smooth animation in most cases even without OMTA.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 631852 [details] [diff] [review]
Patch proposal

This is pretty cool :).

r- for three reasons
 1. The drawWindow() variant you're using here doesn't work for out-of-process content.  (Ping me for details.)  In addition, remote content never repaints again after a rotation change with this patch; not sure where that bug lies.  But the patch can't land as-is.

 2. It's sort of an optical illusion, but the visual effect of the transition can deform content.  Try rotating the lock screen: the "ball" deforms in an unpleasant way.  (Yes, it's a bug that the lock screen isn't locking orientation, but this effect has to work for arbitrary content.)  We need to investigate this and check with UX.

 3. We shouldn't need the second canvas.  I guess you introduced this to work around perf problems?  That would be OK for now, but it's not needed.

Sorry!  This is neat.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's revisit this issue :)

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> r- for three reasons
>  1. The drawWindow() variant you're using here doesn't work for
> out-of-process content.  (Ping me for details.)  In addition, remote content
> never repaints again after a rotation change with this patch; not sure where
> that bug lies.  But the patch can't land as-is.

We can now make the screenshot of the OOP frame right? This shouldn't be a problem anymore.

>  2. It's sort of an optical illusion, but the visual effect of the
> transition can deform content.  Try rotating the lock screen: the "ball"
> deforms in an unpleasant way.  (Yes, it's a bug that the lock screen isn't
> locking orientation, but this effect has to work for arbitrary content.)  We
> need to investigate this and check with UX.

I look into this a bit, including enabling slow motion on iOS simulator and try to "rotate" the phone. iOS does in a way that for some native widget, it will recalculate the width & layout as the transition happens (for example, see status bar); for other content like WebView or keyboard, it would do that etienne do right here (cross fade as the screen resizes).

The screen currently flashes as we rotate screen. I don't know if the flash deserve blocking though.

Josh, would you mind playing with the orientation a bit and see if we should block? Thank.

Comment 11

[Etienne Segonzac (:etienne)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #10)
> Let's revisit this issue :)

Hehe, I'll happily rebase the patch :)

> Josh, would you mind playing with the orientation a bit and see if we should
> block? Thank.

Also is the proposed transition good enough?
ie. launches a CSS transition where the "screen" is rotating while fading between the before and after content.

Comment 12

[Etienne Segonzac (:etienne)]

> Also is the proposed transition good enough?
> ie. launches a CSS transition where the "screen" is rotating while fading
> between the before and after content.

Here is an old demo video to give you an idea:
https://www.dropbox.com/s/lxu6di7t2scp0kn/b2g-smooth-orientation.mov

Comment 13

[Josh Carpenter [:jcarpenter]]

(In reply to Etienne Segonzac (:etienne) from comment #12)
> > Also is the proposed transition good enough?
> > ie. launches a CSS transition where the "screen" is rotating while fading
> > between the before and after content.
> 
> Here is an old demo video to give you an idea:
> https://www.dropbox.com/s/lxu6di7t2scp0kn/b2g-smooth-orientation.mov

Great work. Looks solid to me for a v1 release. cc'ing Patryk for his input.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Josh, Patryk,

Devs are asked to hands off non-blocking bugs, so if this should land we need a reason to block.

Again, from comment 10, if the current orientation is undesired and this change look solid, we should block the release on this. Please try the current orientation change transition (quite strange, actually) and nominate for blocking if you think so.

Comment 15

[Josh Carpenter [:jcarpenter]]

The difference I see between what Etienne has posted and the current implementation is FPS. When I rotate in browser on my Unagi, the animation only shows one frame before it snaps into new position. That I would like to fix, yes. But I'd call it a UX-P2 or UX-P3.

Comment 16

[Patryk Adamczyk [:patryk] UX]

In browser or video it really makes the device feel so when it rotates. So if this isn't hardware related but rather inefficient code I'd opt to get it fixed. The only reason I would not make it UX-P1 is that we have limited apps using rotation.

Comment 17

[Deepa Subramanian]

The user finds the play icon displaced and not rotating along with the video orientation when a video is shot and viewed in the Gallery app. 

Please note that this does not happen always but I saw it few times on my device. 

Leo Build ID: 20130411070205
Kernel Date: Mar 15
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/f671fa539473
Gaia: e7e338a765e22334b40ced41489a785941382c66

Comment 19

[Etienne Segonzac (:etienne)]

Taking a quick look, can't promise anything but at least we'll know if some of the patch is still usable.
It's the perfect Friday afternoon project :)

Comment 20

[Etienne Segonzac (:etienne)]

Attachment: reverting bug 946449

Comment 21

[Etienne Segonzac (:etienne)]

Attachment: WIP

Rebased and cleaned-up.

Still fighting with drawWindow but we should be able to do this since the screenshot feature uses it to.

Comment 22

[Etienne Segonzac (:etienne)]

Update: just found a promising new approach removing the screenshoting issues I was having, new patch tomorrow hopefully!

Comment 23

[Chris Lord [:cwiiis]]

Excited!

Comment 24

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

After battling a long time (with help from Nical) to make drawWindow work properly without any luck, I tried a new approach: giving up the fade between the "before" and "after" screenshots and just transitioning the system app frame directly. No more screenshots. It's only the "after" rendering that is transitioning but I mitigate this by making the transition go quickly at the start (when the content is stretched).

This makes for a much simpler patch and removes the need for the MozBeforeResize event.
Moving away from the (2!) screenshots is also probably a memory win.

Anyway this looks cool enough for me to ask for feedback. If positive I'll probably add a pref for this (default on) and move to review.

Cheers!

PS: I also tried doing this on the gaia side (transitioning #screen) but it's really janky, not sure why.

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 8556553 [details] [diff] [review]
Patch proposal

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

::: b2g/app/b2g.js
@@ +873,5 @@
>  // Enable native identity (persona/browserid)
>  pref("dom.identity.enabled", true);
>  
>  // Wait up to this much milliseconds when orientation changed
> +pref("layers.orientation.sync.timeout", 250);

Not sure who to ask but I'd love to know more about this pref and if the tweak makes sense.

Comment 26

[Chris Lord [:cwiiis]]

Comment on attachment 8556553 [details] [diff] [review]
Patch proposal

Giving this two thumbs up! So much better than nothing and also not bad full stop :) It would be preferable to have a cross-fade and no stretch, but understandable that we can't.

I wonder if this would just look better without the stretch at all? It happens so fast, if the background colour was the same as the app's, that might look alright...

Honestly though, we should consider landing this as is and improving from there, this is superior to having nothing.

Comment 27

[Etienne Segonzac (:etienne)]

\o/

Just found (and fixed) a bug when you do a complete 180 rotation (no resize event in this case), will be in tomorrow's patch!

Comment 28

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Given that the first version of this patch is from more that 2.5 years ago I'm going to go with the "let's land the simple version then iterate" strategy :)

Hi Fabrice!
It's a small patch, it's behind a pref (but I think it's important to default it to "true" at first), and yes I tried implementing in on the gaia side but it's super janky despite my best efforts :/

Hope you like it, let me know what should change.

Comment 29

[[:fabrice] Fabrice Desré]

Comment on attachment 8557110 [details] [diff] [review]
Patch v2

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

f+ because I'd like to see the cleaned up version.

I tested on device, and this is just great! super nice job Etienne!

::: b2g/components/OrientationChangeHandler.jsm
@@ +6,5 @@
> +
> +this.EXPORTED_SYMBOLS = [];
> +
> +const Cu = Components.utils;
> +Cu.import("resource://gre/modules/Services.jsm");

you have both single and double quoted strings in this file. The usual style in chrome js is double quotes.

@@ +11,5 @@
> +
> +let window = Services.wm.getMostRecentWindow("navigator:browser");
> +let document = window.document;
> +let system = document.getElementById('systemapp');
> +

nit: just do let system = window.document.getElementById('systemapp')

@@ +20,5 @@
> +                 'portrait-primary'],
> +
> +  lastOrientation: 'portrait-primary',
> +
> +  init: function och_init() {

no need to name functions anymore!

@@ +27,5 @@
> +
> +  handleEvent: function och_handleEvent(evt) {
> +    let newOrientation = window.screen.mozOrientation;
> +    let orientationIndex = this.orientations.indexOf(this.lastOrientation);
> +    let nextClockwiseOrientation = this.orientations[orientationIndex + 1];

shouldn't this be | this.orientations[orientationIndex + 1] % this.orientations.length| ?

@@ +41,5 @@
> +    if (fullSwitch) {
> +      angle = 180;
> +      xFactor = 1;
> +      yFactor = 1;
> +    }

I think it would be a bit more readable this way:
let angle, xFactor, yFactor;
if (fullSwitch) {
  angle = 180;
  xFactor = 1;
} else {
  angle = (nextClockwiseOrientation == newOrientation) ? 90 : -90;
  xFactor = window.innerWidth / window.innerHeight;
}
yFactor = 1 / xFactor;

@@ +60,5 @@
> +      window.setTimeout(trigger);
> +      return;
> +    }
> +
> +    window.addEventListener('resize', function ap(e) {

nit: no idea where this 'ap' name comes from :P

Comment 30

[Etienne Segonzac (:etienne)]

Attachment: Patch v3

Thanks!
Here it is.

Comment 31

[Etienne Segonzac (:etienne)]

Overcautious try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce473ad645f

Comment 32

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/b22ddf26ab47

Comment 33

[Chris Lord [:cwiiis]]

Sorry to muddy the usefulness of the comment thread, but this is just so great.

Comment 34

[[:fabrice] Fabrice Desré]

(In reply to Chris Lord [:cwiiis] from comment #33)
> Sorry to muddy the usefulness of the comment thread, but this is just so
> great.

Feel free to ask for 2.2 approval

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b22ddf26ab47

Comment 36

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #34)
> (In reply to Chris Lord [:cwiiis] from comment #33)
> > Sorry to muddy the usefulness of the comment thread, but this is just so
> > great.
> 
> Feel free to ask for 2.2 approval

Yes please :)

Comment 37

[Chris Lord [:cwiiis]]

Comment on attachment 8557506 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: Janky orientation changes
Testing completed: On master.
Risk to taking this patch (and alternatives if risky): Low risk of transient oddness during orientation changes, easily backed out if an issue.
String or UUID changes made by this patch: None.

Comment 38

[Etienne Segonzac (:etienne)]

Comment on attachment 8557506 [details] [diff] [review]
Patch v3

[User impact] if declined: in applications supporting multiple orientations (browser, gallery, third party...) when rotating the phone, the app will just pop in the new orientation. If you listen very carefully you can actually hear the phone, slightly out of breath, trying to recompute everything.
This patch makes the orientation change look effortless :)

[Testing completed]: lots of shaking

[String changes made]: none



Hope the whimsy isn't completely out of place, triaging approval request isn't always funny and I do know how important it is. Truth is: this patch just makes the phone a little bit cooler, it's safe in the sense that it does not rely on weird timeout/events and it's super easy to deactivate altogether by toggling a pref if we discover issues a bit late.

cheers!

Comment 39

[Etienne Segonzac (:etienne)]

Comment on attachment 8557506 [details] [diff] [review]
Patch v3

Mid-air, my bad.

Comment 40

[:gerard-majax]

Damn, that works pretty well on my Nexus S \o/

Comment 41

[Chris Lord [:cwiiis]]

One unfortunate thing about this patch is that it doesn't seem to benefit from bug 927349 - if the app takes too long to redraw in its reoriented position, the transition is skipped entirely.

n?bbirtles in case he has any comment - I'm particularly interested in whether this is a bug, or just the way we do this means it circumvents the delayed animation start code somehow.

(still think we should uplift regardless, and if we manage to fix that, then great, we can try to uplift the fix and get an even nicer experience :))


Maybe just calling trigger in a setTimeout would work too? (about to try this)

Comment 42

[Chris Lord [:cwiiis]]

For reference, my go-to app that exhibits this bug is Facebook - it takes so long to render you don't see the transition at all. Twitter lets you see a little bit of it most of the time.

Comment 43

[Chris Lord [:cwiiis]]

Attachment: Follow-up: Work around long paint times killing orientation change animation

Etienne, what do you think of this? I realise it's not ideal, but this was the cleanest way I found to work around the issue (I tried lots of combinations of nested timeouts, requestAnimationFrames, will-change, etc.)

I assume the fact that MozAfterPaint isn't 100% reliable to fix this is because, in the bad case, there are multiple paints triggered by the resize that are interfering with the animation. I'd have thought bug 927349 should make this unnecessary, but perhaps there's something about this chain of events that causes that code to fail.

This also, irritatingly, makes some cases that looked good before look worse, as in those cases you can now see the entire animation and you see the stretched frame for a longer period of time.

Comment 44

[Chris Lord [:cwiiis]]

Comment on attachment 8557982 [details] [diff] [review]
Follow-up: Work around long paint times killing orientation change animation

Scrap that, this doesn't work nearly as consistently as I thought :(

Comment 45

[Chris Lord [:cwiiis]]

Also been messing around with nsIContentViewer.pausePainting/resumePainting, but can't seem to get them in a place where it can pause the drawing that causes the animation to hitch... (yet)

Comment 46

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Chris Lord [:cwiiis] from comment #41)
> One unfortunate thing about this patch is that it doesn't seem to benefit
> from bug 927349 - if the app takes too long to redraw in its reoriented
> position, the transition is skipped entirely.
> 
> n?bbirtles in case he has any comment - I'm particularly interested in
> whether this is a bug, or just the way we do this means it circumvents the
> delayed animation start code somehow.

Nothing in that code sticks out as disqualifying this from that optimization. There are a few tweaks to that optimization I'm about to land for bug 1123196 but I'm not sure they'll help. I'll fire up some builds and have a look.

Comment 47

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #46)
> Nothing in that code sticks out as disqualifying this from that
> optimization. There are a few tweaks to that optimization I'm about to land
> for bug 1123196 but I'm not sure they'll help. I'll fire up some builds and
> have a look.

No change I'm afraid.

Comment 48

[bhavana bajaj [:bajaj]]

:etienne/ :cwiis, I was holding-off the approval here as there seemed to be further comments with follow-up patches. Is this ok to land or are you going to have a revised patch?

Comment 49

[Chris Lord [:cwiiis]]

(In reply to bhavana bajaj [:bajaj] from comment #48)
> :etienne/ :cwiis, I was holding-off the approval here as there seemed to be
> further comments with follow-up patches. Is this ok to land or are you going
> to have a revised patch?

I think we should land it anyway and try to fix the case where it doesn't play and uplift that. I think Etienne should decide though.

Comment 50

[Etienne Segonzac (:etienne)]

(In reply to Chris Lord [:cwiiis] from comment #49)
> (In reply to bhavana bajaj [:bajaj] from comment #48)
> > :etienne/ :cwiis, I was holding-off the approval here as there seemed to be
> > further comments with follow-up patches. Is this ok to land or are you going
> > to have a revised patch?
> 
> I think we should land it anyway and try to fix the case where it doesn't
> play and uplift that. I think Etienne should decide though.

Yes I think we can move forward with this patch.

Apparently we might have other low hanging fruits on the orientation change performance front (see bug 1117314) which aren't directly related to this patch. So I don't foresee a long string of follow up patches for this one :)

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6e565575e031