Last Comment Bug 437957 - Animate zoom
: Animate zoom
Status: RESOLVED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: Panning/Zooming (show other bugs)
: Trunk
: All All
: P2 enhancement (vote)
: fennec1.0b1
Assigned To: Jaakko Kiviluoto
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-09 03:48 PDT by Christian Sejersen
Modified: 2011-06-12 02:15 PDT (History)
16 users (show)
pavlov: wanted‑fennec1.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (7.91 KB, patch)
2008-08-19 18:18 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
New patch, same idea (13.98 KB, patch)
2009-09-15 15:40 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Updated patch proposal (15.10 KB, patch)
2010-04-07 02:05 PDT, Jaakko Kiviluoto
no flags Details | Diff | Review
Updated patch proposal (23.47 KB, patch)
2010-04-07 02:07 PDT, Jaakko Kiviluoto
mbrubeck: feedback-
Details | Diff | Review
Patch to send pinch gesture ending event on Qt port (1.46 KB, patch)
2010-04-12 00:56 PDT, Jaakko Kiviluoto
dougt: review-
Details | Diff | Review
Additional patch: More animated zoom (4.08 KB, patch)
2010-04-16 09:34 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Review
Patch to send pinch gesture ending event on Qt port (1.64 KB, patch)
2010-04-20 03:54 PDT, Jaakko Kiviluoto
dougt: review+
Details | Diff | Review
Updated "animated zooming with pinch" patch (26.57 KB, patch)
2010-04-20 04:00 PDT, Jaakko Kiviluoto
mark.finkle: review-
Details | Diff | Review
Updated "animated zooming with pinch" patch 2 (25.20 KB, patch)
2010-05-14 00:59 PDT, Jaakko Kiviluoto
pavlov: review-
Details | Diff | Review
Updated patch with checkerboard, render freezing and fps issues fixed (34.95 KB, patch)
2010-06-04 04:49 PDT, Jaakko Kiviluoto
no flags Details | Diff | Review
Updated patch with checkerboard, render freezing and fps issues fixed (12.63 KB, patch)
2010-06-04 05:10 PDT, Jaakko Kiviluoto
no flags Details | Diff | Review
Updated patch with checkerboard, render freezing and fps issues fixed (26.30 KB, patch)
2010-06-06 23:35 PDT, Jaakko Kiviluoto
mark.finkle: review+
Details | Diff | Review

Description Christian Sejersen 2008-06-09 03:48:57 PDT
 
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-08-19 18:18:20 PDT
Created attachment 334611 [details] [diff] [review]
WIP

This uses a second canvas and drawImage calls to animate zooming to elements. It works OK on the desktop but it's too slow on my n810.
Comment 2 Oleg Romashin (:romaxa) 2008-09-13 18:07:30 PDT
I'll check what we can do with Xomap optimizations to make it faster...
Comment 3 Oleg Romashin (:romaxa) 2008-09-14 00:16:32 PDT
I have apply this patch to the latest fennec with 450930 bugfix...
But I don't see any animation at all...
Comment 4 Benjamin Stover (:stechz) 2009-09-15 15:40:51 PDT
Created attachment 400889 [details] [diff] [review]
New patch, same idea

Here's a rough patch for animated zooming, using the same trick Gavin used.  There are edge cases that don't work quite right and the performance is rather bad on N810.
Comment 5 Oleg Romashin (:romaxa) 2010-02-24 12:10:59 PST
which fennec sources are you using? or what is the revision number? it is fail to apply for me..
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2010-02-24 12:15:37 PST
from the patch it looks like http://hg.mozilla.org/mobile-browser/rev/5076b5dc9f32
Comment 7 Jaakko Kiviluoto 2010-04-07 02:05:15 PDT
Created attachment 437516 [details] [diff] [review]
Updated patch proposal

This version updates AnimatedZoom class to provide an arbitrary zoom for pinch gesture purposes. It also uses larger screen snapshot area.

