Closed Bug 617454 Opened 14 years ago Closed 11 years ago

faster zoom with imposter technique (particularly for slower machines)

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: iangilman, Unassigned)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

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).
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.
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).
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.
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
mozRAF looks like what I'm looking for.

I'll start this bug next.
Most excellent. Glad I read the change logs sometimes ;)
Attached patch WIP for UI feedback (obsolete) — Splinter Review
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.
Attachment #497657 - Flags: ui-review?(ian)
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?
Attachment #497657 - Flags: ui-review?(ian) → ui-review+
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.
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #497657 - Attachment is obsolete: true
Attachment #498273 - Flags: feedback?(ian)
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?
Attachment #498273 - Flags: feedback?(ian) → feedback+
Attached patch v2Splinter Review
(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.
Attachment #498273 - Attachment is obsolete: true
Attachment #501483 - Flags: feedback?(ian)
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.
Attachment #501483 - Flags: feedback?(ian) → feedback+
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().
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
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
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...
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
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.
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.
Depends on: 606148
Blocks: 615704
Please do not try using -moz-element at this time.
See bug 624931 for a patch that makes zoom fast with accelerated GL layers.
Summary: Faster zoom for low end machines → Faster zoom for low end machines (using imposter technique)
Summary: Faster zoom for low end machines (using imposter technique) → faster zoom with imposter technique (particularly for slower machines)
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?
bugspam. Moving b10 to b11
Blocks: 627096
bugspam. Removing b10
No longer blocks: 608028
Priority: P2 → P3
Time to punt?
(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.
Sean, if you feel like you'll get to this, go ahead and bring it back.
Blocks: 603789
No longer blocks: 615704, 627096
Target Milestone: --- → Future
bugspam: returning Sean's bugs to the pool
Assignee: seanedunn → nobody
bugspam
Target Milestone: Future → ---
No longer blocks: 603789
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
We're not going to address this with Panorama being slated for removal.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: