Closed Bug 584865 Opened 11 years ago Closed 11 years ago

Animated zooming shows black boxes

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect)

defect
Not set
normal

Tracking

(fennec2.0a1+)

VERIFIED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: stechz, Assigned: mbrubeck)

References

Details

Attachments

(1 file, 8 obsolete files)

Probably should wait for async canvas render event before we begin zooming in.
Blocks: 584509
tracking-fennec: --- → 2.0a1+
The other option is to grab canvas images already drawn.  This doesn't work as well for zooming out.
I think we should take data from existing tiles, and rest missing data request from remote process.
Actually here are two problems here:
1. We don't wait for canvas update and get black screen in the beginning on animation. Waiting for nsCanvasRenderingContext2D::Swap doesn't give any help because we don't even get it.
2. Canvas contains wrong scaled image and animation looks ugly. But it is update to proper one when animation finished. So, probably, calculations are wrong.
Sounds like scaling info is broken or missing in remote canvas.
 I still think that we should create snapshot from existing tiles
Yeah, will try this way now.
Attachment #464102 - Flags: review?(webapps)
I think this is your problem:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/AnimatedZoom.js#65

For this canvas, you should only use "let ctx = aCanvas.MozGetIPCContext("2d");"  If you do getContext("2d") first, I believe it will be treated as a non-IPC canvas from then on.
OS: Mac OS X → All
Hardware: x86 → All
Just a few quick notes to expedite this process:
* be sure to remove the element from the DOM when the animation actually begins
* we will need to address the flash of the old content when the animation is finished
* this can be done in a follow up bug
Duplicate of this bug: 584509
Comment on attachment 464102 [details] [diff] [review]
getting MozAsyncCanvasRender event

>diff -r ba71fa57f436 chrome/content/AnimatedZoom.js

