437957 - Animate zoom

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, enhancement, P2)

Description

[Christian Sejersen]

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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)]

I'll check what we can do with Xomap optimizations to make it faster...

Comment 3

[Oleg Romashin (:romaxa)]

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)]

Attachment: 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)]

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?)]

from the patch it looks like http://hg.mozilla.org/mobile-browser/rev/5076b5dc9f32

Comment 7

[Jaakko Kiviluoto]

Attachment: 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]

Attachment: Updated patch proposal

Patch updated to include missing AnimatedZoom.js

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

brad or mfinkle might be the best person to review?

Comment 10

[Jaakko Kiviluoto]

Attachment: 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)]

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)]

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)]

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?)]

Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

I want Matt to have a look too

Comment 15

[Matt Brubeck (:mbrubeck)]

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)]

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)]

Attachment: 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]

Attachment: 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]

Attachment: 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?)]

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)]

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)]

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]

Attachment: 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]]

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]

(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)]

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]]

(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]

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]

the checkerboard is just the background -- if a tile is missing nothing will be in there showing through.

Comment 30

[Matt Brubeck (:mbrubeck)]

(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?)]

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]

(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]

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]

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]

Attachment: Updated patch with checkerboard, render freezing and fps issues fixed

Comment 36

[Jaakko Kiviluoto]

Attachment: Updated patch with checkerboard, render freezing and fps issues fixed

Fixed garbled patch.. sorry about that.

Comment 37

[Mark Finkle (:mfinkle) (use needinfo?)]

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]

Attachment: 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?)]

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?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/cb09e0392c8a