Currently the zoom starts with urlbar and sidebars hidden in order to avoid unspecified states after the zoom.
Comment 8 Jaakko Kiviluoto 2010-04-07 02:07:05 PDT
Created attachment 437517 [details] [diff] [review]
 Updated patch proposal

Patch updated to include missing AnimatedZoom.js
Comment 9 Doug Turner (:dougt) 2010-04-07 08:32:00 PDT
Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

brad or mfinkle might be the best person to review?
Comment 10 Jaakko Kiviluoto 2010-04-12 00:56:07 PDT
Created attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

Also, on Qt port we need the "MozGestureMagnify" event to know when pinch gesture ends.
Comment 11 Doug Turner (:dougt) 2010-04-15 15:36:09 PDT
Comment on attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

you probably can set outside of all fo the pinch->state() checks.
Comment 12 Doug Turner (:dougt) 2010-04-15 15:36:56 PDT
Comment on attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

otherwise looks fine. please post another patch
Comment 13 Doug Turner (:dougt) 2010-04-15 15:39:53 PDT
Comment on attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

wrong way.  i want a new patch.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-15 15:45:02 PDT
Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

I want Matt to have a look too
Comment 15 Matt Brubeck (:mbrubeck) 2010-04-15 17:17:51 PDT
Still looking through this, just one note before I head to dinner:

> AnimatedZoom.prototype.browserToClientRect = function(r) {
>   let [l, t] = Browser.browserViewToClient(r.left, r.top);
>   let [r, b] = Browser.browserViewToClient(r.right, r.bottom);
>   return new Rect(l, t, r - l, b - t);
> }

Can you use Browser.browserViewToClientRect instead?
Comment 16 Matt Brubeck (:mbrubeck) 2010-04-16 09:31:05 PDT
Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

>+ * Portions created by the Initial Developer are Copyright (C) 2008

The year should be fixed, along with the list of contributors.

>+      let zoom = new AnimatedZoom(this._browserView);
>+      return zoom.animateTo(zoomRect);

Let's move these lines from Browser.zoomFromPoint and zoomToPoint into a new "Browser.animatedZoomTo" function, so we can easily use animated zoom in even more places (like Browser.zoom and FormHelper.zoom).

The rest of the patch looks good to me.
Comment 17 Matt Brubeck (:mbrubeck) 2010-04-16 09:34:50 PDT
Created attachment 439554 [details] [diff] [review]
Additional patch: More animated zoom

