617454 - faster zoom with imposter technique (particularly for slower machines)

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Ian Gilman [:iangilman]]

If a machine is too slow to give a reasonable full-pixel zoom animation, we should cut over to a simpler animation, such as a plain bounding box. 

There are two aspects to this bug: determining if the machine is too slow, and doing the alternate animation (possibly with design suggestions from Aza).

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Another option, if we can indeed determine that the machine is too slow, would be to simply turn off the animation, with animate_zoom = false.

Comment 2

[Aza Raskin [:aza]]

Sean, would it be useful to create another bug simply for keeping track of the average framerate for a number of the basic actions (zoom in/zoom out/first load/dragging a tab)? We can use those numbers to determine appropriate levels of animation fidelity (bounding box, or none). We could also expose those numbers in about:support so we can ask perf questions to users later (and get the test pilot team to gather those metrics).

Comment 3

[Sean Dunn]

I think one of the keys of making zoom work quickly is to first make an imposter of the whole layout, hide everything, and then paint the zoom over the imposter. This should work very quickly.

I think only at that point will we know whether webgl is going to be a perf gain, or just another layer of complexity on slow machines. Without testing it, I can't say that the software gl fallback is faster than the underlying 2D Cairo rasterization (although.. probably).

As for metrics, YES! I tried briefly to put in a framerate counter a while ago, but FF doesn't appear to have a way via JS to do this directly. There's using setInterval() at some really low timing, but I can't prove that that correlates to the frame rate. I might be able to shove some JS into a zooming CSS value, and see how frequently it gets read during an animation. Any ideas? I'll check on #developers as well.

Comment 4

[Aza Raskin [:aza]]

We should try the layout hiding trick you talk about really soon. Getting performance here will be a major factor of the result user experience, especially on slower machines. But even for fast machines, the feel of speed will be the difference between a sticky feature and a eeeehhhh feature. Implementation of that may be as simple as creative a div which uses -moz-element to get it's background?

Would also like to try the WebGL method sooner rather than later as at least on my machine the demo (http://azarask.in/projects/webgl) is buttery smooth. Better than CSS-based animations ever were.

As for framerate, have you looked at https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame

Comment 5

[Sean Dunn]

mozRAF looks like what I'm looking for.

I'll start this bug next.

Comment 6

[Aza Raskin [:aza]]

Most excellent. Glad I read the change logs sometimes ;)

Comment 7

[Sean Dunn]

Attachment: WIP for UI feedback

This is not for code review: I just organically cobbled together a proof of concept that shows how much faster imposters are. The gist is the following:

zoomIn:
  If imposter has been created
    Do zoom in animation, and when finished:
      Assume tabs hidden
      Show imposter under the zooming tab
      Show all tabs on completion
      Hide imposter

zoomOut:
    Create new imposter screenshot
    Show imposter
    Hide all tabs except zoomer
    Start zoomOut animation

All tabs are hidden because they SEVERELY hamper the rendering, even when the imposter (which is opaque) is at a higher z-Index covering the whole screen. I'd think there would be an optimization in the layout engine to not render anything underneath the imposter, but I guess not.

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 497657 [details] [diff] [review]
WIP for UI feedback

Looks very promising! I can't see any seams in the illusion. Seems like this would be a good solution for all machines, not just low-end, yes?

Comment 9

[Sean Dunn]

Yes, it should work on any system. I'm going to put in some frame counting code and clean it up. I'll have a patch for feedback soon.

Comment 10

[Sean Dunn]

Attachment: v1

Here's a cleaned up version, with protection against pathological memory usage (GC'd every time you enter Panorama). There are some instances where it's apparent that we're pulling tricks if the tabview ever changes while in browser mode. Notably: 

* Reduce size of window in Panorama. Go to browser view. Increase window size. Enter Panorama. Zoom happens smoothly, and then the real tabview snaps in.
* Add/remove tabs or groups while in browser view. Go to Panorama, and everything snaps after zoom.

Also, I currently hide/show all groups and items to reduce the layout processing which was killing zoom before. It occurred to me that maybe we can just change the whole window to one showing only the imposter canvas during zoom, and then switch to browser mode. This may be overkill, but something to think about as a next step if it's too slow.

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 498273 [details] [diff] [review]
v1

> * Reduce size of window in Panorama. Go to browser view. Increase window size.
> Enter Panorama. Zoom happens smoothly, and then the real tabview snaps in.
> * Add/remove tabs or groups while in browser view. Go to Panorama, and
> everything snaps after zoom.

We could avoid these by updating the imposter right before the zoom, right? Would that happen fast enough?

> Also, I currently hide/show all groups and items to reduce the layout
> processing which was killing zoom before. It occurred to me that maybe we can
> just change the whole window to one showing only the imposter canvas during
> zoom, and then switch to browser mode. This may be overkill, but something to
> think about as a next step if it's too slow.

Perhaps we could add another div in tabview.html that everything gets attached to, rather than just attaching things to the body. That way we can just hide that div and everything goes with it, rather than having to go through each item.

Hmm, I guess the problem with that is you'd be hiding even the one tab you want to be shown, and if you reparent it for the animation, you'll have to deal with reparenting issues. On the other hand, maybe you could zoom an imposter as well, so there are no real tabs on the screen. 

