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

VERIFIED FIXED

Status

defect
P2
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

({perf})

Dependency tree / graph

Firefox Tracking Flags

(status2.0 ?)

Details

(Whiteboard: [frame-rate-monitor])

Attachments

(3 attachments, 8 obsolete attachments)

436.83 KB, video/ogg
Details
22.56 KB, patch
sdwilsh
: approval2.0+
Details | Diff | Splinter Review
17.67 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
I get a big speed boost on Panorama zoom when using -moz-transform as in this patch.
(Assignee)

Comment 1

8 years ago
Posted patch Proposed patch. (obsolete) — Splinter Review
Patch attached.

See also bug 617454.
Assignee: nobody → pwalton
Attachment #503002 - Flags: feedback?(mitcho)
Version: unspecified → Trunk
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?
Attachment #503002 - Flags: feedback?(mitcho) → feedback-
(Assignee)

Comment 5

8 years ago
> 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...
(Assignee)

Comment 6

8 years ago
(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)?
(Assignee)

Comment 7

8 years ago
(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

8 years ago
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.
(Assignee)

Comment 9

8 years ago
Posted patch Proposed patch, version 3. (obsolete) — Splinter Review
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.
Attachment #503002 - Attachment is obsolete: true
Attachment #503401 - Flags: feedback?(mitcho)
status2.0: --- → ?
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!
Attachment #503401 - Flags: feedback?(mitcho) → feedback-
Patrick, fyi, you may also want to follow developments in bug 604215, though perhaps the task in this bug is completely orthogonal to that.
(Assignee)

Comment 12

8 years ago
Posted patch Proposed patch, version 4. (obsolete) — Splinter Review
Patch version 4 updates to tip.
Attachment #503401 - Attachment is obsolete: true
Attachment #504515 - Flags: feedback?(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! :)
Attachment #504515 - Flags: feedback?(mitcho) → feedback+
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. :)
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. :(
Attachment #503039 - Attachment is obsolete: true
Attachment #503040 - Attachment is obsolete: true
(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.
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.
(Assignee)

Comment 18

8 years ago
Posted patch Proposed patch, version 5. (obsolete) — Splinter Review
Patch version 5 fixes the issue.
Attachment #504515 - Attachment is obsolete: true
Attachment #505293 - Flags: feedback?(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?
Attachment #505293 - Flags: review?(ian)
Attachment #505293 - Flags: feedback?(mitcho)
Attachment #505293 - Flags: feedback+
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!
Attachment #505293 - Flags: review?(ian) → review-

Comment 21

8 years ago
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.
(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.
(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

8 years ago
Yes, I'm combining this into a single patch. Should this patch be marked as a duplicate?
(Assignee)

Comment 25

8 years ago
(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 :)
Blocks: 627096
Priority: -- → P2
(Assignee)

Updated

8 years ago
Whiteboard: [frame-rate-monitor]
(Assignee)

Comment 26

8 years ago
(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.
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

8 years ago
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

8 years ago
Ok, land this patch separately from 617454. It's all yours, Patrick.
Mitcho says this fixes bug 628764. If for some reason we can't land this, we really should at least fix that bug.
Blocks: 626940
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.
(Assignee)

Comment 32

8 years ago
(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.
(Assignee)

Comment 33

8 years ago
Posted patch Proposed patch, version 6. (obsolete) — Splinter Review
Patch version 6 addresses review comments.
Attachment #505293 - Attachment is obsolete: true
Attachment #507983 - Flags: review?(ian)
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?
Attachment #507983 - Flags: review?(ian) → review-
(Assignee)

Comment 35

8 years ago
Posted patch Proposed patch, version 7. (obsolete) — Splinter Review
Patch version 7 addresses review comments and adds a simple test.
Attachment #507983 - Attachment is obsolete: true
Attachment #508058 - Flags: review?(ian)
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!
Attachment #508058 - Flags: review?(ian) → review+
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/ */
(Assignee)

Comment 38

8 years ago
Patch version 8 addresses review comments. Requesting approval for this big animation perf win.
Attachment #508058 - Attachment is obsolete: true
Attachment #508448 - Flags: approval2.0?
(Assignee)

Comment 39

8 years ago
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.
(Assignee)

Comment 40

8 years ago
Here's a version of the patch without the indentation changes.
(Assignee)

Updated

8 years ago
Attachment #508454 - Attachment is patch: true
Attachment #508454 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #508448 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 43

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d94a56d78123
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.