518959 - Only repaint dirty parts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Only repaint dirty parts of tiles

When we receive a repaint event, we currently invalidate entire tiles.  This patch invalidates only the portion of the tile that is actually dirty.

Comment 1

[Benjamin Stover (:stechz)]

Are we trying this for b4?

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Increase tile size and better dirtying of rectangle

Fennec becomes much more responsive for panning with a larger tilesize, so I've increased tilesize to 512x512.  Since we only rerender dirty parts, there is little downside.

Comment 3

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

(In reply to comment #2)

> Fennec becomes much more responsive for panning with a larger tilesize, so I've
> increased tilesize to 512x512.  Since we only rerender dirty parts, there is
> little downside.

Much more responsive on n810/n900? or on desktop?

Comment 4

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

Comment on attachment 404689 [details] [diff] [review]
Increase tile size and better dirtying of rectangle

>+    if (tile.isDirty()) {
>+/*      let ctx = tile._canvas.getContext("2d");
>+      ctx.save();
>+      ctx.fillStyle = "rgba(0,255,0,.5)";
>+      ctx.translate(-tile.boundRect.left, -tile.boundRect.top);
>+      ctx.fillRect(tile._dirtyTileCanvasRect.left, tile._dirtyTileCanvasRect.top,
>+        tile._dirtyTileCanvasRect.width, tile._dirtyTileCanvasRect.height);
>+      ctx.restore();
>+      window.setTimeout(function(bv) {
>+        tile.render(bv);
>+      }, 1000, this._browserView); */
>+

I assume we can remove this commented block of code?

>-    } else {  // XXX this case is not very well-tested
>+    } else {
> 
>       if (!this._dirtyTileCanvasRect)
>-        this._dirtyTileCanvasRect = dirtyRect.intersect(this.boundRect);
>+        this._dirtyTileCanvasRect = dirtyRect.intersect(this.boundRect).round();
>       else if (dirtyRect.intersects(this.boundRect))
>-        this._dirtyTileCanvasRect.expandToContain(dirtyRect.intersect(this.boundRect));
>+        this._dirtyTileCanvasRect.expandToContain(dirtyRect.intersect(this.boundRect)).round();
> 
>     }

We can remove the extra whitespace at the top and bottom of this block

> 
>+    ctx.translate(x, y);
>     browserView.browserToViewportCanvasContext(ctx);
> 
>-    ctx.translate(x, y);
> 

And remove the extra blank line that will appear here

>     mouseDown: function mouseDown(cX, cY) {
>+//      this._dispatchMouseEvent("mousedown", cX, cY);
>+//      Util.executeSoon(this._browserView.renderNow);

and remove this

I wish we had tests to support 128, 256 or 512 as better for speed vs memory - but oh well

Comment 5

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

pushed with whitespace tweaks:
https://hg.mozilla.org/mobile-browser/rev/95f148771c77

Comment 6

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

If we feel this is sluggish on device, we can always backout the 512px canvas size.

Comment 7

[Aakash Desai [:aakashd]]

What do you mean by dirty portions of tiles?

Comment 8

[Benjamin Stover (:stechz)]

> What do you mean by dirty portions of tiles?

Before this patch, if even the cursor needed to be repainted, we repainted the entire tile the cursor was located in.  Now, only the cursor is repainted.  If I did it right, there should be no discernible difference after this patch is applied.

> If we feel this is sluggish on device, we can always backout the 512px canvas
size.

At least in my testing (and dougt played some with it too) on the N900 it actually felt faster panning.  Even the actual rendering of tiles is as fast as 128x128 in my testing using Date.now dumps.   This could be explained by the reduced drawWindow calls.

> I assume we can remove this commented block of code?

Yes.  They are useful for figuring out what is being repainted, so I wouldn't mind seeing this as a debug option.