624931 - Panorama zoom should use -moz-transform() for speed

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Patrick Walton (:pcwalton)]

I get a big speed boost on Panorama zoom when using -moz-transform as in this patch.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Patch attached.

See also bug 617454.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 503002 [details] [diff] [review]
Proposed patch.

>   // ----------
>-  // Function: getZoomRect
>-  // Returns a faux rect (just an object with top, left, width, height)
>-  // which represents the maximum bounds of the tab thumbnail in the zoom
>-  // animation. Note that this is not just the rect of the window itself,
>-  // due to scaleCheat.
>-  getZoomRect: function TabItem_getZoomRect(scaleCheat) {
>-    let $tabEl = iQ(this.container);
>-    let orig = $tabEl.bounds();
>-    // The scaleCheat is a clever way to speed up the zoom-in code.
>-    // Because image scaling is slowest on big images, we cheat and stop
>-    // the image at scaled-down size and placed accordingly. Because the
>-    // animation is fast, you can't see the difference but it feels a lot
>-    // zippier. The only trick is choosing the right animation function so
>-    // that you don't see a change in percieved animation speed.
>-    if (!scaleCheat)
>-      scaleCheat = 1.7;
>+  // Function: getZoomTransform
>+  // Returns the transform function which represents the maximum bounds of the
>+  // tab thumbnail in the zoom animation.
>+  getZoomTransform: function TabItem_getZoomTransform() {
>+    let { left, top, width, height } = iQ(this.canvasEl).bounds();
>+    let { innerWidth: windowWidth, innerHeight: windowHeight } = window;
> 
>-    let zoomWidth = orig.width + (window.innerWidth - orig.width) / scaleCheat;
>+    let zoomScaleFactor = windowWidth / width;
>+    let xPreTranslate = windowWidth / 2 - (left + width / 2);
>+    let yPreTranslate = -(top + height / 2);
>+    let yPostTranslate = height / 2;
>+
>     return {
>-      top:    orig.top    * (1 - 1/scaleCheat),
>-      left:   orig.left   * (1 - 1/scaleCheat),
>-      width:  zoomWidth,
>-      height: orig.height * zoomWidth / orig.width
>+        '-moz-transform':
>+            "translate(" + xPreTranslate + "px, " + yPreTranslate + "px) " +
>+            "scale(" + zoomScaleFactor + ", " + zoomScaleFactor + ") " +
>+            "translate(0px, " + yPostTranslate + "px)"

I'm not completely following the translate - scale - translate logic, and that last translate in particular seems to make it jittery at the end... the transition to the tab. Why don't we just use -moz-translation-origin? I'll post my copy of this patch where I modified it to use -moz-translation-origin, with the original positioning calculation. I'm still not liking the jittery-ness at the beginning of the zoom out, though.

Also, I think we want to keep scaleCheat. This helps with the jitteryness of the transition to the actual tab, as we never actually show that last frame of the animation, so there's not as much perceived jumping. Also, it makes it feel faster.

> .front {
>   z-index: 999999 !important;
>-  border-radius: 0 !important;
>-  box-shadow: none !important;
>-  -moz-transform: none !important;
>-  image-rendering: -moz-crisp-edges;
>+  -moz-transform: scale(1.0, 1.0);
> }

What does -moz-crisp-edges do? Do we still want it? I'm pretty sure the -moz-transform: scale(1) can be set on .tab; it doesn't need to be set on .front.

Overall: Somehow the way in which moz-transform scales actually seems more jarring than the pixelating quality of the current zoom animation. There are also some weird artifacts (though maybe this is just my machine, as per bug 624946) on the border of the intermediate frames, where the border itself (interpolated) is visible, but there's some translucent region *within* the border. I'll attach a screenshot. Weird.

Also, in general, why are we applying this -moz-transform to the entire container element, not just the image or just the canvas?

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch, modified to use -moz-transition-origin and to preserve the previous target frame calculation

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Screenshot of weird border artifact, as noted in the feedback

Comment 5

[Patrick Walton (:pcwalton)]

> Overall: Somehow the way in which moz-transform scales actually seems more
> jarring than the pixelating quality of the current zoom animation.
It's not -moz-transform, it's -moz-crisp-edges that creates the pixelation. I believe you can turn on the crisp edges again and still get the acceleration. I thought turning it off looked better, but it's a simple switch to turn it on again.

> There are
> also some weird artifacts (though maybe this is just my machine, as per bug
> 624946) on the border of the intermediate frames, where the border itself
> (interpolated) is visible, but there's some translucent region *within* the
> border. I'll attach a screenshot. Weird.

We could just remove the border.

> 
> Also, in general, why are we applying this -moz-transform to the entire
> container element, not just the image or just the canvas?

Not sure. That's just the way it originally was...

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to comment #2)
> Comment on attachment 503002 [details] [diff] [review]
> Proposed patch.
> 
> >   // ----------
> >-  // Function: getZoomRect
> >-  // Returns a faux rect (just an object with top, left, width, height)
> >-  // which represents the maximum bounds of the tab thumbnail in the zoom
> >-  // animation. Note that this is not just the rect of the window itself,
> >-  // due to scaleCheat.
> >-  getZoomRect: function TabItem_getZoomRect(scaleCheat) {
> >-    let $tabEl = iQ(this.container);
> >-    let orig = $tabEl.bounds();
> >-    // The scaleCheat is a clever way to speed up the zoom-in code.
> >-    // Because image scaling is slowest on big images, we cheat and stop
> >-    // the image at scaled-down size and placed accordingly. Because the
> >-    // animation is fast, you can't see the difference but it feels a lot
> >-    // zippier. The only trick is choosing the right animation function so
> >-    // that you don't see a change in percieved animation speed.
> >-    if (!scaleCheat)
> >-      scaleCheat = 1.7;
> >+  // Function: getZoomTransform
> >+  // Returns the transform function which represents the maximum bounds of the
> >+  // tab thumbnail in the zoom animation.
> >+  getZoomTransform: function TabItem_getZoomTransform() {
> >+    let { left, top, width, height } = iQ(this.canvasEl).bounds();
> >+    let { innerWidth: windowWidth, innerHeight: windowHeight } = window;
> > 
> >-    let zoomWidth = orig.width + (window.innerWidth - orig.width) / scaleCheat;
> >+    let zoomScaleFactor = windowWidth / width;
> >+    let xPreTranslate = windowWidth / 2 - (left + width / 2);
> >+    let yPreTranslate = -(top + height / 2);
> >+    let yPostTranslate = height / 2;
> >+
> >     return {
> >-      top:    orig.top    * (1 - 1/scaleCheat),
> >-      left:   orig.left   * (1 - 1/scaleCheat),
> >-      width:  zoomWidth,
> >-      height: orig.height * zoomWidth / orig.width
> >+        '-moz-transform':
> >+            "translate(" + xPreTranslate + "px, " + yPreTranslate + "px) " +
> >+            "scale(" + zoomScaleFactor + ", " + zoomScaleFactor + ") " +
> >+            "translate(0px, " + yPostTranslate + "px)"
> 
> I'm not completely following the translate - scale - translate logic, and that
> last translate in particular seems to make it jittery at the end... the
> transition to the tab. Why don't we just use -moz-translation-origin? I'll post
> my copy of this patch where I modified it to use -moz-translation-origin, with
> the original positioning calculation. I'm still not liking the jittery-ness at
> the beginning of the zoom out, though.

Ah, thinking about this again, I think I know what you mean: do you mean that the animation shows the tab zooming from the whole window, while the correct thing to do would be to zoom to only the content area of the browser window (i.e. not the toolbars and addon bar)?

Comment 7

[Patrick Walton (:pcwalton)]

(In reply to comment #2)
> where the border itself
> (interpolated) is visible, but there's some translucent region *within* the
> border. I'll attach a screenshot. Weird.

Again, thinking about this some more, this makes sense from a graphics texture perspective: the 1px border hits the texel centers. We could fix this by using a background color or just with -moz-crisp-edges.

Comment 8

[Sean Dunn]

Patrick, I'll take a look at this as a possible replacement for the current patch on bug 617454. If it looks good and is faster, it seems like a clean solution.

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

Here's a new version of your patch. It fixes the border issue by removing the border, switches to crisp edges, and zooms the canvas only.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 503401 [details] [diff] [review]
Proposed patch, version 3.

Sorry, this has bitrotted (already!). We have a lot of patches flying around as of late. :) Could you produce this diff again against a more recent revision? Thanks!

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Patrick, fyi, you may also want to follow developments in bug 604215, though perhaps the task in this bug is completely orthogonal to that.

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 4.

Patch version 4 updates to tip.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 504515 [details] [diff] [review]
Proposed patch, version 4.

First off: wow! I haven't done any real profiling on this, but anecdotally, it feels great!

>diff -r 0d8c14c175f1 browser/base/content/tabview/tabitems.js
>--- a/browser/base/content/tabview/tabitems.js	Sat Jan 15 10:24:41 2011 -0800
>+++ b/browser/base/content/tabview/tabitems.js	Mon Jan 17 10:24:07 2011 -0800

So, this first part was just removing the if statement in setBounds (as it's no longer used during zoom) and lowering the indentation, right? Me like.

TabItem_zoomOut:
>@@ -691,17 +683,18 @@
>     
>     let animateZoom = gPrefBranch.getBoolPref("animate_zoom");
>     if (animateZoom) {
>+      if (!this._zoomPrep)
>+        this.setZoomPrep(true);
>+
>       TabItems.pausePainting();
>-      $tab.animate({
>-        left: box.left,
>-        top: box.top,
>-        width: box.width,
>-        height: box.height
>-      }, {
>+      let $tab = iQ(this.container), $canvas = iQ(this.canvasEl);
>+      $canvas.animate({ "-moz-transform": "scale(1.0)" }, {
>         duration: 300,
>         easing: 'cubic-bezier', // note that this is legal easing, even without parameters
>         complete: function() {
>           TabItems.resumePainting();
>+          $tab.removeClass("front");
>+          $canvas.css('-moz-transform', null);
>           onZoomDone();
>         }
>       });

We can't always assume that the tab that we zoom out of (go back into panorama through) will have been a tab that we zoomed into... therefore, we need to set the -moz-transform-origin on zoomOut as well. Perhaps this could be done as part of setZoomPrep?

>+  // Function: getZoomTransform
>     return {
>-      top:    orig.top    * (1 - 1/scaleCheat),
>-      left:   orig.left   * (1 - 1/scaleCheat),
>-      width:  zoomWidth,
>-      height: orig.height * zoomWidth / orig.width
>+        '-moz-transform-origin': xOrigin + "% " + yOrigin + "%",
>+        '-moz-transform': "scale(" + zoomScaleFactor + ")"
>     };
>   },

Why not call these values just "transform-origin" and "transform"?

feedback+ with that one substantive item (setting -moz-transform-origin on independent zoomOut). Please review? Ian on your next patch! :)

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Patrick, I don't think there should be any issues, but bug 604215 just landed. Please verify that your next patch is built on top of that. :)

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video of zoom only working with some tabs

The first time I tried patch v4 I believe it was working with all tabs, but right now when I tried it again, it's actually only showing the animation for one out of four of my tabs. :(

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #15)
> Created attachment 504607 [details]
> Video of zoom only working with some tabs

Note, new tabs' zooms are working. But reloading one of those "dead zoom" tabs doesn't fix it.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Patrick, word on the street says beta 10 freeze may be Friday. Would you have a chance to look at this in the next couple days? If not, I'd be happy to take over and try to get this prepped. I think it could be a huge win and I'd love to see it land.

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.

Patch version 5 fixes the issue.

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 505293 [details] [diff] [review]
Proposed patch, version 5.

I love it, and I feel it does make a difference for me.

I personally am not sure how we could test this... maybe just a litmus to make sure that we have a zoom in animation, in and out?

Comment 20

[Ian Gilman [:iangilman]]

Comment on attachment 505293 [details] [diff] [review]
Proposed patch, version 5.

Looks promising! I'm on a fast machine, so I'll take others' word that it improves performance. 

Sean has suggested, and I agree, that we probably don't need zoomPrep at all. What if you cleared the transform at the end of a zoom in, and reapplied it just before a zoom out, and otherwise got rid of zoomPrep entirely? This would clean up the code a good deal.

The other big issue, which Sean has also pointed out, is that only our canvases get zoomed; not the cached image stand-ins. I like that we're not zooming the whole tab, just the thumbnail, but we really do need to zoom the cached image stand-in if that's what's showing. 

Thank you for doing this!

Comment 21

[Sean Dunn]

Combining this technique into bug 617454 is looking promising: it's a bit faster than either one on its own. 

From other conversations, it would seem that the canvas is optimized for transformations. Correct? For now, I force an _update() to the canvas right before zooming in, so we only need the canvas, and it will always be correct. There doesn't appear to be a perf hitch associated with that, but I haven't tried this on a really low end machine yet. I think I can copy from the image thumb to the canvas if the perf need arises? That should be quite fast.

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #21)
> For now, I force an _update() to the canvas right
> before zooming in, so we only need the canvas, and it will always be correct.

This seems like it would simplify the code path, too. I.e., the canvas is the *only* thing we ever move in doing the zoom animation. I like that approach rather than special-casing the cached thumbs.

Comment 23

[Ian Gilman [:iangilman]]

(In reply to comment #22)
> (In reply to comment #21)
> > For now, I force an _update() to the canvas right
> > before zooming in, so we only need the canvas, and it will always be correct.
> 
> This seems like it would simplify the code path, too. I.e., the canvas is the
> *only* thing we ever move in doing the zoom animation. I like that approach
> rather than special-casing the cached thumbs.

Agreed; please do that. 

Is any sort of coordination needed between this bug and bug 617454? Should they be combined into a single patch? Who's working on what?

Comment 24

[Sean Dunn]

Yes, I'm combining this into a single patch. Should this patch be marked as a duplicate?

Comment 25

[Patrick Walton (:pcwalton)]

(In reply to comment #24)
> Yes, I'm combining this into a single patch. Should this patch be marked as a
> duplicate?

I'm fine with that. Feel free to ping me on IRC/email if you have any questions. I'd love to get this patch in :)

Comment 26

[Patrick Walton (:pcwalton)]

(In reply to comment #24)
> Yes, I'm combining this into a single patch. Should this patch be marked as a
> duplicate?

Do you have an ETA on this by any chance? If it's going to be a while I can fix up this patch very quickly and get it in.

Comment 27

[Brendan Eich [:brendan]]

It's now 1-25, that's "a while" at this point in the release. Making and landing bigger patches in one bug, instead of landing patches faster in more bugs, is not always good practice -- with mq and at this endgame release stage, I'd say more likely it's bad practice. So why shouldn't this bug's patch land now?

/be

Comment 28

[(no longer active)]

Sean, please address Ian's review comments on this patch here and land this separately.  This doesn't really need to land with bug 617454.

Comment 29

[Sean Dunn]

Ok, land this patch separately from 617454. It's all yours, Patrick.

Comment 30

[Ian Gilman [:iangilman]]

Mitcho says this fixes bug 628764. If for some reason we can't land this, we really should at least fix that bug.

Comment 31

[Michael Yoshitaka Erlewine [:mitcho]]

Patrick, what's the status of this bug? I'd be happy to pitch in if there are some last test failures or review comments to act on. Let's get this landed.

Comment 32

[Patrick Walton (:pcwalton)]

(In reply to comment #31)
> Patrick, what's the status of this bug? I'd be happy to pitch in if there are
> some last test failures or review comments to act on. Let's get this landed.

Rebased and updated per review comments, just need to run the tests.

Comment 33

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 6.

Patch version 6 addresses review comments.

Comment 34

[Ian Gilman [:iangilman]]

Comment on attachment 507983 [details] [diff] [review]
Proposed patch, version 6.

Generally looking good; great to be able to be able to get rid of the zoomPrep stuff. When running it, I did find I could confuse the z-order easily: 

1. Enter Panorama
2. Resize a group with Tabs in it
3. Click on one of those tabs

Note that the tab zooms from behind the group, rather than from in front of it. 

Lots of variations on this are possible with the zooming tab being behind the group or its fellow tabs. 

Additional comments:

>   zoomIn: function TabItem_zoomIn(isNewBlankTab) {
>     // don't allow zoom in if its group is hidden
>     if (this.parent && this.parent.hidden)
>       return;
> 
>     var self = this;
>-    var $tabEl = this.$container;
>+    var $tabEl = this.$container, $canvas = this.$canvas;
>     var childHitResult = { shouldZoom: true };
>     if (this.parent)
>       childHitResult = this.parent.childHit(this);
> 
>+    TabItems._update(this.tab);

This won't necessarily hide the cached image; you need to set this.shouldHideCachedData to true before calling _update. 

>       function onZoomDone() {
>+        $canvas.css({ '-moz-transform': null });

You're resetting the transform here, but not removing the .front class; seems like that needs to happen, too. Perhaps this explains some of the z-order stuff I was noticing? Yup, adding the removeClass call seems to fix the issue.

>   zoomOut: function TabItem_zoomOut(complete) {
>-    var $tab = this.$container;
>+    let $tab = this.$container, $canvas = this.$canvas;
>     var self = this;
>     
>     let onZoomDone = function onZoomDone() {
>-      self.setZoomPrep(false);
>+      $tab.removeClass('front');
>+      $canvas.css('-moz-transform', null);
> 
>       GroupItems.setActiveOrphanTab(null);
> 
>       if (typeof complete == "function")
>         complete();
>     };
>-    
>+
>+    TabItems._update(this.tab);

Same comment re: shouldHideCachedData here. 

>-  // ----------
>-  // Function: getZoomRect
>-  // Returns a faux rect (just an object with top, left, width, height)
>-  // which represents the maximum bounds of the tab thumbnail in the zoom
>-  // animation. Note that this is not just the rect of the window itself,
>-  // due to scaleCheat.
>-  getZoomRect: function TabItem_getZoomRect(scaleCheat) {
>-    let $tabEl = iQ(this.container);
>-    let orig = $tabEl.bounds();
>+  // Function: getZoomTransform

Please don't kill that hyphen line! What has it ever done to you?

>+      // The scaleCheat of 2 here is a clever way to speed up the zoom-out
>+      // code. Because image scaling is slowest on big images, we cheat and
>+      // start the image at half-size and placed accordingly. Because the
>+      // animation is fast, you can't see the difference but it feels a lot
>+      // zippier. The only trick is choosing the right animation function so
>+      // that you don't see a change in percieved animation speed from frame #1
>+      // (the tab) to frame #2 (the half-size image) to frame #3 (the first
>+      // frame of real animation). Choosing an animation that starts fast is
>+      // key.

>     // The scaleCheat is a clever way to speed up the zoom-in code.
>     // Because image scaling is slowest on big images, we cheat and stop
>     // the image at scaled-down size and placed accordingly. Because the
>     // animation is fast, you can't see the difference but it feels a lot
>     // zippier. The only trick is choosing the right animation function so
>     // that you don't see a change in percieved animation speed.

I realize the large amount of overlap between these two comments is not your fault, but perhaps the redundancy could be toned down a bit?

Also, seems like it's worth having a test? I'm actually not sure how you would test this, though... any thoughts?

Comment 35

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 7.

Patch version 7 addresses review comments and adds a simple test.

Comment 36

[Ian Gilman [:iangilman]]

Comment on attachment 508058 [details] [diff] [review]
Proposed patch, version 7.

Looks great!

I like how you dealt with the redundant comments, but please add the last bit of the deleted comment into the non-deleted version:

      // The only trick is choosing the right animation function so
      // that you don't see a change in percieved animation speed from frame #1
      // (the tab) to frame #2 (the half-size image) to frame #3 (the first
      // frame of real animation). Choosing an animation that starts fast is
      // key.

R+ with that addressed. Let's land this thing; I expect it to make a big difference for our zooming animations!

Comment 37

[Ian Gilman [:iangilman]]

Comment on attachment 508058 [details] [diff] [review]
Proposed patch, version 7.

One more thing: we've moved to using the standard public domain license block in our tests. Please update that in your test.

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

Comment 38

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 8.

Patch version 8 addresses review comments. Requesting approval for this big animation perf win.

Comment 39

[Patrick Walton (:pcwalton)]

Risks vs. rewards of this patch:

Risks: This redoes the way we do Panorama zoom animations, so there's a chance they could look strange in some edge cases. This patch isn't really as big as it looks; at least half of it is the test case and adjusting indentation.

Reward: We get fast Panorama zoom, which is one of the biggest UI polish problems in Firefox right now IMO.

Comment 40

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 8, without the indentation changes.

Here's a version of the patch without the indentation changes.

Comment 41

[Patrick Walton (:pcwalton)]

Note that this is showing up in input: http://input.mozilla.com/en-US/beta/search?product=firefox&q=panorama+zoom

Comment 42

[Shawn Wilsher :sdwilsh]

Comment on attachment 508448 [details] [diff] [review]
Proposed patch, version 8.

a=sdwilsh, but any test failures or regressions means this gets pulled and punted to Firefox.next.

Comment 43

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/mozilla-central/rev/d94a56d78123

Comment 44

[Tracy Walker [:tracy]]

verified with nightly build of minefield.