717283 - On-demand tile usage would allow screen updates to be made more efficiently

Status: RESOLVED WONTFIX

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

Description

[Chris Lord [:cwiiis]]

Currently, our tiled back-buffer is tied directly to the tiles used when rendering. This means that our back-buffer always has to hold the current screen, which limits the amount of buffering that can be done during panning.

If we used tiles on-demand to represent what's currently on screen, tiles could be distributed and used more intelligently and viewport updates could be done with more apprehensive parameters.

On-demand tile-usage, coupled with patches to take advantage of it, ought to be able to eliminate (or at least, greatly decrease) checkerboarding. For example, we'll be able to decrease the page buffer size (which will decrease render-time), but have a larger tile-buffer to compensate.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Sub-class SingleTileLayer and store metrics instead of calculating them

This patch just stores sub-tile metrics in a sub-class of SingleTileLayer, instead of recalculating them each time we need them. Just a preparatory step for the much larger work of making tiles on-demand.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage (WIP)

This is a WIP patch for on-demand tile-usage. It clamps rendering to a tile-grid (via a callback I'm not entirely happy with...), and MultiTileLayer uses whatever tiles it deems are most suitable for filling in the dirty region.

There are noticeable glitches, but the core is there. Comments and suggestions would be greatly appreciated.

Comment 3

[Chris Lord [:cwiiis]]

Just a quick warning for anyone that tries this patch out - horizontal scrolling is very broken (noticeable if you zoom in). Probably an easy fix, just a warning.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage (WIP)

Fixed the horizontal scrolling problem. Still plenty of problems left, but this is more usable/testable.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage (WIP)

This is a version of the patch that works a lot better - the last one would upload constantly due to me not updating regions correctly, this fixes that and so performance is much better. It also fixes some other issues with incorrect invalidations.

There are still glitches to work out when zooming - I may be mixing up coordinate spaces somewhere, or making incorrect assumptions (after panning about a bit it's fine, but for the short time after a zoom, some tiles can look quite odd).

Pleasingly, you can see this already helping a lot of pages - especially the situation of screen-wide pages, as tiles that aren't used for the sides of the page get used for the vertical area of the page.

Once the issues with this patch are fixed, I'll work on another patch to do more intelligent viewport requests, to take advantage of the tile cache better (and hopefully eliminate checkerboarding in a lot of common cases).

Comment 6

[Patrick Walton (:pcwalton)]

Comment on attachment 587743 [details] [diff] [review]
Part 1 - Sub-class SingleTileLayer and store metrics instead of calculating them

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

Looks good.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage (WIP)

This fixes issues with zooming (wasn't invalidating tiles correctly), and also makes sure to clear the tile pool when switching between tabs or loading new pages.

This works well for me now, the last remaining bug is not drawing parts of the back-buffer that aren't valid (the back-buffer is now slightly over-sized to allow for offsetting to align to the tile grid). This should be quite easy to fix.

Comment 8

[Patrick Walton (:pcwalton)]

So this is taking too much time per frame -- 15/16 ms on my Galaxy Tab. That's right up against the time limit and is resulting in dropped frames. The sorting and/or the regions need to be optimized, I think.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Changes:

- Removed tile ordering, just use LRU and a LinkedList for storage instead
- Reduce amount of Region operations
- Fix rotation glitches/crashes
- Fix drawing invalid buffer data (mostly)

This works quite well for me - you can still occasionally see invalid buffer regions, as I use a Rect to store the valid texture rectangle rather than a Region - I figured a Region (which you'd have to iterate over for draws) would be greater overhead than getting rid of the glitch is worth. Especially as it's only visible when we start nearing buffer/tile limits, which I'd hope that further patches would mitigate.

Unfortunately, LRU isn't as good as distance-from-viewport as a measure of a tile's priority, but the deficiency is only really noticeable after zooming and that takes too much computation to perform synchronously.

Assuming there aren't any big issues left in this patch, I think it's ready for review. The decrease in visible checkerboarding on my device makes it worth it, even without any further patches, and it opens up a route for more effective optimisations.

Comment 10

[Patrick Walton (:pcwalton)]

Unfortunately, I don't think we can ship a Fennec with any rendering glitches -- once we start painting, we must never show content that is out-of-date. To that end I have a couple of issues, one minor-ish, and one major, with this patch.

(1) The relatively minor issue is that if using a region is too slow, maybe we could be more conservative and have a region that has a maximum of 3 rects, for example, and then intersect rects as necessary beyond that. I don't think we can just union rects together.

(2) The major issue is that I'm concerned about is the behavior when stuff outside the buffer gets invalidated. As it stands, that doesn't invalidate the tile cache outside the buffer rect because we never know about it -- Gecko saw that the invalidation was offscreen and never called nsWindow::Invalidate(). In my testing, I immediately saw the header at cnn.com duplicate itself for this reason. I think users will notice this and will grade Fennec more harshly than they would if we simply displayed a checkerboard.

The only solution that I can think of for issue #2 while still maintaining the approach of this patch is to make the XUL window very large, and to never save tiles outside that window. If we are rendering tile-by-tile that shouldn't be a problem. Of course, with gralloc and without tile-by-tile rendering, this will make checkerboarding and memory usage much worse because we'll be rendering a very large buffer. So maybe it's better to get tile-by-tile rendering working first.

Comment 11

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #10)
> Unfortunately, I don't think we can ship a Fennec with any rendering
> glitches -- once we start painting, we must never show content that is
> out-of-date. To that end I have a couple of issues, one minor-ish, and one
> major, with this patch.

Something to consider; checkerboarding is a rendering glitch as far as a user is concerned - if we can't eliminate it, we'll need to pick the least bad option. This isn't to say I disagree with you, mind. Details ahead;

> (1) The relatively minor issue is that if using a region is too slow, maybe
> we could be more conservative and have a region that has a maximum of 3
> rects, for example, and then intersect rects as necessary beyond that. I
> don't think we can just union rects together.

It shouldn't be too slow - my initial patch did way too much work in the update function, and I've removed most of it in this latest revision. You shouldn't see such large numbers as before. This performs well on my HTC Flyer (a pretty modest, single-core device).

Also worth noting that the update function doesn't run for that long. We could even make the async part just happen all at once (the buffered area isn't as large as the screen size, so it ought not to be a problem), to reduce the amount of time spent in performUpdates.

> (2) The major issue is that I'm concerned about is the behavior when stuff
> outside the buffer gets invalidated. As it stands, that doesn't invalidate
> the tile cache outside the buffer rect because we never know about it --
> Gecko saw that the invalidation was offscreen and never called
> nsWindow::Invalidate(). In my testing, I immediately saw the header at
> cnn.com duplicate itself for this reason. I think users will notice this and
> will grade Fennec more harshly than they would if we simply displayed a
> checkerboard.

> The only solution that I can think of for issue #2 while still maintaining
> the approach of this patch is to make the XUL window very large, and to
> never save tiles outside that window. If we are rendering tile-by-tile that
> shouldn't be a problem. Of course, with gralloc and without tile-by-tile
> rendering, this will make checkerboarding and memory usage much worse
> because we'll be rendering a very large buffer. So maybe it's better to get
> tile-by-tile rendering working first.

One option is to immediately invalidate any tiles that are off-screen and not included in an update when we receive/handle an update. This would solve this problem, but still let us do viewport prediction and make better use of tiling. I think this would be acceptable.

I think tile-by-tile rendering will work better on top of this patch, so we can easily apply this and just invalidate all off-buffer tiles during an update - This would maintain past behaviour, but let us take advantage of having a proper tile pool. I think we'll want this to efficiently use gralloc to back tiles.

Another option is to keep the current way of working, but listen for things like reflows happening to invalidate off-buffer tiles. This would let us still buffer large, static pages. I don't like this option however, as we reflow pretty often at the moment. (also, when transforms don't cause reflows, this will just be broken anyway)

Another option, we could display off-buffer, semi-valid tiles (i.e. tiles that were valid, but we can't guarantee their current validity) in some way to signify that, to make it an alternative for checkerboarding. Perhaps lowlighted slightly, or semi-transparent. I think this would be more useful than displaying nothing, especially as in most cases, it's actually the correct information. The most common time it isn't correct is during loading - we can disable it during that time if need be. I think this would be acceptable too.

Lastly, we could just try this and see how it's received - I don't think it's that great on its own, but coupled with predictive viewport and a reduction in buffer size, I think any glitches would be pretty unnoticeable, vs. the massive amount of checkerboarding we have currently. It's worth noting that there are visible glitches in the ICS browser when scrolling fast too (you can see low-res tiles replaced with high-res ones). It may be better to do something like this in the interim between what we have now and perfect rendering.

Comment 12

[Patrick Walton (:pcwalton)]

(In reply to Chris Lord [:cwiiis] from comment #11)
> It shouldn't be too slow - my initial patch did way too much work in the
> update function, and I've removed most of it in this latest revision. You
> shouldn't see such large numbers as before. This performs well on my HTC
> Flyer (a pretty modest, single-core device).

Yeah, I think I was mismeasuring the speed. I profiled and most of our slowness during panning and zooming is actually due to all the logging in the PanZoomController. So I'm pretty sure this can be changed back to use a Region. Sorry for ringing the performance alarm bells too early!

> One option is to immediately invalidate any tiles that are off-screen and
> not included in an update when we receive/handle an update. This would solve
> this problem, but still let us do viewport prediction and make better use of
> tiling. I think this would be acceptable.
> 
> I think tile-by-tile rendering will work better on top of this patch, so we
> can easily apply this and just invalidate all off-buffer tiles during an
> update - This would maintain past behaviour, but let us take advantage of
> having a proper tile pool. I think we'll want this to efficiently use
> gralloc to back tiles.

Yup, sounds good. I'll r+ a more conservative patch that does this. And once we have tile-by-tile rendering, we can start aggressively bumping up the size of the XUL window -- maybe even make it a constant decoupled from the tile size, which might simplify some code in widget and browser.js.

> Another option is to keep the current way of working, but listen for things
> like reflows happening to invalidate off-buffer tiles. This would let us
> still buffer large, static pages. I don't like this option however, as we
> reflow pretty often at the moment. (also, when transforms don't cause
> reflows, this will just be broken anyway)

I think this would work if we listened for retained layer invalidation instead, since what we're really talking about is invaliding our own retained layers. Not sure how feasible this is. It should definitely be a later followup IMHO.

> Another option, we could display off-buffer, semi-valid tiles (i.e. tiles
> that were valid, but we can't guarantee their current validity) in some way
> to signify that, to make it an alternative for checkerboarding. Perhaps
> lowlighted slightly, or semi-transparent. I think this would be more useful
> than displaying nothing, especially as in most cases, it's actually the
> correct information. The most common time it isn't correct is during loading
> - we can disable it during that time if need be. I think this would be
> acceptable too.

That's a possibility as well, but I'd like to land the more conservative patch first.

> Lastly, we could just try this and see how it's received - I don't think
> it's that great on its own, but coupled with predictive viewport and a
> reduction in buffer size, I think any glitches would be pretty unnoticeable,
> vs. the massive amount of checkerboarding we have currently. It's worth
> noting that there are visible glitches in the ICS browser when scrolling
> fast too (you can see low-res tiles replaced with high-res ones). It may be
> better to do something like this in the interim between what we have now and
> perfect rendering.

Again, I'm cautious about this. IMHO we should land the conservative version (your first option), then do tile-by-tile rendering, then bump up the XUL window size, then explore other options as schedule and risk permit. Eventually we will hit the async GL layers singularity and none of this attempting-to-approximate-GL-layers-in-widget stuff will be necessary :) (That's not to disparage the work here though -- it's great stuff!)

Comment 13

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #12)
> (In reply to Chris Lord [:cwiiis] from comment #11)
> > It shouldn't be too slow - my initial patch did way too much work in the
> > update function, and I've removed most of it in this latest revision. You
> > shouldn't see such large numbers as before. This performs well on my HTC
> > Flyer (a pretty modest, single-core device).
> 
> Yeah, I think I was mismeasuring the speed. I profiled and most of our
> slowness during panning and zooming is actually due to all the logging in
> the PanZoomController. So I'm pretty sure this can be changed back to use a
> Region. Sorry for ringing the performance alarm bells too early!
> 
> > One option is to immediately invalidate any tiles that are off-screen and
> > not included in an update when we receive/handle an update. This would solve
> > this problem, but still let us do viewport prediction and make better use of
> > tiling. I think this would be acceptable.
> > 
> > I think tile-by-tile rendering will work better on top of this patch, so we
> > can easily apply this and just invalidate all off-buffer tiles during an
> > update - This would maintain past behaviour, but let us take advantage of
> > having a proper tile pool. I think we'll want this to efficiently use
> > gralloc to back tiles.
> 
> Yup, sounds good. I'll r+ a more conservative patch that does this. And once
> we have tile-by-tile rendering, we can start aggressively bumping up the
> size of the XUL window -- maybe even make it a constant decoupled from the
> tile size, which might simplify some code in widget and browser.js.

Cool, I'll do this on Monday. Along with viewport prediction, I think this will get us where we want to be with this patch anyway - I'll file a follow-up bug for that.

> > Another option is to keep the current way of working, but listen for things
> > like reflows happening to invalidate off-buffer tiles. This would let us
> > still buffer large, static pages. I don't like this option however, as we
> > reflow pretty often at the moment. (also, when transforms don't cause
> > reflows, this will just be broken anyway)
> 
> I think this would work if we listened for retained layer invalidation
> instead, since what we're really talking about is invaliding our own
> retained layers. Not sure how feasible this is. It should definitely be a
> later followup IMHO.

That would be better, though we'd still be caught out by transforms and it'd probably happen too often... Might be worth exploring in another bug.

> > Another option, we could display off-buffer, semi-valid tiles (i.e. tiles
> > that were valid, but we can't guarantee their current validity) in some way
> > to signify that, to make it an alternative for checkerboarding. Perhaps
> > lowlighted slightly, or semi-transparent. I think this would be more useful
> > than displaying nothing, especially as in most cases, it's actually the
> > correct information. The most common time it isn't correct is during loading
> > - we can disable it during that time if need be. I think this would be
> > acceptable too.
> 
> That's a possibility as well, but I'd like to land the more conservative
> patch first.

You're right, I think I'm trying to do too much at once here. I was thinking that showing the semi-valid tiles desaturated and animating in their colour when they update would be a nice way of visualising this though, I'll file another bug if it seems worthwhile.

> > Lastly, we could just try this and see how it's received - I don't think
> > it's that great on its own, but coupled with predictive viewport and a
> > reduction in buffer size, I think any glitches would be pretty unnoticeable,
> > vs. the massive amount of checkerboarding we have currently. It's worth
> > noting that there are visible glitches in the ICS browser when scrolling
> > fast too (you can see low-res tiles replaced with high-res ones). It may be
> > better to do something like this in the interim between what we have now and
> > perfect rendering.
> 
> Again, I'm cautious about this. IMHO we should land the conservative version
> (your first option), then do tile-by-tile rendering, then bump up the XUL
> window size, then explore other options as schedule and risk permit.
> Eventually we will hit the async GL layers singularity and none of this
> attempting-to-approximate-GL-layers-in-widget stuff will be necessary :)
> (That's not to disparage the work here though -- it's great stuff!)

Having thought about it more, I do think that the patch is a bit too aggressive as it is - it should at least always invalidate during loading. I'll take the more conservative route and file follow-up bugs. Thanks for the thorough review!

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

As discussed, this patch invalidates off-screen, off-buffer tiles, to maintain prior behaviour and not show tiles that could possibly be invalid. I've also reinstated pushing invalidated tiles to the head of the tile pool, for more efficient tile re-use.

This still allows for viewport prediction, which I'll file a follow-up bug for.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Realised that the early break for non-screen tiles was breaking async updates, since I changed the way the invalid region is updated.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Sorry for spam, that wasn't completely correct either - this is.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Removed an unused variable, clarified a comment and removed an unnecessary else block.

Comment 18

[Patrick Walton (:pcwalton)]

Comment on attachment 588962 [details] [diff] [review]
Part 2 - On-demand tile usage

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

r+ with nits. This works fine for me.

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +447,5 @@
>          } else if ("Viewport:UpdateLater".equals(event)) {
>              mUpdateViewportOnEndDraw = true;
> +        } else if ("DocumentShown".equals(event) ||
> +                   "Tab:Selected".equals(event)) {
> +            if (mTileLayer instanceof MultiTileLayer)

nit: {} and move condition into the if above

::: mobile/android/base/gfx/Layer.java
@@ +69,4 @@
>       */
>      public final boolean update(GL10 gl, RenderContext context) {
> +        boolean startTransaction = true;
> +        if (mTransactionLock.isHeldByCurrentThread())

nit: {}

@@ +74,5 @@
>  
> +        // If we're not already in a transaction and we can't acquire the lock,
> +        // bail out.
> +        if (startTransaction && !mTransactionLock.tryLock())
> +            return false;

ditto

::: mobile/android/base/gfx/LayerRenderer.java
@@ +105,5 @@
>          CairoImage checkerboardImage = new BufferedCairoImage(controller.getCheckerboardPattern());
>          mCheckerboardLayer = new SingleTileLayer(true, checkerboardImage);
> +        mCheckerboardLayer.beginTransaction(null);
> +        mCheckerboardLayer.invalidate();
> +        mCheckerboardLayer.endTransaction();

I usually put endTransaction() in a finally block just for defensiveness.

@@ +111,5 @@
>          CairoImage shadowImage = new BufferedCairoImage(controller.getShadowPattern());
>          mShadowLayer = new NinePatchTileLayer(shadowImage);
> +        mShadowLayer.beginTransaction(null);
> +        mShadowLayer.invalidate();
> +        mShadowLayer.endTransaction();

Same here.

::: mobile/android/base/gfx/MultiTileLayer.java
@@ +51,2 @@
>  import java.nio.ByteBuffer;
> +import java.util.Collections;

This line is not needed and can be removed.

@@ +51,3 @@
>  import java.nio.ByteBuffer;
> +import java.util.Collections;
> +import java.util.Comparator;

Ditto.

@@ +128,5 @@
> +            // Remove tiles from the beginning of the list, as these are
> +            // least recently used tiles
> +            for (int i = mTiles.size(); i > nTiles; i--) {
> +                SubTile tile = mTiles.get(0);
> +                if (tile.key != null)

nit: style checker checks for {} now, I'm told

@@ +156,5 @@
> +            if (!tile.performUpdates(gl, context))
> +                Log.e(LOGTAG, "Sub-tile failed to update fully");
> +
> +            // (Re)Place in the position hash for quick retrieval.
> +            if (tile.key != null)

nit: {} and above too

@@ +172,5 @@
>  
>          validateTiles();
>  
> +        // Bail out early if we have nothing to do.
> +        if (mDirtyRegion.isEmpty() || mTiles.isEmpty())

nit: {}

@@ +270,5 @@
> +                Rect tilespaceTileRect = new Rect(x - origin.x, y - origin.y,
> +                                                  (x - origin.x) + mTileSize.width,
> +                                                  (y - origin.y) + mTileSize.height);
> +                if (!opRegion.op(tilespaceTileRect, updateRegion, Region.Op.INTERSECT))
> +                    continue;

nit: {}

@@ +363,1 @@
>      class SubTile extends SingleTileLayer {

nit: private static class?

@@ +377,5 @@
> +     * A CairoImage implementation that returns a tile from a parent CairoImage.
> +     * This assumes that the parent image has a size that is a multiple of the
> +     * tile size.
> +     */
> +    private class SubImage extends CairoImage {

private static class?

::: mobile/android/chrome/content/browser.js
@@ +1980,5 @@
>        case "document-shown":
>          // Is it on the top level?
>          let contentDocument = aSubject;
>          if (contentDocument == this.browser.contentDocument) {
> +          sendMessageToJava({ gecko: { type: "DocumentShown" } });

Document:Shown perhaps, to be more consistent with the namespacey-ness of the rest of the messages?

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

After more testing, I found some significant issues with the previous patch - mostly that due to various circumstances, it was still possible to see invalid buffer region. Also, there were issues with zooming and rotation. I've fixed these and tidied the patch up significantly.

One major change is that I've removed the assumption of offsetting in MultiTileLayer and changed it so that it only handles tile-aligned origins - the offsetting is now limited to GeckoSoftwareLayerClient only, which I think makes MultiTileLayer much easier to read/understand (and doesn't really have any significant effect on GeckoSoftwareLayerClient).

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Bonus, here's one that's rebased and doesn't break the gralloc path.

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Let a debugging comment slip through, sorry about that.

Comment 22

[Patrick Walton (:pcwalton)]

Comment on attachment 589916 [details] [diff] [review]
Part 2 - On-demand tile usage

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

r=me

::: mobile/android/base/gfx/MultiTileLayer.java
@@ +242,5 @@
> +        //     Note that this could provide inconsistent views, so we may not
> +        //     want to do this.
> +        Rect tilespaceViewport;
> +        float scaleFactor = getResolution() / context.zoomFactor;
> +        if (FloatUtils.fuzzyEquals(scaleFactor, 1.0f)) {

I guess this "if" is an optimization?

@@ +275,5 @@
> +        // off-screen and off-buffer, as we cannot guarantee their validity.
> +        //
> +        // Note that we also cannot guarantee the validity of on-screen,
> +        // off-buffer tiles, but this is a rare case that we allow for
> +        // optimisation purposes.

Ah, now I see why you invalidate the buffer in Document:Shown and Tab:Selected; you want to delete all the on-screen, off-buffer tiles.

Comment 23

[Chris Lord [:cwiiis]]

(In reply to Patrick Walton (:pcwalton) from comment #22)
> Comment on attachment 589916 [details] [diff] [review]
> Part 2 - On-demand tile usage
> 
> Review of attachment 589916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: mobile/android/base/gfx/MultiTileLayer.java
> @@ +242,5 @@
> > +        //     Note that this could provide inconsistent views, so we may not
> > +        //     want to do this.
> > +        Rect tilespaceViewport;
> > +        float scaleFactor = getResolution() / context.zoomFactor;
> > +        if (FloatUtils.fuzzyEquals(scaleFactor, 1.0f)) {
> 
> I guess this "if" is an optimization?

Right, but removed - there second clause had more steps before, but I simplified it and didn't notice how trivial it had become.

> @@ +275,5 @@
> > +        // off-screen and off-buffer, as we cannot guarantee their validity.
> > +        //
> > +        // Note that we also cannot guarantee the validity of on-screen,
> > +        // off-buffer tiles, but this is a rare case that we allow for
> > +        // optimisation purposes.
> 
> Ah, now I see why you invalidate the buffer in Document:Shown and
> Tab:Selected; you want to delete all the on-screen, off-buffer tiles.

Right.

Pushing to inbound, thanks for the review!

Comment 24

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

Unfortunately, this required a reasonably significant rebase (due to DirectTexture detection changes).

Carrying forward pcwalton's r+ and asking for snorp to review the gralloc-related nsWindow and GeckoSoftwareLayerClient changes.

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 590213 [details] [diff] [review]
Part 2 - On-demand tile usage

Looks good to me. This is probably even better than the original way.

Comment 26

[Matt Brubeck (:mbrubeck)]

Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25002e370287

because all native Android tests were crashing at startup:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=47584f117727

Comment 27

[Chris Lord [:cwiiis]]

Ugh, this was mainly because LinkedList.removeLastOccurrence isn't available in earlier Android and I managed to miss it. Fixed that, and a possible (non-fatal) null pointer exception.

Green try run:
https://tbpl.mozilla.org/?tree=Try&rev=5a50ec4ccc65

Re-push:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d776b291014
http://hg.mozilla.org/integration/mozilla-inbound/rev/91eebc6bdb59

Comment 28

[Ed Morley [:emorley]]

Backed out of inbound for reftest crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=91eebc6bdb59
https://tbpl.mozilla.org/php/getParsedLog.php?id=8732061&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff0e8b46d70

Comment 29

[Ed Morley [:emorley]]

(Turns out reftests were hidden on try but shown on inbound, have set them to now show on try too).

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

The previous rebase caused many, many problems... This is a more thoroughly tested and thought-out version that I think passes try runs (but I'm running another one now to make sure).

Carrying over pcwalton's review, will r? snorp for gralloc initialisation related changes.

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Lord [:cwiiis] from comment #30)
> Created attachment 590769 [details] [diff] [review]
> Part 2 - On-demand tile usage
> 
> The previous rebase caused many, many problems... This is a more thoroughly
> tested and thought-out version that I think passes try runs (but I'm running
> another one now to make sure).
> 
> Carrying over pcwalton's review, will r? snorp for gralloc initialisation
> related changes.

Looks good to me

Comment 32

[Chris Lord [:cwiiis]]

Comment on attachment 590769 [details] [diff] [review]
Part 2 - On-demand tile usage

This version fixes switching between layer types, but also removes the possibility of it happening.

This runs fine on my Flyer (no-gralloc) and my Galaxy Nexus (gralloc), including surface size changes.

I expect a try run to complete green (earlier versions of this patch did, despite having problems I've since fixed), I'll update this bug with the link when it does and I won't be pushing to inbound until then.

Comment 33

[Chris Lord [:cwiiis]]

Attachment: Part 2 - On-demand tile usage

And the final version... The problem causing reftests failures was crashing due to bad viewports - this happened because an exception would be thrown in beginDrawing. Previously, it was thrown in endDrawing, which doesn't have the same consequences.

Really, the root cause should be addressed (this only seems to happen in ref-tests, and it's because of an exception in browser.js), but I already think this patch is doing too much in one go, so I've changed up the ordering of things in beginDrawing slightly and let it discard draws that have bad viewports.

This allows reftests to pass: https://tbpl.mozilla.org/?tree=Try&rev=805041ba16cf (though the error is still output to logcat)

Comment 34

[Chris Lord [:cwiiis]]

Third time lucky...

http://hg.mozilla.org/integration/mozilla-inbound/rev/2a9c391e2a89
http://hg.mozilla.org/integration/mozilla-inbound/rev/d8fc13006aa4

Comment 35

[Matt Brubeck (:mbrubeck)]

Fix some build bustage caused by the merge with bug 718961:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4976f17b061a

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a9c391e2a89
https://hg.mozilla.org/mozilla-central/rev/d8fc13006aa4
https://hg.mozilla.org/mozilla-central/rev/4976f17b061a

Comment 37

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

Chris, please request aurora approval

Comment 38

[Chris Lord [:cwiiis]]

Until Patrick finishes on bug 717283, there's not any point in having this in aurora - it may end up causing regressions, and on its own, it's probably a small performance decrease.

Comment 39

[Alex Keybl [:akeybl]]

(In reply to Chris Lord [:cwiiis] from comment #38)
> Until Patrick finishes on bug 717283, there's not any point in having this
> in aurora - it may end up causing regressions, and on its own, it's probably
> a small performance decrease.

Which bug is blocking? This is bug 717283.

Comment 40

[Chris Lord [:cwiiis]]

(In reply to Alex Keybl [:akeybl] from comment #39)
> (In reply to Chris Lord [:cwiiis] from comment #38)
> > Until Patrick finishes on bug 717283, there's not any point in having this
> > in aurora - it may end up causing regressions, and on its own, it's probably
> > a small performance decrease.
> 
> Which bug is blocking? This is bug 717283.

Sorry, I meant bug 716581, which is now assigned to me. I'll be doing a back-out of this and related bugs on inbound tomorrow and we'll re-land this when it's ready/if it's needed, along with work on bug 716581.

Comment 41

[Chris Lord [:cwiiis]]

Just to note, I'm going back on doing that back-out. It's taking too much time to untangle the patches that have landed on top of it now and I'd be better off spending time working on the optimisations it enables.

Comment 42

[Chris Lord [:cwiiis]]

Sorry for the spam and flip-flopping - I have a patch that backs this out cleanly (I think). If/when this is complete, I'll open a new bug specifically for it and on checking that in, I'll reopen this.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Reopening this bug as explained above. The backout patch was landed on inbound as f9eb58a6dd6a in bug 724230.

Comment 44

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

Landed one of the patches. Now that I see these last few comments though, I wonder if that's the right thing to do, please advise

https://hg.mozilla.org/releases/mozilla-beta/rev/ca2b7c1b2ce0

Comment 45

[Kartikaya Gupta (email:[e-mail])]

If there are patches that depend on these ones and that would be tricky to rebase, you could just land these patches, the dependent patches, and then the backout from bug 724230. That would also keep beta in sync with m-c and simplify landing additional patches. The other alternative is to just keep all of the tiling stuff out of beta and rebase anything that depends on it.

Comment 46

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brad Lassey [:blassey] from comment #44)
> Landed one of the patches. Now that I see these last few comments though, I
> wonder if that's the right thing to do, please advise
> 
> https://hg.mozilla.org/releases/mozilla-beta/rev/ca2b7c1b2ce0

So based on the patches that depend on this and what has already landed it makes more sense to not land this at all. I'm going to back this patch out so that none of it is in beta (Fx 11).

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Backed out the partial beta landing:

https://hg.mozilla.org/releases/mozilla-beta/rev/7e6301419a31

Comment 48

[Joe Drew (not getting mail)]

Correct me if I'm wrong, but I don't think we're doing tiles at all any more.