(In reply to comment #16)
> Let's move these lines from Browser.zoomFromPoint and zoomToPoint into a new
> "Browser.animatedZoomTo" function, so we can easily use animated zoom in even
> more places (like Browser.zoom and FormHelper.zoom).

Apply this additional patch on top of attachment 437517 [details] [diff] [review] to animate volume-rocker zoom and FormHelper zoom.
Comment 18 Jaakko Kiviluoto 2010-04-20 03:54:43 PDT
Created attachment 440184 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

Updated gesture finishing event catching. I hope I understood the comments to previous proposal correctly.
Comment 19 Jaakko Kiviluoto 2010-04-20 04:00:27 PDT
Created attachment 440185 [details] [diff] [review]
Updated "animated zooming with pinch" patch

This patch contains all the suggested corrections so far, including Matt's additions. Also corrected magic numbers from pinch calculation.

I hope I got the contributor lists right. Please add any missing contributors if you know.
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2010-04-20 07:53:26 PDT
Comment on attachment 440185 [details] [diff] [review]
Updated "animated zooming with pinch" patch

>diff -r 7a4d3865b316 chrome/content/AnimatedZoom.js

I'm on a mission to use JavaScript Code Modules more and <script src=""/> less. We pay a price for including a lot of JS during startup. This particular code may not be used during startup, so we could end up delay loading it using a lazy property in browser.js and Components.utils.import

I think the only part we need to add to the contructor is aBrowser, so you have access to the "Browser" object in the code module.

NOTE: for now, I think we could convert AnimatedZoom to a JS module in a followup bug.

>+AnimatedZoom.prototype._callback = function() {

>+    }
>+    else // last cycle already rendered final scaled image, now clean up
>+      this.finish();

When one side of an if/else requires "{}" we add them to both sides, whether it needs them or not.

>diff -r 7a4d3865b316 chrome/content/browser.js

>+// Simple gestures support

I wonder if this should be merged into InputHandler. There is some overlap. We are planning a InputHandler refactor for 2.0 and we should consider this code when we start.

>+  init: function GS_init(aAddListener) {
>+    const gestureEvents = ["SwipeGesture",
>+      "MagnifyGestureStart", "MagnifyGestureUpdate", "MagnifyGesture",
>+      "RotateGestureStart", "RotateGestureUpdate", "RotateGesture",
>+      "TapGesture", "PressTapGesture"];

Why do we add listeners for all of these gestures, but we only seem to handle the MozMagnifyXxx events

>+
>+    //Add or remove listener according to given parameter
>+    let addRemove = aAddListener ? window.addEventListener :
>+      window.removeEventListener;

This is too fancy for me. Call int init with a boolean to handle uninit too is a bit confusing. Let's just separate the init and unint calls.

Do we uninit in this patch anyway?

>+  _pinchStart: function GS_pinchStart(aEvent) {

>+        // hide element highlight
>+        for (let elem = aEvent.target; elem; elem = elem.parentNode)
>+          if (elem.customClicker) {
>+            elem.customClicker.panBegin();
>+            break;
>+          }

Since this is for content only, I assume, can we just grab the cutsomClicker directly?

document.getElementById("tile-container").customClicker

I guess customClicker.panBegin works for now, but I think we need a better way to hide the highlight. File a followup bug?

>diff -r 7a4d3865b316 chrome/content/browser.xul

>+    <html:canvas id="zoom-canvas" style="display: none; image-rendering: optimizeSpeed;" top="0" left="0" moz-opaque="true">
>+    </html:canvas>

You are adding this canvas, but somewhere in browser.xul we have a canvas used for the current "fuzzy" zoom (id="view-buffer", iirc). We should remove it now since all zooming is using the new canvas.

In general, this is a very nice patch
Comment 21 Doug Turner (:dougt) 2010-04-20 19:52:06 PDT
Comment on attachment 440184 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

Please don't add spaces on new lines:

+        
+        mLastPinchDistance = mTouchPointDistance;
     }

otherwise great.  I fixed this locally and push this patch to m-c:


http://hg.mozilla.org/mozilla-central/rev/58e9ce25c23f
Comment 22 Matt Brubeck (:mbrubeck) 2010-04-27 12:58:29 PDT
This should also be updated to respect the "user-scalable" viewport metadata.  Bug 561815's patch adds a BrowserView.prototype.allowZoom getter you can use to see if zoom is enabled or disabled.
Comment 23 Jaakko Kiviluoto 2010-05-14 00:59:08 PDT
Created attachment 445306 [details] [diff] [review]
Updated "animated zooming with pinch" patch 2

New version of the patch that catches up with default branch and implements the changes from previous review comments. More specifically:

- GestureHandler converted to GestureModule in InputHandler.js
- simply re-use existing view-buffer as drawing canvas
- respect BrowserView.allowZoom
- animation timer updated to make it a bit smoother
- prevent pinch gesture on top of XUL elements/views
- fixed pinch gesture algorithm to more intuitive behavior

Omitted:
- removing need to be loaded as <script src="...">
Comment 24 Michael Wu [:mwu] 2010-05-21 16:18:57 PDT
The zooming behavior in this patch feels pretty great. However, I'm having issues with the page not updating while zoomed in. (zoomed in area shows and everything outside the area is checkerboard) Have you seen this issue? I'm hitting it in slashdot.org and kotaku.com .
Comment 25 Jaakko Kiviluoto 2010-05-24 03:59:15 PDT
(In reply to comment #24)
> issues with the page not updating while zoomed in. (zoomed in area shows and
> everything outside the area is checkerboard) Have you seen this issue? I'm

Checkerboard becomes visible because we just take a snapshot of the page with 1.5 times the critical rectangle dimensions (look for the use of Rect.inflate), which of course is not enough to cover very big leaps outwards. With bigger values the size of bitmap data would be too much and could unnecessarily OOM resource limited devices.

If there would be a possibility to render smaller resolution snapshots as fast as we currently can say renderToCanvas(), please let me know, so we could always render enough overscan data to cover the whole zoomed-out area.
Comment 26 Matt Brubeck (:mbrubeck) 2010-05-24 08:38:36 PDT
I'm not sure if this is what Michael means, but I'm seeing some cases where the page does not render (checkerboard does not go away) even after zooming stops.
Comment 27 Michael Wu [:mwu] 2010-05-24 11:00:15 PDT
(In reply to comment #26)
> I'm not sure if this is what Michael means, but I'm seeing some cases where the
> page does not render (checkerboard does not go away) even after zooming stops.

Yeah this is what I mean. Persistent checkerboard.
Comment 28 Jaakko Kiviluoto 2010-05-25 01:20:07 PDT
Oh, ok. Are the remaining checkerboard areas possibly about the same size as TileManager's tiles? Because that's what I've sometimes seen, that some tiles are not rendered even after the last call to Browser.setVisibleRect() (which in the end *should* put us to the same state as the non-animated zoom did). But if I pan after such a situation a bit, the missing parts usually appear, so the rendered data probably is there but it just isn't painted correctly after setVisibleRect().
Comment 29 Stuart Parmenter 2010-05-27 08:28:18 PDT
the checkerboard is just the background -- if a tile is missing nothing will be in there showing through.
Comment 30 Matt Brubeck (:mbrubeck) 2010-06-01 12:16:53 PDT
(In reply to comment #28)
> But if I pan after such a situation a bit, the missing parts usually appear,
> so the rendered data probably is there but it just isn't painted correctly
> after setVisibleRect().

When this happens to me, panning does *not* cause the missing tiles to appear.  Zooming again will cause the current screen contents to be drawn, but there will still be missing tiles until a new page is loaded.
Comment 31 Mark Finkle (:mfinkle) (use needinfo?) 2010-06-01 22:30:25 PDT
Comment on attachment 445306 [details] [diff] [review]
Updated "animated zooming with pinch" patch 2

>diff --git a/chrome/content/AnimatedZoom.js b/chrome/content/AnimatedZoom.js

>+function AnimatedZoom(aBrowserView) {
>+  this.bv = aBrowserView;
>+
>+  // Render a snapshot of the viewport contents around the visible rect
>+  let [w, h] = this.bv.getViewportDimensions();
>+  let viewportRect = new Rect(0, 0, w, h);
>+  this.zoomFrom = this.bv.getVisibleRect().translateInside(viewportRect);
>+  // try to cover 1.5 times the size of the current visible rect
>+  this.snapshotRect = this.bv.getVisibleRect().inflate(1.5);
>+  // sanitize the snapshot rectangle to fit inside viewport
>+  this.snapshotRect.translateInside(viewportRect).restrictTo(viewportRect).expandToIntegers();
>+  this.snapshot = this._createCanvas(this.snapshotRect.width, this.snapshotRect.height);
>+  let snapshotCtx = this.snapshot.getContext("2d");
>+  snapshotCtx.clearRect(0, 0, this.snapshotRect.width, this.snapshotRect.height);
>+  this.bv.renderToCanvas(this.snapshot, this.snapshotRect.width, this.snapshotRect.height, this.snapshotRect.clone());

nit: Adding a linebreak before commented lines make them easier to read.


>+AnimatedZoom.prototype._createCanvas = function(width, height) {
>+  let canvas = document.createElementNS(kXHTMLNamespaceURI, "canvas");
>+  canvas.setAttribute("width", String(width));
>+  canvas.setAttribute("height", String(height));

nit: The String() calls shouldn't be needed, right?

>+AnimatedZoom.prototype.finish = function() {
>+  try {
>+    // restore canvas state
>+    let ctx = Elements.viewBuffer.getContext("2d");
>+    ctx.mozImageSmoothingEnabled = this.defaultMozImageSmoothing;
>+    ctx.globalCompositeOperation = this.defaultCompositeOperation;
>+    // resume live rendering
>+    this.bv.resumeRendering();
>+    // if we actually zoomed somewhere, clean up the UI to normal
>+    if (this.zoomRect)
>+      Browser.setVisibleRect(this.zoomRect);

nit: linebreaks for readability

>diff --git a/chrome/jar.mn b/chrome/jar.mn
>   content/prompt/prompt.js             (content/prompt/prompt.js)
>+* content/AnimatedZoom.js              (content/AnimatedZoom.js)

Do we need this file to be preprocessed? Just wondering why you have the "*" at the beginning of the line?

Great patch!

r+, but let me know about the preprocessed flag. Whoever lands this patch, please add some linebreaks to the larger code blocks I identified.
Comment 32 Jaakko Kiviluoto 2010-06-01 23:27:42 PDT
(In reply to comment #31)
> >+* content/AnimatedZoom.js              (content/AnimatedZoom.js)
> 
> Do we need this file to be preprocessed? Just wondering why you have the "*" at
> the beginning of the line?

You are right, preprocessing is not needed. The star can be replaced by space, I just verified it.
Comment 33 Stuart Parmenter 2010-06-02 10:52:35 PDT
Comment on attachment 445306 [details] [diff] [review]
Updated "animated zooming with pinch" patch 2

the patch is still broken, needs to be updated to not break rendering
Comment 34 Jaakko Kiviluoto 2010-06-04 01:25:53 PDT
As a status update, the checkerboard problem is easily fixed by calling bv,resumeRendering(true) instead of just bv,resumeRendering() at AnimatedZoom.finish(). BUT, there is another issue that (at least) after pinch zoom the rendering kind of gets frozen, i.e. animations/flash are not updating. Panning a bit after that restarts rendering again, so I'm now trying to find out what else should we call at finish() to fix that.

Also, in next update I'll set the temporary canvas element's mozOpaque=true and implement clipped checkerboard filling in order to get into pixman fast path and have *much* better fps.
Comment 35 Jaakko Kiviluoto 2010-06-04 04:49:42 PDT
Created attachment 449238 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed
Comment 36 Jaakko Kiviluoto 2010-06-04 05:10:58 PDT
Created attachment 449242 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Fixed garbled patch.. sorry about that.
Comment 37 Mark Finkle (:mfinkle) (use needinfo?) 2010-06-04 06:35:11 PDT
Comment on attachment 449242 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Jaakko, This patch seems to be an interdiff with a previous patch which is nice for checking new changes.

Can you also attach a full patch so I can test it?
Comment 38 Jaakko Kiviluoto 2010-06-06 23:35:32 PDT
Created attachment 449580 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Ah, of course, here are differences against the default branch.
Comment 39 Mark Finkle (:mfinkle) (use needinfo?) 2010-06-25 14:04:49 PDT
Comment on attachment 449580 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Works well for local pages. I see some rendering issues during the animation for remote pages though. Hard to tell where the problem is though.

Rendering remote pages isn't working 100% so it could be something in the system.

I unbitrotted with not much trouble.
Comment 40 Mark Finkle (:mfinkle) (use needinfo?) 2010-06-25 14:19:08 PDT
pushed:
http://hg.mozilla.org/mobile-browser/rev/cb09e0392c8a

Note You need to log in before you can comment on or make changes to this bug.