>+AnimatedZoom.prototype.handleEvent = function(aEvent) {
>+  if (aEvent.type == "MozAsyncCanvasRender") {
>+    dump("BINGO!\n");

I think we need to remove event listener and remove the canvas from the document here, now that the event has been caught.
Attachment #464102 - Flags: review?(webapps) → review-
Attached patch updated patch (obsolete) — Splinter Review
Updated to remove the canvas on render, and use MozGetIPCContext.

Still has a brief flash at the end of the animation; this might not be worth fixing before we get rid of TileManager.
Assignee: nobody → mbrubeck
Attachment #464102 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #464498 - Flags: review?(mark.finkle)
(In reply to comment #11)
> Created attachment 464498 [details] [diff] [review]
> updated patch
> 
> Updated to remove the canvas on render, and use MozGetIPCContext.
> 
> Still has a brief flash at the end of the animation; this might not be worth
> fixing before we get rid of TileManager.

With this patch we still start to update from canvas and not waiting if it is ready or not. Just add listener and remove it.
Using this patch on Nexus One, the animation itself is fixed but there is:
- noticeable delay before the animation starts
- full-screen black flash at the start of the animation
- flash of previously-scaled content at the end of the animation

With the patch there is an especially long delay (2-3 seconds) before the animation starts for a multi-touch pinch zoom gesture.
Attachment #464498 - Flags: review?(mark.finkle)
Attached patch Updated patch (obsolete) — Splinter Review
Tried Matt's patch, feels much better, but still have problem with blinking in the beginning of animation.
This patch starts animation only when canvas is ready and helper saveCanvas function gives proper image. Calling saveCanvas in previous patch gave just black png image.
Attached patch final patch v1 (obsolete) — Splinter Review
This patch fixes black boxes completely, but Browser.setVisibleRect(this.zoomRect); in finish() still gives the image flash at the end of animation. But if it is suitable then solution is fine.
Attachment #464794 - Flags: review?
Attachment #464794 - Flags: review? → review?(mark.finkle)
Attachment #464772 - Attachment is obsolete: true
Comment on attachment 464794 [details] [diff] [review]
final patch v1

This patch looks like it has all the right things happening. We still need to test on a device (N900 or Android) to see what the performance is like.
Attachment #464498 - Attachment is obsolete: true
Comment on attachment 464794 [details] [diff] [review]
final patch v1

This patch fixes the initial black flash from the previous patch, but it still adds about 2 seconds delay before zooming starts on Nexus One.

Some timing data from Nexus One shows around 600ms to 1500ms spent in MozGetIPCContext (the time is different on each trial), and another ~100ms spent waiting for the MozAsyncCanvasRender event.
Duplicate of this bug: 581168
Can we just cache the IPC snapshot canvas?
I'll try and hack together a Singleton solution for caching the AnimatedZoom object.
Depends on: 586353
> Some timing data from Nexus One shows around 600ms to 1500ms spent in
> MozGetIPCContext (the time is different on each trial), and another ~100ms
> spent waiting for the MozAsyncCanvasRender event.

and I guess it depends from the page and how heavy that page... and that was the reason why I wanted to have snapshot painted with existing tiles content
Attached patch WIP: cache canvas element (obsolete) — Splinter Review
This patch caches the canvas element instead of recreating it each time, reducing the delay to less than 200ms.

It still has a ~2sec delay the first time AnimateZoom() is called.  We could do the caching on startup instead if we want to.  This patch is just a proof of concept, not ready for checkin.
Attached patch cache canvas element (obsolete) — Splinter Review
Updated the patch to create the canvas element and get the context at browser startup.  (Actually, I had to wait for the "resize" event so that window.innerWidth/Height are correct.)  Instrumentation is still there but commented out, for convenience if we do any further testing and tweaking.

On Android 2.2 (Nexus One) this patch adds ~2sec to startup, and the delay before zooming starts is 100-200ms.  This is not a real fix, but we can use it as a band-aid if we don't have something better in time for alpha.
Attachment #464965 - Attachment is obsolete: true
Attachment #464987 - Flags: review?(mark.finkle)
Comment on attachment 464987 [details] [diff] [review]
cache canvas element

I'm getting exceptions when doing multitouch zoom with this patch applied, investigating...
Attachment #464987 - Flags: review?(mark.finkle)
Attached patch cache canvas element v2 (obsolete) — Splinter Review
Updated to fix errors in some error-handling functions, so you can see the actual errors.

This fails during multitouch zoom with NS_ERROR_ILLEGAL_VALUE from drawImage in AnimatedZoom.prototype.updateTo, probably because snapshotRect no longer matches the real snapshot canvas dimensions.  I haven't figured out the right way to fix this yet.  I have no air conditioning here and can no longer think straight...
Attachment #464987 - Attachment is obsolete: true
Attached patch cache canvas element v3 (obsolete) — Splinter Review
Multitouch was broken because we were starting the animation timer in startAnimation even if animateTo() was never called.  This is fixed, and ready for checkin (if we can't speed this up for alpha in some better way).

Removed the commented-out timing code. Also fixed some more broken debug statements.

Tried moving the canvas creation to the DOMContentLoaded handler, but it still did not get the right size, so I kept it in the resize handler.
Attachment #465021 - Attachment is obsolete: true
Attachment #465261 - Flags: review?(mark.finkle)
Attachment #465261 - Flags: review?(mark.finkle) → review+
Ugh, the last version of this patch broke zoom on local tabs (e.g. about:home).  Looks like we can't use the MozAsyncCanvasRender for local rendering.  This version adds a second code path that doesn't use the MozAsyncCanvasRender event.

I also discovered that adding the snapshot canvas to the DOM is unnecessary, causes a black flash, and slows things down.  That code has been removed.
Attachment #465261 - Attachment is obsolete: true
Attachment #465378 - Flags: review?(mark.finkle)
Comment on attachment 465378 [details] [diff] [review]
cache canvas element v4

>+      // Preload the zoom snapshot canvas, because it's slow on Android (bug 586353)
>+      AnimatedZoom.createCanvas().MozGetIPCContext("2d")

add ";" semicolon
Attachment #465378 - Flags: review?(mark.finkle) → review+
pushed with semicolon: http://hg.mozilla.org/mobile-browser/rev/35669c5a9843

Still need to fix the underlying performance problems; that work will happen in
bug 586353.  (And most of this code might go away soon anyways.)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b4pre) Gecko/20100812 Namoroka/4.0b4pre Fennec/2.0a1pre

Holding off moving this to verified until bug 587000 is fixed for android builds.
Attachment #464794 - Attachment is obsolete: true
Attachment #464794 - Flags: review?(mark.finkle)
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919
Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.