709120 - There is often a perceptible delay when the screen is updated during panning

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Chris Lord [:cwiiis]]

Attachment: Add a delay after Gecko sends a drawing update

Often-times when panning, there are jerks as checker-boarded areas are filled in.

Doing some investigation, these jerks correspond with texture-upload time (where we block), but that texture upload time can vary wildly. I expect this is down to saturating the bus.

Experimenting a bit, I've found that adding a small delay after Gecko draws can really help even out these times and improve perceived performance, especially on single-core devices it seems.

A delay is a bit rough though, I expect that waiting for a draw, and then doing the texture upload while blocking Gecko may have a similar effect.

Experimental patch attached.

Comment 1

[Patrick Walton (:pcwalton)]

How close is snorp's stuff? If it's close then that might just fix the issue.

Comment 2

[Chris Lord [:cwiiis]]

Just to note that snorp's DirectTexture work (bug 670930) completely fixes this issue, but unfortunately isn't available on all devices (notably, any device with an Adreno chipset, it seems).

pcwalton is working on a patch to use horizontal double-buffered texture slices and threaded texture upload using multiple contexts/texture sharing.

I'd like to investigate doing something similar without the threading/double-buffering (which may be a memory concern/concurrency headache) when there's time.

ajuma has pointed out that doing sub-image uploads on some chipsets is much much slower than doing a whole texture upload, to the point where it sometimes isn't worth doing at all - this is something that needs to be considered and investigated. Thankfully, the Adreno is not one of these chipsets.

I have a patch in bug 711959 that would allow the delay patch on this bug to be implemented more elegantly, I think, if this is something worth doing at all.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add tiled buffer support

This patch adds optional support for rendering to a tiled buffer. This could be sped up slightly, I believe, by using Cairo recording surfaces instead of calling draw multiple times.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Add MultiTileLayer class to Java code

This patch adds a new layer type that uses multiple SingleTileLayer's and uploads from a tiled image buffer.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

This uses MultiTileLayer instead of SingleTileLayer, but is really only for testing and very much WIP - the chosen tile size is arbitrary and this breaks image thumbnailing.

Due to how things are structured, this code will not be activated when gralloc is used.

Comment 6

[Chris Lord [:cwiiis]]

This is the start of some work that will hopefully speed things up on devices that:

- Can't use gralloc
- Have possibly slow sub-image upload
- Can't specify the image stride when doing sub-image upload

The three patches I just submitted add support for rendering to a tiled buffer and having a tiled texture layer.

The next step is to make tile updates lazy so that only tiles that are intersecting the screen are uploaded, and other tiles are uploaded in idle time, or as-needed.

Any and all feedback is appreciated!

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add tiled buffer support

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Add MultiTileLayer class to Java code

As well as rebase, fix a few bugs (notably, tile resolution not being correctly set).

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Use tiles to update asynchronously

This patch only synchronously updates tiles that intersect with the screen, other tiles are updated 1-by-1 in subsequent redraws. Also stops drawing tiles that aren't on-screen.

With this patch, scrolling is a lot smoother on my HTC Flyer, but there appear to be some drawing glitches occasionally when scrolling fast that I haven't worked out yet.

Comment 11

[Patrick Walton (:pcwalton)]

Comment on attachment 585476 [details] [diff] [review]
Part 1 - Add tiled buffer support

Review of attachment 585476 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/android/AndroidJavaWrappers.h
@@ +531,5 @@
>          FORCED_RESIZE = 16,
>          ACTIVITY_START = 17,
>          BROADCAST = 19,
>          VIEWPORT = 20,
> +        TILE_SIZE = 22, /* Leaving space for EXPOSE */

I think I'm going to get rid of EXPOSE, so this isn't necessary.