>+.uiimposter {

This name seems a little awkward... how about ui-imposter?

>+let Wu = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+    getInterface(Components.interfaces.nsIDOMWindowUtils)      

Seems like we've been sticking stuff like this in tabview.js; maybe this goes there as well?

>+// Class: UIImposter
>+// Takes care of the actual canvas for the tab thumbnail
>+// Does not need to be accessed from outside of tabitems.js

It's accessed from both ui.js and tabitems.js, so this comment doesn't seem accurate.

>+    if (this._showing && this._$imposter) {
>+      iQ(".uiimposter").hide().remove();      

Shouldn't that be this._$imposter rather than iQ(".uiimposter")?

>+  refresh: function UIImposter_refresh() {
>+    Utils.assert(!this._showing, "refresh called when imposter not active");

I'd prefer something like "refresh should only be called when imposter not active" for clarity.

>+    let w = this._$tabviewContent.width();
>+    let h = this._$tabviewContent.height();

Why are you using this width and height? Is it actually different from the window's innerWidth/Height?

>+    if (!w || !h) {
>+      Utils.log('null window size in UIImposter.refresh()');
>+      return;
>+    }

Are these log statements just for debugging, or are they intended to stay? I suppose either is fine, though if it's the latter, perhaps we want to add "TabView Error" at the beginning or something.

>+  release: function UIImposter_release() {
>+    this._$imposter = null;
>+  }

Perhaps this should also be called from UI.uninit, to avoid leaks?

Comment 12

[Sean Dunn]

Attachment: v2

(In reply to comment #11)
> We could avoid these by updating the imposter right before the zoom, right?
> Would that happen fast enough?

I currently update the imposter before zooming into a tab, which happens pretty fast. We can't update the imposter until we can actually see the tabview, so we can't do this right before zooming out.

> Perhaps we could add another div...

Implemented.

> >+    if (this._showing && this._$imposter) {
> >+      iQ(".uiimposter").hide().remove();      
> Shouldn't that be this._$imposter rather than iQ(".uiimposter")?

No, because it is possible for more .ui-imposter objects to be created than are removed, if you hold down ctrl-e. By not relying on _$imposter, we can clean up the extras easily.

> >+  release: function UIImposter_release() {
> >+    this._$imposter = null;
> >+  }
> Perhaps this should also be called from UI.uninit, to avoid leaks?

When the UI object is garbage collected, wouldn't any remaining _$imposter be cleaned up as well? Cleaning it up in uninit seems like a case of sweeping the deck of the Titanic before it sinks?

All other comments were resolved how you wanted them.

I've added some more code to this patch. Sometimes tabitem updates take 30ms, sometimes they only take 2ms--I've added some code to make the heartbeat greedily update as many tabitems as it can within a tunable time limit (currently set to 200ms). It feels much faster now.

As much as I'd like this code to give butter smooth animation in all cases, it simply doesn't when you have lots of real-world loaded pages. This isn't because of the updates, but probably because we're very main-thread limited.

Comment 13

[Ian Gilman [:iangilman]]

Comment on attachment 501483 [details] [diff] [review]
v2

(In reply to comment #12)
> I currently update the imposter before zooming into a tab, which happens pretty
> fast. We can't update the imposter until we can actually see the tabview, so we
> can't do this right before zooming out.

Why can't we? We can draw the contents of other tabs that aren't shown... is there some reason it doesn't work for our frame?

> No, because it is possible for more .ui-imposter objects to be created than are
> removed, if you hold down ctrl-e. By not relying on _$imposter, we can clean up
> the extras easily.

Clever! Might be worth a comment in the code.

> When the UI object is garbage collected, wouldn't any remaining _$imposter be
> cleaned up as well? Cleaning it up in uninit seems like a case of sweeping the
> deck of the Titanic before it sinks?

Good point... I guess I'm just leak paranoid. If _$imposter had a reference back to UI, then I think we'd need to clear it explicitly  (because of the cycle), but since we don't I think it's clean.

> I've added some more code to this patch. Sometimes tabitem updates take 30ms,
> sometimes they only take 2ms--I've added some code to make the heartbeat
> greedily update as many tabitems as it can within a tunable time limit
> (currently set to 200ms). It feels much faster now.

Nice!

>+        // Even though we've paused painting, there can still be a tab 
>+        // update processing, which will cause hitches in our animation. 
>+        // Delay the animation by a small amount.
>+//        setTimeout( function() {
>+          // Replace the active UI with an imposter, which speeds up
>+          // zoom in/out
>+          UI.imposter.refresh();
>+          UI.imposter.enableItem(self);
>+          UI.imposter.show();
>+  
>+          self.$container.animate(self.getZoomRect(), {
>+            duration: 230,
>+            easing: 'fast',
>+            complete: function() {
>+              self.$container.css(orig.css());
>+              onZoomDone();
>+  
>+              // When zooming into tab, hiding can cause stutter. Let it get
>+              // processed some point in the future.
>+              setTimeout( function() {
>+                UI.imposter.hide();
>+                TabItems.resumePainting();
>+              }, 0);            
>+            }
>+          });
>+
>+//          }, 
>+          // Delay by 2 x average time, to evade outliers.
>+//          100);
>+//          2.0 * TabItems._averageUpdateTime);

Please remove the commented code and related comments.

Comment 14

[Patrick Walton (:pcwalton)]

If anyone has a try build handy, I'd love to do some framerate comparisons and profiles. Profiling Panorama zoom, I get a plurality (24.8%) of time spent in nsCanvasRenderingContext2D::DrawPath(), followed by gfxSurfaceDrawable::Draw().

Comment 15

[Patrick Walton (:pcwalton)]

Just tried this patch out. This is much faster for me when blank tabs are open (50+ fps). However, when I open Google and TechCrunch (one of the worst-performing sites on the internet, sadly), it's still very laggy; average 20 fps on my top-of-the-line MacBook Pro, and occasionally under 10.

Top callers:

	10.9%	10.9%	XUL	nsCanvasRenderingContext2D::DrawPath(nsCanvasRenderingContext2D::Style, gfxRect*)
	6.2%	6.2%	XUL	nsAppShell::ProcessNextNativeEvent(int)
	3.6%	3.6%	XUL	PL_DHashTableOperate
	3.4%	3.4%	XUL	mozilla::gl::GLContext::UploadSurfaceToTexture(gfxASurface*, nsIntRect const&, unsigned int&, bool, nsIntPoint const&, bool)
	3.4%	3.4%	XUL	js::Shape::trace(JSTracer*) const
	3.0%	3.0%	XUL	js_TraceObject(JSTracer*, JSObject*)
	2.5%	2.5%	XUL	nsCSSRendering::PaintBackgroundWithSC(nsPresContext*, nsIRenderingContext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleContext*, nsStyleBorder const&, unsigned int, nsRect*)
	2.4%	2.4%	XUL	js_TraceScript(JSTracer*, JSScript*)
	1.9%	1.9%	XUL	ChangeTable

Comment 16

[Boris Zbarsky [:bzbarsky]]

Jordan, was there a particular reason to cc me here?

For the earlier comments about measuring fps, see http://weblogs.mozillazine.org/roc/archives/2010/11/measuring_fps.html

Comment 17

[Boris Zbarsky [:bzbarsky]]

And note that DrawPath has wildly different performance characteristics on Mac, Windows + D2D, everything else (windows w/o D2D and Linux), last I checked.  So if it's showing up high (and it's not clear that 11% is high; that depends on what the percentages in comment 15 mean), profiling only on Mac will only go so far...

Comment 18

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 19

[Kevin Hanes]

bugspam (removing b9)

Comment 20

[The 8472]

I would suggest trying -moz-element (see Bug 597258) again in combination with this patch to see if we can maintain reasonable performance.

The advantage of using -moz-element would be having more details while zooming in since that transition is based on the low-detail preview, i.e. it can look blocky.

Comment 21

[The 8472]

some snippets from IRC:
<pcwalton> I'm pretty sure I know how to fix zoom speed
<pcwalton> just use -moz-transform
<The_8472> i think they're already doing that
<pcwalton> preliminary testing shows huge framerate gains
<pcwalton> nope
<pcwalton> it's animating on { left, right, top, bottom }
<mitcho> iangilman: fyi: in the qa call now
<pcwalton> instead use -moz-transform: scale(x, y)


and

<pcwalton> oh, and you want the fix in bug 624505
<pcwalton> profiling turned up some very inefficient canvas->GPU code which vlad fixed
<The_8472> pcwalton, the other thing is that resizing is getting slower when the there are more previews in the page, which do not get scaled
<The_8472> shouldn't retained layers take care of that?
<pcwalton> that may be bug 624505
<pcwalton> there's a bug in layers that's uploading canvas data to the GPU a gazillion times

this might make the workaround in the current patch redundant, since it does what retained layers should be doing.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please do not try using -moz-element at this time.

Comment 23

[Patrick Walton (:pcwalton)]

See bug 624931 for a patch that makes zoom fast with accelerated GL layers.

Comment 24

[Michael Yoshitaka Erlewine [:mitcho]]

Sorry, late to the game taking a look at this. One thought:

(In reply to comment #12)
> Created attachment 501483 [details] [diff] [review]
> v2
> 
> (In reply to comment #11)
> > We could avoid these by updating the imposter right before the zoom, right?
> > Would that happen fast enough?
> 
> I currently update the imposter before zooming into a tab, which happens pretty
> fast. We can't update the imposter until we can actually see the tabview, so we
> can't do this right before zooming out.

What happens when the window has been resized while not in Panorama? Have you tested this case at all?

Comment 25

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 26

[Kevin Hanes]

bugspam. Removing b10

Comment 27

[Michael Yoshitaka Erlewine [:mitcho]]

Time to punt?

Comment 28

[Kevin Hanes]

(In reply to comment #27)
> Time to punt?

Indeed.

Note that this bug blocked bug 615704, though I am uncertain if either that status is warranted or if that bug is still relevant. 

Either way. Punting.

Comment 29

[Kevin Hanes]

Sean, if you feel like you'll get to this, go ahead and bring it back.

Comment 30

[Ian Gilman [:iangilman]]

bugspam: returning Sean's bugs to the pool

Comment 31

[Kevin Hanes]

bugspam

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

We're not going to address this with Panorama being slated for removal.