::: widget/src/android/nsWindow.cpp
@@ +1227,5 @@
> +                    break;
> +                } else {
> +                    if (sHasDirectTexture) {
> +                        // XXX: lock only the dirty rect above and pass it in here
> +                        DrawTo(targetSurface);

It would be nice to only redraw the changed tiles when we're using gralloc. Tiling is useful even with gralloc for this reason. (Can be a followup, of course.)

Comment 12

[Patrick Walton (:pcwalton)]

Comment on attachment 585477 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code

Review of attachment 585477 [details] [diff] [review]:
-----------------------------------------------------------------

So when I coded the StripLayer I didn't use a separate SingleTileLayer for each strip; rather, the StripLayer drew each strip manually. Your way may well be cleaner though.

::: mobile/android/base/gfx/MultiTileLayer.java
@@ +43,5 @@
> +import android.graphics.Point;
> +import android.graphics.Rect;
> +import android.util.Log;
> +import java.nio.ByteBuffer;
> +import java.nio.ByteOrder;

ByteOrder is not used and can be removed.

@@ +48,5 @@
> +import java.util.ArrayList;
> +import javax.microedition.khronos.opengles.GL10;
> +
> +/**
> + * Encapsulates the logic needed to draw a single textured tile.

Comment needs to be updated.

@@ +104,5 @@
> +
> +    private void validateTiles() {
> +        IntSize size = getSize();
> +
> +        if (!size.equals(mBufferSize)) {

nit:
if (size.equals(mBufferSize))
    return;

Saves a level of indentation.

@@ +106,5 @@
> +        IntSize size = getSize();
> +
> +        if (!size.equals(mBufferSize)) {
> +            // If the area hasn't change, we can re-use the tiles
> +            if (size.getArea() != mBufferSize.getArea()) {

Might want to factor this if out into a separate function to save some more indentation. Or not, it's up to you.

Comment 13

[Patrick Walton (:pcwalton)]

Comment on attachment 585478 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

Review of attachment 585478 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, as long as the commented-out code gets removed.

::: mobile/android/base/GeckoEvent.java
@@ +78,5 @@
>      public static final int GECKO_EVENT_SYNC = 15;
>      public static final int ACTIVITY_START = 17;
>      public static final int BROADCAST = 19;
>      public static final int VIEWPORT = 20;
> +    public static final int TILE_SIZE = 22; // Leaving space for EXPOSE

See above re. EXPOSE.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add tiled buffer support

Rebased. I'll add the cairo recording surface optimisation in a further patch.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Add MultiTileLayer class to Java code

Rebased, comments addressed.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

A complete version of this patch, rebased. I also un-broke thumbnailing.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Use tiles to update asynchronously

Rebased and fixed up. There was a problem in that sometimes tiles are drawn without being updated, and updating asyncronously means you could expose tiles with incorrect data. I've now taken care to mark which tiles are valid so that invalid tiles are never drawn.

Comment 18

[Chris Lord [:cwiiis]]

Irritatingly, there seems to be some kind of rendering glitch where very occasionally a tile looks like its data was uploaded with an incorrect stride. I've no idea what's causing this, it's most visible on very animated sites (such as animalsbeingdicks.com).

I don't think this should stop this hitting central, but it certainly shouldn't hit aurora unless we find the source of that problem. It seems to only happen with sub-image updates, and could feasibly be a driver bug, but I hope not.

Comment 19

[Chris Lord [:cwiiis]]

So it ends up that recording surfaces are much slower than just issuing multiple draw commands, so these patches stand as is - will do a try run.

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

Noticed that the tiling path is still capping to the maximum texture size, when it doesn't need to - fixed.

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

Whoops, bad patch (uploaded before testing properly, silly mistake, fixed)

Comment 22

[Chris Lord [:cwiiis]]

Attachment: Part X - Use recording surface

I'm adding this to the patch so it doesn't get lost, in case it's useful in the future. This uses cairo recording surface and then paints those to the tiles, instead of invoking multiple draws. This would be faster, but unfortunately our Cairo isn't new enough :(

For reference, we would need development cairo - and apparently none of the development snapshots have what we need yet, so for all intents and purposes, we'd need Cairo 1.12.

Comment 23

[Chris Lord [:cwiiis]]

https://tbpl.mozilla.org/?tree=Try&rev=9de9e6c83d69 - Try run is green, but this is to be expected as none of this code is exercised if there is gralloc support.

This confirms that these patches don't break the gralloc path at least, I'm going to push another try run with a patch to force the non-gralloc path and see how that does.

Comment 24

[Patrick Walton (:pcwalton)]

(In reply to Chris Lord [:cwiiis] from comment #18)
> Irritatingly, there seems to be some kind of rendering glitch where very
> occasionally a tile looks like its data was uploaded with an incorrect
> stride. I've no idea what's causing this, it's most visible on very animated
> sites (such as animalsbeingdicks.com).
> 
> I don't think this should stop this hitting central, but it certainly
> shouldn't hit aurora unless we find the source of that problem. It seems to
> only happen with sub-image updates, and could feasibly be a driver bug, but
> I hope not.

If we're tiling, can we just not use glTexSubImage2D()? Some drivers have reaalllly insanely stupidly slow versions of glTexSubImage2D(), and if we tile we can just reupload a tile at a time and it won't cost us much.

Comment 25

[Patrick Walton (:pcwalton)]

Comment on attachment 586021 [details] [diff] [review]
Part 1 - Add tiled buffer support

Review of attachment 586021 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 26

[Patrick Walton (:pcwalton)]

Comment on attachment 586022 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code

Review of attachment 586022 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to move to async texture upload at some point on a separate thread, but this can be a followup.

r=me with nit.

::: mobile/android/base/gfx/Layer.java
@@ +162,5 @@
>  
>      /**
>       * Subclasses may override this method to perform custom layer updates. This will be called
>       * with the transaction lock held. Subclass implementations of this method must call the
>       * superclass implementation.

nit: Describe what the return value does here.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +172,5 @@
> +            updated &= mShadowLayer.update(gl, pageContext);
> +            updated &= mCheckerboardLayer.update(gl, screenContext);
> +            updated &= mFrameRateLayer.update(gl, screenContext);
> +            updated &= mVertScrollLayer.update(gl, pageContext);
> +            updated &= mHorizScrollLayer.update(gl, pageContext);

Bitwise-and works on booleans? Crazy.

Comment 27

[Patrick Walton (:pcwalton)]

Comment on attachment 586024 [details] [diff] [review]
Part 4 - Use tiles to update asynchronously

Review of attachment 586024 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it's the same as part 2 above...

Comment 28

[Patrick Walton (:pcwalton)]

Comment on attachment 586022 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code

This is identical to 4; I think this is the wrong patch.

Comment 29

[Patrick Walton (:pcwalton)]

Comment on attachment 586106 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

Review of attachment 586106 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +248,5 @@
>                  return null;
>              try {
>                  Bitmap b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
>                                                 CairoUtils.cairoFormatTobitmapConfig(mFormat));
> +                if (mTileLayer instanceof MultiTileLayer) {

nit: Could you factor this out into a separate function? The nesting is getting awfully deep here.

@@ +270,5 @@
> +                            tile.recycle();
> +
> +                            // Progress the buffer to the next tile
> +                            tileBuffer.position(tileSize.getArea() * bpp);
> +                            tileBuffer = tileBuffer.slice();

Should the order of these two statements be reversed? You don't want to update the position of the previous tile buffer, you want to make a new view on the current data and update its position, right?

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Add MultiTileLayer class to Java code

Upload the right patch, sorry about that.

Comment 31

[Patrick Walton (:pcwalton)]

Comment on attachment 586183 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code

Review of attachment 586183 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: mobile/android/base/gfx/MultiTileLayer.java
@@ +213,5 @@
> +    }
> +
> +    @Override
> +    public void draw(RenderContext context) {
> +        for (SingleTileLayer layer : mTiles) {

nit: no {}

Comment 32

[Patrick Walton (:pcwalton)]

Could we, in the near future, prioritize tiles in regions where no tile at all is being displayed (in other words, where there's a checkerboard) before we rerender dirty tiles when panning? This could eliminate lots of perceived checkerboarding during panning on sites that are slow to render, as the renderer would start to fill in the areas that aren't rendered yet first.

Comment 33

[Chris Lord [:cwiiis]]

Try run with forced tiling path is green: https://tbpl.mozilla.org/?tree=Try&rev=252f5e353d1e

I'm going to assume that you meant to r+ part 3 too if that's not too presumptious (going on the prior comments here and your twitter) - just so I can get this on inbound before the weekend.

Comment 34

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #29)
> Comment on attachment 586106 [details] [diff] [review]
> Part 3 - Use MultiTileLayer instead of SingleTileLayer in
> GeckoSoftwareLayerClient
> 
> Review of attachment 586106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> @@ +248,5 @@
> >                  return null;
> >              try {
> >                  Bitmap b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
> >                                                 CairoUtils.cairoFormatTobitmapConfig(mFormat));
> > +                if (mTileLayer instanceof MultiTileLayer) {
> 
> nit: Could you factor this out into a separate function? The nesting is
> getting awfully deep here.

Sure, sounds reasonable.

> @@ +270,5 @@
> > +                            tile.recycle();
> > +
> > +                            // Progress the buffer to the next tile
> > +                            tileBuffer.position(tileSize.getArea() * bpp);
> > +                            tileBuffer = tileBuffer.slice();
> 
> Should the order of these two statements be reversed? You don't want to
> update the position of the previous tile buffer, you want to make a new view
> on the current data and update its position, right?

No, if you slice a positioned buffer, you end up with a buffer whose position zero is the position that was previously set. This is handy so that code that gets this buffer can call position itself without having to worry about the current position of the buffer. Really, I should set the limit of the buffer too (and in fact, I'll do that before pushing this).

Comment 35

[Chris Lord [:cwiiis]]

Whoops, that limit comment was actually meant for the code in MultiTileLayer that implements CairoBitmap - the rest still applies though.

Comment 36

[Chris Lord [:cwiiis]]

Pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a664e3507340
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a363485a656
http://hg.mozilla.org/integration/mozilla-inbound/rev/32d494dce342
http://hg.mozilla.org/integration/mozilla-inbound/rev/a1f1e4d685c0

Comment 37

[Chris Lord [:cwiiis]]

Comment on attachment 580443 [details] [diff] [review]
Add a delay after Gecko sends a drawing update

Removing feedback request for this - while it seems to help, tiling/gralloc pretty much solve the problem this bug is titled after. Further optimisation can be dealt with in new bugs.

Comment 38

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #32)
> Could we, in the near future, prioritize tiles in regions where no tile at
> all is being displayed (in other words, where there's a checkerboard) before
> we rerender dirty tiles when panning? This could eliminate lots of perceived
> checkerboarding during panning on sites that are slow to render, as the
> renderer would start to fill in the areas that aren't rendered yet first.

Yes, I was thinking of doing something along these lines. What we could now do reasonably easily, is alter refreshTileMetrics so that it just marks tiles as needing refreshing instead of immediately doing it, and store invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid state - they're in the correct place, but their image data may be out of date. Most of the time, this will look correct.

Then, we continue to prioritise dirty tiles that are intersecting with the viewport, but instead of moving the viewport bit-by-bit when scrolling, we just move it an entire viewport-height/width at a time, in anticipation of where it will be in the future. The newly-exposed tiles can replace the tiles that are off-screen, and the semi-valid tiles that intersect with the viewport can remain there. This way, we have a virtual buffer size that can be twice the real buffer size (between consecutive updates).

The down-side is that when scrolling, animated parts of the page may appear to pause, and possibly only part of a page will pause (so you may see half a video pause, but the bottom half continue, for example). We could either just allow this (I don't think it's a big deal really), or perhaps set some flags when scrolling so that animations are temporarily paused (if this is feasible).

The other down-side is that I don't think this would work with gralloc (it requires the texture and buffer to not share memory), so I'm eager to explore other avenues first.

Another alternative is to do this and have a layer size that's greater than the buffer size - this makes the timing/viewport manipulation a bit less tricky (but again, doesn't necessarily help much for the gralloc case, at least how we currently use it).

Comment 39

[Patrick Walton (:pcwalton)]

(In reply to Chris Lord [:cwiiis] from comment #38)
> Yes, I was thinking of doing something along these lines. What we could now
> do reasonably easily, is alter refreshTileMetrics so that it just marks
> tiles as needing refreshing instead of immediately doing it, and store
> invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> state - they're in the correct place, but their image data may be out of
> date. Most of the time, this will look correct.

What about using per-tile viewport metrics? (At least, as far as position is concerned -- I don't mind if we throw away all the tiles during zooming.) Then we know the origin of each tile in page coordinates, and we can keep around the old tiles during redraw after panning. We prioritize the new tiles that are appearing in the checkerboarded regions before redrawing the tiles with invalid content.

> The other down-side is that I don't think this would work with gralloc (it
> requires the texture and buffer to not share memory), so I'm eager to
> explore other avenues first.

I'd like to see whether we can move gralloc to tile as well. Is there any reason why the buffers for each tile must be contiguous, if we're painting a tile at a time anyhow?

Comment 40

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #39)
> (In reply to Chris Lord [:cwiiis] from comment #38)
> > Yes, I was thinking of doing something along these lines. What we could now
> > do reasonably easily, is alter refreshTileMetrics so that it just marks
> > tiles as needing refreshing instead of immediately doing it, and store
> > invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> > state - they're in the correct place, but their image data may be out of
> > date. Most of the time, this will look correct.
> 
> What about using per-tile viewport metrics? (At least, as far as position is
> concerned -- I don't mind if we throw away all the tiles during zooming.)
> Then we know the origin of each tile in page coordinates, and we can keep
> around the old tiles during redraw after panning. We prioritize the new
> tiles that are appearing in the checkerboarded regions before redrawing the
> tiles with invalid content.

Yes, this is kind of what I mean - the tile's origin and resolution act as its metrics and we can derive their position from those. I think we should still draw synchronously any changed tiles within the viewport (unless we have devices that struggle even with that?) That said, pre-empting the viewport position as I suggest would have this effect anyway.

> > The other down-side is that I don't think this would work with gralloc (it
> > requires the texture and buffer to not share memory), so I'm eager to
> > explore other avenues first.
> 
> I'd like to see whether we can move gralloc to tile as well. Is there any
> reason why the buffers for each tile must be contiguous, if we're painting a
> tile at a time anyhow?

No reason at all, I was talking to snorp about this earlier - we could pass in a tile map instead of a pointer to make this work with gralloc. If we start with optimisations like this, we definitely want to make this work with gralloc too.

Comment 41

[Patrick Walton (:pcwalton)]

(In reply to Chris Lord [:cwiiis] from comment #40)
> (In reply to Patrick Walton (:pcwalton) from comment #39)
> > (In reply to Chris Lord [:cwiiis] from comment #38)
> > > Yes, I was thinking of doing something along these lines. What we could now
> > > do reasonably easily, is alter refreshTileMetrics so that it just marks
> > > tiles as needing refreshing instead of immediately doing it, and store
> > > invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> > > state - they're in the correct place, but their image data may be out of
> > > date. Most of the time, this will look correct.
> > 
> > What about using per-tile viewport metrics? (At least, as far as position is
> > concerned -- I don't mind if we throw away all the tiles during zooming.)
> > Then we know the origin of each tile in page coordinates, and we can keep
> > around the old tiles during redraw after panning. We prioritize the new
> > tiles that are appearing in the checkerboarded regions before redrawing the
> > tiles with invalid content.
> 
> Yes, this is kind of what I mean - the tile's origin and resolution act as
> its metrics and we can derive their position from those. I think we should
> still draw synchronously any changed tiles within the viewport (unless we
> have devices that struggle even with that?)

As long as we don't know of any pages where that leads to bad performance, sure, I agree that's desirable. A good test case here is http://taskjs.org/, which has a gigantic, screen-sized, Cairo- (and Skia)-killing gradient. Our paint times (as well as those of the stock browser) are horribly slow there and we may have to resort to drawing a tile at a time, so that the user can see something instead of staring at a checkerboard...

Comment 42

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #41)
> (In reply to Chris Lord [:cwiiis] from comment #40)
> > (In reply to Patrick Walton (:pcwalton) from comment #39)
> > > (In reply to Chris Lord [:cwiiis] from comment #38)
> > > > Yes, I was thinking of doing something along these lines. What we could now
> > > > do reasonably easily, is alter refreshTileMetrics so that it just marks
> > > > tiles as needing refreshing instead of immediately doing it, and store
> > > > invalidations in the MultiTileLayer. Then, these tiles are in a semi-valid
> > > > state - they're in the correct place, but their image data may be out of
> > > > date. Most of the time, this will look correct.
> > > 
> > > What about using per-tile viewport metrics? (At least, as far as position is
> > > concerned -- I don't mind if we throw away all the tiles during zooming.)
> > > Then we know the origin of each tile in page coordinates, and we can keep
> > > around the old tiles during redraw after panning. We prioritize the new
> > > tiles that are appearing in the checkerboarded regions before redrawing the
> > > tiles with invalid content.
> > 
> > Yes, this is kind of what I mean - the tile's origin and resolution act as
> > its metrics and we can derive their position from those. I think we should
> > still draw synchronously any changed tiles within the viewport (unless we
> > have devices that struggle even with that?)
> 
> As long as we don't know of any pages where that leads to bad performance,
> sure, I agree that's desirable. A good test case here is http://taskjs.org/,
> which has a gigantic, screen-sized, Cairo- (and Skia)-killing gradient. Our
> paint times (as well as those of the stock browser) are horribly slow there
> and we may have to resort to drawing a tile at a time, so that the user can
> see something instead of staring at a checkerboard...

Oh I see, you're suggesting making the Gecko painting async per-tile too? I guess we could do that - perhaps start a timer when beginDrawing has, and after a certain time (like 50ms, say) just paint whatever tiles have finished and the rest as they come in. I like the idea, I'll have a look at doing this next week.

Comment 43

[Chris Lord [:cwiiis]]

We could easily make this interruptable too, even better!

Comment 44

[Patrick Walton (:pcwalton)]

(In reply to Chris Lord [:cwiiis] from comment #42)
> Oh I see, you're suggesting making the Gecko painting async per-tile too? I
> guess we could do that - perhaps start a timer when beginDrawing has, and
> after a certain time (like 50ms, say) just paint whatever tiles have
> finished and the rest as they come in. I like the idea, I'll have a look at
> doing this next week.

Yeah, something like that. Coupled with painting the checkerboarded tiles first this could have a huge positive impact on our checkerboarding.

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a664e3507340
https://hg.mozilla.org/mozilla-central/rev/4a363485a656
https://hg.mozilla.org/mozilla-central/rev/32d494dce342
https://hg.mozilla.org/mozilla-central/rev/a1f1e4d685c0

Comment 46

[Brad Lassey [:blassey] (use needinfo?)]

Chris, please request approval-aurora on these patches

Comment 47

[Patrick Walton (:pcwalton)]

Comment on attachment 586106 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

Review of attachment 586106 [details] [diff] [review]:
-----------------------------------------------------------------

r+ to get this out of my queue.

Comment 48

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

(In reply to Brad Lassey [:blassey] from comment #46)
> Chris, please request approval-aurora on these patches

And talk about the risks associated with the patches.

Comment 49

[Patrick Walton (:pcwalton)]

(In reply to Mark Finkle (:mfinkle) from comment #48)
> (In reply to Brad Lassey [:blassey] from comment #46)
> > Chris, please request approval-aurora on these patches
> 
> And talk about the risks associated with the patches.

Just weighing in: This is less risky than gralloc because it uses no private APIs. It adds some risk, mostly in the possibility of transient incorrect display, because of the complexity of managing multiple tiles over a single tile as we do today.

Comment 50

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #49)
> (In reply to Mark Finkle (:mfinkle) from comment #48)
> > (In reply to Brad Lassey [:blassey] from comment #46)
> > > Chris, please request approval-aurora on these patches
> > 
> > And talk about the risks associated with the patches.
> 
> Just weighing in: This is less risky than gralloc because it uses no private
> APIs. It adds some risk, mostly in the possibility of transient incorrect
> display, because of the complexity of managing multiple tiles over a single
> tile as we do today.

This is correct, and would be my concern. It would take some refactoring for this to land without snorp's gralloc work, but I see that's re-landing now, so I'll request this as well.

Comment 51

[Chris Lord [:cwiiis]]

Comment on attachment 586021 [details] [diff] [review]
Part 1 - Add tiled buffer support

[Approval Request Comment]
User impact if declined: Unable to apply further tiling-related patches
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): None

Comment 52

[Chris Lord [:cwiiis]]

Comment on attachment 586183 [details] [diff] [review]
Part 2 - Add MultiTileLayer class to Java code

[Approval Request Comment]
User impact if declined: Unable to apply further tiling-related patches
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): None

Comment 53

[Chris Lord [:cwiiis]]

Comment on attachment 586106 [details] [diff] [review]
Part 3 - Use MultiTileLayer instead of SingleTileLayer in GeckoSoftwareLayerClient

[Approval Request Comment]
User impact if declined: Longer pauses during panning on certain devices
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): Small risk of rectangular areas of the screen looking incorrect for a short while on certain devices

Comment 54

[Chris Lord [:cwiiis]]

Comment on attachment 586024 [details] [diff] [review]
Part 4 - Use tiles to update asynchronously

[Approval Request Comment]
User impact if declined: Pauses during panning on certain devices
Testing completed (on m-c, etc.): Baked on m-c for a while now
Risk to taking this patch (and alternatives if risky): Small risk of rectangular areas of the page looking incorrect for a short while on certain devices

Comment 55

[Alex Keybl [:akeybl]]

Comment on attachment 586021 [details] [diff] [review]
Part 1 - Add tiled buffer support

[Triage Comment]
Mobile only - approved for Aurora.

Comment 56

[Chris Lord [:cwiiis]]

Marking as dependent on bug 670930 - these patches are based on top of those and require significant rebasing to work without. I understand that that may land, but default to being switched off (which is fine for this).

Comment 57

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9c4467a28d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a9ddc2ce235
https://hg.mozilla.org/releases/mozilla-aurora/rev/a8a7f67fac95
https://hg.mozilla.org/releases/mozilla-aurora/rev/19a5f584abb0